Re: [PATCH rfc v2 08/10] net: dsa: qca8k: Pass error code from reply decoder to requester

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Thu, Sep 22, 2022 at 07:58:19PM +0200, Andrew Lunn wrote:
> The code which decodes the frame and signals the complete can detect
> error within the reply, such as fields have unexpected values. Pass an
> error code between the completer and the function waiting on the
> complete. This simplifies the error handling, since all errors are
> combined into one place.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> v2:
> 
> Remove EPROTO if the sequence numbers don't match, drop the reply
> ---
> @@ -1439,6 +1400,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
>  	const struct qca8k_mib_desc *mib;
>  	struct mib_ethhdr *mib_ethhdr;
>  	int i, mib_len, offset = 0;
> +	int err = 0;
>  	u64 *data;
>  	u8 port;
>  
> @@ -1449,8 +1411,10 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
>  	 * parse only the requested one.
>  	 */
>  	port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, ntohs(mib_ethhdr->hdr));
> -	if (port != mib_eth_data->req_port)
> +	if (port != mib_eth_data->req_port) {
> +		err = -EPROTO;
>  		goto exit;
> +	}
>  
>  	data = mib_eth_data->data;
>  
> @@ -1479,7 +1443,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
>  exit:
>  	/* Complete on receiving all the mib packet */
>  	if (refcount_dec_and_test(&mib_eth_data->port_parsed))
> -		dsa_inband_complete(&mib_eth_data->inband);
> +		dsa_inband_complete(&mib_eth_data->inband, err);
>  }

My understanding of the autocast function (I could be wrong) is that
it's essentially one request with 10 (or how many ports there are)
responses. At least this is what the code appears to handle.

I don't think you can say "if port isn't the requested port => set err
to -EPROTO". Because this will make dsa_inband_wait_for_completion() see
as a return code -EPROTO whenever the requested port was anything except
for the last port for which the switch had something to autocast. Which
is probably the last port.