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

[Joris Vaisvila <joey@tinyisr.com>]

Hi, thank you for the review

On Fri, Aug 14, 2026 at 07:04:32PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Using `devm_mdiobus_alloc()` and `devm_mdiobus_register()` in the DSA `.setup()` callback leads to resource leaks and sysfs collisions on re-bind.
> - [High] The VLAN Untag Bitmap (VUB) register stride is incorrectly set to 7 instead of 8, which will write to the wrong hardware bits for VLANs 1, 2, and 3 within a register block.
> --
> 
> --- Patch [4]: [PATCH net-next v7 4/4] net: dsa: initial support for MT7628 embedded switch ---
> commit 94f2355ae398ce31cce3f01105a3f2af1b3c61d6
> 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 (ports 0-4) and one 1Gbps
> port that is internally attached to the SoCs CPU MAC and serves as the
> CPU port.
> 
> > diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> > new file mode 100644
> > index 0000000000000..fb63f6f644b9e
> > --- /dev/null
> > +++ b/drivers/net/dsa/mt7628.c
> 
> [ ... ]
> 
> > +#define MT7628_ESW_VMSC_PREP(vlan, vmsc) \
> > +	(((vmsc) & MT7628_ESW_VMSC_M) << MT7628_ESW_VMSC_SHIFT(vlan))
> > +
> > +#define MT7628_ESW_VUB_S 7
> > +#define MT7628_ESW_VUB_M GENMASK(6, 0)
> > +#define MT7628_ESW_VUB_SHIFT(vlan) \
> > +	(MT7628_ESW_VUB_S * ((vlan) % 4))
> > +#define MT7628_ESW_VUB_MASK(vlan) \
> 
> [Severity: High]
> Is the register stride for the VLAN Untag Bitmap (VUB) incorrectly set to 7
> instead of 8?
> 
> The MT7628 switch packs 4 VLAN configurations per 32-bit register. Using a
> 7-bit stride means the shift offsets will be 7, 14, and 21 for the upper
> VLANs in the register block, rather than the standard byte-aligned 8, 16,
> and 24.
> 
> Could this write the untagged port mask into the wrong hardware bits and
> corrupt the VLAN configuration?
> 
> [ ... ]
> 

This matches the datasheet and has been tested to work correctly on real
hardware.

As per the datasheet, VLAN_0_UNTAG_EN is bits 6-0, VLAN_1_UNTAG_EN is
bits 13-7, VLAN_2_UNTAG_EN is bits 20-14, VLAN_3_UNTAG_EN 27-21 and bits
31-28 are unused. The layout is the same for all of the VUB registers.

> > +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);
> 
> [Severity: High]
> Does allocating a devm-managed MDIO bus inside a DSA setup callback
> cause resource leaks or sysfs collisions on re-bind?
> 
> The mt7628_setup_internal_mdio() function is called from the main
> mt7628_setup() callback. This setup callback runs every time the master
> conduit device binds. If the conduit interface is unbound and bound again,
> the setup callback is invoked a second time.
> 
> Because the bus lifecycle is tied to the switch platform device via devm_,
> the old bus is not freed when the conduit unbinds. Will the second
> devm_mdiobus_register() attempt fail with a sysfs name collision (-EEXIST),
> preventing the switch from initializing?
> 

This can't happen. The switch device is unbound when the conduit device
is unbound. Switch setup is only called through probe, so it can only be
called once per device lifetime.

In net/dsa/conduit.c:428 a device link is set up for the switch to
depend on the conduit device:
```
	/* The DSA conduit must use SET_NETDEV_DEV for this to work. */
	if (!netif_is_lag_master(dev)) {
		consumer_link = device_link_add(ds->dev, dev->dev.parent,
						DL_FLAG_AUTOREMOVE_CONSUMER);
		if (!consumer_link)
			netdev_err(dev,
				   "Failed to create a device link to DSA switch %s\n",
				   dev_name(ds->dev));
	}
```

MT7628 does not support lag, so the device link is set up for this
device. This pattern is also used in other drivers, a few examples:
	- in NXP NETC priv->dev is set to the &pci_dev->dev, which is used
	  in devm_mdiobus_alloc_size().
	- in mt7530-mmio priv->dev is set to the &platform_device->dev,
	  which is then used in devm_mdiobus_alloc().

> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	bus->name = "MT7628 internal MDIO bus";
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260813190241.789323-1-joey@tinyisr.com?part=4

Thanks,
Joris