Re: [PATCH v4 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 3f6621fc167b9cbb00feef9399db3022a584cb8e
Author: Caleb James DeLisle <cjd@cjdns.fr>

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

This commit records 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 MMIO MT7530 instances can perform
PLL and core register configuration via the internal PHY MDIO bus.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index c5b3b00acb956..d45388e2efc83 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 and core_write() or core_rmw() falls back to
priv->child_bus, does mt7530_mutex_lock() leave bus->mdio_lock unlocked?

Looking at mt7530_mutex_lock() and mt7530_mutex_unlock() in
drivers/net/dsa/mt7530.c:

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);
}

On MMIO switches where priv->bus is NULL, mt7530_mutex_lock() becomes a
no-op. However, core_write() and core_rmw() perform multi-step Clause 22
MMD indirect sequences across MII_MMD_CTRL and MII_MMD_DATA using raw
bus->write() and bus->read() calls.

Even though the underlying indirect PHY accessors acquire priv->reg_mutex for
each individual Clause 22 register transfer, priv->reg_mutex is released
between each step of the MII_MMD_CTRL and MII_MMD_DATA sequence. Once
mt7530_setup_mdio() registers priv->child_bus, runtime callers such as
mt753x_phylink_mac_config() -> mt7530_mac_config() -> mt7530_setup_port6()
call core_clear(), core_write(), and core_set() (which use core_rmw() and
core_write()). Could concurrent phylib or MDIO accesses on priv->child_bus
(which serialize on priv->child_bus->mdio_lock) interleave between those
steps and corrupt the MMD register accesses?

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