Re: [PATCH v3 5/7] net: dsa: mediatek: support MDIO switch downstream of MMIO switch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] net: dsa: mt7530: broken PHY IRQs due to late mt7530_setup_mdio_irq()
- [High] net: dsa: mt7530: premature mt7530_setup_mdio() call in mt7988_probe()
- [High] net: dsa: mt7530: recursive reg_mutex lockdep class on downstream switch
- [Medium] net: dsa: mt7530: unchecked mt7530_setup_mdio() error in mt7988_probe()
--

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

net: dsa: mediatek: support MDIO switch downstream of MMIO switch

This commit exports mt7530_setup_mdio(), makes it idempotent across repeated
calls, and invokes it from mt7988_probe() after dsa_register_switch() so that
a downstream switch attached to the MMIO switch's MDIO bus can probe and
complete the DSA switch tree.

> diff --git a/drivers/net/dsa/mt7530-mmio.c b/drivers/net/dsa/mt7530-mmio.c
> index cf3bdb23a2d54..f387190a95391 100644
> --- a/drivers/net/dsa/mt7530-mmio.c
> +++ b/drivers/net/dsa/mt7530-mmio.c
> @@ -61,7 +61,14 @@ mt7988_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->regmap))
>  		return PTR_ERR(priv->regmap);
>  
> -	return dsa_register_switch(priv->ds);
> +	ret = dsa_register_switch(priv->ds);
> +
> +	if (ret)
> +		return ret;
> +
> +	mt7530_setup_mdio(priv);

[Severity: High]
When the DSA switch tree is not yet complete during mt7988_probe(),
dsa_register_switch() returns 0 before dsa_switch_setup() and mt753x_setup()
have run on the upstream MMIO switch. Can calling mt7530_setup_mdio(priv) at
this point register and scan the MDIO bus before the hardware and DSA switch
state are initialized?

First, in mt7988_setup() (called later from mt753x_setup() once the tree is
complete), the driver pulses priv->rstc, configures AN7583 clocks/AFE if
applicable, and resets the internal PHYs:

    reset_control_assert(priv->rstc);
    usleep_range(20, 50);
    reset_control_deassert(priv->rstc);
    usleep_range(20, 50);
    ...
    regmap_write(priv->regmap, MT7530_SYS_CTRL, SYS_CTRL_PHY_RST);

If mt7530_setup_mdio(priv) runs before mt7988_setup(), MDIO bus accesses over
MT7531_PHY_IAC occur before reset deassertion, and when the downstream switch
finishes probing and triggers dsa_tree_setup_switches(), mt7988_setup() will
reset the upstream switch and any PHYs that were already probed on its MDIO
bus.

Second, ds->phys_mii_mask is only populated during dsa_switch_setup():

    ds->phys_mii_mask |= dsa_user_ports(ds);

When mt7530_setup_mdio(priv) runs before dsa_switch_setup(), ds->phys_mii_mask
is still 0, so bus->phy_mask is set to ~0U (masking all PHY addresses when
there is no "mdio" child node) and priv->child_bus is set, causing the
subsequent mt7530_setup_mdio() call in mt753x_setup() to return early.

Third, mt7530_setup_irq(priv) has not run yet, so priv->irq_domain is still
NULL when devm_of_mdiobus_register() parses interrupt properties for child
nodes on the MDIO bus.

[Severity: Medium]
Also in mt7988_probe(), if mt7530_setup_mdio(priv) fails (for example,
with -ENOMEM from devm_mdiobus_alloc() or -EPROBE_DEFER from
devm_of_mdiobus_register()), isn't the error return value ignored here?

Since priv->child_bus is reset to NULL on failure and mt7988_probe() returns
0 without unwinding dsa_register_switch(priv->ds), deferred probing will not
be retried and the upstream switch will remain registered in an incomplete
DSA tree that can never finish setup.

[Severity: High]
Furthermore, when an MDIO-based MT7530/MT7531 switch is connected downstream
of an MMIO-based switch's MDIO bus, will lockdep report a recursive locking
violation on priv->reg_mutex?

Both the upstream MMIO switch (priv_up) and the downstream MDIO switch
(priv_down) initialize priv->reg_mutex at the same mutex_init(&priv->reg_mutex)
call site in mt7530_probe_common(), giving both mutexes the same lockdep
class.

When functions on the downstream switch hold priv_down->reg_mutex across a
regmap operation, such as in mt7530_port_disable():

    mutex_lock(&priv->reg_mutex);
    priv->ports[port].enable = false;
    regmap_update_bits(priv->regmap, MT7530_PCR_P(port), PCR_MATRIX_MASK,
                       PCR_MATRIX_CLR);
    mutex_unlock(&priv->reg_mutex);

the regmap access on priv_down locks the parent MDIO bus via
mt7530_mdio_regmap_lock() (acquiring priv_up->child_bus->mdio_lock at
MDIO_MUTEX_NESTED) and then invokes priv_up->child_bus->write
(mt753x_phy_write_c22() -> mt7531_ind_c22_phy_write()), which calls
mutex_lock(&priv_up->reg_mutex) at subclass 0 while priv_down->reg_mutex is
already held at subclass 0.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index d09ff207e2dfc..dbe3887097dbc 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
[ ... ]
> @@ -2481,9 +2485,6 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
>  	bus->parent = dev;
>  	bus->phy_mask = ~ds->phys_mii_mask;
>  
> -	if (priv->irq_domain && !mnp)
> -		mt7530_setup_mdio_irq(priv);
> -
>  	priv->child_bus = bus;
>  
>  	ret = devm_of_mdiobus_register(dev, bus, mnp);
> @@ -2498,6 +2499,7 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
>  	of_node_put(mnp);
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(mt7530_setup_mdio);
>  
>  static int
>  mt7530_setup(struct dsa_switch *ds)
> @@ -3328,6 +3330,9 @@ mt753x_setup(struct dsa_switch *ds)
>  	if (ret)
>  		return ret;
>  
> +	if (priv->irq_domain && ds->user_mii_bus)
> +		mt7530_setup_mdio_irq(priv);

[Severity: High]
Does moving mt7530_setup_mdio_irq(priv) after mt7530_setup_mdio(priv) break
PHY interrupts when there is no "mdio" Device Tree child node (mnp == NULL)?

In mt7530_setup_mdio(), devm_mdiobus_alloc() initializes all bus->irq[i]
entries to PHY_POLL, and devm_of_mdiobus_register(dev, bus, NULL) immediately
scans the bus and instantiates each struct phy_device via:

    __mdiobus_register()
      -> mdiobus_scan_bus_c22()
        -> mdiobus_scan()
          -> get_phy_device()
            -> phy_device_create()

Inside phy_device_create(), the PHY's interrupt is copied from the bus array
at creation time:

    dev->irq = bus->irq[addr];

Because mt7530_setup_mdio_irq(priv) now runs only after
devm_of_mdiobus_register() has completed, bus->irq[p] is still PHY_POLL when
phy_device_create() runs, so all PHYs on ds->user_mii_bus are registered in
polling mode and the IRQ mappings later stored into ds->user_mii_bus->irq[p]
by mt7530_setup_mdio_irq() are never propagated to phydev->irq.

In addition, the error path of devm_of_mdiobus_register() in
mt7530_setup_mdio() still calls mt7530_free_mdio_irq(priv):

    ret = devm_of_mdiobus_register(dev, bus, mnp);
    if (ret) {
        priv->child_bus = NULL;
        dev_err(dev, "failed to register MDIO bus: %d\n", ret);
        if (priv->irq_domain && !mnp)
            mt7530_free_mdio_irq(priv);
    }

even though mt7530_setup_mdio_irq(priv) has not been called yet at that point.

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