Re: [PATCH v2 net-next 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 5 potential issue(s) to consider:
- [Critical] dsa: mt7530: hardware access while in reset causes bus hangs
- [High] dsa: mt7530: PHYs are permanently masked out due to early MDIO setup
- [High] dsa: mt7530: PHY interrupts are broken due to late IRQ assignment
- [Medium] dsa: mt7530: resource leak and silent failure in mt7988_probe()
- [Medium] dsa: mt7530: data race on static idx in unlocked mt7988_probe()
--

commit b6de045bf619c20c22824d1e2470474ed02da7d7
Author: Caleb James DeLisle <cjd@cjdns.fr>
Subject: net: dsa: mediatek: support MDIO switch downstream of MMIO switch

The MT7530 / MT7531 can connect via either MDIO or MMIO. In MMIO mode the
switch provides its own MDIO bus which connects to its integrated PHYs. This
commit updates mt7530_setup_mdio() to be called from mt7988_probe() to allow
downstream switches to register.

> 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: Critical]
Does calling mt7530_setup_mdio() here access the hardware while the chip is
still held in reset?

If the DSA tree is incomplete, dsa_register_switch() returns 0 without
executing the switch's setup callback, which is normally responsible for
deasserting the hardware reset.

When mt7530_setup_mdio() registers the MDIO bus, it immediately triggers
MDIO read/write operations to probe PHYs. Can this cause invalid data reads
or fatal AXI bus hangs if the chip is still in reset?

[Severity: Medium]
Also, if mt7530_setup_mdio() returns an error (such as -ENOMEM or -ENODEV),
this unconditionally ignores the return value.

Does this allow the driver to bind successfully even if MDIO setup fails, and
leak the successfully registered DSA switch (priv->ds) since
dsa_unregister_switch() is not called on the error path?

> +
> +	return 0;
>  }
>  
>  static void mt7988_remove(struct platform_device *pdev)
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index b152b73315d50..fe93be1ece7d7 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
[ ... ]
> @@ -2414,6 +2414,10 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
>  	static int idx;
>  	int ret = 0;

[Severity: Medium]
Before this patch, mt7530_setup_mdio() was exclusively called from
mt753x_setup(), which is serialized by the DSA subsystem's dsa2_mutex.

With the new call path in mt7988_probe() executing concurrently outside of
dsa2_mutex, can multiple switches probing concurrently race on the lockless
idx++ increment?

Could this lead to duplicate MDIO bus IDs and sysfs registration failures
here?

mt7530.c:mt7530_setup_mdio() {
    ...
    snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d", idx++);
    ...
}

> +	/* Already done */
> +	if (priv->child_bus)
> +		return 0;
> +
>  	mnp = of_get_child_by_name(np, "mdio");
>  
>  	if (mnp && !of_device_is_available(mnp))
> @@ -2438,9 +2442,6 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
>  	bus->parent = dev;
>  	bus->phy_mask = ~ds->phys_mii_mask;

[Severity: High]
Since mt7988_probe() calls mt7530_setup_mdio() before the DSA tree is
completed, is ds->phys_mii_mask initialized at this point?

ds->phys_mii_mask is initialized by dsa_switch_setup() only after the tree
completes. Does this mean ds->phys_mii_mask is 0 here, causing bus->phy_mask
to be ~0?

If there are PHYs relying on auto-probing, this mask would cause the MDIO
core to skip probing them entirely. When the tree eventually completes,
mt7530_setup_mdio() will return early due to priv->child_bus already being
set, potentially leaving those PHYs permanently unprobed.

> -	if (priv->irq_domain && !mnp)
> -		mt7530_setup_mdio_irq(priv);
> -
>  	priv->child_bus = bus;
>  
>  	ret = devm_of_mdiobus_register(dev, bus, mnp);
[ ... ]
> @@ -3285,6 +3287,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 the mt7530_setup_mdio_irq() call here populate the IRQs too late?

mt7530_setup_mdio() invokes devm_of_mdiobus_register(), which scans the MDIO
bus and creates PHY devices. During creation, the PHY framework copies
bus->irq[addr] into phydev->irq.

Since bus->irq is populated by mt7530_setup_mdio_irq() only after the PHYs
are already instantiated, won't the PHYs receive an initial value of 0
(PHY_POLL), silently breaking interrupt-driven link events?

>  	/* Initialise the PCS devices */
>  	for (i = 0; i < priv->ds->num_ports; i++) {
>  		priv->pcs[i].pcs.ops = priv->info->pcs_ops;

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