[PATCH net v2 0/5] net: dsa: mt7530: fix swallowed MDIO read errors

[PATCH net v2 0/5] net: dsa: mt7530: fix swallowed MDIO read errors

[Daniel Golle]

While working on a register access cleanup for the mt7530 driver, the
Sashiko AI reviewers flagged long-standing error handling gaps in the
driver's read paths [1].

The MDIO regmap backend truncates negative bus->read() errnos into u16
register halves and returns success, handing garbage data to callers
and to read-modify-write cycles which then write it back to the switch.

The ATC/VTCR command polls and the MT7531 indirect PHY polls consume
reads through a helper which returns 0 when the underlying read fails.
A failed bus transaction thus clears the polled busy bit and is
mistaken for command completion, defeats the subsequent
ATC_INVALID/VTCR_INVALID checks the same way, and lets the indirect
PHY access functions return garbage PHY register data.

Fix the backend to propagate bus->read() errors, and convert the
command and PHY access polls to regmap_read_poll_timeout(), which
terminates polling on read errors and propagates them. The driver's
legacy read helpers return plain u32 values and cannot convey errors,
hence the affected reads switch to the plain regmap API the helpers
are built on, while all other accesses are deliberately left
untouched; the cleanup series converts the rest of the driver.

The Sashiko reviewers went over v1 as well [2] and found that the same
unchecked-read pattern recurs in a couple of other places, and that the
read-error propagation above widens a pre-existing gap in a PCS driver
consumer; patches 1-3 grew companion fixes for those. They also found a
separate locking gap between the switch regmap and its IRQ chip, fixed
in the new patch 4 with its own Fixes: tag.

The cleanup series depending on these fixes will be submitted to
net-next separately.

Changes since v1:
 * 1/4: also check the identical unchecked bus->read() in core_rmw(),
   and fix mtk_pcs_lynxi_get_state() (drivers/net/pcs/pcs-mtk-lynxi.c)
   to check regmap_read() now that a failed read can leave its output
   unwritten instead of holding truncated-but-defined garbage
 * 2/4: drop the redundant regmap_read() after
   regmap_read_poll_timeout(), and print ret in the error messages
 * 3/4: also check mt7530_mii_write()'s return for the PHY_IAC command
   word in all four PHY access helpers, and print ret in the poll
   error messages
 * add 4/4, calling mt7530_mutex_lock()/unlock() around the regmap IRQ
   chip's own accesses via handle_pre_irq/handle_post_irq/
   handle_mask_sync, the same driver-provided locking every other user
   of the unlocked switch regmap already goes through, instead of
   adding a second regmap instance

[1] https://lore.kernel.org/netdev/cover.1784481922.git.daniel@makrotopia.org/
[2] https://lore.kernel.org/netdev/cover.1785213071.git.daniel@makrotopia.org/

Daniel Golle (5):
  net: pcs: mtk-lynxi: check regmap reads in mtk_pcs_lynxi_get_state()
  net: dsa: mt7530: check bus->read() errors in the MDIO regmap backend
  net: dsa: mt7530: error out on failed reads in ATC/VTCR command
    polling
  net: dsa: mt7530: error out on failed reads in MT7531 PHY polling
  net: dsa: mt7530: serialize the regmap IRQ chip like every other user

 drivers/net/dsa/mt7530-mdio.c   |  11 ++-
 drivers/net/dsa/mt7530.c        | 170 ++++++++++++++++++--------------
 drivers/net/pcs/pcs-mtk-lynxi.c |   5 +-
 3 files changed, 108 insertions(+), 78 deletions(-)


base-commit: 51b093a7ba27476e1f639455f005e8d2e75390e4
prerequisite-patch-id: 0000000000000000000000000000000000000000
-- 
2.55.0

[PATCH net v2 1/5] 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>
---
v2: new patch

 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

[PATCH net v2 2/5] net: dsa: mt7530: check bus->read() errors in the MDIO regmap backend

[Daniel Golle]

bus->read() returns a negative errno on failure, but
mt7530_regmap_read() assigns it to a u16, truncating e.g. -ETIMEDOUT
into 0xff92, and returns success. The garbage word is then consumed as
register data, and read-modify-write cycles write it back to the
switch. Check both reads and propagate their errors.

The same defect existed in mt7530_mii_read() since the driver was
introduced and moved into the regmap backend unchanged. core_rmw(),
which accesses the MMD core registers directly rather than through the
regmap, has the identical unchecked bus->read(); fix it the same way.

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>
---
v2:
 * also check the identical unchecked bus->read() in core_rmw()
   (found by Sashiko AI review)
 * split the mtk_pcs_lynxi_get_state() fix into its own patch

 drivers/net/dsa/mt7530-mdio.c | 11 +++++++++--
 drivers/net/dsa/mt7530.c      |  6 +++++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mt7530-mdio.c b/drivers/net/dsa/mt7530-mdio.c
index 11ea924a9f35..784dd58a7158 100644
--- a/drivers/net/dsa/mt7530-mdio.c
+++ b/drivers/net/dsa/mt7530-mdio.c
@@ -55,8 +55,15 @@ mt7530_regmap_read(void *context, unsigned int reg, unsigned int *val)
 	if (ret < 0)
 		return ret;
 
-	lo = bus->read(bus, priv->mdiodev->addr, r);
-	hi = bus->read(bus, priv->mdiodev->addr, 0x10);
+	ret = bus->read(bus, priv->mdiodev->addr, r);
+	if (ret < 0)
+		return ret;
+	lo = ret;
+
+	ret = bus->read(bus, priv->mdiodev->addr, 0x10);
+	if (ret < 0)
+		return ret;
+	hi = ret;
 
 	*val = (hi << 16) | (lo & 0xffff);
 
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3c2a3029b10c..f349b54c2d9e 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

[PATCH net v2 3/5] net: dsa: mt7530: error out on failed reads in ATC/VTCR command polling

[Daniel Golle]

mt7530_fdb_cmd() and mt7530_vlan_cmd() poll the command register
through a helper which returns 0 when the underlying read fails. A
failed bus transaction thus clears ATC_BUSY/VTCR_BUSY and is treated
as successful command completion, and the subsequent ATC_INVALID and
VTCR_INVALID checks are defeated the same way.

Poll using regmap_read_poll_timeout(), which stops on read errors and
propagates them, and use the register value it already read back
instead of re-reading it. Take the MDIO bus lock across the sequence
as the switch regmap is set up with locking disabled.

Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Fixes: 83163f7dca56 ("net: dsa: mediatek: add VLAN support for MT7530")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
v2:
 * drop the extra regmap_read() after regmap_read_poll_timeout(); the
   macro already leaves the polled value in val (found by Sashiko AI
   review)
 * print ret in the error messages and stop mislabeling propagated bus
   errors as "reset timeout"/"poll timeout"

 drivers/net/dsa/mt7530.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index f349b54c2d9e..aab26970f39b 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -252,24 +252,26 @@ mt7530_fdb_cmd(struct mt7530_priv *priv, enum mt7530_fdb_cmd cmd, u32 *rsp)
 {
 	u32 val;
 	int ret;
-	struct mt7530_dummy_poll p;
 
 	/* Set the command operating upon the MAC address entries */
 	val = ATC_BUSY | ATC_MAT(0) | cmd;
 	mt7530_write(priv, MT7530_ATC, val);
 
-	INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_ATC);
-	ret = readx_poll_timeout(_mt7530_read, &p, val,
-				 !(val & ATC_BUSY), 20, 20000);
+	mt7530_mutex_lock(priv);
+
+	ret = regmap_read_poll_timeout(priv->regmap, MT7530_ATC, val,
+				       !(val & ATC_BUSY), 20, 20000);
+
+	mt7530_mutex_unlock(priv);
+
 	if (ret < 0) {
-		dev_err(priv->dev, "reset timeout\n");
+		dev_err(priv->dev, "ATC poll failed: %d\n", ret);
 		return ret;
 	}
 
 	/* Additional sanity for read command if the specified
 	 * entry is invalid
 	 */
-	val = mt7530_read(priv, MT7530_ATC);
 	if ((cmd == MT7530_FDB_READ) && (val & ATC_INVALID))
 		return -EINVAL;
 
@@ -1630,22 +1632,24 @@ mt7530_port_bridge_join(struct dsa_switch *ds, int port,
 static int
 mt7530_vlan_cmd(struct mt7530_priv *priv, enum mt7530_vlan_cmd cmd, u16 vid)
 {
-	struct mt7530_dummy_poll p;
 	u32 val;
 	int ret;
 
 	val = VTCR_BUSY | VTCR_FUNC(cmd) | vid;
 	mt7530_write(priv, MT7530_VTCR, val);
 
-	INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_VTCR);
-	ret = readx_poll_timeout(_mt7530_read, &p, val,
-				 !(val & VTCR_BUSY), 20, 20000);
+	mt7530_mutex_lock(priv);
+
+	ret = regmap_read_poll_timeout(priv->regmap, MT7530_VTCR, val,
+				       !(val & VTCR_BUSY), 20, 20000);
+
+	mt7530_mutex_unlock(priv);
+
 	if (ret < 0) {
-		dev_err(priv->dev, "poll timeout\n");
+		dev_err(priv->dev, "VTCR poll failed: %d\n", ret);
 		return ret;
 	}
 
-	val = mt7530_read(priv, MT7530_VTCR);
 	if (val & VTCR_INVALID) {
 		dev_err(priv->dev, "read VTCR invalid\n");
 		return -EINVAL;
-- 
2.55.0

[PATCH net v2 4/5] net: dsa: mt7530: error out on failed reads in MT7531 PHY polling

[Daniel Golle]

The MT7531 indirect PHY access functions poll MT7531_PHY_IAC through
a helper which returns 0 when the underlying read fails, so a failed
bus transaction clears MT7531_PHY_ACS_ST and the access carries on,
returning garbage PHY register data to phylib.

Poll using regmap_read_poll_timeout(), which stops on read errors and
propagates them. These functions hold the MDIO bus lock across the
whole sequence, so the unlocked regmap accesses remain correct. Remove
the now-unused _mt7530_unlocked_read().

MT7531_PHY_ACS_ST is only ever set by the command write that precedes
each poll, and that write's return value was discarded. A failed write
leaves ACS_ST at 0 from the previous access, so the hardened poll above
succeeds on its first iteration and the functions return stale IAC
contents as if they were fresh PHY data, exactly the failure this patch
otherwise closes. Check the write too 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>
---
v2:
 * also check mt7530_mii_write()'s return for the PHY_IAC command word
   in all four helpers; an unchecked failed write left ACS_ST clear
   and made the poll succeed instantly on stale data (found by
   Sashiko AI review)
 * print ret in the poll timeout messages instead of a fixed string,
   since ret can now be a propagated bus error rather than -ETIMEDOUT

 drivers/net/dsa/mt7530.c | 110 ++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 60 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index aab26970f39b..29b8b28e8d2c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -195,12 +195,6 @@ mt7530_write(struct mt7530_priv *priv, u32 reg, u32 val)
 	mt7530_mutex_unlock(priv);
 }
 
-static u32
-_mt7530_unlocked_read(struct mt7530_dummy_poll *p)
-{
-	return mt7530_mii_read(p->priv, p->reg);
-}
-
 static u32
 _mt7530_read(struct mt7530_dummy_poll *p)
 {
@@ -555,40 +549,41 @@ static int
 mt7531_ind_c45_phy_read(struct mt7530_priv *priv, int port, int devad,
 			int regnum)
 {
-	struct mt7530_dummy_poll p;
 	u32 reg, val;
 	int ret;
 
-	INIT_MT7530_DUMMY_POLL(&p, priv, MT7531_PHY_IAC);
-
 	mt7530_mutex_lock(priv);
 
-	ret = readx_poll_timeout(_mt7530_unlocked_read, &p, val,
-				 !(val & MT7531_PHY_ACS_ST), 20, 100000);
+	ret = regmap_read_poll_timeout(priv->regmap, MT7531_PHY_IAC, val,
+				       !(val & MT7531_PHY_ACS_ST), 20, 100000);
 	if (ret < 0) {
-		dev_err(priv->dev, "poll timeout\n");
+		dev_err(priv->dev, "PHY_IAC poll failed: %d\n", ret);
 		goto out;
 	}
 
 	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 = readx_poll_timeout(_mt7530_unlocked_read, &p, val,
-				 !(val & MT7531_PHY_ACS_ST), 20, 100000);
+	ret = regmap_read_poll_timeout(priv->regmap, MT7531_PHY_IAC, val,
+				       !(val & MT7531_PHY_ACS_ST), 20, 100000);
 	if (ret < 0) {
-		dev_err(priv->dev, "poll timeout\n");
+		dev_err(priv->dev, "PHY_IAC poll failed: %d\n", ret);
 		goto out;
 	}
 
 	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 = readx_poll_timeout(_mt7530_unlocked_read, &p, val,
-				 !(val & MT7531_PHY_ACS_ST), 20, 100000);
+	ret = regmap_read_poll_timeout(priv->regmap, MT7531_PHY_IAC, val,
+				       !(val & MT7531_PHY_ACS_ST), 20, 100000);
 	if (ret < 0) {
-		dev_err(priv->dev, "poll timeout\n");
+		dev_err(priv->dev, "PHY_IAC poll failed: %d\n", ret);
 		goto out;
 	}
 
@@ -603,42 +598,41 @@ static int
 mt7531_ind_c45_phy_write(struct mt7530_priv *priv, int port, int devad,
 			 int regnum, u16 data)
 {
-	struct mt7530_dummy_poll p;
 	u32 val, reg;
 	int ret;
 
-	INIT_MT7530_DUMMY_POLL(&p, priv, MT7531_PHY_IAC);
-
 	mt7530_mutex_lock(priv);
 
-	ret = readx_poll_timeout(_mt7530_unlocked_read, &p, val,
-				 !(val & MT7531_PHY_ACS_ST), 20, 100000);
+	ret = regmap_read_poll_timeout(priv->regmap, MT7531_PHY_IAC, val,
+				       !(val & MT7531_PHY_ACS_ST), 20, 100000);
 	if (ret < 0) {
-		dev_err(priv->dev, "poll timeout\n");
+		dev_err(priv->dev, "PHY_IAC poll failed: %d\n", ret);
 		goto out;
 	}
 
 	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 = readx_poll_timeout(_mt7530_unlocked_read, &p, val,
-				 !(val & MT7531_PHY_ACS_ST), 20, 100000);
+	ret = regmap_read_poll_timeout(priv->regmap, MT7531_PHY_IAC, val,
+				       !(val & MT7531_PHY_ACS_ST), 20, 100000);
 	if (ret < 0) {
-		dev_err(priv->dev, "poll timeout\n");
+		dev_err(priv->dev, "PHY_IAC poll failed: %d\n", ret);
 		goto out;
 	}
 
 	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 = readx_poll_timeout(_mt7530_unlocked_read, &p, val,
-				 !(val & MT7531_PHY_ACS_ST), 20, 100000);
-	if (ret < 0) {
-		dev_err(priv->dev, "poll timeout\n");
+	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);
+	if (ret < 0)
+		dev_err(priv->dev, "PHY_IAC poll failed: %d\n", ret);
 
 out:
 	mt7530_mutex_unlock(priv);
@@ -649,30 +643,29 @@ mt7531_ind_c45_phy_write(struct mt7530_priv *priv, int port, int devad,
 static int
 mt7531_ind_c22_phy_read(struct mt7530_priv *priv, int port, int regnum)
 {
-	struct mt7530_dummy_poll p;
 	int ret;
 	u32 val;
 
-	INIT_MT7530_DUMMY_POLL(&p, priv, MT7531_PHY_IAC);
-
 	mt7530_mutex_lock(priv);
 
-	ret = readx_poll_timeout(_mt7530_unlocked_read, &p, val,
-				 !(val & MT7531_PHY_ACS_ST), 20, 100000);
+	ret = regmap_read_poll_timeout(priv->regmap, MT7531_PHY_IAC, val,
+				       !(val & MT7531_PHY_ACS_ST), 20, 100000);
 	if (ret < 0) {
-		dev_err(priv->dev, "poll timeout\n");
+		dev_err(priv->dev, "PHY_IAC poll failed: %d\n", ret);
 		goto out;
 	}
 
 	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 = readx_poll_timeout(_mt7530_unlocked_read, &p, val,
-				 !(val & MT7531_PHY_ACS_ST), 20, 100000);
+	ret = regmap_read_poll_timeout(priv->regmap, MT7531_PHY_IAC, val,
+				       !(val & MT7531_PHY_ACS_ST), 20, 100000);
 	if (ret < 0) {
-		dev_err(priv->dev, "poll timeout\n");
+		dev_err(priv->dev, "PHY_IAC poll failed: %d\n", ret);
 		goto out;
 	}
 
@@ -687,32 +680,29 @@ static int
 mt7531_ind_c22_phy_write(struct mt7530_priv *priv, int port, int regnum,
 			 u16 data)
 {
-	struct mt7530_dummy_poll p;
 	int ret;
 	u32 reg;
 
-	INIT_MT7530_DUMMY_POLL(&p, priv, MT7531_PHY_IAC);
-
 	mt7530_mutex_lock(priv);
 
-	ret = readx_poll_timeout(_mt7530_unlocked_read, &p, reg,
-				 !(reg & MT7531_PHY_ACS_ST), 20, 100000);
+	ret = regmap_read_poll_timeout(priv->regmap, MT7531_PHY_IAC, reg,
+				       !(reg & MT7531_PHY_ACS_ST), 20, 100000);
 	if (ret < 0) {
-		dev_err(priv->dev, "poll timeout\n");
+		dev_err(priv->dev, "PHY_IAC poll failed: %d\n", ret);
 		goto out;
 	}
 
 	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 = readx_poll_timeout(_mt7530_unlocked_read, &p, reg,
-				 !(reg & MT7531_PHY_ACS_ST), 20, 100000);
-	if (ret < 0) {
-		dev_err(priv->dev, "poll timeout\n");
+	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);
+	if (ret < 0)
+		dev_err(priv->dev, "PHY_IAC poll failed: %d\n", ret);
 
 out:
 	mt7530_mutex_unlock(priv);
-- 
2.55.0

[PATCH net v2 5/5] 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>
---
v2: new patch

 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 29b8b28e8d2c..575eb90ce3f0 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2303,6 +2303,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,
@@ -2312,12 +2327,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;
@@ -2337,10 +2354,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 v2 0/5] net: dsa: mt7530: fix swallowed MDIO read errors

[Jakub Kicinski]

On Thu, 30 Jul 2026 01:14:30 +0100 Daniel Golle wrote:
> While working on a register access cleanup for the mt7530 driver, the
> Sashiko AI reviewers flagged long-standing error handling gaps in the
> driver's read paths [1].

I just applied v1, please send a rebased / incremental series.
-- 
pw-bot: cr