diff mbox series

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

Message ID 20220919221853.4095491-7-andrew@lunn.ch (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series DSA: Move parts of inband signalling into the DSA | expand

Commit Message

Andrew Lunn Sept. 19, 2022, 10:18 p.m. UTC 
Replace the boolean that the sequence numbers matches with an error
code. Set the error code to -EINVAL if the sequence numbers are wrong,
otherwise 0.

The value is only safe to us if the completion happens. Ensure the
return from the completion is always considered, and if it fails, a
timeout error is returned.

This is a preparation step to moving the error tracking into the DSA
core.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 50 ++++++++++++++------------------
 drivers/net/dsa/qca/qca8k.h      |  2 +-
 2 files changed, 23 insertions(+), 29 deletions(-)

Comments

Vladimir Oltean Sept. 19, 2022, 11:30 p.m. UTC | #1 
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?

>  }
Andrew Lunn Sept. 20, 2022, 12:05 a.m. UTC | #2 
> > -	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.

Good catch. Thanks.

> 
> 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.

A lot of this code gets removed in the next patch. I'm just trying to
keep to lots of small, easy to review patches, which in this case
results in some not so nice intermediary state, but the next patch
cleans it up.

       Andrew
diff mbox series

Patch

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index a354ba070d33..69b807d87367 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -147,7 +147,9 @@  static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 
 	/* Make sure the seq match the requested packet */
 	if (mgmt_ethhdr->seq == dsa_inband_seqno(&mgmt_eth_data->inband))
-		mgmt_eth_data->ack = true;
+		mgmt_eth_data->err = 0;
+	else
+		mgmt_eth_data->err = -EINVAL;
 
 	if (cmd == MDIO_READ) {
 		mgmt_eth_data->data[0] = mgmt_ethhdr->mdio_data;
@@ -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;
 
 	return 0;
 }
@@ -274,7 +275,7 @@  static int qca8k_write_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_WRITE, reg, val,
@@ -292,21 +293,20 @@  static int qca8k_write_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,
 				 QCA8K_ETHERNET_TIMEOUT);
 
-	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 err;
 
 	return 0;
 }
@@ -431,22 +431,20 @@  qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 			struct sk_buff *read_skb, u32 *val)
 {
 	struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL);
-	bool ack;
+	int err;
 	int ret;
 
-	mgmt_eth_data->ack = false;
-
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
 				 qca8k_mdio_header_fill_seq_num,
 				 QCA8K_ETHERNET_TIMEOUT);
 
-	ack = mgmt_eth_data->ack;
+	err = mgmt_eth_data->err;
 
 	if (ret)
 		return ret;
 
-	if (!ack)
-		return -EINVAL;
+	if (err)
+		return err;
 
 	*val = mgmt_eth_data->data[0];
 
@@ -462,7 +460,7 @@  qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	u32 write_val, clear_val = 0, val;
 	struct net_device *mgmt_master;
 	int ret, ret1;
-	bool ack;
+	int err;
 
 	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
 		return -EINVAL;
@@ -519,21 +517,20 @@  qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	read_skb->dev = mgmt_master;
 	clear_skb->dev = mgmt_master;
 	write_skb->dev = mgmt_master;
-	mgmt_eth_data->ack = false;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, write_skb,
 				 qca8k_mdio_header_fill_seq_num,
 				 QCA8K_ETHERNET_TIMEOUT);
 
-	ack = mgmt_eth_data->ack;
+	err = mgmt_eth_data->err;
 
 	if (ret) {
 		kfree_skb(read_skb);
 		goto exit;
 	}
 
-	if (!ack) {
-		ret = -EINVAL;
+	if (err) {
+		ret = err;
 		kfree_skb(read_skb);
 		goto exit;
 	}
@@ -549,19 +546,17 @@  qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	}
 
 	if (read) {
-		mgmt_eth_data->ack = false;
-
 		ret = dsa_inband_request(&mgmt_eth_data->inband, read_skb,
 					 qca8k_mdio_header_fill_seq_num,
 					 QCA8K_ETHERNET_TIMEOUT);
 
-		ack = mgmt_eth_data->ack;
+		err = mgmt_eth_data->err;
 
 		if (ret)
 			goto exit;
 
-		if (!ack) {
-			ret = -EINVAL;
+		if (err) {
+			ret = err;
 			goto exit;
 		}
 
@@ -570,7 +565,6 @@  qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 		kfree_skb(read_skb);
 	}
 exit:
-	mgmt_eth_data->ack = false;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
 				 qca8k_mdio_header_fill_seq_num,
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index a5abc340471c..79f7197a1790 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -348,7 +348,7 @@  enum {
 struct qca8k_mgmt_eth_data {
 	struct dsa_inband inband;
 	struct mutex mutex; /* Enforce one mdio read/write at time */
-	bool ack;
+	int err;
 	u32 data[4];
 };