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

This patch adds a DSA driver for the MT7628 embedded switch, which has
five 100Mbps user ports and a 1Gbps CPU port. tag_8021q is used to
isolate ports via the switch's 16-entry VLAN table, with the double tag
feature enabled to simulate VLAN-unaware behaviour.

> diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> new file mode 100644
> index 000000000000..eccdb05fce9b
> --- /dev/null
> +++ b/drivers/net/dsa/mt7628.c

[ ... ]

> +static const struct dsa_switch_ops mt7628_switch_ops = {
> +	.get_tag_protocol = mt7628_get_tag_proto,
> +	.setup = mt7628_setup,
> +	.teardown = mt7628_teardown,
> +	.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,
> +};

[Medium]
How is a user expected to place two of the user ports into a Linux bridge?

The ops table does not implement port_bridge_join or port_bridge_leave,
so dsa_switch_bridge_join() in net/dsa/switch.c returns -EOPNOTSUPP:

    if (!ds->ops->port_bridge_join)
        return -EOPNOTSUPP;

That makes ip link set swpN master brX fail for every user port, which
seems to contradict the commit message:

    The switch hardware has a very limited 16 entry VLAN table.
    Configuring VLANs is the only way to control switch forwarding.
    Currently 6 entries are used by tag_8021q to isolate the ports.

Without a port_bridge_join hook, dsa_tag_8021q_bridge_join() is never
invoked, so there is no path to migrate a port's tag_8021q VID from its
standalone VID to a bridge VID.

Would wiring the tag_8021q helpers directly, similar to what
vitesse-vsc73xx-core.c does, be sufficient here:

    .port_bridge_join = dsa_tag_8021q_bridge_join,
    .port_bridge_leave = dsa_tag_8021q_bridge_leave,

Related bridge-lifecycle callbacks such as port_stp_state_set,
port_pre_bridge_flags, port_bridge_flags and port_fast_age also appear
to be absent, is that intentional for this initial submission?