[PATCH net 0/4] net: dsa: mt7530: fix remaining swallowed MDIO access errors

[PATCH net 0/4] net: dsa: mt7530: fix remaining swallowed MDIO access errors

[Daniel Golle]

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/cover.1785213071.git.daniel@makrotopia.org/
[2] https://lore.kernel.org/netdev/cover.1785368701.git.daniel@makrotopia.org/

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

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

[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 <daniel@makrotopia.org>
---
 drivers/net/pcs/pcs-mtk-lynxi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c
index a753bd88cbc2..454f2924831c 100644
--- a/drivers/net/pcs/pcs-mtk-lynxi.c
+++ b/drivers/net/pcs/pcs-mtk-lynxi.c
@@ -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),
-- 
2.55.0

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

[Andrew Lunn]

On Thu, Jul 30, 2026 at 05:03:11PM +0100, Daniel Golle wrote:
> 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 <daniel@makrotopia.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[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 <daniel@makrotopia.org>
---
 drivers/net/dsa/mt7530.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index aa33d94e11b5..9be6fb63c311 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -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 */
-- 
2.55.0

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

[Andrew Lunn]

On Thu, Jul 30, 2026 at 05:03:17PM +0100, Daniel Golle wrote:
> 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 <daniel@makrotopia.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[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 <daniel@makrotopia.org>
---
 drivers/net/dsa/mt7530.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 9be6fb63c311..2a17696e6a45 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -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);
-- 
2.55.0

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

[Andrew Lunn]

On Thu, Jul 30, 2026 at 05:03:29PM +0100, Daniel Golle wrote:
> 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 <daniel@makrotopia.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[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 <daniel@makrotopia.org>
---
 drivers/net/dsa/mt7530.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 2a17696e6a45..f8f0799ede63 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -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;
-- 
2.55.0

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

[Andrew Lunn]

On Thu, Jul 30, 2026 at 05:03:47PM +0100, Daniel Golle wrote:
> 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 <daniel@makrotopia.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew