Re: [PATCH net-next v2 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 (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.

> +	help
> +	  This enables support for the built-in Ethernet switch found
> +	  in the MT7628 SoC.
> +
[ ... ]
> diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> --- /dev/null
> +++ b/drivers/net/dsa/mt7628.c
[ ... ]
> +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.

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

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

> +	if (!bus) {
> +		ret = -ENOMEM;
> +		goto out_put_node;
> +	}
> +
> +	bus->name = "MT7628 internal MDIO bus";
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(ds->dev));
> +	bus->priv = esw;
> +	bus->read = mt7628_mii_read;
> +	bus->write = mt7628_mii_write;
> +	bus->parent = esw->dev;
> +	if (!mdio) {
> +		ds->user_mii_bus = bus;
> +		bus->phy_mask = ~ds->phys_mii_mask;
> +	}
> +
> +	ret = devm_of_mdiobus_register(esw->dev, bus, mdio);
> +
> +out_put_node:
> +	of_node_put(mdio);
> +	return ret;
> +}
[ ... ]
> +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.

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

> +	.port_enable = mt7628_port_enable,
> +	.port_disable = mt7628_port_disable,
> +	.phylink_get_caps = mt7628_phylink_get_caps,
> +	.tag_8021q_vlan_add = mt7628_dsa_8021q_vlan_add,
> +	.tag_8021q_vlan_del = mt7628_dsa_8021q_vlan_del,
> +};
-- 
pw-bot: cr