Skip to contentSkip to conversation

net: dsa: mt7530: fix remaining swallowed MDIO access errors

PATCH Superseded Daniel Golle 3 participants 10 messages Jul 30 – Aug 4

Subsystems (7)
Networking Network drivers Arm MediaTek Ethernet PHY +3 more subsystems

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Daniel Golle

author maintainer
#0 lore raw

cover net: dsa: mt7530: fix remaining swallowed MDIO access errors

v1 of "net: dsa: mt7530: fix swallowed MDIO read errors" [1] was applied to net just before v2 [2] went out. This series carries everything from v2 which was not already applied with v1 and still qualifies as a fix: the two standalone patches v2 had grown following the Sashiko AI reviewers' findings on v1, and, as patches of their own, the companion fixes v2 added inside the already applied patches:
the unchecked bus->read() in core_rmw() and the unchecked PHY_IAC command writes in the MT7531 indirect PHY access functions.

The remaining non-fix changes from v2, dropping a redundant read-back and improving the poll failure messages, will follow via net-next.

[1] https://lore.kernel.org/netdev/[email protected]/
[2] https://lore.kernel.org/netdev/[email protected]/

Daniel Golle (4):
net: pcs: mtk-lynxi: check regmap reads in mtk_pcs_lynxi_get_state()
net: dsa: mt7530: check bus->read() error in core_rmw()
net: dsa: mt7530: error out on failed PHY_IAC command writes
net: dsa: mt7530: serialize the regmap IRQ chip like every other user

 drivers/net/dsa/mt7530.c        | 56 ++++++++++++++++++++++++++++-----
 drivers/net/pcs/pcs-mtk-lynxi.c |  5 +--
 2 files changed, 51 insertions(+), 10 deletions(-)

base-commit: dd16f1b5720f5dae33a79b5305e188e8290a3973

Signature · 1 line
2.55.0
author maintainer
#1 lore raw

PATCH 1/4 net: pcs: mtk-lynxi: check regmap reads in mtk_pcs_lynxi_get_state()

mtk_pcs_lynxi_get_state() ignores regmap_read()'s return value; a failed read leaves bm and adv holding uninitialized stack values which are then decoded into the reported link state. The regmaps backing the MT7531 SGMII PCS instances sit on an MDIO bus where reads can fail. Check both reads and leave the state untouched on error.

Fixes 4765a9722e09 ("net: pcs: add driver for MediaTek SGMII PCS")
Signed-off-by Daniel Golle <[email protected]>
---
 drivers/net/pcs/pcs-mtk-lynxi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/net/pcs/pcs-mtk-lynxi.c +3 -2
index a753bd88cbc2..454f2924831c 100644
@@ -113,8 +113,9 @@ static void mtk_pcs_lynxi_get_state(struct phylink_pcs *pcs,
 	unsigned int bm, adv;
 
 	/* Read the BMSR and LPA */
-	regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &bm);
-	regmap_read(mpcs->regmap, SGMSYS_PCS_ADVERTISE, &adv);
+	if (regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &bm) ||
+	    regmap_read(mpcs->regmap, SGMSYS_PCS_ADVERTISE, &adv))
+		return;
 
 	phylink_mii_c22_pcs_decode_state(state, neg_mode,
 					 FIELD_GET(SGMII_BMSR, bm),
Signature · 1 line
2.55.0
maintainer Reviewed-by on v1 1/4
#2 lore raw

Re: [PATCH net 1/4] net: pcs: mtk-lynxi: check regmap reads in mtk_pcs_lynxi_get_state()

Quoted 9 lines · Daniel Golle

mtk_pcs_lynxi_get_state() ignores regmap_read()'s return value; a failed read leaves bm and adv holding uninitialized stack values which are then decoded into the reported link state. The regmaps backing the MT7531 SGMII PCS instances sit on an MDIO bus where reads can fail. Check both reads and leave the state untouched on error.

Fixes 4765a9722e09 ("net: pcs: add driver for MediaTek SGMII PCS")
Signed-off-by Daniel Golle <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>

Andrew

author maintainer
#3 lore raw

PATCH 2/4 net: dsa: mt7530: check bus->read() error in core_rmw()

In reply to Daniel Golle

core_rmw() accesses the MMD core registers directly rather than through the regmap and has the same unchecked bus->read() as the one just fixed in the MDIO regmap backend: a negative errno is consumed as register data, modified and written back to the switch. Check the read and bail out like the surrounding bus accesses do.

Fixes b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Signed-off-by Daniel Golle <[email protected]>
---
 drivers/net/dsa/mt7530.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/net/dsa/mt7530.c +5 -1
index aa33d94e11b5..9be6fb63c311 100644
@@ -124,8 +124,12 @@ core_rmw(struct mt7530_priv *priv, u32 reg, u32 mask, u32 set)
 		goto err;
 
 	/* Read the content of the MMD's selected register */
-	val = bus->read(bus, MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
+	ret = bus->read(bus, MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
 			MII_MMD_DATA);
+	if (ret < 0)
+		goto err;
+	val = ret;
+
 	val &= ~mask;
 	val |= set;
 	/* Write the data into MMD's selected register */
Signature · 1 line
2.55.0
maintainer Reviewed-by on v1 2/4
#4 lore raw

Re: [PATCH net 2/4] net: dsa: mt7530: check bus->read() error in core_rmw()

Quoted 8 lines · Daniel Golle

core_rmw() accesses the MMD core registers directly rather than through the regmap and has the same unchecked bus->read() as the one just fixed in the MDIO regmap backend: a negative errno is consumed as register data, modified and written back to the switch. Check the read and bail out like the surrounding bus accesses do.

Fixes b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Signed-off-by Daniel Golle <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>

Andrew

author maintainer
#5 lore raw

PATCH 3/4 net: dsa: mt7530: error out on failed PHY_IAC command writes

In reply to Daniel Golle

MT7531_PHY_ACS_ST is only ever set by the command write that precedes each poll in the MT7531 indirect PHY access functions, and that write's return value is discarded. A failed write leaves ACS_ST at 0 from the previous access, so the poll succeeds on its first iteration and the functions return stale IAC contents as if they were fresh PHY data. Check the writes and bail out before polling.

Fixes c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
Signed-off-by Daniel Golle <[email protected]>
---
 drivers/net/dsa/mt7530.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)
drivers/net/dsa/mt7530.c +18 -6
index 9be6fb63c311..2a17696e6a45 100644
@@ -565,7 +565,9 @@ mt7531_ind_c45_phy_read(struct mt7530_priv *priv, int port, int devad,
 
 	reg = MT7531_MDIO_CL45_ADDR | MT7531_MDIO_PHY_ADDR(port) |
 	      MT7531_MDIO_DEV_ADDR(devad) | regnum;
-	mt7530_mii_write(priv, MT7531_PHY_IAC, reg | MT7531_PHY_ACS_ST);
+	ret = mt7530_mii_write(priv, MT7531_PHY_IAC, reg | MT7531_PHY_ACS_ST);
+	if (ret < 0)
+		goto out;
 
 	ret = regmap_read_poll_timeout(priv->regmap, MT7531_PHY_IAC, val,
 				       !(val & MT7531_PHY_ACS_ST), 20, 100000);

@@ -576,7 +578,9 @@ mt7531_ind_c45_phy_read(struct mt7530_priv *priv, int port, int devad,
 
 	reg = MT7531_MDIO_CL45_READ | MT7531_MDIO_PHY_ADDR(port) |
 	      MT7531_MDIO_DEV_ADDR(devad);
-	mt7530_mii_write(priv, MT7531_PHY_IAC, reg | MT7531_PHY_ACS_ST);
+	ret = mt7530_mii_write(priv, MT7531_PHY_IAC, reg | MT7531_PHY_ACS_ST);
+	if (ret < 0)
+		goto out;
 
 	ret = regmap_read_poll_timeout(priv->regmap, MT7531_PHY_IAC, val,
 				       !(val & MT7531_PHY_ACS_ST), 20, 100000);

@@ -610,7 +614,9 @@ mt7531_ind_c45_phy_write(struct mt7530_priv *priv, int port, int devad,
 
 	reg = MT7531_MDIO_CL45_ADDR | MT7531_MDIO_PHY_ADDR(port) |
 	      MT7531_MDIO_DEV_ADDR(devad) | regnum;
-	mt7530_mii_write(priv, MT7531_PHY_IAC, reg | MT7531_PHY_ACS_ST);
+	ret = mt7530_mii_write(priv, MT7531_PHY_IAC, reg | MT7531_PHY_ACS_ST);
+	if (ret < 0)
+		goto out;
 
 	ret = regmap_read_poll_timeout(priv->regmap, MT7531_PHY_IAC, val,
 				       !(val & MT7531_PHY_ACS_ST), 20, 100000);

@@ -621,7 +627,9 @@ mt7531_ind_c45_phy_write(struct mt7530_priv *priv, int port, int devad,
 
 	reg = MT7531_MDIO_CL45_WRITE | MT7531_MDIO_PHY_ADDR(port) |
 	      MT7531_MDIO_DEV_ADDR(devad) | data;
-	mt7530_mii_write(priv, MT7531_PHY_IAC, reg | MT7531_PHY_ACS_ST);
+	ret = mt7530_mii_write(priv, MT7531_PHY_IAC, reg | MT7531_PHY_ACS_ST);
+	if (ret < 0)
+		goto out;
 
 	ret = regmap_read_poll_timeout(priv->regmap, MT7531_PHY_IAC, val,
 				       !(val & MT7531_PHY_ACS_ST), 20, 100000);

@@ -654,7 +662,9 @@ mt7531_ind_c22_phy_read(struct mt7530_priv *priv, int port, int regnum)
 	val = MT7531_MDIO_CL22_READ | MT7531_MDIO_PHY_ADDR(port) |
 	      MT7531_MDIO_REG_ADDR(regnum);
 
-	mt7530_mii_write(priv, MT7531_PHY_IAC, val | MT7531_PHY_ACS_ST);
+	ret = mt7530_mii_write(priv, MT7531_PHY_IAC, val | MT7531_PHY_ACS_ST);
+	if (ret < 0)
+		goto out;
 
 	ret = regmap_read_poll_timeout(priv->regmap, MT7531_PHY_IAC, val,
 				       !(val & MT7531_PHY_ACS_ST), 20, 100000);

@@ -689,7 +699,9 @@ mt7531_ind_c22_phy_write(struct mt7530_priv *priv, int port, int regnum,
 	reg = MT7531_MDIO_CL22_WRITE | MT7531_MDIO_PHY_ADDR(port) |
 	      MT7531_MDIO_REG_ADDR(regnum) | data;
 
-	mt7530_mii_write(priv, MT7531_PHY_IAC, reg | MT7531_PHY_ACS_ST);
+	ret = mt7530_mii_write(priv, MT7531_PHY_IAC, reg | MT7531_PHY_ACS_ST);
+	if (ret < 0)
+		goto out;
 
 	ret = regmap_read_poll_timeout(priv->regmap, MT7531_PHY_IAC, reg,
 				       !(reg & MT7531_PHY_ACS_ST), 20, 100000);
Signature · 1 line
2.55.0
maintainer Reviewed-by on v1 3/4
#6 lore raw

Re: [PATCH net 3/4] net: dsa: mt7530: error out on failed PHY_IAC command writes

Quoted 9 lines · Daniel Golle

MT7531_PHY_ACS_ST is only ever set by the command write that precedes each poll in the MT7531 indirect PHY access functions, and that write's return value is discarded. A failed write leaves ACS_ST at 0 from the previous access, so the poll succeeds on its first iteration and the functions return stale IAC contents as if they were fresh PHY data. Check the writes and bail out before polling.

Fixes c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
Signed-off-by Daniel Golle <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>

Andrew

author maintainer
#7 lore raw

PATCH 4/4 net: dsa: mt7530: serialize the regmap IRQ chip like every other user

In reply to Daniel Golle

The switch register regmap is created with .disable_locking = true; every other user in this driver calls mt7530_mutex_lock()/unlock() around it, which takes priv->bus->mdio_lock, since the underlying mt7530_regmap_read()/write() issue raw, unserialized bus->read()/ write() MDIO transactions.

mt7530_setup_irq() hands this same unlocked regmap straight to devm_regmap_add_irq_chip_fwnode(), whose threaded IRQ handler then calls regmap_read()/regmap_update_bits() on it without ever calling mt7530_mutex_lock(). An interrupt firing while another thread is mid-transaction on the same regmap (e.g. a paged register access, or an indirect PHY access) can interleave with the IRQ handler's own paged access and corrupt page selection on either side.

Use struct regmap_irq_chip's handle_mask_sync hook to call mt7530_mutex_lock()/unlock() around the mask register write regmap-irq issues whenever a consumer of one of the mapped sub-IRQs enables, disables, requests or frees its line. This needs a per-device copy of mt7530_regmap_irq_chip, since devm_regmap_add_irq_chip_fwnode() keeps a pointer to it rather than copying it.

handle_pre_irq/handle_post_irq, which would additionally cover the status read and ack write the threaded handler does directly, bracket the whole handler including its handle_nested_irq() calls. Lockdep caught this on hardware: those calls reach phy_interrupt() for the per-port PHY IRQ lines mapped through this chip, which takes phydev->lock, while phy_attach_direct() and this driver's own indirect PHY access already establish the opposite order (phydev->lock, then priv->bus->mdio_lock) elsewhere. Using them here would close that cycle, so they are not used.

regmap_irq_sync_unlock() also has its own init_ack_masked path, used by this chip, which unconditionally does its own regmap_write() to ack currently-masked IRQs; that path has no per-driver hook. Together with the threaded handler's own status read and ack write, these stay unprotected -- a narrower, harder-to-hit gap than the recurring mask sync above -- and will be closed once the switch regmap moves to regmap's own locking in the driver-wide register access cleanup.

Fixes 254f6b272e3b ("dsa: mt7530: Utilize REGMAP_IRQ for interrupt handling")
Signed-off-by Daniel Golle <[email protected]>
---
 drivers/net/dsa/mt7530.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)
drivers/net/dsa/mt7530.c +25 -1
index 2a17696e6a45..f8f0799ede63 100644
@@ -2311,6 +2311,21 @@ static const struct regmap_irq mt7530_irqs[] = {
 	REGMAP_IRQ_REG_LINE(31, 32), /* ACL */
 };
 
+/* Serialize regmap-irq's mask sync like every other regmap user */
+static int mt7530_irq_mask_sync(int index, unsigned int mask_buf_def,
+				unsigned int mask_buf, void *irq_drv_data)
+{
+	struct mt7530_priv *priv = irq_drv_data;
+	int ret;
+
+	mt7530_mutex_lock(priv);
+	ret = regmap_update_bits(priv->regmap, MT7530_SYS_INT_EN,
+				 mask_buf_def, ~mask_buf);
+	mt7530_mutex_unlock(priv);
+
+	return ret;
+}
+
 static const struct regmap_irq_chip mt7530_regmap_irq_chip = {
 	.name = KBUILD_MODNAME,
 	.status_base = MT7530_SYS_INT_STS,

@@ -2320,12 +2335,14 @@ static const struct regmap_irq_chip mt7530_regmap_irq_chip = {
 	.irqs = mt7530_irqs,
 	.num_irqs = ARRAY_SIZE(mt7530_irqs),
 	.num_regs = 1,
+	.handle_mask_sync = mt7530_irq_mask_sync,
 };
 
 static int
 mt7530_setup_irq(struct mt7530_priv *priv)
 {
 	struct regmap_irq_chip_data *irq_data;
+	struct regmap_irq_chip *chip;
 	struct device *dev = priv->dev;
 	struct device_node *np = dev->of_node;
 	int irq, ret;

@@ -2345,10 +2362,17 @@ mt7530_setup_irq(struct mt7530_priv *priv)
 	if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
 		mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL);
 
+	chip = devm_kmemdup(dev, &mt7530_regmap_irq_chip, sizeof(*chip),
+			    GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->irq_drv_data = priv;
+
 	ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev),
 					      priv->regmap, irq,
 					      IRQF_ONESHOT,
-					      0, &mt7530_regmap_irq_chip,
+					      0, chip,
 					      &irq_data);
 	if (ret)
 		return ret;
Signature · 1 line
2.55.0
maintainer Reviewed-by on v1 4/4
#8 lore raw

Re: [PATCH net 4/4] net: dsa: mt7530: serialize the regmap IRQ chip like every other user

Quoted 39 lines · Daniel Golle

The switch register regmap is created with .disable_locking = true; every other user in this driver calls mt7530_mutex_lock()/unlock() around it, which takes priv->bus->mdio_lock, since the underlying mt7530_regmap_read()/write() issue raw, unserialized bus->read()/ write() MDIO transactions.

mt7530_setup_irq() hands this same unlocked regmap straight to devm_regmap_add_irq_chip_fwnode(), whose threaded IRQ handler then calls regmap_read()/regmap_update_bits() on it without ever calling mt7530_mutex_lock(). An interrupt firing while another thread is mid-transaction on the same regmap (e.g. a paged register access, or an indirect PHY access) can interleave with the IRQ handler's own paged access and corrupt page selection on either side.

Use struct regmap_irq_chip's handle_mask_sync hook to call mt7530_mutex_lock()/unlock() around the mask register write regmap-irq issues whenever a consumer of one of the mapped sub-IRQs enables, disables, requests or frees its line. This needs a per-device copy of mt7530_regmap_irq_chip, since devm_regmap_add_irq_chip_fwnode() keeps a pointer to it rather than copying it.

handle_pre_irq/handle_post_irq, which would additionally cover the status read and ack write the threaded handler does directly, bracket the whole handler including its handle_nested_irq() calls. Lockdep caught this on hardware: those calls reach phy_interrupt() for the per-port PHY IRQ lines mapped through this chip, which takes phydev->lock, while phy_attach_direct() and this driver's own indirect PHY access already establish the opposite order (phydev->lock, then priv->bus->mdio_lock) elsewhere. Using them here would close that cycle, so they are not used.

regmap_irq_sync_unlock() also has its own init_ack_masked path, used by this chip, which unconditionally does its own regmap_write() to ack currently-masked IRQs; that path has no per-driver hook. Together with the threaded handler's own status read and ack write, these stay unprotected -- a narrower, harder-to-hit gap than the recurring mask sync above -- and will be closed once the switch regmap moves to regmap's own locking in the driver-wide register access cleanup.

Quoted 2 lines
Fixes 254f6b272e3b ("dsa: mt7530: Utilize REGMAP_IRQ for interrupt handling")
Signed-off-by Daniel Golle <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>

Andrew

maintainer
#9 lore raw
In reply to Daniel Golle
Quoted 11 lines · On Thu, 30 Jul 2026 17:03:00 +0100 Daniel Golle wrote:

v1 of "net: dsa: mt7530: fix swallowed MDIO read errors" [1] was applied to net just before v2 [2] went out. This series carries everything from v2 which was not already applied with v1 and still qualifies as a fix: the two standalone patches v2 had grown following the Sashiko AI reviewers' findings on v1, and, as patches of their own, the companion fixes v2 added inside the already applied patches:
the unchecked bus->read() in core_rmw() and the unchecked PHY_IAC command writes in the MT7531 indirect PHY access functions.

The remaining non-fix changes from v2, dropping a redundant read-back and improving the poll failure messages, will follow via net-next.

Let me ask explicitly this time -- are you planing to address any of the Clashiko nit picks and send a v2? :)

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/c41c256a5b4695bec24abc6baa4f990fca88f834.1785427248.git.daniel@makrotopia.org