Re: [PATCH net-next v1 1/6] net: dsa: mt7628: rework vlan block allocator

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The tag_8021q VLAN-entry reservation in mt7628_setup()
  (drivers/net/dsa/mt7628.c:511-520) hard-codes…

--

net: dsa: mt7628: rework vlan block allocator

This reworks the MT7628 VLAN hardware entry allocator into
find/alloc/join/leave helpers, replaces the per-entry active flag with
vid == 0, and reserves the first six VLAN table entries for tag_8021q
use so the remaining entries can later serve VLAN filtering.

> diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> index fb63f6f644b9e..990556cc2f655 100644
> --- a/drivers/net/dsa/mt7628.c
> +++ b/drivers/net/dsa/mt7628.c

[ ... ]

> @@ -383,6 +390,82 @@ static void mt7628_esw_set_vub(struct mt7628_esw *esw, unsigned int vlan,
> +static struct mt7628_vlan *mt7628_alloc_vlan_block(struct dsa_switch *ds,
> +						   u16 vid, unsigned int type)
> +{
> +	struct mt7628_esw *esw = ds->priv;
> +	struct mt7628_vlan *vlan;
> +	int i;
> +
> +	for (i = 0; i < MT7628_NUM_VLANS; i++) {
> +		vlan = &esw->vlans[i];
> +		if (vlan->vid)
> +			continue;
> +		if (vlan->type != type)
> +			continue;
> +		vlan->vid = vid;
> +		return vlan;
> +	}
> +
> +	return NULL;
> +}

[ ... ]

> @@ -425,6 +508,17 @@ static int mt7628_setup(struct dsa_switch *ds)
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Dedicate the first num_user_ports + 1 VLAN slots for tag_8021q.
> +	 * Since bridges are only offloaded when they have at least one member
> +	 * port, the worst case entry requirement is 1 per port. The extra slot
> +	 * is needed because when changing the configuration, tag_8021q adds a
> +	 * new VLAN before removing the old one. The rest of the VLAN slots can
> +	 * be used for filtering.
> +	 */
> +	for (int i = 0; i < MT7628_ESW_NUM_USER_PORTS + 1; i++)
> +		esw->vlans[i].type = MT7628_VLAN_TYPE_UNAWARE;
> +

[Severity: Medium]
Is the worst case really one entry per local user port plus one?

tag_8021q VIDs are not only generated for this switch's own ports.
dsa_tag_8021q_bridge_join() adds the bridge VID with broadcast = true:

net/dsa/tag_8021q.c:dsa_tag_8021q_bridge_join() {
	...
	err = dsa_port_tag_8021q_vlan_add(dp, bridge_vid, true);
	if (err)
		return err;

	dsa_port_tag_8021q_vlan_del(dp, standalone_vid, false);
	...
}

which reaches dsa_broadcast(), documented as notifying all DSA trees in
the system, and the per-port match is:

net/dsa/tag_8021q.c:dsa_port_tag_8021q_vlan_match() {
	return dsa_port_is_dsa(dp) || dsa_port_is_cpu(dp) || dp == info->dp;
}

so this switch's CPU port installs an entry for every foreign bridge VID
and for every peer switch standalone VID in the same tree as well.

With the five local standalone VIDs from dsa_tag_8021q_port_setup()
occupying five slots and one foreign VID taking the sixth, does
mt7628_alloc_vlan_block() then return -ENOSPC for the next tag_8021q VID
even though ten hardware entries are still free?  The type match:

	if (vlan->type != type)
		continue;

prevents a tag_8021q request from ever falling back to the entries typed
MT7628_VLAN_TYPE_AWARE, and up to DSA_TAG_8021Q_MAX_NUM_BRIDGES == 7
distinct bridge VIDs can be broadcast.  Before this patch,
mt7628_dsa_8021q_vlan_add() could use any of the 16 entries, so the
reachable pool for these callbacks shrinks from 16 to 6.

Along the same lines, the commit message says:

    6 VLAN table entries are reserved for tag_8021q use and the rest are
    left for the upcoming VLAN filtering support. No functional change
    intended.

Given the reduced pool, is "no functional change intended" accurate for
configurations with a second tag_8021q tree or a cross-chip peer?

Would it make sense to derive the reservation from the ports DSA actually
instantiates (dsa_switch_for_each_user_port) and to let tag_8021q
requests fall back to unused entries of the other type, rather than
keying it off the constant MT7628_ESW_NUM_USER_PORTS while ds->num_ports
is MT7628_ESW_NUM_PORTS?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906171625.533915-1-joey%40tinyisr.com