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

[Joris Vaisvila <joey@tinyisr.com>]

Hi Jakub, thank you for the review.

On Wed, Apr 01, 2026 at 08:31:03PM -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 (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/Kconfig b/drivers/net/dsa/Kconfig
> > --- a/drivers/net/dsa/Kconfig
> > +++ b/drivers/net/dsa/Kconfig
> > @@ -28,6 +28,14 @@ source "drivers/net/dsa/hirschmann/Kconfig"
> >  
> >  source "drivers/net/dsa/lantiq/Kconfig"
> >  
> > +config NET_DSA_MT7628
> > +	tristate "MediaTek MT7628 Embedded Ethernet switch support"
> > +	select NET_DSA_TAG_MT7628
> > +	select MEDIATEK_FE_SOC_PHY
> 
> Does this configuration need to select NET_DSA_TAG_8021Q?
> 
> The driver relies on the tag_8021q mechanism for switch port isolation. If
> the kernel is configured without NET_DSA_TAG_8021Q, dsa_tag_8021q_register()
> will return -EOPNOTSUPP, causing the switch to boot with broken port
> isolation.
> 

tag_8021q.o is in DSA core, there's no option to enable or disable it.

> [ ... ]
> > +static int mt7628_mii_read(struct mii_bus *bus, int port, int regnum)
> > +{
> > +	struct mt7628_esw *esw = bus->priv;
> > +	int ret;
> > +	u32 val;
> > +
> > +	ret = regmap_read_poll_timeout(esw->regmap, MT7628_ESW_REG_PCR1, val,
> > +				       !(val & MT7628_ESW_PCR1_RD_DONE), 10,
> > +				       5000);
> 
> Will this polling logic cause an MDIO timeout?
> 
> If RD_DONE is a completion status bit that remains 1 after a successful
> operation, this first poll will wait indefinitely for the bit to become 0.
> 
> If RD_DONE is an active-low busy bit, the second poll below waits for the
> controller to become busy instead of waiting for it to be idle.
> 

RD_DONE gets set when the read operations completes and is reset on
register read, so this will not cause a timeout.

> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = regmap_write(esw->regmap, MT7628_ESW_REG_PCR0,
> > +			   FIELD_PREP(MT7628_ESW_PCR0_CPU_PHY_REG,
> > +				      regnum) |
> > +			   FIELD_PREP(MT7628_ESW_PCR0_CPU_PHY_ADDR,
> > +				      port) | MT7628_ESW_PCR0_RD_PHY_CMD);
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = regmap_read_poll_timeout(esw->regmap, MT7628_ESW_REG_PCR1, val,
> > +				       (val & MT7628_ESW_PCR1_RD_DONE), 10,
> > +				       5000);
> > +out:
> > +	if (ret) {
> > +		dev_err(&bus->dev, "read failed. MDIO timeout?\n");
> > +		return ret;
> > +	}
> > +	return FIELD_GET(MT7628_ESW_PCR1_RD_DATA, val);
> > +}
> > +
> > +static int mt7628_mii_write(struct mii_bus *bus, int port, int regnum, u16 dat)
> > +{
> > +	struct mt7628_esw *esw = bus->priv;
> > +	u32 val;
> > +	int ret;
> > +
> > +	ret = regmap_read_poll_timeout(esw->regmap, MT7628_ESW_REG_PCR1, val,
> > +				       !(val & MT7628_ESW_PCR1_WT_DONE), 10,
> > +				       5000);
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = regmap_write(esw->regmap, MT7628_ESW_REG_PCR0,
> > +			   FIELD_PREP(MT7628_ESW_PCR0_WT_NWAY_DATA, dat) |
> > +			   FIELD_PREP(MT7628_ESW_PCR0_CPU_PHY_REG,
> > +				      regnum) |
> > +			   FIELD_PREP(MT7628_ESW_PCR0_CPU_PHY_ADDR,
> > +				      port) | MT7628_ESW_PCR0_WT_PHY_CMD);
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = regmap_read_poll_timeout(esw->regmap, MT7628_ESW_REG_PCR1, val,
> > +				       (val & MT7628_ESW_PCR1_WT_DONE), 10,
> > +				       5000);
> 
> Is this second poll waiting for the controller to become busy instead of
> idle, similar to the read function?
> 
WT_DONE is set when the data is ready and reset on read, same as
RD_DONE.

> > +out:
> > +	if (ret) {
> > +		dev_err(&bus->dev, "write failed. MDIO timeout?\n");
> > +		return ret;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int mt7628_setup_internal_mdio(struct dsa_switch *ds)
> > +{
> > +	struct mt7628_esw *esw = ds->priv;
> > +	struct device_node *mdio;
> > +	struct mii_bus *bus;
> > +	int ret = 0;
> > +
> > +	mdio = of_get_available_child_by_name(ds->dev->of_node, "mdio");
> > +
> > +	bus = devm_mdiobus_alloc(esw->dev);
> 
> Can allocating devm_ resources in the DSA .setup callback cause a sysfs name
> collision?
> 
> The .setup callback is executed whenever the DSA tree binds. Since devm_
> resources are tied to the platform device (esw->dev) rather than the DSA
> tree lifecycle, they are not released when the DSA tree unbinds. On a
> subsequent re-bind, allocating a new MDIO bus with the same ID will fail
> because the original bus was never unregistered.
> 

I'm not sure how this would work. Other switches appear to be doing the
same. For example mt7530-mmio.c does `priv->dev = &pdev->dev` and later
uses priv->dev to register the mdio bus. Is this a real issue?

> [ ... ]
> > +static int mt7628_setup(struct dsa_switch *ds)
> > +{
> > +	struct mt7628_esw *esw = ds->priv;
> > +	int ret;
> > +
> > +	reset_control_reset(esw->rst_esw);
> > +	usleep_range(1000, 2000);
> > +	reset_control_reset(esw->rst_ephy);
> > +	usleep_range(1000, 2000);
> > +	/*
> > +	 * all MMIO reads hang if esw is not out of reset
> > +	 * ephy needs extra time to get out of reset or it ends up misconfigured
> > +	 */
> > +	mt7628_switch_init(ds);
> > +	rtnl_lock();
> > +	dsa_tag_8021q_register(ds, htons(ETH_P_8021Q));
> 
> Is it safe to ignore the return value of dsa_tag_8021q_register()?
> 
> If the registration fails, the core function frees the allocated context but
> does not clear ds->tag_8021q_ctx. The setup will continue, and the DSA core
> will later dereference the non-NULL ds->tag_8021q_ctx, leading to a
> use-after-free crash.
> 
> Also, if mt7628_setup_internal_mdio() below fails, the setup returns an
> error without unregistering the tagger, which leaks the tag_8021q context.
> 

No, will fix in v3. 

> > +	rtnl_unlock();
> > +
> > +	ret = mt7628_setup_internal_mdio(ds);
> > +	return ret;
> > +}
> [ ... ]
> > +static struct dsa_switch_ops mt7628_switch_ops = {
> > +	.get_tag_protocol = mt7628_get_tag_proto,
> > +	.setup = mt7628_setup,
> 
> Does this switch ops structure need a .teardown callback?
> 
> Without calling dsa_tag_8021q_unregister() in a .teardown callback, the
> tag_8021q context memory allocated during .setup will be permanently leaked
> whenever the DSA switch is unbound or the module is removed.
> 
Yes, will fix in V3. 

Thanks,
Joris