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

[netdev-bot+sashiko]

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 the patch has no visible effect on its
  own, but that is only true for standalone ports.

--

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

This commit sets up the VLAN4k entry of VID 0 with Independent VLAN
Learning. The goal is for the IVL-keyed FDB and MDB entries that DSA
installs with VID 0 to match frames classified to that VLAN. The commit
message says the change has no visible effect on its own.

> DSA installs the FDB and MDB entries of standalone ports and of
> VLAN-unaware bridges with VID 0, and the driver writes them with
> Independent VLAN Learning. No bridge VLAN sets up the VLAN4k entry of
> VID 0 though, and it defaults to Shared VLAN Learning, so the switch
> looks up and learns frames classified to VID 0 by FID. Lookups never hit
> the IVL-keyed static entries, and a learned entry for the same MAC sits
> alongside the static one.

[ ... ]

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

[Severity: Low]
Does the "no visible effect" claim hold for VLAN-unaware bridge ports
that have no PVID?

This can happen with a bridge created with vlan_default_pvid 0, or with a
VLAN-unaware port whose PVID VLAN was deleted. In both cases
rtl8365mb_vlan_pvid_port_clear() points the port at VLANMC index 0. The
rtl8365mb_vlan_mc_find() kernel-doc describes that index like this:

 * Although 0 is a valid VLAN MC index, it is reserved for ports without PVID,
 * including standalone, non-member ports. It uses VID == 0.

These ports do learn, because rtl83xx_port_bridge_join() turns learning
on:

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

Before this patch, their untagged frames were learned and looked up in
the SVL/FID key space of VLAN4k[0]. The static entries, however, are
always IVL-keyed in rtl8365mb_l2_add_uc():

	uc.key.ivl = true;
	uc.key.vid = vid;

So unicast to the host was flooded to the other bridge ports. Assisted
learning on the CPU port also could not override learned entries.

Once rtl8365mb_vlan_vlan_setup()->rtl8365mb_vlan_4k_setup_neutral() turns
on IVL in VID 0, those static entries start to match for these ports.
Learned entries then use the VID 0 IVL key. That changes forwarding
behaviour and the FDB contents.

The first paragraph also seems to contradict the "no visible effect"
claim. "a learned entry for the same MAC sits alongside the static one"
can only happen if some port learns on VID 0, and standalone ports do
not learn.

Could the commit message describe the effect on VLAN-unaware bridge ports
without a PVID? The later patch in the series that keeps the neutral PVID
in VLAN-unaware bridges does not mention this case either.

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