net: dsa: mt7530: fix swallowed MDIO read errors
Subsystems (7)
Daniel Golle
maintainerv2 cover net: dsa: mt7530: fix swallowed MDIO read errors
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/[email protected]/
[2] https://lore.kernel.org/netdev/[email protected]/
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
Signature · 1 line
2.55.0
Daniel Golle
maintainerPATCH v2 1/5 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.
---
v2: new patch
drivers/net/pcs/pcs-mtk-lynxi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/net/pcs/pcs-mtk-lynxi.c +3 -2
@@ -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
Re: [PATCH net v2 1/5] 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]>
Andrew
Daniel Golle
maintainerPATCH v2 2/5 net: dsa: mt7530: check bus->read() errors in the MDIO regmap backend
In reply to Daniel Gollebus->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.
---
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(-)
drivers/net/dsa/mt7530-mdio.c +9 -2
@@ -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 );
drivers/net/dsa/mt7530.c +5 -1
@@ -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
Daniel Golle
maintainerPATCH v2 3/5 net: dsa: mt7530: error out on failed reads in ATC/VTCR command polling
In reply to Daniel Gollemt7530_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.
---
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(-)
drivers/net/dsa/mt7530.c +16 -12
@@ -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 ;
Signature · 1 line
2.55.0
Daniel Golle
maintainerPATCH v2 4/5 net: dsa: mt7530: error out on failed reads in MT7531 PHY polling
In reply to Daniel GolleThe 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.
---
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(-)
drivers/net/dsa/mt7530.c +50 -60
@@ -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);
Signature · 1 line
2.55.0
Daniel Golle
maintainerPATCH v2 5/5 net: dsa: mt7530: serialize the regmap IRQ chip like every other user
In reply to Daniel GolleThe 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.
---
v2: new patch
drivers/net/dsa/mt7530.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-)
drivers/net/dsa/mt7530.c +25 -1
@@ -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;
Signature · 1 line
2.55.0
Re: [PATCH net v2 5/5] 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 todevm_regmap_add_irq_chip_fwnode(), whose threaded IRQ handler then calls regmap_read()/regmap_update_bits() on it without ever callingmt7530_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_synchook 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 ofmt7530_regmap_irq_chip, sincedevm_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 reachphy_interrupt()for the per-port PHY IRQ lines mapped through this chip, which takesphydev->lock, whilephy_attach_direct()and this driver's own indirect PHY access already establish the opposite order (phydev->lock, thenpriv->bus->mdio_lock) elsewhere. Using them here would close that cycle, so they are not used.
regmap_irq_sync_unlock()also has its owninit_ack_maskedpath, used by this chip, which unconditionally does its ownregmap_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]>
Andrew
Jakub Kicinski
maintainerQuoted 3 lines · 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.
Signature · 1 line
pw-bot: cr