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

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 <[email protected]>

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?

[ ... ]

> +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?

> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->name = "MT7628 internal MDIO bus";

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4