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

On Mon, Jul 20, 2026 at 04:52:19PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> 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.
> 
> > 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
> > +	help
> 
> [Severity: Low]
> Can this lead to a build or link failure on randconfig? It looks like this
> is missing a select REGMAP_MMIO and depends on HAS_IOMEM, which might cause
> devm_regmap_init_mmio() to be unresolved when CONFIG_REGMAP_MMIO is not set.

Will fix with v7.

> > diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> > new file mode 100644
> > index 0000000000000..eccdb05fce9b7
> > --- /dev/null
> > +++ b/drivers/net/dsa/mt7628.c
> > @@ -0,0 +1,649 @@
> > [ ... ]
> > +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]
> Does this cause a resource leak and sysfs name collision if the switch setup
> callback runs multiple times?
> 
> If the DSA master device unbinds and rebinds, dsa_tree_teardown() and
> dsa_tree_setup() can be triggered again. Because devm_mdiobus_alloc() and
> devm_mdiobus_register() tie the resource to the platform device's lifetime
> rather than the DSA tree setup/teardown cycle, the first registration will
> leak, and the second might fail with -EEXIST.

I am not able to find any code path that would trigger dsa_switch_ops
setup other than through `dsa_register_switch`, which is called in the
platform device probe. That would make it impossible for setup to be
called again without removing the platform device first, which tears
down the mdiobus.

This pattern is used in multiple other DSA drivers. A few examples:

	- in NXP NETC priv->dev is sset 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().
	
This seems like an AI review false positive. 

> > [ ... ]
> > +static void mt7628_switch_init(struct dsa_switch *ds)
> > +{
> > +	struct mt7628_esw *esw = ds->priv;
> > +
> > +	regmap_write(esw->regmap, MT7628_ESW_REG_FCT0,
> > +		     FIELD_PREP(MT7628_ESW_FCT0_DROP_SET_TH, 0x50) |
> > +		     FIELD_PREP(MT7628_ESW_FCT0_DROP_RLS_TH, 0x78) |
> > +		     FIELD_PREP(MT7628_ESW_FCT0_FC_SET_TH, 0xa0) |
> > +		     FIELD_PREP(MT7628_ESW_FCT0_FC_RLS_TH, 0xc8));
> 
> [Severity: High]
> Can traffic leak across isolated ports (like WAN and LAN) during boot?
> 
> It looks like mt7628_switch_init() resets the switch but fails to explicitly
> set the MT7628_ESW_POC0_PORT_DISABLE bits for the user ports. Since the
> DSA core relies on port_disable during ndo_close and doesn't automatically
> disable ports upon initialization, does the hardware default to acting as
> an unmanaged switch, bridging all networks until the interfaces are brought
> up administratively?

The hardware defaults to an all ports disabled state. Will add this as a
comment in v7.