Re: [PATCH net 1/2] net: dsa: realtek: rtl8365mb: use IVL in VLAN 0

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says "On its own this has no visible effect".

--

net: dsa: realtek: rtl8365mb: use IVL in VLAN 0

This commit adds rtl8365mb_vlan_4k_setup_neutral() and calls it from
rtl8365mb_vlan_setup(). The helper sets Independent VLAN Learning on the
VLAN4k entry of VID 0, so that lookups and learning match the IVL-keyed
FDB and MDB entries the driver installs for VID 0.

> On its own this has no visible effect: standalone ports do not learn and
> the driver does not control host flooding, so the unused static entries
> only meant that traffic was flooded to the CPU. The next patch, which
> moves VLAN-unaware bridges onto VID 0, depends on it.

[ ... ]

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_vlan.c b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> index 8d49ffadce926..58948e76f122a 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> @@ -322,6 +322,37 @@ rtl8365mb_vlan_4k_port_set(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> +/*
> + * rtl8365mb_vlan_4k_setup_neutral() - Use IVL in the neutral VLAN
> + * @priv: realtek_priv pointer
> + *
> + * Untagged frames of standalone ports and of ports in a VLAN-unaware bridge
> + * are classified to VID 0, the VID of the neutral PVID. No bridge VLAN sets
> + * up its VLAN4k entry, which by default uses Shared VLAN Learning, so the
> + * switch would learn these frames by FID while the driver installs its FDB
> + * and MDB entries for VID 0 with Independent VLAN Learning. Use IVL in VID 0
> + * as in every other VLAN.

[Severity: Low]
The commit message paragraph quoted above says "On its own this has no
visible effect". Does that hold for ports in a VLAN-unaware bridge that
have no PVID?

You can reach that configuration by creating the bridge with
vlan_default_pvid 0, or by removing a port's PVID with bridge vlan del. At
this commit such a port falls back to the neutral PVID, which is VLANMC[0]
with EVID 0. rtl8365mb_vlan_pvid_port_clear() keeps the frame filter when
the port is not filtering, then sets the PVID index to 0:

rtl8365mb_vlan_pvid_port_clear() {
    ...
	} else {
		/* skip undo_set_framefilter */
		accepted_frame = prev_accepted_frame;
	}

	ret = rtl8365mb_vlan_port_set_pvid(priv, port, 0);
    ...
}

As a result, untagged frames on that port are classified to VID 0.

Standalone ports don't learn, but bridge ports do:

rtl83xx_port_bridge_join() {
    ...
	if (priv->ops->port_set_learning) {
		ret = priv->ops->port_set_learning(priv, port, true);
    ...
}

Before this patch, VLAN4k[0] used SVL, so the switch learned and looked
up these frames by FID 0. The IVL {VID 0} FDB and MDB entries that DSA
installs never matched. That includes bridge static entries and the
assisted learning entries on the CPU port.

This looks like the same roaming and hairpin drop scenario that the
follow-up patch "net: dsa: realtek: rtl8365mb: keep the neutral PVID in
VLAN-unaware bridges" describes. This patch fixes it on its own for
PVID-less VLAN-unaware bridge ports.

This patch has its own Fixes: tag. Could the commit message say that it
already changes forwarding for PVID-less VLAN-unaware bridge ports,
instead of only discussing standalone ports?

A related point about the new kernel-doc: it says untagged frames "of
ports in a VLAN-unaware bridge are classified to VID 0". At this commit,
ds->configure_vlan_while_not_filtering is set, so rtl8365mb_port_vlan_add()
programs the bridge PVID (VID 1 by default) even when filtering is off.
rtl8365mb_port_vlan_filtering() also doesn't reset the port to the
neutral PVID.

So the comment is only true for PVID-less ports until the next patch in
the series lands. The code change itself looks correct.

> + *
> + * Context: Can sleep. Must be called with &priv->vlan_lock held.
> + * Takes and releases &priv->map_lock.
> + * Return: 0 on success, or a negative error code on failure.
> + */

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104082514.2139278-1-stijncleynhens%40gmail.com