Re: [PATCH net-next v20 08/10] net: dsa: mt7530: move MDIO bus locking into regmap

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Recursive mutex deadlock in `mt7530_fdb_cmd`, `mt7530_vlan_cmd`, and `mt7530_irq_mask_sync` due to unremoved manual locks.
- [High] Mismatched lock acquisition and release, plus recursive self-deadlock, in MT7531 indirect PHY access functions.
--

--- Patch [8]: [PATCH net-next v20 08/10] net: dsa: mt7530: move MDIO bus locking into regmap ---
commit 93dc44aef1eb650f9b327c234ee71f5777a75b62
Author: Daniel Golle <[email protected]>

net: dsa: mt7530: move MDIO bus locking into regmap

This patch moves MDIO bus locking into the switch register regmap by
configuring the .lock and .unlock callbacks. It removes several manual locking
wrappers, relying on the regmap to protect register accesses automatically.
It also attempts to switch MT7531 indirect PHY accesses to use reg_mutex.

> --- a/drivers/net/dsa/mt7530-mdio.c
> +++ b/drivers/net/dsa/mt7530-mdio.c
> @@ -148,12 +148,14 @@ static const struct regmap_config regmap_config = {
>  	.val_bits = 32,
>  	.reg_stride = 4,
>  	.max_register = MT7530_CREV,
> -	.disable_locking = true,
> +	.lock = mt7530_mdio_regmap_lock,
> +	.unlock = mt7530_mdio_regmap_unlock,
>  };

[Severity: High]
Will adding the lock to the regmap configuration cause a recursive deadlock
in other functions that still hold the manual lock?

For example, in mt7530_fdb_cmd(), the manual lock is still held across a
regmap_read_poll_timeout() call:

mt7530_fdb_cmd() {
	mt7530_mutex_lock(priv);

	ret = regmap_read_poll_timeout(priv->regmap, MT7530_ATC, val,
				       !(val & ATC_BUSY), 20, 20000);
	...
}

Since mt7530_mutex_lock() takes the non-recursive mdio_lock, and the regmap
now automatically attempts to take the same lock, does this code self-deadlock?

It looks like mt7530_vlan_cmd() and mt7530_irq_mask_sync() also still hold
mt7530_mutex_lock() while calling regmap operations.

> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
[ ... ]
> @@ -597,7 +577,7 @@ mt7531_ind_c45_phy_read(struct mt7530_priv *priv, int port, int devad,
>  
>  	ret = val & MT7531_MDIO_RW_DATA_MASK;
>  out:
> -	mt7530_mutex_unlock(priv);
> +	mutex_unlock(&priv->reg_mutex);
>  
>  	return ret;
>  }

[Severity: High]
Does this leave the locks in an unbalanced state?

The entry path of mt7531_ind_c45_phy_read() still calls
mt7530_mutex_lock(priv), which takes the mdio_lock:

mt7531_ind_c45_phy_read() {
	mt7530_mutex_lock(priv);
	...
	ret = regmap_read_poll_timeout(...)
	...
}

But the exit path now releases a completely different lock (reg_mutex),
which wasn't previously held.

This pattern also seems to be present in mt7531_ind_c45_phy_write(),
mt7531_ind_c22_phy_read(), and mt7531_ind_c22_phy_write(), where the
semantic patch updated the unlock label but missed replacing the entry lock.

Additionally, since the entry path still takes the manual mdio_lock, do the
subsequent regmap calls inside these functions also trigger the same
recursive deadlock mentioned earlier?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8