Re: [PATCH v3 4/7] net: dsa: mediatek: support PLL setup on MMIO MT7530

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: dsa: mt7530: missing child_bus mdio_lock in core_write() and core_rmw()
--

commit 282f0d4460f738148996f33bfc68b0cd30432c06
Author: Caleb James DeLisle <cjd@cjdns.fr>

net: dsa: mediatek: support PLL setup on MMIO MT7530

This commit stores the switch's internal MDIO bus in priv->child_bus during
mt7530_setup_mdio() and updates core_write() and core_rmw() to fall back to
priv->child_bus when priv->bus is NULL so that MMIO MT7530 switches can
configure core PLL registers via PHY MMD accesses.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3eb2e11d69856..d09ff207e2dfc 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -77,6 +77,12 @@ core_write(struct mt7530_priv *priv, u32 reg, u32 val)
>  	struct mii_bus *bus = priv->bus;
>  	int ret;
>  
> +	if (!bus)
> +		bus = priv->child_bus;
> +
> +	if (WARN_ON_ONCE(!bus))
> +		return;
> +
>  	mt7530_mutex_lock(priv);
>  
>  	/* Write the desired MMD Devad */
> @@ -112,6 +118,12 @@ core_rmw(struct mt7530_priv *priv, u32 reg, u32 mask, u32 set)
>  	u32 val;
>  	int ret;
>  
> +	if (!bus)
> +		bus = priv->child_bus;
> +
> +	if (WARN_ON_ONCE(!bus))
> +		return;
> +
>  	mt7530_mutex_lock(priv);
>  
>  	/* Write the desired MMD Devad */

[Severity: Medium]
When priv->bus is NULL on an MMIO MT7530 switch and core_write() or
core_rmw() falls back to priv->child_bus, does mt7530_mutex_lock() leave
bus->mdio_lock unlocked across the multi-step MMD sequence?

In drivers/net/dsa/mt7530.c, mt7530_mutex_lock() and mt7530_mutex_unlock()
only check priv->bus:

static void
mt7530_mutex_lock(struct mt7530_priv *priv)
{
	if (priv->bus)
		mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
}

static void
mt7530_mutex_unlock(struct mt7530_priv *priv)
{
	if (priv->bus)
		mutex_unlock(&priv->bus->mdio_lock);
}

Both core_write() and core_rmw() perform a multi-transfer Clause 22 MMD
sequence (writing MII_MMD_CTRL, MII_MMD_DATA, MII_MMD_CTRL with
MII_MMD_CTRL_NOINCR, and reading or writing MII_MMD_DATA) by invoking
bus->write() and bus->read() directly.

Although each individual C22 transfer on priv->child_bus acquires and
releases priv->reg_mutex inside mt7531_ind_c22_phy_read() and
mt7531_ind_c22_phy_write(), priv->child_bus->mdio_lock is never held across
the whole MMD sequence when priv->bus is NULL. When core_write() or
core_rmw() is called at runtime (for example via
mt753x_phylink_mac_config() -> mt7530_mac_config() -> mt7530_setup_port6()
after mt7530_setup_mdio() has registered priv->child_bus), can concurrent
PHY polling or MDIO operations on priv->child_bus interleave between the
MII_MMD_CTRL and MII_MMD_DATA transfers or race with the read-modify-write
in core_rmw()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028011317.1557238-1-cjd@cjdns.fr?part=4