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

[Jakub Kicinski <kuba@kernel.org>]

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.

> 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.

> [ ... ]
> +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?