Re: [PATCH net-next 04/12] net: dsa: qca8k: Drop replies with wrong sequence numbers

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: dsa: qca8k: Drop replies with wrong sequence numbers

The change makes qca8k_rw_reg_ack_handler() return early when the reply
sequence number does not match, rather than only setting ack on match.
The intent is to avoid signalling completion for a late reply that could
be mistaken for the next genuine reply.

> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index b0cbe72c15b4..3b3fe9601617 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -185,9 +185,9 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
>  	/* We can ignore odd value, we always round up them in the alloc function. */
>  	len *= sizeof(u16);
>  
> -	/* Make sure the seq match the requested packet */
> -	if (get_unaligned_le32(&mgmt_ethhdr->seq) == mgmt_eth_data->seq)
> -		mgmt_eth_data->ack = true;
> +	/* Make sure the seq match the requested packet. If not, drop. */
> +	if (get_unaligned_le32(&mgmt_ethhdr->seq) != mgmt_eth_data->seq)
> +		return;

[High]
After this change, is mgmt_eth_data->ack ever assigned true anywhere in
the driver?  A grep across drivers/net/dsa/qca/ shows five sites that
initialise it to false before dsa_inband_request(), five sites that read
it afterwards, and no remaining site that ever writes true.

The callers still do, for example in qca8k_read_eth():

	mgmt_eth_data->ack = false;

	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
				 QCA8K_ETHERNET_TIMEOUT);
	...
	ack = mgmt_eth_data->ack;

	mutex_unlock(&mgmt_eth_data->mutex);

	if (ret)
		return ret;

	if (!ack)
		return -EINVAL;

With the assignment gone, does every successful reply now leave ack ==
false and cause qca8k_read_eth(), qca8k_write_eth() and the read/write/
clear paths in qca8k_phy_eth_command() to return -EINVAL on their fast
path?

The higher-level callers (qca8k_bulk_read(), qca8k_bulk_write(),
qca8k_regmap_update_bits(), qca8k_internal_mdio_read() and
qca8k_internal_mdio_write()) fall back to the slow MII path on error, so
the device keeps functioning, but is the Ethernet management fast path
effectively disabled between this patch and its follow-up cleanup?

Would it be cleaner to fold the removal of the ack field (and the
matching if (!ack) return -EINVAL checks in every caller) into this same
commit, so that each commit in the series is self-contained and bisects
cleanly?