diff mbox series

[net-next,v6,05/25] net: dsa: qca8k: handle error with qca8k_read operation

Message ID 20210514210015.18142-6-ansuelsmth@gmail.com (mailing list archive)
State Accepted
Commit 028f5f8ef44fcf87a456772cbb9f0d90a0a22884
Delegated to: Netdev Maintainers
Headers show
Series Multiple improvement to qca8k stability | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count fail Series longer than 15 patches
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 177 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Christian Marangi May 14, 2021, 8:59 p.m. UTC 
qca8k_read can fail. Rework any user to handle error values and
correctly return.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/qca8k.c | 73 ++++++++++++++++++++++++++++++++---------
 1 file changed, 58 insertions(+), 15 deletions(-)

Comments

Russell King (Oracle) May 14, 2021, 11:01 p.m. UTC | #1 
On Fri, May 14, 2021 at 10:59:55PM +0200, Ansuel Smith wrote:
> -static void
> +static int
>  qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
>  {
> -	u32 reg[4];
> +	u32 reg[4], val;

val is unsigned.

>  	int i;
>  
>  	/* load the ARL table into an array */
> -	for (i = 0; i < 4; i++)
> -		reg[i] = qca8k_read(priv, QCA8K_REG_ATU_DATA0 + (i * 4));
> +	for (i = 0; i < 4; i++) {
> +		val = qca8k_read(priv, QCA8K_REG_ATU_DATA0 + (i * 4));
> +		if (val < 0)
> +			return val;

So this return statement will never be reached.

> @@ -374,6 +386,8 @@ qca8k_fdb_access(struct qca8k_priv *priv, enum qca8k_fdb_cmd cmd, int port)
>  	/* Check for table full violation when adding an entry */
>  	if (cmd == QCA8K_FDB_LOAD) {
>  		reg = qca8k_read(priv, QCA8K_REG_ATU_FUNC);
> +		if (reg < 0)
> +			return reg;

"reg" here is also a u32, and therefore unsigned, so this will have no
effect.

>  		if (reg & QCA8K_ATU_FUNC_FULL)
>  			return -1;
>  	}
> @@ -388,10 +402,10 @@ qca8k_fdb_next(struct qca8k_priv *priv, struct qca8k_fdb *fdb, int port)
>  
>  	qca8k_fdb_write(priv, fdb->vid, fdb->port_mask, fdb->mac, fdb->aging);
>  	ret = qca8k_fdb_access(priv, QCA8K_FDB_NEXT, port);
> -	if (ret >= 0)
> -		qca8k_fdb_read(priv, fdb);
> +	if (ret < 0)
> +		return ret;

This looks fine to me.

>  
> -	return ret;
> +	return qca8k_fdb_read(priv, fdb);
>  }
>  
>  static int
> @@ -449,6 +463,8 @@ qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
>  	/* Check for table full violation when adding an entry */
>  	if (cmd == QCA8K_VLAN_LOAD) {
>  		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC1);
> +		if (reg < 0)
> +			return reg;

reg is unsigned... unreachable.

>  		if (reg & QCA8K_VTU_FUNC1_FULL)
>  			return -ENOMEM;
>  	}
> @@ -475,6 +491,8 @@ qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool untagged)
>  		goto out;
>  
>  	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
> +	if (reg < 0)
> +		return reg;

reg is unsigned... unreachable.

>  	reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
>  	reg &= ~(QCA8K_VTU_FUNC0_EG_MODE_MASK << QCA8K_VTU_FUNC0_EG_MODE_S(port));
>  	if (untagged)
> @@ -506,6 +524,8 @@ qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
>  		goto out;
>  
>  	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
> +	if (reg < 0)
> +		return reg;

reg is unsigned... unreachable.

>  	reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
>  	reg |= QCA8K_VTU_FUNC0_EG_MODE_NOT <<
>  			QCA8K_VTU_FUNC0_EG_MODE_S(port);
> @@ -621,8 +641,11 @@ qca8k_mdio_read(struct qca8k_priv *priv, int port, u32 regnum)
>  			    QCA8K_MDIO_MASTER_BUSY))
>  		return -ETIMEDOUT;
>  
> -	val = (qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL) &
> -		QCA8K_MDIO_MASTER_DATA_MASK);
> +	val = qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL);
> +	if (val < 0)
> +		return val;

val is unsigned... unreachable.

> +
> +	val &= QCA8K_MDIO_MASTER_DATA_MASK;
>  
>  	return val;
>  }
> @@ -978,6 +1001,8 @@ qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
>  	u32 reg;
>  
>  	reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
> +	if (reg < 0)
> +		return reg;

reg is unsigned... unreachable.

>  
>  	state->link = !!(reg & QCA8K_PORT_STATUS_LINK_UP);
>  	state->an_complete = state->link;
> @@ -1078,18 +1103,26 @@ qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
>  {
>  	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
>  	const struct qca8k_mib_desc *mib;
> -	u32 reg, i;
> +	u32 reg, i, val;
>  	u64 hi;
>  
>  	for (i = 0; i < ARRAY_SIZE(ar8327_mib); i++) {
>  		mib = &ar8327_mib[i];
>  		reg = QCA8K_PORT_MIB_COUNTER(port) + mib->offset;
>  
> -		data[i] = qca8k_read(priv, reg);
> +		val = qca8k_read(priv, reg);
> +		if (val < 0)
> +			continue;

val is unsigned... unreachable....

> +
>  		if (mib->size == 2) {
>  			hi = qca8k_read(priv, reg + 4);
> -			data[i] |= hi << 32;
> +			if (hi < 0)
> +				continue;

hi is a u64, so this condition is always false.

>  		}
> +
> +		data[i] = val;
> +		if (mib->size == 2)
> +			data[i] |= hi << 32;
>  	}
>  }
>  
> @@ -1107,18 +1140,25 @@ qca8k_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *eee)
>  {
>  	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
>  	u32 lpi_en = QCA8K_REG_EEE_CTRL_LPI_EN(port);
> +	int ret = 0;

No need to zero-initialise this.

>  	u32 reg;
>  
>  	mutex_lock(&priv->reg_mutex);
>  	reg = qca8k_read(priv, QCA8K_REG_EEE_CTRL);
> +	if (reg < 0) {
> +		ret = reg;
> +		goto exit;
> +	}
> +
>  	if (eee->eee_enabled)
>  		reg |= lpi_en;
>  	else
>  		reg &= ~lpi_en;
>  	qca8k_write(priv, QCA8K_REG_EEE_CTRL, reg);
> -	mutex_unlock(&priv->reg_mutex);
>  
> -	return 0;
> +exit:
> +	mutex_unlock(&priv->reg_mutex);
> +	return ret;
>  }
>  
>  static int
> @@ -1443,6 +1483,9 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
>  
>  	/* read the switches ID register */
>  	id = qca8k_read(priv, QCA8K_REG_MASK_CTRL);
> +	if (id < 0)
> +		return id;

id is unsigned ...
diff mbox series

Patch

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index c9830286fd6d..5eb4d13fe0ba 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -231,8 +231,13 @@  static int
 qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+	int ret;
+
+	ret = qca8k_read(priv, reg);
+	if (ret < 0)
+		return ret;
 
-	*val = qca8k_read(priv, reg);
+	*val = ret;
 
 	return 0;
 }
@@ -300,15 +305,20 @@  qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
 	return ret;
 }
 
-static void
+static int
 qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
 {
-	u32 reg[4];
+	u32 reg[4], val;
 	int i;
 
 	/* load the ARL table into an array */
-	for (i = 0; i < 4; i++)
-		reg[i] = qca8k_read(priv, QCA8K_REG_ATU_DATA0 + (i * 4));
+	for (i = 0; i < 4; i++) {
+		val = qca8k_read(priv, QCA8K_REG_ATU_DATA0 + (i * 4));
+		if (val < 0)
+			return val;
+
+		reg[i] = val;
+	}
 
 	/* vid - 83:72 */
 	fdb->vid = (reg[2] >> QCA8K_ATU_VID_S) & QCA8K_ATU_VID_M;
@@ -323,6 +333,8 @@  qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
 	fdb->mac[3] = (reg[0] >> QCA8K_ATU_ADDR3_S) & 0xff;
 	fdb->mac[4] = (reg[0] >> QCA8K_ATU_ADDR4_S) & 0xff;
 	fdb->mac[5] = reg[0] & 0xff;
+
+	return 0;
 }
 
 static void
@@ -374,6 +386,8 @@  qca8k_fdb_access(struct qca8k_priv *priv, enum qca8k_fdb_cmd cmd, int port)
 	/* Check for table full violation when adding an entry */
 	if (cmd == QCA8K_FDB_LOAD) {
 		reg = qca8k_read(priv, QCA8K_REG_ATU_FUNC);
+		if (reg < 0)
+			return reg;
 		if (reg & QCA8K_ATU_FUNC_FULL)
 			return -1;
 	}
@@ -388,10 +402,10 @@  qca8k_fdb_next(struct qca8k_priv *priv, struct qca8k_fdb *fdb, int port)
 
 	qca8k_fdb_write(priv, fdb->vid, fdb->port_mask, fdb->mac, fdb->aging);
 	ret = qca8k_fdb_access(priv, QCA8K_FDB_NEXT, port);
-	if (ret >= 0)
-		qca8k_fdb_read(priv, fdb);
+	if (ret < 0)
+		return ret;
 
-	return ret;
+	return qca8k_fdb_read(priv, fdb);
 }
 
 static int
@@ -449,6 +463,8 @@  qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
 	/* Check for table full violation when adding an entry */
 	if (cmd == QCA8K_VLAN_LOAD) {
 		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC1);
+		if (reg < 0)
+			return reg;
 		if (reg & QCA8K_VTU_FUNC1_FULL)
 			return -ENOMEM;
 	}
@@ -475,6 +491,8 @@  qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool untagged)
 		goto out;
 
 	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
+	if (reg < 0)
+		return reg;
 	reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
 	reg &= ~(QCA8K_VTU_FUNC0_EG_MODE_MASK << QCA8K_VTU_FUNC0_EG_MODE_S(port));
 	if (untagged)
@@ -506,6 +524,8 @@  qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
 		goto out;
 
 	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
+	if (reg < 0)
+		return reg;
 	reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
 	reg |= QCA8K_VTU_FUNC0_EG_MODE_NOT <<
 			QCA8K_VTU_FUNC0_EG_MODE_S(port);
@@ -621,8 +641,11 @@  qca8k_mdio_read(struct qca8k_priv *priv, int port, u32 regnum)
 			    QCA8K_MDIO_MASTER_BUSY))
 		return -ETIMEDOUT;
 
-	val = (qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL) &
-		QCA8K_MDIO_MASTER_DATA_MASK);
+	val = qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL);
+	if (val < 0)
+		return val;
+
+	val &= QCA8K_MDIO_MASTER_DATA_MASK;
 
 	return val;
 }
@@ -978,6 +1001,8 @@  qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
 	u32 reg;
 
 	reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
+	if (reg < 0)
+		return reg;
 
 	state->link = !!(reg & QCA8K_PORT_STATUS_LINK_UP);
 	state->an_complete = state->link;
@@ -1078,18 +1103,26 @@  qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	const struct qca8k_mib_desc *mib;
-	u32 reg, i;
+	u32 reg, i, val;
 	u64 hi;
 
 	for (i = 0; i < ARRAY_SIZE(ar8327_mib); i++) {
 		mib = &ar8327_mib[i];
 		reg = QCA8K_PORT_MIB_COUNTER(port) + mib->offset;
 
-		data[i] = qca8k_read(priv, reg);
+		val = qca8k_read(priv, reg);
+		if (val < 0)
+			continue;
+
 		if (mib->size == 2) {
 			hi = qca8k_read(priv, reg + 4);
-			data[i] |= hi << 32;
+			if (hi < 0)
+				continue;
 		}
+
+		data[i] = val;
+		if (mib->size == 2)
+			data[i] |= hi << 32;
 	}
 }
 
@@ -1107,18 +1140,25 @@  qca8k_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *eee)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	u32 lpi_en = QCA8K_REG_EEE_CTRL_LPI_EN(port);
+	int ret = 0;
 	u32 reg;
 
 	mutex_lock(&priv->reg_mutex);
 	reg = qca8k_read(priv, QCA8K_REG_EEE_CTRL);
+	if (reg < 0) {
+		ret = reg;
+		goto exit;
+	}
+
 	if (eee->eee_enabled)
 		reg |= lpi_en;
 	else
 		reg &= ~lpi_en;
 	qca8k_write(priv, QCA8K_REG_EEE_CTRL, reg);
-	mutex_unlock(&priv->reg_mutex);
 
-	return 0;
+exit:
+	mutex_unlock(&priv->reg_mutex);
+	return ret;
 }
 
 static int
@@ -1443,6 +1483,9 @@  qca8k_sw_probe(struct mdio_device *mdiodev)
 
 	/* read the switches ID register */
 	id = qca8k_read(priv, QCA8K_REG_MASK_CTRL);
+	if (id < 0)
+		return id;
+
 	id >>= QCA8K_MASK_CTRL_ID_S;
 	id &= QCA8K_MASK_CTRL_ID_M;
 	if (id != QCA8K_ID_QCA8337)