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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Using devres (devm_) memory allocation and registration inside the DSA .setup callback causes a memory leak and duplicate sysfs name failure upon dynamic DSA tree teardown and setup.
- [Medium] The Kconfig entry fails to select REGMAP_MMIO and depends on HAS_IOMEM, leading to runtime failures.
--

commit 43985d7959e86c4777c7631d696ad0d94372a424
Author: Joris Vaisvila <joey@tinyisr.com>

net: dsa: initial support for MT7628 embedded switch

Add support for the MT7628 embedded switch. The switch has 5 built-in 100Mbps
user ports and one 1Gbps CPU port. It uses a 16 entry VLAN table for
forwarding.

> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 4ab567c5bbafc..daa1d3d4ba609 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -63,6 +63,14 @@ config NET_DSA_MT7530_MMIO
>  	  are directly mapped into the SoCs register space rather than being
>  	  accessible via MDIO.
>  
> +config NET_DSA_MT7628
> +	tristate "MediaTek MT7628 Embedded Ethernet switch support"
> +	select NET_DSA_TAG_MT7628
> +	select MEDIATEK_FE_SOC_PHY

[Severity: Medium]
Does this Kconfig entry need to select REGMAP_MMIO and depend on HAS_IOMEM?

The driver uses devm_regmap_init_mmio(), which requires CONFIG_REGMAP_MMIO
to be enabled. If it is disabled, the stub function will return -EINVAL and
cause the driver probe to fail unconditionally.

> +	help
> +	  This enables support for the built-in Ethernet switch found
> +	  in the MT7628 SoC.
> +

[ ... ]

> diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> new file mode 100644
> index 0000000000000..cedf063ad749c
> --- /dev/null
> +++ b/drivers/net/dsa/mt7628.c

[ ... ]

> +static int mt7628_setup_internal_mdio(struct dsa_switch *ds)
> +{
> +	struct mt7628_esw *esw = ds->priv;
> +	struct device *dev = ds->dev;
> +	struct mii_bus *bus;
> +
> +	bus = devm_mdiobus_alloc(dev);
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->name = "MT7628 internal MDIO bus";
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(dev));
> +	bus->priv = esw;
> +	bus->read = mt7628_mii_read;
> +	bus->write = mt7628_mii_write;
> +	bus->parent = dev;
> +
> +	ds->user_mii_bus = bus;
> +	bus->phy_mask = ~ds->phys_mii_mask;
> +
> +	return devm_mdiobus_register(dev, bus);
> +}

[Severity: High]
Can this memory leak and fail on dynamic DSA tree setups?

Since mt7628_setup_internal_mdio() is called from the DSA .setup callback
via mt7628_setup(), it can be invoked multiple times during the device's
lifetime (for example, if the conduit or master network device is unbound
and later rebound).

Because the MDIO bus is allocated and registered using the devm_ variants
tied to the physical device, won't the bus remain allocated after .teardown()
runs? If .setup() is called again, this would leak the previous allocation
and cause devm_mdiobus_register() to fail with a sysfs name conflict.

Should the MDIO bus be allocated in the probe function instead, or use the
non-devm variants here with explicit cleanup in .teardown()?

[ ... ]

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