Re: [PATCH rfc v0 6/9] net: dsa: qca8k: Refactor sequence number mismatch to use error code

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

On Tue, Sep 20, 2022 at 12:18:50AM +0200, Andrew Lunn wrote:
> @@ -229,7 +231,7 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  {
>  	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
>  	struct sk_buff *skb;
> -	bool ack;
> +	int err;
>  	int ret;
>  
>  	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL,
> @@ -247,7 +249,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  	}
>  
>  	skb->dev = priv->mgmt_master;
> -	mgmt_eth_data->ack = false;
>  
>  	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
>  			      qca8k_mdio_header_fill_seq_num,
> @@ -257,15 +258,15 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  	if (len > QCA_HDR_MGMT_DATA1_LEN)
>  		memcpy(val + 1, mgmt_eth_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN);
>  
> -	ack = mgmt_eth_data->ack;
> +	err = mgmt_eth_data->err;
>  
>  	mutex_unlock(&mgmt_eth_data->mutex);
>  
>  	if (ret)
>  		return ret;
>  
> -	if (!ack)
> -		return -EINVAL;
> +	if (err)
> +		return -ret;

Probably "if (err) return -ret" is not what you intend. We know ret is 0,
we just checked for it earlier.

Also, maybe a variable named "match" would be more expressive? This
shows how easy it is to make mistakes, mixing "err" with "ret" in the
same function.

>  
>  	return 0;

Can it actually be expressed as "return err", here and everywhere else?

>  }