Re: [PATCH net-next v4 4/4] net: dsa: initial support for MT7628 embedded switch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Using devm_mdiobus_alloc() and devm_mdiobus_register() within the DSA .setup() callback causes memory leaks and sysfs naming collisions on device unbind/rebind.
--

commit 3622b1e586d001c3ba3f067ff8677605bcde34cc
Author: Joris Vaisvila <joey@tinyisr.com>

net: dsa: initial support for MT7628 embedded switch

This commit adds support for the MT7628 embedded switch, which includes 5 user
ports and 1 CPU port. It uses a 16-entry VLAN table for port isolation and
configures a double tag feature to simulate VLAN-unaware functionality.

> diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> new file mode 100644
> index 0000000000000..a2338f02f78a0
> --- /dev/null
> +++ b/drivers/net/dsa/mt7628.c
> @@ -0,0 +1,648 @@
[ ... ]
> +static int mt7628_setup_internal_mdio(struct dsa_switch *ds)
> +{
> +	struct mt7628_esw *esw = ds->priv;
> +	struct mii_bus *bus;
> +
> +	bus = devm_mdiobus_alloc(esw->dev);
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->name = "MT7628 internal MDIO bus";
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(esw->dev));
> +	bus->priv = esw;
> +	bus->read = mt7628_mii_read;
> +	bus->write = mt7628_mii_write;
> +	bus->parent = esw->dev;
> +
> +	ds->user_mii_bus = bus;
> +	bus->phy_mask = ~ds->phys_mii_mask;
> +
> +	return devm_mdiobus_register(esw->dev, bus);

[Severity: High]
Is it safe to use devm_mdiobus_alloc() and devm_mdiobus_register() tied to
the platform device here?

Since mt7628_setup_internal_mdio() is called from the DSA .setup() callback
below, it can be executed multiple times without the platform device unbinding
(for instance, when the conduit interface unbinds and rebinds).

Because the devm_ functions tie the MDIO bus allocation and registration to
the platform device, the bus is never unregistered or freed during the DSA
.teardown() phase.

When the tree is re-setup, won't it attempt to register a new MDIO bus with
the same ID, causing a sysfs name collision and resource leak?

[ ... ]
> +static int mt7628_setup(struct dsa_switch *ds)
> +{
> +	struct mt7628_esw *esw = ds->priv;
> +	int ret;
[ ... ]
> +	mt7628_switch_init(ds);
> +
> +	ret = mt7628_setup_internal_mdio(ds);
> +	if (ret)
> +		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608192948.289745-1-joey@tinyisr.com?part=4