Re: [PATCH 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 4 potential issue(s) to consider:
- [High] Missing dsa_unregister_switch() on the error path in mt7988_probe() leads to a Use-After-Free in the DSA core.
- [High] Race condition between mt7988_probe() and mt753x_setup() leading to double initialization of the MDIO bus.
- [High] PHY IRQ setup is skipped when mt7530_setup_mdio() is called from mt7988_probe() and the DSA tree is initially incomplete.
- [Medium] Data race on the static variable idx when allocating MDIO bus IDs in mt7530_setup_mdio().
--

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

net: dsa: mediatek: support MDIO switch downstream of MMIO switch
    
This commit updates mt7530_setup_mdio() so it can be called multiple
times and exports it. It then calls mt7530_setup_mdio() at the end
of mt7988_probe() to allow a downstream MDIO switch to register
when the upstream switch connects via MMIO.

> diff --git a/drivers/net/dsa/mt7530-mmio.c b/drivers/net/dsa/mt7530-mmio.c
> index cf3bdb23a2d54..29fbd0fc50d1a 100644
> --- a/drivers/net/dsa/mt7530-mmio.c
> +++ b/drivers/net/dsa/mt7530-mmio.c
> @@ -61,7 +61,12 @@ 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;
> +
> +	return mt7530_setup_mdio(priv);

[Severity: High]
Does this code leak the registered DSA switch on the error path?

If mt7530_setup_mdio() fails, the probe function returns the error directly
without calling dsa_unregister_switch(priv->ds). 

This can cause devres cleanup to free the priv->ds structure while the DSA
subsystem still holds pointers to it, potentially leading to a use-after-free
if the DSA core attempts to access the partially registered switch later.

>  }
>  
>  static void mt7988_remove(struct platform_device *pdev)

[ ... ]

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index a165b4c8ed8d7..0c0c5358b8b80 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;

[Severity: Medium]
Does this code have a data race on idx when allocating MDIO bus IDs?

mt7988_probe() now calls mt7530_setup_mdio() directly without holding the
dsa2_mutex. 

If multiple MMIO MT7530-compatible switches are probed concurrently, they
could execute the unlocked idx++ at the same time, potentially generating
duplicate MDIO bus IDs and causing sysfs registration to fail.

>  	int ret = 0;
>  
> +	/* Already done */
> +	if (priv->child_bus)
> +		return 0;

[Severity: High]
Can concurrent callers bypass this check and execute the MDIO setup logic
twice?

mt7988_probe() now calls mt7530_setup_mdio() outside of any lock. If a
sibling switch completes the DSA tree concurrently, dsa_tree_setup() can
call mt753x_setup() under the dsa2_mutex, which also calls
mt7530_setup_mdio(). 

Both threads could simultaneously observe priv->child_bus == NULL, leading
to corrupted pointer states and duplicate MDIO bus registrations.


[Severity: High]
Will this early return permanently skip PHY IRQ setup if the DSA tree is
initially incomplete?

If mt7988_probe() calls mt7530_setup_mdio() while the DSA tree is incomplete,
mt753x_setup() will not have run yet, so priv->irq_domain will be NULL and
IRQ setup is skipped.

Later, when the tree completes and mt753x_setup() is executed, it will set up
the irq_domain and call mt7530_setup_mdio() again. 

However, since priv->child_bus is already set from the earlier probe call,
this check will return 0 immediately, and mt7530_setup_mdio_irq(priv) will
never be executed for those ports.

> +
>  	mnp = of_get_child_by_name(np, "mdio");
>  
>  	if (mnp && !of_device_is_available(mnp))

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