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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220919233057.ppfarnbf25znkzj2@skbuf>
Date:   Mon, 19 Sep 2022 23:30:57 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "mattias.forsblad@...il.com" <mattias.forsblad@...il.com>,
        netdev <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Christian Marangi <ansuelsmth@...il.com>
Subject: Re: [PATCH rfc v0 6/9] net: dsa: qca8k: Refactor sequence number
 mismatch to use error code

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?

>  }