diff mbox series

[RFC,net-next,v3,07/20] net: dsa: qca8k: handle error with qca8k_rmw operation

Message ID 20210504222915.17206-7-ansuelsmth@gmail.com
State Superseded
Headers show
Series Multiple improvement to qca8k stability | expand

Commit Message

Christian Marangi May 4, 2021, 10:29 p.m. UTC 
qca8k_rmw can fail. Rework any user to handle error values and
correctly return.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 130 +++++++++++++++++++++++++---------------
 1 file changed, 83 insertions(+), 47 deletions(-)

Comments

Andrew Lunn May 5, 2021, 12:46 a.m. UTC | #1 
> -static void

> +static int

>  qca8k_reg_set(struct qca8k_priv *priv, u32 reg, u32 val)

>  {

> -	qca8k_rmw(priv, reg, 0, val);

> +	int ret;

> +

> +	ret = qca8k_rmw(priv, reg, 0, val);

> +	if (ret < 0)

> +		return ret;

> +

> +	return 0;


Maybe return qca8k_rmw(priv, reg, 0, val); ??

> -static void

> +static int

>  qca8k_reg_clear(struct qca8k_priv *priv, u32 reg, u32 val)

>  {

> -	qca8k_rmw(priv, reg, val, 0);

> +	int ret;

> +

> +	ret = qca8k_rmw(priv, reg, val, 0);

> +	if (ret < 0)

> +		return ret;

> +

> +	return 0;

>  }


Maybe return qca8k_rmw(priv, reg, val, 0);

> @@ -1249,17 +1280,20 @@ qca8k_port_bridge_join(struct dsa_switch *ds, int port, struct net_device *br)

>  		/* Add this port to the portvlan mask of the other ports

>  		 * in the bridge

>  		 */

> -		qca8k_reg_set(priv,

> -			      QCA8K_PORT_LOOKUP_CTRL(i),

> -			      BIT(port));

> +		ret = qca8k_reg_set(priv,

> +				    QCA8K_PORT_LOOKUP_CTRL(i),

> +				    BIT(port));

> +		if (ret)

> +			return ret;

>  		if (i != port)

>  			port_mask |= BIT(i);

>  	}

> +

>  	/* Add all other ports to this ports portvlan mask */

> -	qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),

> -		  QCA8K_PORT_LOOKUP_MEMBER, port_mask);

> +	ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),

> +			QCA8K_PORT_LOOKUP_MEMBER, port_mask);

>  

> -	return 0;

> +	return ret < 0 ? ret : 0;


Can this is simplified to 

	return  = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
			    QCA8K_PORT_LOOKUP_MEMBER, port_mask);

> @@ -1396,18 +1430,19 @@ qca8k_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,

>  			  struct netlink_ext_ack *extack)

>  {

>  	struct qca8k_priv *priv = ds->priv;

> +	int ret;

>  

>  	if (vlan_filtering) {

> -		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),

> -			  QCA8K_PORT_LOOKUP_VLAN_MODE,

> -			  QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);

> +		ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),

> +				QCA8K_PORT_LOOKUP_VLAN_MODE,

> +				QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);

>  	} else {

> -		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),

> -			  QCA8K_PORT_LOOKUP_VLAN_MODE,

> -			  QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);

> +		ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),

> +				QCA8K_PORT_LOOKUP_VLAN_MODE,

> +				QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);

>  	}

>  

> -	return 0;

> +	return ret < 0 ? ret : 0;


What does qca8k_rmw() actually return?

     Andrew
Christian Marangi May 5, 2021, 12:51 a.m. UTC | #2 
On Wed, May 05, 2021 at 02:46:23AM +0200, Andrew Lunn wrote:
> > -static void

> > +static int

> >  qca8k_reg_set(struct qca8k_priv *priv, u32 reg, u32 val)

> >  {

> > -	qca8k_rmw(priv, reg, 0, val);

> > +	int ret;

> > +

> > +	ret = qca8k_rmw(priv, reg, 0, val);

> > +	if (ret < 0)

> > +		return ret;

> > +

> > +	return 0;

> 

> Maybe return qca8k_rmw(priv, reg, 0, val); ??

> 

> > -static void

> > +static int

> >  qca8k_reg_clear(struct qca8k_priv *priv, u32 reg, u32 val)

> >  {

> > -	qca8k_rmw(priv, reg, val, 0);

> > +	int ret;

> > +

> > +	ret = qca8k_rmw(priv, reg, val, 0);

> > +	if (ret < 0)

> > +		return ret;

> > +

> > +	return 0;

> >  }

> 

> Maybe return qca8k_rmw(priv, reg, val, 0);

> 

> > @@ -1249,17 +1280,20 @@ qca8k_port_bridge_join(struct dsa_switch *ds, int port, struct net_device *br)

> >  		/* Add this port to the portvlan mask of the other ports

> >  		 * in the bridge

> >  		 */

> > -		qca8k_reg_set(priv,

> > -			      QCA8K_PORT_LOOKUP_CTRL(i),

> > -			      BIT(port));

> > +		ret = qca8k_reg_set(priv,

> > +				    QCA8K_PORT_LOOKUP_CTRL(i),

> > +				    BIT(port));

> > +		if (ret)

> > +			return ret;

> >  		if (i != port)

> >  			port_mask |= BIT(i);

> >  	}

> > +

> >  	/* Add all other ports to this ports portvlan mask */

> > -	qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),

> > -		  QCA8K_PORT_LOOKUP_MEMBER, port_mask);

> > +	ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),

> > +			QCA8K_PORT_LOOKUP_MEMBER, port_mask);

> >  

> > -	return 0;

> > +	return ret < 0 ? ret : 0;

> 

> Can this is simplified to 

> 

> 	return  = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),

> 			    QCA8K_PORT_LOOKUP_MEMBER, port_mask);

> 

> > @@ -1396,18 +1430,19 @@ qca8k_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,

> >  			  struct netlink_ext_ack *extack)

> >  {

> >  	struct qca8k_priv *priv = ds->priv;

> > +	int ret;

> >  

> >  	if (vlan_filtering) {

> > -		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),

> > -			  QCA8K_PORT_LOOKUP_VLAN_MODE,

> > -			  QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);

> > +		ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),

> > +				QCA8K_PORT_LOOKUP_VLAN_MODE,

> > +				QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);

> >  	} else {

> > -		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),

> > -			  QCA8K_PORT_LOOKUP_VLAN_MODE,

> > -			  QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);

> > +		ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),

> > +				QCA8K_PORT_LOOKUP_VLAN_MODE,

> > +				QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);

> >  	}

> >  

> > -	return 0;

> > +	return ret < 0 ? ret : 0;

> 

> What does qca8k_rmw() actually return?

>

>      Andrew


qca8k_rmw in the original code returns the value read from the reg and
modified. So this is why all the strange checks. The idea is to not
change the original return values of the users so I check if every return
value is negative and return 0 in all the other cases.
diff mbox series

Patch

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 899bf93118eb..33875ad58d59 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -190,12 +190,13 @@  qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
 static u32
 qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 val)
 {
+	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
-	u32 ret;
+	int ret;
 
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
-	mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
 
 	ret = qca8k_set_page(priv->bus, page);
 	if (ret < 0)
@@ -207,21 +208,32 @@  qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 val)
 	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, ret);
 
 exit:
-	mutex_unlock(&priv->bus->mdio_lock);
-
+	mutex_unlock(&bus->mdio_lock);
 	return ret;
 }
 
-static void
+static int
 qca8k_reg_set(struct qca8k_priv *priv, u32 reg, u32 val)
 {
-	qca8k_rmw(priv, reg, 0, val);
+	int ret;
+
+	ret = qca8k_rmw(priv, reg, 0, val);
+	if (ret < 0)
+		return ret;
+
+	return 0;
 }
 
-static void
+static int
 qca8k_reg_clear(struct qca8k_priv *priv, u32 reg, u32 val)
 {
-	qca8k_rmw(priv, reg, val, 0);
+	int ret;
+
+	ret = qca8k_rmw(priv, reg, val, 0);
+	if (ret < 0)
+		return ret;
+
+	return 0;
 }
 
 static int
@@ -572,13 +584,17 @@  qca8k_mib_init(struct qca8k_priv *priv)
 	int ret;
 
 	mutex_lock(&priv->reg_mutex);
-	qca8k_reg_set(priv, QCA8K_REG_MIB, QCA8K_MIB_FLUSH | QCA8K_MIB_BUSY);
+	ret = qca8k_reg_set(priv, QCA8K_REG_MIB, QCA8K_MIB_FLUSH | QCA8K_MIB_BUSY);
+	if (ret)
+		goto exit;
 
 	ret = qca8k_busy_wait(priv, QCA8K_REG_MIB, QCA8K_MIB_BUSY);
 	if (ret)
 		goto exit;
 
-	qca8k_reg_set(priv, QCA8K_REG_MIB, QCA8K_MIB_CPU_KEEP);
+	ret = qca8k_reg_set(priv, QCA8K_REG_MIB, QCA8K_MIB_CPU_KEEP);
+	if (ret)
+		goto exit;
 
 	ret = qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
 
@@ -754,9 +770,8 @@  qca8k_setup_mdio_bus(struct qca8k_priv *priv)
 		 * a dt-overlay and driver reload changed the configuration
 		 */
 
-		qca8k_reg_clear(priv, QCA8K_MDIO_MASTER_CTRL,
-				QCA8K_MDIO_MASTER_EN);
-		return 0;
+		return qca8k_reg_clear(priv, QCA8K_MDIO_MASTER_CTRL,
+				       QCA8K_MDIO_MASTER_EN);
 	}
 
 	priv->ops.phy_read = qca8k_phy_read;
@@ -789,8 +804,12 @@  qca8k_setup(struct dsa_switch *ds)
 		return ret;
 
 	/* Enable CPU Port */
-	qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
-		      QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
+	ret = qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
+			    QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
+	if (ret) {
+		pr_err("failed enabling CPU port");
+		return ret;
+	}
 
 	/* Enable MIB counters */
 	ret = qca8k_mib_init(priv);
@@ -807,9 +826,12 @@  qca8k_setup(struct dsa_switch *ds)
 	}
 
 	/* Disable forwarding by default on all ports */
-	for (i = 0; i < QCA8K_NUM_PORTS; i++)
-		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
-			  QCA8K_PORT_LOOKUP_MEMBER, 0);
+	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+		ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
+				QCA8K_PORT_LOOKUP_MEMBER, 0);
+		if (ret < 0)
+			return ret;
+	}
 
 	/* Disable MAC by default on all ports */
 	for (i = 1; i < QCA8K_NUM_PORTS; i++)
@@ -828,28 +850,37 @@  qca8k_setup(struct dsa_switch *ds)
 	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
 		/* CPU port gets connected to all user ports of the switch */
 		if (dsa_is_cpu_port(ds, i)) {
-			qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(QCA8K_CPU_PORT),
-				  QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
+			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(QCA8K_CPU_PORT),
+					QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
+			if (ret < 0)
+				return ret;
 		}
 
 		/* Individual user ports get connected to CPU port only */
 		if (dsa_is_user_port(ds, i)) {
 			int shift = 16 * (i % 2);
 
-			qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
-				  QCA8K_PORT_LOOKUP_MEMBER,
-				  BIT(QCA8K_CPU_PORT));
+			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
+					QCA8K_PORT_LOOKUP_MEMBER,
+					BIT(QCA8K_CPU_PORT));
+			if (ret < 0)
+				return ret;
 
 			/* Enable ARP Auto-learning by default */
-			qca8k_reg_set(priv, QCA8K_PORT_LOOKUP_CTRL(i),
-				      QCA8K_PORT_LOOKUP_LEARN);
+			ret = qca8k_reg_set(priv, QCA8K_PORT_LOOKUP_CTRL(i),
+					    QCA8K_PORT_LOOKUP_LEARN);
+			if (ret)
+				return ret;
 
 			/* For port based vlans to work we need to set the
 			 * default egress vid
 			 */
-			qca8k_rmw(priv, QCA8K_EGRESS_VLAN(i),
-				  0xfff << shift,
-				  QCA8K_PORT_VID_DEF << shift);
+			ret = qca8k_rmw(priv, QCA8K_EGRESS_VLAN(i),
+					0xfff << shift,
+					QCA8K_PORT_VID_DEF << shift);
+			if (ret < 0)
+				return ret;
+
 			ret = qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(i),
 					  QCA8K_PORT_VLAN_CVID(QCA8K_PORT_VID_DEF) |
 					  QCA8K_PORT_VLAN_SVID(QCA8K_PORT_VID_DEF));
@@ -1241,7 +1272,7 @@  qca8k_port_bridge_join(struct dsa_switch *ds, int port, struct net_device *br)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	int port_mask = BIT(QCA8K_CPU_PORT);
-	int i;
+	int i, ret;
 
 	for (i = 1; i < QCA8K_NUM_PORTS; i++) {
 		if (dsa_to_port(ds, i)->bridge_dev != br)
@@ -1249,17 +1280,20 @@  qca8k_port_bridge_join(struct dsa_switch *ds, int port, struct net_device *br)
 		/* Add this port to the portvlan mask of the other ports
 		 * in the bridge
 		 */
-		qca8k_reg_set(priv,
-			      QCA8K_PORT_LOOKUP_CTRL(i),
-			      BIT(port));
+		ret = qca8k_reg_set(priv,
+				    QCA8K_PORT_LOOKUP_CTRL(i),
+				    BIT(port));
+		if (ret)
+			return ret;
 		if (i != port)
 			port_mask |= BIT(i);
 	}
+
 	/* Add all other ports to this ports portvlan mask */
-	qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
-		  QCA8K_PORT_LOOKUP_MEMBER, port_mask);
+	ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
+			QCA8K_PORT_LOOKUP_MEMBER, port_mask);
 
-	return 0;
+	return ret < 0 ? ret : 0;
 }
 
 static void
@@ -1396,18 +1430,19 @@  qca8k_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
 			  struct netlink_ext_ack *extack)
 {
 	struct qca8k_priv *priv = ds->priv;
+	int ret;
 
 	if (vlan_filtering) {
-		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
-			  QCA8K_PORT_LOOKUP_VLAN_MODE,
-			  QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
+		ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
+				QCA8K_PORT_LOOKUP_VLAN_MODE,
+				QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
 	} else {
-		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
-			  QCA8K_PORT_LOOKUP_VLAN_MODE,
-			  QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
+		ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
+				QCA8K_PORT_LOOKUP_VLAN_MODE,
+				QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
 	}
 
-	return 0;
+	return ret < 0 ? ret : 0;
 }
 
 static int
@@ -1429,16 +1464,17 @@  qca8k_port_vlan_add(struct dsa_switch *ds, int port,
 	if (pvid) {
 		int shift = 16 * (port % 2);
 
-		qca8k_rmw(priv, QCA8K_EGRESS_VLAN(port),
-			  0xfff << shift, vlan->vid << shift);
+		ret = qca8k_rmw(priv, QCA8K_EGRESS_VLAN(port),
+				0xfff << shift, vlan->vid << shift);
+		if (ret < 0)
+			return ret;
+
 		ret = qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(port),
 				  QCA8K_PORT_VLAN_CVID(vlan->vid) |
 				  QCA8K_PORT_VLAN_SVID(vlan->vid));
-		if (ret)
-			return ret;
 	}
 
-	return 0;
+	return ret;
 }
 
 static int