[PATCH net v2 0/2] net: dsa: realtek: rtl8365mb: fix FDB lookups in VLAN-unaware bridges

[PATCH net v2 0/2] net: dsa: realtek: rtl8365mb: fix FDB lookups in VLAN-unaware bridges

[Stijn Cleynhens]

In a VLAN-unaware bridge, DSA installs FDB and MDB entries with VID 0,
but the switch classifies untagged frames to the bridge PVID and looks up
VID 0 by FID, while the driver installs IVL entries. Nothing matches, so
a station that roams from behind a user port to behind the CPU port (a
Wi-Fi client moving between access points) stays unreachable until the
learned entry ages out.

Patch 1 makes VID 0 use IVL; patch 2 depends on it. Patch 2 keeps the
neutral PVID (VID 0) on ports of a VLAN-unaware bridge and only programs
the bridge PVID while the port filters VLANs. Tagged frames in a
VLAN-unaware bridge are not covered, see patch 2.

Tested on OpenWrt's 6.18 kernel, which carries the rtl8365mb backports up
to v7.4; the code these patches touch is the same as in net. A station
learned behind the uplink that moves behind the CPU port answers 10/10
pings, its learned entry replaced by the CPU one, with VLAN filtering
switched off at runtime: on an RTL8367S (TP-Link Deco X50-Outdoor), and
on an RTL8367S-VB (Deco X50-PoE, with the family D series applied and a
family D branch in rtl8365mb_vlan_pvid_port_commit()), where it also
passes with VLAN filtering on. Before these patches the RTL8367S
answered 0/10. Both patches build without warnings with W=1 C=1.

Note for Mieczyslaw's RTL8367S-VB series in net-next (v8): patch 2
replaces rtl8365mb_vlan_pvid_port_set() and
rtl8365mb_vlan_pvid_port_clear() with rtl8365mb_vlan_pvid_port_commit(),
which gets the target VID (0 for the neutral PVID) and the filtering
state. The family D paths that series adds to set and clear become one
branch there that writes the PVID register and the frame filter from
those two inputs. Happy to send that as a follow-up once both are in.

Changes in v2:
- Patch 1: add Alvin's Reviewed-by.
- Patch 2: replace rtl8365mb_vlan_pvid_port_set() and
  rtl8365mb_vlan_pvid_port_clear() with rtl8365mb_vlan_pvid_port_commit(),
  which is now the only place that sets the frame filter;
  rtl8365mb_port_vlan_filtering() no longer does (Alvin).
- Patch 2: one variable per declaration and an explicit else (Alvin).
- Patch 2: commit message: say what "the VID of the bridge entries" is
  and that the per-bridge EFID keeps lookups local (Alvin).
- v1: https://lore.kernel.org/netdev/20261104082514.2139278-1-stijncleynhens@gmail.com/

Stijn Cleynhens (2):
  net: dsa: realtek: rtl8365mb: use IVL in VLAN 0
  net: dsa: realtek: rtl8365mb: keep the neutral PVID in VLAN-unaware
    bridges

 drivers/net/dsa/realtek/rtl8365mb_main.c | 157 ++++++++-------
 drivers/net/dsa/realtek/rtl8365mb_vlan.c | 233 ++++++++++-------------
 drivers/net/dsa/realtek/rtl8365mb_vlan.h |   7 +-
 3 files changed, 191 insertions(+), 206 deletions(-)


base-commit: 787268686c64be60023743b527645a907e38ccca
-- 
2.56.0

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

[Stijn Cleynhens]

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.

Set up VID 0 with IVL, like every other VLAN the driver programs.

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.

Fixes: 336e3e4a1ab3 ("net: dsa: realtek: rtl8365mb: add FDB support")
Reviewed-by: Alvin Šipraga <alvin.sipraga@analog.com>
Signed-off-by: Stijn Cleynhens <stijncleynhens@gmail.com>
---
 drivers/net/dsa/realtek/rtl8365mb_main.c |  8 ++++++
 drivers/net/dsa/realtek/rtl8365mb_vlan.c | 31 ++++++++++++++++++++++++
 drivers/net/dsa/realtek/rtl8365mb_vlan.h |  1 +
 3 files changed, 40 insertions(+)

diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
index da06f952a..b90e46c16 100644
--- a/drivers/net/dsa/realtek/rtl8365mb_main.c
+++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
@@ -2232,6 +2232,14 @@ static int rtl8365mb_vlan_setup(struct dsa_switch *ds)
 		}
 	}
 
+	mutex_lock(&priv->vlan_lock);
+	ret = rtl8365mb_vlan_4k_setup_neutral(priv);
+	mutex_unlock(&priv->vlan_lock);
+	if (ret) {
+		dev_err(priv->dev, "Failed to set up VLAN 0\n");
+		return ret;
+	}
+
 	/* VLAN is always enabled. */
 	ret = regmap_update_bits(priv->map, RTL8365MB_VLAN_CTRL_REG,
 				 RTL8365MB_VLAN_CTRL_EN_MASK,
diff --git a/drivers/net/dsa/realtek/rtl8365mb_vlan.c b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
index 8d49ffadc..58948e76f 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.
+ *
+ * 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.
+ */
+int rtl8365mb_vlan_4k_setup_neutral(struct realtek_priv *priv)
+{
+	struct rtl8365mb_vlan4k vlan4k = {0};
+	int ret;
+
+	lockdep_assert_held(&priv->vlan_lock);
+
+	ret = rtl8365mb_vlan_4k_read(priv, 0, &vlan4k);
+	if (ret)
+		return ret;
+
+	vlan4k.ivl_en = true;
+
+	return rtl8365mb_vlan_4k_write(priv, &vlan4k);
+}
+
 /*
  * rtl8365mb_vlan_4k_port_add() - Add a port to a VLAN 4K table entry
  * @ds: dsa switch instance
diff --git a/drivers/net/dsa/realtek/rtl8365mb_vlan.h b/drivers/net/dsa/realtek/rtl8365mb_vlan.h
index 27a526b61..809034009 100644
--- a/drivers/net/dsa/realtek/rtl8365mb_vlan.h
+++ b/drivers/net/dsa/realtek/rtl8365mb_vlan.h
@@ -28,6 +28,7 @@ int
 rtl8365mb_vlan_port_set_framefilter(struct realtek_priv *priv,
 				    int port,
 				    enum rtl8365mb_frame_ingress frame_type);
+int rtl8365mb_vlan_4k_setup_neutral(struct realtek_priv *priv);
 int rtl8365mb_vlan_4k_port_add(struct dsa_switch *ds, int port,
 			       const struct switchdev_obj_port_vlan *vlan,
 			       struct netlink_ext_ack *extack);
-- 
2.56.0

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

[PATCH net v2 2/2] net: dsa: realtek: rtl8365mb: keep the neutral PVID in VLAN-unaware bridges

[Stijn Cleynhens]

The switch looks up its forwarding database by the VLAN that it
classifies a frame to, and untagged frames are classified to the port
PVID. With configure_vlan_while_not_filtering, the bridge PVID is
programmed into the port also while the bridge is VLAN-unaware, so
untagged frames of such a bridge are looked up in VLAN 1. DSA however
installs the FDB and MDB entries of a VLAN-unaware bridge with VID 0,
so these entries never match a lookup.

This breaks assisted learning on the CPU port: when a station that was
first learned behind a user port moves behind the CPU port (e.g. a
wireless client roaming between access points), the static entry that
DSA installs for it on the CPU port does not override the learned one,
and unicast to the station is dropped as a hairpin until the learned
entry ages out. Clients that need a DHCP reply in that window do not
get one.

Keep the PVID requested by the bridge per port and only program it
while the port filters VLANs. Otherwise keep the neutral PVID (VLANMC
index 0, VID 0) that standalone ports already use, so that untagged
frames are classified to VID 0, the VID that DSA uses for the FDB and
MDB entries of a VLAN-unaware bridge. The per-bridge EFID keeps these
lookups local to the bridge.

The PVID and the accepted frame types now both follow from the bridge
PVID and the filtering state. Replace rtl8365mb_vlan_pvid_port_set() and
rtl8365mb_vlan_pvid_port_clear() with rtl8365mb_vlan_pvid_port_commit(),
which programs both, and call it whenever either input changes. The
filtering state is passed in, as DSA still reports the old one while
VLAN filtering is being toggled.

This only covers untagged frames. Tagged frames in a VLAN-unaware bridge
are still classified by their tag, so a tagged station that roams keeps
hitting the stale learned entry until it ages out, as before. Fixing
that would need the switch to ignore the C-tag on ingress of these
ports, which is left for a separate change.

Fixes: 336e3e4a1ab3 ("net: dsa: realtek: rtl8365mb: add FDB support")
Signed-off-by: Stijn Cleynhens <stijncleynhens@gmail.com>
---
 drivers/net/dsa/realtek/rtl8365mb_main.c | 149 +++++++++--------
 drivers/net/dsa/realtek/rtl8365mb_vlan.c | 202 ++++++++---------------
 drivers/net/dsa/realtek/rtl8365mb_vlan.h |   6 +-
 3 files changed, 151 insertions(+), 206 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
index b90e46c16..4f0e0ab7f 100644
--- a/drivers/net/dsa/realtek/rtl8365mb_main.c
+++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
@@ -838,6 +838,8 @@ struct rtl8365mb_cpu {
  *         access via rtl8365mb_get_stats64
  * @stats_lock: protect the stats structure during read/update
  * @mib_work: delayed work for polling MIB counters
+ * @bridge_pvid: PVID requested by the bridge, 0 if none. Only programmed
+ *               while the port filters VLANs. Protected by vlan_lock.
  */
 struct rtl8365mb_port {
 	struct realtek_priv *priv;
@@ -845,6 +847,7 @@ struct rtl8365mb_port {
 	struct rtnl_link_stats64 stats;
 	spinlock_t stats_lock;
 	struct delayed_work mib_work;
+	u16 bridge_pvid;
 };
 
 /**
@@ -2004,42 +2007,44 @@ rtl8365mb_port_set_vlan_egress_mode(struct realtek_priv *priv, int port,
 			RTL8365MB_PORT_MISC_CFG_VLAN_EGRESS_MODE_MASK, val);
 }
 
+/* Program the PVID that the port should use for untagged frames: the PVID of
+ * the bridge while the port filters VLANs, and the neutral PVID (VID 0)
+ * otherwise. A VLAN-unaware bridge, like a standalone port, gets its FDB and
+ * MDB entries with VID 0 from DSA. As the switch looks up its forwarding
+ * database by the VLAN that it classifies a frame to, a bridge PVID would make
+ * those entries miss every lookup.
+ */
+static int rtl8365mb_port_commit_pvid(struct dsa_switch *ds, int port,
+				      bool vlan_filtering,
+				      struct netlink_ext_ack *extack)
+{
+	struct realtek_priv *priv = ds->priv;
+	struct rtl8365mb *mb = priv->chip_data;
+	u16 pvid;
+
+	if (vlan_filtering)
+		pvid = mb->ports[port].bridge_pvid;
+	else
+		pvid = 0;
+
+	return rtl8365mb_vlan_pvid_port_commit(ds, port, pvid, vlan_filtering,
+					       extack);
+}
+
 static int rtl8365mb_port_vlan_filtering(struct dsa_switch *ds, int port,
 					 bool vlan_filtering,
 					 struct netlink_ext_ack *extack)
 {
-	enum rtl8365mb_frame_ingress accepted_frame, prev_accepted_frame;
 	enum rtl8365mb_vlan_egress_mode mode;
 	struct realtek_priv *priv = ds->priv;
 	u32 configured_ports = 0;
 	struct dsa_port *dp;
-	u16 pvid_vid;
 	int ret;
 
 	dev_dbg(priv->dev, "port %d: %s VLAN filtering\n", port,
 		vlan_filtering ? "enable" : "disable");
 
-	ret = rtl8365mb_vlan_port_get_framefilter(priv, port,
-						  &prev_accepted_frame);
-	if (ret) {
-		NL_SET_ERR_MSG_MOD(extack,
-				   "Failed to get current framefilter");
-		return ret;
-	}
-
-	/* While filtering, only accepts untagged frames if PVID is enabled */
-	if (vlan_filtering) {
-		ret = rtl8365mb_vlan_port_get_pvid(priv, port, &pvid_vid);
-		if (ret)
-			return ret;
-
-		if (pvid_vid)
-			accepted_frame = RTL8365MB_FRAME_TYPE_ANY_FRAME;
-		else
-			accepted_frame = RTL8365MB_FRAME_TYPE_TAGGED_ONLY;
-	} else {
-		accepted_frame = RTL8365MB_FRAME_TYPE_ANY_FRAME;
-	}
+	mutex_lock(&priv->vlan_lock);
 
 	/* When vlan filter is enable/disabled in a bridge, this function is
 	 * called for all member ports. We need to enable/disable ingress
@@ -2047,7 +2052,7 @@ static int rtl8365mb_port_vlan_filtering(struct dsa_switch *ds, int port,
 	 */
 	ret = rtl8365mb_port_set_ingress_filtering(priv, port, vlan_filtering);
 	if (ret)
-		return ret;
+		goto out_unlock;
 
 	/* However, we also enable/disable egress filtering because the switch
 	 * still consider the egress interface VLAN membership to forward the
@@ -2064,15 +2069,12 @@ static int rtl8365mb_port_vlan_filtering(struct dsa_switch *ds, int port,
 		configured_ports |= BIT(dp->index);
 	}
 
-	if (accepted_frame != prev_accepted_frame) {
-		ret = rtl8365mb_vlan_port_set_framefilter(priv, port,
-							  accepted_frame);
-		if (ret) {
-			NL_SET_ERR_MSG_MOD(extack,
-					   "Failed to set port framefilter");
-			goto undo_transparent;
-		}
-	}
+	/* Also sets which frames the port accepts: while filtering, untagged
+	 * frames are only accepted if the bridge gave the port a PVID.
+	 */
+	ret = rtl8365mb_port_commit_pvid(ds, port, vlan_filtering, extack);
+	if (ret)
+		goto undo_transparent;
 
 	/* When VLAN filtering is disabled, preserve frames exactly as received.
 	 * Otherwise, the VLAN egress pipeline may still alter tag state
@@ -2085,14 +2087,12 @@ static int rtl8365mb_port_vlan_filtering(struct dsa_switch *ds, int port,
 
 	ret = rtl8365mb_port_set_vlan_egress_mode(priv, port, mode);
 	if (ret)
-		goto undo_set_framefilter;
+		goto undo_commit_pvid;
 
-	return ret;
+	goto out_unlock;
 
-undo_set_framefilter:
-	if (prev_accepted_frame != accepted_frame)
-		rtl8365mb_vlan_port_set_framefilter(priv, port,
-						    prev_accepted_frame);
+undo_commit_pvid:
+	(void)rtl8365mb_port_commit_pvid(ds, port, !vlan_filtering, NULL);
 undo_transparent:
 	/* The DSA core guarantees this callback is only invoked on an actual
 	 * state transition, ensuring the previous hardware state was the
@@ -2111,6 +2111,8 @@ static int rtl8365mb_port_vlan_filtering(struct dsa_switch *ds, int port,
 
 	rtl8365mb_port_set_ingress_filtering(priv, port, !vlan_filtering);
 
+out_unlock:
+	mutex_unlock(&priv->vlan_lock);
 	return ret;
 }
 
@@ -2120,8 +2122,11 @@ static int rtl8365mb_port_vlan_add(struct dsa_switch *ds, int port,
 {
 	bool untagged = !!(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED);
 	bool pvid = !!(vlan->flags & BRIDGE_VLAN_INFO_PVID);
-	u16 pvid_vid;
 	struct realtek_priv *priv = ds->priv;
+	struct rtl8365mb *mb = priv->chip_data;
+	u16 prev_bridge_pvid;
+	u16 *bridge_pvid;
+	bool vlan_filtering;
 	int ret;
 
 	dev_dbg(priv->dev, "add VLAN %d on port %d, %s, %s\n",
@@ -2137,41 +2142,33 @@ static int rtl8365mb_port_vlan_add(struct dsa_switch *ds, int port,
 
 	mutex_lock(&priv->vlan_lock);
 
-	ret = rtl8365mb_vlan_port_get_pvid(priv, port, &pvid_vid);
-	if (ret)
-		goto out_unlock;
+	vlan_filtering = dsa_port_is_vlan_filtering(dsa_to_port(ds, port));
+	bridge_pvid = &mb->ports[port].bridge_pvid;
 
-	/* Set PVID if needed */
-	if (pvid) {
-		ret = rtl8365mb_vlan_pvid_port_set(ds, port, vlan->vid,
-						   extack);
-		if (ret)
-			goto out_unlock;
-	} else {
-		/* or try to unset it if not */
-		ret = rtl8365mb_vlan_pvid_port_clear(ds, port, vlan->vid);
-		if (ret)
-			goto out_unlock;
-	}
+	/* Set PVID if needed, or unset it if this VLAN was the PVID */
+	prev_bridge_pvid = *bridge_pvid;
+	if (pvid)
+		*bridge_pvid = vlan->vid;
+	else if (*bridge_pvid == vlan->vid)
+		*bridge_pvid = 0;
+
+	ret = rtl8365mb_port_commit_pvid(ds, port, vlan_filtering, extack);
+	if (ret)
+		goto undo_bridge_pvid;
 
 	/* add port to vlan4k. It knows nothing about PVID */
 	ret = rtl8365mb_vlan_4k_port_add(ds, port, vlan, extack);
 	if (ret)
-		goto undo_set_pvid;
+		goto undo_commit_pvid;
 
-	ret = 0;
 	goto out_unlock;
 
-undo_set_pvid:
-	/* undo the pvid definition */
-	if (pvid != (pvid_vid == vlan->vid)) {
-		if (pvid_vid)
-			(void)rtl8365mb_vlan_pvid_port_set(ds, port, pvid_vid,
-							   NULL);
-		else
-			(void)rtl8365mb_vlan_pvid_port_clear(ds, port,
-							     vlan->vid);
-	}
+undo_commit_pvid:
+	*bridge_pvid = prev_bridge_pvid;
+	(void)rtl8365mb_port_commit_pvid(ds, port, vlan_filtering, NULL);
+	goto out_unlock;
+undo_bridge_pvid:
+	*bridge_pvid = prev_bridge_pvid;
 out_unlock:
 	mutex_unlock(&priv->vlan_lock);
 	return ret;
@@ -2183,6 +2180,10 @@ static int rtl8365mb_port_vlan_del(struct dsa_switch *ds, int port,
 	bool untagged = !!(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED);
 	bool pvid = !!(vlan->flags & BRIDGE_VLAN_INFO_PVID);
 	struct realtek_priv *priv = ds->priv;
+	struct rtl8365mb *mb = priv->chip_data;
+	u16 prev_bridge_pvid;
+	u16 *bridge_pvid;
+	bool vlan_filtering;
 	int ret;
 
 	dev_dbg(priv->dev, "del VLAN %d on port %d, %s, %s\n",
@@ -2193,10 +2194,20 @@ static int rtl8365mb_port_vlan_del(struct dsa_switch *ds, int port,
 	if (vlan->vid == 0)
 		return -EOPNOTSUPP;
 
+	vlan_filtering = dsa_port_is_vlan_filtering(dsa_to_port(ds, port));
+	bridge_pvid = &mb->ports[port].bridge_pvid;
+
 	mutex_lock(&priv->vlan_lock);
-	ret = rtl8365mb_vlan_pvid_port_clear(ds, port, vlan->vid);
-	if (ret)
+	prev_bridge_pvid = *bridge_pvid;
+	if (*bridge_pvid == vlan->vid)
+		*bridge_pvid = 0;
+
+	ret = rtl8365mb_port_commit_pvid(ds, port, vlan_filtering, NULL);
+	if (ret) {
+		/* keep the state in line with the PVID still in hardware */
+		*bridge_pvid = prev_bridge_pvid;
 		goto out_unlock;
+	}
 
 	ret = rtl8365mb_vlan_4k_port_del(ds, port, vlan);
 	/* There is little incentive to try to undo the removal of PVID (if it
diff --git a/drivers/net/dsa/realtek/rtl8365mb_vlan.c b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
index 58948e76f..64acd762e 100644
--- a/drivers/net/dsa/realtek/rtl8365mb_vlan.c
+++ b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
@@ -42,7 +42,8 @@
  *    may be useful for ACL rules which specify alternative forwarding
  *    decisions.
  *
- * VLANMC index 0 is reserved as a neutral PVID, used for standalone ports.
+ * VLANMC index 0 is reserved as a neutral PVID, used for standalone ports and
+ * for the ports of a VLAN-unaware bridge.
  *
  */
 
@@ -780,35 +781,41 @@ rtl8365mb_vlan_port_set_framefilter(struct realtek_priv *priv,
 }
 
 /*
- * rtl8365mb_vlan_pvid_port_set() - Configure a port's PVID and associated
- * VLANMC entry
+ * rtl8365mb_vlan_pvid_port_commit() - Program a port's PVID and frame filter
  * @ds: dsa switch instance
  * @port: port index
- * @vid: target VID
+ * @vid: VID to use as PVID, or 0 for the neutral PVID (VLANMC index 0)
+ * @vlan_filtering: whether the port filters VLANs
  * @extack: netlink extended ACK for error reporting
  *
- * Allocates or reuses a hardware VLANMC entry to map the given port to its new
- * PVID. Gracefully unwinds and restores previous configuration if a hardware
- * write operation fails during execution.
+ * Points the port at the VLANMC entry of @vid, allocating it if needed, and
+ * drops the port from the VLANMC entry of its previous PVID, which is freed
+ * once no port uses it anymore. Untagged frames are accepted unless the port
+ * filters VLANs and has no PVID. Restores the previous configuration if a
+ * write fails.
  *
  * 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.
  */
-int rtl8365mb_vlan_pvid_port_set(struct dsa_switch *ds, int port, u16 vid,
-				 struct netlink_ext_ack *extack)
+int rtl8365mb_vlan_pvid_port_commit(struct dsa_switch *ds, int port, u16 vid,
+				    bool vlan_filtering,
+				    struct netlink_ext_ack *extack)
 {
 	enum rtl8365mb_frame_ingress accepted_frame, prev_accepted_frame;
 	struct realtek_priv *priv = ds->priv;
-	struct rtl8365mb_vlanmc prev_vlanmc = {0};
+	struct rtl8365mb_vlanmc prev_vlanmc;
 	u8 _unused_first_free_idx;
+	u8 vlanmc_idx = 0;
 	u8 prev_vlanmc_idx;
-	u8 vlanmc_idx;
+	bool vid_changed;
 	int ret;
 
 	lockdep_assert_held(&priv->vlan_lock);
 
-	/* Read the old PVID exclusively to undo in case of error */
+	/* The VLANMC entry of a PVID always has a non-zero VID, so the previous
+	 * VID reads as 0 if the port used the neutral PVID.
+	 */
 	ret = rtl8365mb_vlan_get_pvid_mc(priv, port, &prev_vlanmc_idx,
 					 &prev_vlanmc);
 	if (ret) {
@@ -823,153 +830,80 @@ int rtl8365mb_vlan_pvid_port_set(struct dsa_switch *ds, int port, u16 vid,
 		return ret;
 	}
 
-	/* Find or allocate a new vlan MC and add port to members,
-	 * although members are not checked by the HW in vlan MC.
-	 */
-	ret = rtl8365mb_vlan_mc_port_set(ds, port, vid, true, extack, true);
-	if (ret)
-		return ret;
-
-	/* look for existing entry */
-	ret = rtl8365mb_vlan_mc_find(priv, vid, &vlanmc_idx,
-				     &_unused_first_free_idx);
-	if (ret) {
-		dev_err(priv->dev, "Failed to find a VLAN MC table index\n");
-		goto undo_vlan_mc_port_set;
-	}
-
-	if (!vlanmc_idx) {
-		dev_err(priv->dev, "VLAN should already exist in VLAN MC\n");
-		ret = -ENOENT;
-		goto undo_vlan_mc_port_set;
-	}
-
-	ret = rtl8365mb_vlan_port_set_pvid(priv, port, vlanmc_idx);
-	if (ret) {
-		dev_err(priv->dev, "Failed to set port PVID\n");
-		goto undo_vlan_mc_port_set;
-	}
+	vid_changed = vid != prev_vlanmc.evid;
 
-	/* Changing accept frame is what enables PVID (if not enabled before) */
-	accepted_frame = RTL8365MB_FRAME_TYPE_ANY_FRAME;
-	ret = rtl8365mb_vlan_port_set_framefilter(priv, port, accepted_frame);
-	if (ret) {
-		dev_err(priv->dev, "Failed to set port frame filter\n");
-		goto undo_vlan_port_set_pvid;
-	}
+	if (vid_changed && vid) {
+		/* Find or allocate a VLANMC entry and add the port to its
+		 * members, which only serve as a reference count.
+		 */
+		ret = rtl8365mb_vlan_mc_port_set(ds, port, vid, true, extack,
+						 true);
+		if (ret)
+			return ret;
 
-	/* A VLAN can be added with PVID without removing from the old
-	 * PVID VLAN. Clear PVID from the old VLAN MC (if needed).
-	 */
-	if (prev_vlanmc_idx && (prev_vlanmc.evid != vid)) {
-		ret = rtl8365mb_vlan_mc_port_set(ds, port, prev_vlanmc.evid,
-						 false, NULL, false);
+		ret = rtl8365mb_vlan_mc_find(priv, vid, &vlanmc_idx,
+					     &_unused_first_free_idx);
 		if (ret) {
-			dev_err(priv->dev, "Failed to clear old VLAN MC\n");
-			goto undo_set_framefilter;
+			dev_err(priv->dev,
+				"Failed to find a VLAN MC table index\n");
+			goto undo_vlan_mc_port_set;
 		}
-	}
-
-	return 0;
-
-undo_set_framefilter:
-	(void)rtl8365mb_vlan_port_set_framefilter(priv, port,
-						  prev_accepted_frame);
-
-undo_vlan_port_set_pvid:
-	(void)rtl8365mb_vlan_port_set_pvid(priv, port, prev_vlanmc_idx);
 
-undo_vlan_mc_port_set:
-	if (prev_vlanmc.evid != vid)
-		(void)rtl8365mb_vlan_mc_port_set(ds, port, vid, false, NULL,
-						 false);
-
-	return ret;
-}
-
-/*
- * rtl8365mb_vlan_pvid_port_clear() - Remove a port's PVID configuration
- * @ds: dsa switch instance
- * @port: port index
- * @vid:  VLAN VID for PVID
- *
- * Resets the target port's hardware PVID allocation to 0. Cleans up and frees
- * the associated VLANMC entry if no other ports are referencing it.
- *
- * 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.
- */
-int rtl8365mb_vlan_pvid_port_clear(struct dsa_switch *ds, int port, u16 vid)
-{
-	enum rtl8365mb_frame_ingress accepted_frame, prev_accepted_frame;
-	struct realtek_priv *priv = ds->priv;
-	struct rtl8365mb_vlanmc vlanmc = {0};
-	u8 vlanmc_idx;
-	int ret;
-
-	lockdep_assert_held(&priv->vlan_lock);
-
-	ret = rtl8365mb_vlan_get_pvid_mc(priv, port, &vlanmc_idx,
-					 &vlanmc);
-	if (ret) {
-		dev_err(priv->dev, "Failed to read current VLAN MC\n");
-		return ret;
+		if (!vlanmc_idx) {
+			dev_err(priv->dev,
+				"VLAN should already exist in VLAN MC\n");
+			ret = -ENOENT;
+			goto undo_vlan_mc_port_set;
+		}
 	}
 
-	/* Port is not using PVID. Nothing to remove. */
-	if (!vlanmc_idx)
-		return 0;
-
-	/* We are leaving a non PVID vlan, Nothing to remove. */
-	if (vlanmc.evid != vid)
-		return 0;
-
-	ret = rtl8365mb_vlan_port_get_framefilter(priv, port,
-						  &prev_accepted_frame);
-	if (ret) {
-		dev_err(priv->dev, "Failed to get current framefilter\n");
-		return ret;
+	if (vid_changed) {
+		ret = rtl8365mb_vlan_port_set_pvid(priv, port, vlanmc_idx);
+		if (ret) {
+			dev_err(priv->dev, "Failed to set port PVID\n");
+			goto undo_vlan_mc_port_set;
+		}
 	}
 
-	/* Changing accept frame is what really removes PVID. But only do
-	 * that if we are filtering vlan
-	 */
-	if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port))) {
+	if (vlan_filtering && !vid)
 		accepted_frame = RTL8365MB_FRAME_TYPE_TAGGED_ONLY;
+	else
+		accepted_frame = RTL8365MB_FRAME_TYPE_ANY_FRAME;
 
+	if (accepted_frame != prev_accepted_frame) {
 		ret = rtl8365mb_vlan_port_set_framefilter(priv, port,
 							  accepted_frame);
 		if (ret) {
 			dev_err(priv->dev, "Failed to set port frame filter\n");
-			return ret;
+			goto undo_vlan_port_set_pvid;
 		}
-	} else {
-		/* skip undo_set_framefilter */
-		accepted_frame = prev_accepted_frame;
 	}
 
-	ret = rtl8365mb_vlan_port_set_pvid(priv, port, 0);
-	if (ret) {
-		dev_err(priv->dev, "Failed to set port PVID to 0\n");
-		goto undo_set_framefilter;
+	/* Drop the port from the VLANMC entry of its previous PVID */
+	if (vid_changed && prev_vlanmc_idx) {
+		ret = rtl8365mb_vlan_mc_port_set(ds, port, prev_vlanmc.evid,
+						 false, NULL, false);
+		if (ret) {
+			dev_err(priv->dev, "Failed to clear old VLAN MC\n");
+			goto undo_set_framefilter;
+		}
 	}
 
-	/* Clears the VLAN MC membership and maybe VLAN MC entry if empty */
-	ret = rtl8365mb_vlan_mc_port_set(ds, port, vlanmc.evid,
-					 false, NULL, false);
-	if (ret)
-		goto undo_port_set_pvid;
-
 	return 0;
 
-undo_port_set_pvid:
-	(void)rtl8365mb_vlan_port_set_pvid(priv, port, vlanmc_idx);
-
 undo_set_framefilter:
-	if (prev_accepted_frame != accepted_frame)
+	if (accepted_frame != prev_accepted_frame)
 		(void)rtl8365mb_vlan_port_set_framefilter(priv, port,
 							  prev_accepted_frame);
 
+undo_vlan_port_set_pvid:
+	if (vid_changed)
+		(void)rtl8365mb_vlan_port_set_pvid(priv, port, prev_vlanmc_idx);
+
+undo_vlan_mc_port_set:
+	if (vid_changed && vid)
+		(void)rtl8365mb_vlan_mc_port_set(ds, port, vid, false, NULL,
+						 false);
+
 	return ret;
 }
diff --git a/drivers/net/dsa/realtek/rtl8365mb_vlan.h b/drivers/net/dsa/realtek/rtl8365mb_vlan.h
index 809034009..4c83c9475 100644
--- a/drivers/net/dsa/realtek/rtl8365mb_vlan.h
+++ b/drivers/net/dsa/realtek/rtl8365mb_vlan.h
@@ -34,7 +34,7 @@ int rtl8365mb_vlan_4k_port_add(struct dsa_switch *ds, int port,
 			       struct netlink_ext_ack *extack);
 int rtl8365mb_vlan_4k_port_del(struct dsa_switch *ds, int port,
 			       const struct switchdev_obj_port_vlan *vlan);
-int rtl8365mb_vlan_pvid_port_set(struct dsa_switch *ds, int port, u16 vid,
-				 struct netlink_ext_ack *extack);
-int rtl8365mb_vlan_pvid_port_clear(struct dsa_switch *ds, int port, u16 vid);
+int rtl8365mb_vlan_pvid_port_commit(struct dsa_switch *ds, int port, u16 vid,
+				    bool vlan_filtering,
+				    struct netlink_ext_ack *extack);
 #endif /* _REALTEK_RTL8365MB_VLAN_H */
-- 
2.56.0

Re: [PATCH net v2 2/2] net: dsa: realtek: rtl8365mb: keep the neutral PVID in VLAN-unaware bridges

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In VLAN-unaware bridges, rtl8365mb_port_commit_pvid() now sets
  pvid = 0 (VLANMC index 0, EVID 0).
- [Low] This patch removes the only callers of
  rtl8365mb_vlan_port_get_pvid() (in rtl8365mb_port_vlan_filtering() and…
- [Low] The patch updates the file-header comment in rtl8365mb_vlan.c to
  say that VLANMC index 0 is the neutral PVID "for standalone ports and…

--

net: dsa: realtek: rtl8365mb: keep the neutral PVID in VLAN-unaware bridges

The patch stores the bridge PVID per port in bridge_pvid and only programs
it while the port filters VLANs. Ports of a VLAN-unaware bridge keep the
neutral PVID (VLANMC index 0, VID 0), so untagged frames match the VID 0
FDB/MDB entries that DSA installs. It also replaces
rtl8365mb_vlan_pvid_port_set() and rtl8365mb_vlan_pvid_port_clear() with
rtl8365mb_vlan_pvid_port_commit(), which programs both the PVID and the
accepted frame types.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index b90e46c165bf8..4f0e0ab7f5023 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -2004,42 +2007,44 @@ rtl8365mb_port_set_vlan_egress_mode(struct realtek_priv *priv, int port,
>  			RTL8365MB_PORT_MISC_CFG_VLAN_EGRESS_MODE_MASK, val);
>  }
>  
> +/* Program the PVID that the port should use for untagged frames: the PVID of
> + * the bridge while the port filters VLANs, and the neutral PVID (VID 0)
> + * otherwise. A VLAN-unaware bridge, like a standalone port, gets its FDB and
> + * MDB entries with VID 0 from DSA. As the switch looks up its forwarding
> + * database by the VLAN that it classifies a frame to, a bridge PVID would make
> + * those entries miss every lookup.
> + */
> +static int rtl8365mb_port_commit_pvid(struct dsa_switch *ds, int port,
> +				      bool vlan_filtering,
> +				      struct netlink_ext_ack *extack)
> +{
> +	struct realtek_priv *priv = ds->priv;
> +	struct rtl8365mb *mb = priv->chip_data;
> +	u16 pvid;
> +
> +	if (vlan_filtering)
> +		pvid = mb->ports[port].bridge_pvid;
> +	else
> +		pvid = 0;

[Severity: High]
With this change, untagged frames from every VLAN-unaware bridge are
classified to VID 0, which is the VID standalone ports already use. The
previous commit made VID 0 an IVL VLAN. Taken together, do multicast
lookups from all bridges and all standalone ports now land in one shared
{MAC, VID 0} space?

The commit message says:

  "The per-bridge EFID keeps these lookups local to the bridge."

That looks true for unicast only. Hardware multicast entries have no EFID
field, and rtl8365mb_l2_add_mc() says so:

drivers/net/dsa/realtek/rtl8365mb_l2.c:rtl8365mb_l2_add_mc() {
    ...
		 * Entries from different bridges will collide on the same
		 * MAC+VID slot with an OR'ed member mask, but packets will
		 * only exit through ports allowed by the source port's pmask.
    ...
}

Also, rtl83xx_port_mdb_add() accepts both DSA_DB_PORT and DSA_DB_BRIDGE,
and it only uses the EFID in a dev_dbg():

	/* EFID is not used by hardware MDB entries; debugging only */

Before this patch, bridge traffic was classified to the bridge PVID
(VID 1). No VID 0 multicast entry could match it, so unregistered
multicast was flooded within the pmask. Three cases look affected now.

First, DSA installs DSA_DB_PORT host entries at VID 0 for every user
port's mc list, whether the port is bridged or not:

dsa_user_sync_mc()
  -> dsa_user_vlan_for_each()      /* always calls cb(arg, 0) */
  -> dsa_user_standalone_event_work()
    -> dsa_port_standalone_host_mdb_add()

Say an untagged frame arrives on lan1 for 33:33:00:00:00:01, for example
an IPv6 RA or an MLD general query from an upstream router. It now hits a
{CPU}-only entry. Does it still get flooded to the other ports of the
bridge?

rtl8_4_read_tag() calls dsa_default_offload_fwd_mark() for any frame that
was not trapped. That means the software bridge would not re-flood the
frame either. IPv6 33:33:xx groups use DMAC lookup. Whether IPv4 groups
do as well depends on the IPMC lookup mode, which the driver does not
configure.

Second, suppose bridge B snoops a group G and installs {G, VID 0} -> {b1}.
Would that entry now also match traffic for G inside bridge A? The result
would be member & pmask(a1) = {} (dropped) or {CPU}, where bridge A would
otherwise have flooded the unregistered group.

Third, DSA refcounts CPU port MDB entries per database:
dsa_port_do_mdb_del() looks up {addr, vid, db}. However,
rtl8365mb_l2_del_mc() still clears the port bit on the single shared
slot:

	mc.member &= ~BIT(port);

When one bridge or standalone port leaves G, does this remove host
delivery of G for the other databases that still need it?

Does this need per-bridge separation of VID 0 multicast, for example a
distinct classification VID or FID per VLAN-unaware bridge? The other
option would be per-database handling of the shared slot.

> +
> +	return rtl8365mb_vlan_pvid_port_commit(ds, port, pvid, vlan_filtering,
> +					       extack);
> +}
> +
>  static int rtl8365mb_port_vlan_filtering(struct dsa_switch *ds, int port,
>  					 bool vlan_filtering,
>  					 struct netlink_ext_ack *extack)
>  {
> -	enum rtl8365mb_frame_ingress accepted_frame, prev_accepted_frame;
>  	enum rtl8365mb_vlan_egress_mode mode;
>  	struct realtek_priv *priv = ds->priv;
>  	u32 configured_ports = 0;
>  	struct dsa_port *dp;
> -	u16 pvid_vid;
>  	int ret;
>  
>  	dev_dbg(priv->dev, "port %d: %s VLAN filtering\n", port,
>  		vlan_filtering ? "enable" : "disable");
>  
> -	ret = rtl8365mb_vlan_port_get_framefilter(priv, port,
> -						  &prev_accepted_frame);
> -	if (ret) {
> -		NL_SET_ERR_MSG_MOD(extack,
> -				   "Failed to get current framefilter");
> -		return ret;
> -	}
> -
> -	/* While filtering, only accepts untagged frames if PVID is enabled */
> -	if (vlan_filtering) {
> -		ret = rtl8365mb_vlan_port_get_pvid(priv, port, &pvid_vid);

[Severity: Low]
This hunk and the one in rtl8365mb_port_vlan_add() remove the only
callers of rtl8365mb_vlan_port_get_pvid(). The function is still defined
non-static in rtl8365mb_vlan.c and declared in rtl8365mb_vlan.h, so the
compiler will not warn that it is unused. Can it be dropped?

Similarly, the only remaining users of
rtl8365mb_vlan_port_get_framefilter() and
rtl8365mb_vlan_port_set_framefilter() are in
rtl8365mb_vlan_pvid_port_commit() in rtl8365mb_vlan.c. Could they be made
static and removed from the header?

> -		if (ret)
> -			return ret;

[ ... ]

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_vlan.c b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> index 58948e76f122a..64acd762e912f 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> @@ -42,7 +42,8 @@
>   *    may be useful for ACL rules which specify alternative forwarding
>   *    decisions.
>   *
> - * VLANMC index 0 is reserved as a neutral PVID, used for standalone ports.
> + * VLANMC index 0 is reserved as a neutral PVID, used for standalone ports and
> + * for the ports of a VLAN-unaware bridge.

[Severity: Low]
This isn't a bug, but the matching comment in rtl8365mb_vlan_mc_find()
was not updated. It still says:

	/* By design, VlanMC[0] is reserved as a neutral PVID value for
	 * standalone ports. It always has EVID == 0. That way, we assume that

Should it also mention VLAN-unaware bridge ports?

>   *
>   */

[ ... ]

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