[PATCH net 0/3] net: dsa: motorcomm: FDB fixup

[PATCH net 0/3] net: dsa: motorcomm: FDB fixup

[David Yang]

Various FDB defects. Assisted and verified on the hardware by LLM.

There are more gotcha about multi-bridge setup, but I'd leave that for
net-next since multi-bridge has always been tricky among DSA drivers.

David Yang (3):
  net: dsa: motorcomm: Do not inherit ports from dynamic FDB entries
  net: dsa: motorcomm: Rewrite FDB entry on port migration
  net: dsa: motorcomm: Use VID 0 as the unaware PVID

 drivers/net/dsa/motorcomm/chip.c | 111 +++++++++++++++----------------
 1 file changed, 52 insertions(+), 59 deletions(-)

-- 
2.53.0

[PATCH net 1/3] net: dsa: motorcomm: Do not inherit ports from dynamic FDB entries

[David Yang]

When adding an fdb/mdb entry whose (address, VID) key already exists in
hardware, the existing egress ports were silently merged into the new
static entry. Ports learned by hardware are untracked by DSA and can
never be deleted. Replace the port mask instead of merging when the
existing entry is not static.

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")
Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/chip.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 33e5fc127244..150c51f1aa35 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -799,7 +799,10 @@ yt921x_fdb_join(struct yt921x_priv *priv, const unsigned char *addr, u16 vid,
 
 	ctrl1 = val1 & ~YT921X_FDB_IO1_STATUS_M;
 	ctrl1 |= YT921X_FDB_IO1_STATUS_STATIC;
-	ctrl2 = val2 | YT921X_FDB_IO2_EGR_PORTS(ports_mask);
+	ctrl2 = val2;
+	if (ctrl1 != val1)
+		ctrl2 &= ~YT921X_FDB_IO2_EGR_PORTS_M;
+	ctrl2 |= YT921X_FDB_IO2_EGR_PORTS(ports_mask);
 	if (ctrl1 == val1 && ctrl2 == val2)
 		return 0;
 
-- 
2.53.0

[PATCH net 2/3] net: dsa: motorcomm: Rewrite FDB entry on port migration

[David Yang]

When unicast addresses move between ports, DSA calls .port_fdb_add()
without a prior .port_fdb_del() call, making the new port directly OR
into the existing entries. The old port mask then loses track in DSA and
can never be deleted.

Fuse FDB join/leave into one change function, and rewrite the whole
entry on add with the bridge port mask as the leave mask, so that ports
left behind by the migration are dropped from the entry.

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")
Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/chip.c | 96 ++++++++++++++++----------------
 1 file changed, 49 insertions(+), 47 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 150c51f1aa35..a2be819ccbed 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -729,13 +729,14 @@ yt921x_fdb_add(struct yt921x_priv *priv, const unsigned char *addr, u16 vid,
 }
 
 static int
-yt921x_fdb_leave(struct yt921x_priv *priv, const unsigned char *addr,
-		 u16 vid, u16 ports_mask)
+yt921x_fdb_update(struct yt921x_priv *priv, const unsigned char *addr,
+		  u16 vid, u16 leave_ports_mask, u16 join_ports_mask)
 {
 	u16 index;
 	u32 ctrl1;
 	u32 ctrl2;
 	u32 ctrl;
+	u32 val1;
 	u32 val2;
 	u32 val;
 	int res;
@@ -744,69 +745,63 @@ yt921x_fdb_leave(struct yt921x_priv *priv, const unsigned char *addr,
 	res = yt921x_fdb_has(priv, addr, vid, &index);
 	if (res)
 		return res;
-	if (index >= YT921X_FDB_NUM)
-		return 0;
+	if (index >= YT921X_FDB_NUM) {
+		if (!join_ports_mask)
+			return 0;
+
+		return yt921x_fdb_add(priv, addr, vid, join_ports_mask);
+	}
 
 	/* Check if action required */
 	res = yt921x_reg_read(priv, YT921X_FDB_OUT2, &val2);
 	if (res)
 		return res;
 
-	ctrl2 = val2 & ~YT921X_FDB_IO2_EGR_PORTS(ports_mask);
-	if (ctrl2 == val2)
-		return 0;
-	if (!(ctrl2 & YT921X_FDB_IO2_EGR_PORTS_M)) {
-		ctrl = YT921X_FDB_OP_OP_DEL | YT921X_FDB_OP_START;
-		res = yt921x_reg_write(priv, YT921X_FDB_OP, ctrl);
-		if (res)
-			return res;
+	ctrl2 = val2 & ~YT921X_FDB_IO2_EGR_PORTS(leave_ports_mask);
+	if (!join_ports_mask) {
+		if (ctrl2 == val2)
+			return 0;
 
-		return yt921x_fdb_wait(priv, &val);
+		if (!(ctrl2 & YT921X_FDB_IO2_EGR_PORTS_M)) {
+			ctrl = YT921X_FDB_OP_OP_DEL | YT921X_FDB_OP_START;
+			res = yt921x_reg_write(priv, YT921X_FDB_OP, ctrl);
+			if (res)
+				return res;
+
+			return yt921x_fdb_wait(priv, &val);
+		}
 	}
 
-	res = yt921x_reg_read(priv, YT921X_FDB_OUT1, &ctrl1);
+	res = yt921x_reg_read(priv, YT921X_FDB_OUT1, &val1);
 	if (res)
 		return res;
 
+	ctrl1 = val1;
+	if (join_ports_mask) {
+		ctrl1 &= ~YT921X_FDB_IO1_STATUS_M;
+		ctrl1 |= YT921X_FDB_IO1_STATUS_STATIC;
+		if (ctrl1 != val1)
+			ctrl2 &= ~YT921X_FDB_IO2_EGR_PORTS_M;
+	}
+	ctrl2 |= YT921X_FDB_IO2_EGR_PORTS(join_ports_mask);
+	if (ctrl1 == val1 && ctrl2 == val2)
+		return 0;
+
 	return yt921x_fdb_add_index_in12(priv, index, ctrl1, ctrl2);
 }
 
+static int
+yt921x_fdb_leave(struct yt921x_priv *priv, const unsigned char *addr,
+		 u16 vid, u16 ports_mask)
+{
+	return yt921x_fdb_update(priv, addr, vid, ports_mask, 0);
+}
+
 static int
 yt921x_fdb_join(struct yt921x_priv *priv, const unsigned char *addr, u16 vid,
 		u16 ports_mask)
 {
-	u16 index;
-	u32 ctrl1;
-	u32 ctrl2;
-	u32 val1;
-	u32 val2;
-	int res;
-
-	/* Check for presence */
-	res = yt921x_fdb_has(priv, addr, vid, &index);
-	if (res)
-		return res;
-	if (index >= YT921X_FDB_NUM)
-		return yt921x_fdb_add(priv, addr, vid, ports_mask);
-
-	/* Check if action required */
-	res = yt921x_reg_read(priv, YT921X_FDB_OUT1, &val1);
-	if (res)
-		return res;
-	res = yt921x_reg_read(priv, YT921X_FDB_OUT2, &val2);
-	if (res)
-		return res;
-
-	ctrl1 = val1 & ~YT921X_FDB_IO1_STATUS_M;
-	ctrl1 |= YT921X_FDB_IO1_STATUS_STATIC;
-	ctrl2 = val2;
-	if (ctrl1 != val1)
-		ctrl2 &= ~YT921X_FDB_IO2_EGR_PORTS_M;
-	ctrl2 |= YT921X_FDB_IO2_EGR_PORTS(ports_mask);
-	if (ctrl1 == val1 && ctrl2 == val2)
-		return 0;
-
-	return yt921x_fdb_add_index_in12(priv, index, ctrl1, ctrl2);
+	return yt921x_fdb_update(priv, addr, vid, 0, ports_mask);
 }
 
 static int
@@ -877,10 +872,17 @@ yt921x_dsa_port_fdb_add(struct dsa_switch *ds, int port,
 			const unsigned char *addr, u16 vid, struct dsa_db db)
 {
 	struct yt921x_priv *priv = dsa_to_yt921x_priv(ds);
+	struct dsa_port *dp = dsa_to_port(ds, port);
+	u32 bridge_ports_mask = 0;
+	struct net_device *bdev;
 	int res;
 
+	bdev = dsa_port_bridge_dev_get(dp);
+	if (bdev)
+		bridge_ports_mask = dsa_bridge_ports(ds, bdev);
+
 	mutex_lock(&priv->reg_lock);
-	res = yt921x_fdb_join(priv, addr, vid, BIT(port));
+	res = yt921x_fdb_update(priv, addr, vid, bridge_ports_mask, BIT(port));
 	mutex_unlock(&priv->reg_lock);
 
 	return res;
-- 
2.53.0

Re: [PATCH net 2/3] net: dsa: motorcomm: Rewrite FDB entry on port migration

[netdev-bot+sashiko]

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

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

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] The fix is incomplete: an address that moves between a user port
  and the host (CPU port) still leaves a stale port bit in the hardware…
- [Low] yt921x_dsa_port_fdb_add() now calls dsa_bridge_ports(ds, bdev).

--

net: dsa: motorcomm: Rewrite FDB entry on port migration

This commit merges yt921x_fdb_join() and yt921x_fdb_leave() into a single
yt921x_fdb_update() helper that takes separate leave and join port masks.
yt921x_dsa_port_fdb_add() now passes the bridge port mask as the leave
mask, so ports left behind when an address migrates are dropped from the
hardware entry.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index f34f37b736249..d84e1f6dad843 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -877,10 +872,17 @@ yt921x_dsa_port_fdb_add(struct dsa_switch *ds, int port,
>  			const unsigned char *addr, u16 vid, struct dsa_db db)
>  {
>  	struct yt921x_priv *priv = dsa_to_yt921x_priv(ds);
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	u32 bridge_ports_mask = 0;
> +	struct net_device *bdev;
>  	int res;
>  
> +	bdev = dsa_port_bridge_dev_get(dp);
> +	if (bdev)
> +		bridge_ports_mask = dsa_bridge_ports(ds, bdev);

[Severity: Low]
Is it safe to walk every port's dp->bridge here without rtnl?

dsa_bridge_ports() does a plain load of dp->bridge, then reads
dp->bridge->dev, for each user port via dsa_port_bridge_dev_get(). This
path runs from dsa_user_switchdev_event_work() on dsa_owq, and rtnl_lock is
not held there. Before this patch, the driver only called
dsa_bridge_ports() from the bridge_join and bridge_flags paths, which hold
rtnl.

The bridge leave path looks covered. dsa_port_pre_bridge_leave() flushes
dsa_owq under rtnl before dsa_port_bridge_destroy() runs. The rollback in
dsa_port_bridge_join() in net/dsa/port.c does not flush first, though:

out_rollback:
	dsa_port_bridge_destroy(dp, br);

Say FDB work queued by an earlier static fdb add on port 1 is still running
when a bridge join on port 2 fails. The work could load dp2->bridge just
before the kfree() and then read ->dev from freed memory.

The loaded value is only compared with bdev, so the practical impact is
small. Separately, bridge_join stores to dp->bridge on another port with a
plain write. That is a data race with this reader that KCSAN can see.

DSA core already does similar lockless cross-port reads in
dsa_foreign_dev_check() and dsa_tree_offloads_bridge_dev().

Could the port's own bridge come from the db argument (db.bridge) rather
than re-reading dp->bridge outside rtnl?

> +
>  	mutex_lock(&priv->reg_lock);
> -	res = yt921x_fdb_join(priv, addr, vid, BIT(port));
> +	res = yt921x_fdb_update(priv, addr, vid, bridge_ports_mask, BIT(port));

[Severity: High]
Does this still leave a stale port bit when an address moves between a user
port and the host?

The leave mask comes from the target port's own dp->bridge. This driver
sets ds->assisted_learning_on_cpu_port, so DSA installs host addresses by
calling .port_fdb_add() with port set to the CPU port. dp->bridge is only
set on user ports, so bdev is NULL and bridge_ports_mask is 0.

yt921x_fdb_update(priv, addr, vid, 0, BIT(cpu)) then keeps every existing
egress bit. The existing entry is already STATIC, so ctrl1 == val1 and the
full clear in yt921x_fdb_update() is skipped as well:

	ctrl1 = val1;
	if (join_ports_mask) {
		ctrl1 &= ~YT921X_FDB_IO1_STATUS_M;
		ctrl1 |= YT921X_FDB_IO1_STATUS_STATIC;
		if (ctrl1 != val1)
			ctrl2 &= ~YT921X_FDB_IO2_EGR_PORTS_M;
	}
	ctrl2 |= YT921X_FDB_IO2_EGR_PORTS(join_ports_mask);

The result is still an OR-in, not a rewrite of the whole entry.

One way to reach this: take a static entry on swp1 and use bridge fdb
replace to point it at a foreign interface in the same bridge, or replace
it with a local/permanent entry. fdb_add_entry()->fdb_update_dst() only
sends RTM_NEWNEIGH, so swp1 never gets a SWITCHDEV_FDB_DEL_TO_DEVICE:

fdb_add_entry()
  fdb_update_dst()
  fdb_notify(br, fdb, RTM_NEWNEIGH, true)
    dsa_user_fdb_event()            /* host_addr = true */
      dsa_port_bridge_host_fdb_add()
        dsa_switch_host_fdb_add()
          dsa_port_do_fdb_add(cpu_dp)
            yt921x_dsa_port_fdb_add()   /* port == CPU port */

The swp1 bit then stays in a STATIC entry that DSA no longer tracks. A
later host fdb del clears only the CPU bit. Fast age through
yt921x_fdb_flush_port(..., false) does not flush static entries. Unicast
frames for that MAC keep going out of swp1.

The reverse direction seems to have the same problem.
dsa_switch_for_each_bridge_member() only iterates user ports, so the mask
from dsa_bridge_ports() never includes a CPU port. When a host address is
re-pointed to swp1, the old CPU port bit is never cleared, and frames for
that MAC also go to the CPU.

The commit message says:

  "rewrite the whole entry on add with the bridge port mask as the leave
  mask, so that ports left behind by the migration are dropped from the
  entry."

but egress bits for ports outside the bridge's user ports are kept.

The later patches in the series, up to "Use VID 0 as the unaware PVID",
don't appear to change yt921x_dsa_port_fdb_add() or yt921x_fdb_update().

For DSA_DB_BRIDGE host adds, db.bridge is a copy of *dp->bridge. Only .num
is zeroed, because fdb_isolation is not set.

Could db.bridge.dev be used to work out the bridge mask for CPU port
calls? Could the CPU ports also be added to the leave mask for user port
calls?

>  	mutex_unlock(&priv->reg_lock);
>  
>  	return res;

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

[PATCH net 3/3] net: dsa: motorcomm: Use VID 0 as the unaware PVID

[David Yang]

The hardware classifies untagged and priority-tagged frames to the
port's PVID before the VLAN table lookup, and the FDB lookup key equals
the classified VID (measured on YT9215S).

The driver assigned PVID 4095 to the VLAN-unaware ports, while switchdev
reports the bridge's FDB entries with VID 0. These entries were then
installed under FID 0 and could never be hit by the hardware lookups.
Dynamic entries learnt under VID 4095, if any, took effect instead.

Lift the restriction on VLAN 0, and use it as the PVID of the
VLAN-unaware ports instead. The standalone default of 4095 stays, so
mislearnt entries, if any, would not pollute the bridge FDB.

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")
Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/chip.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index a2be819ccbed..376f16466cdd 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -79,7 +79,6 @@ static const struct yt921x_info yt921x_infos[] = {
 	{}
 };
 
-#define YT921X_VID_UNWARE	4095
 
 struct yt921x_reg_mdio {
 	struct mii_bus *bus;
@@ -962,7 +961,7 @@ yt921x_vlan_filtering(struct yt921x_priv *priv, int port, bool vlan_filtering)
 	bdev = dsa_port_bridge_dev_get(dp);
 
 	if (!bdev || !vlan_filtering)
-		pvid = YT921X_VID_UNWARE;
+		pvid = 0;
 	else
 		br_vlan_get_pvid(bdev, &pvid);
 	res = yt921x_port_set_pvid(priv, port, pvid);
@@ -1030,8 +1029,7 @@ yt921x_pvid_clear(struct yt921x_priv *priv, int port)
 
 	vlan_filtering = dsa_port_is_vlan_filtering(dp);
 
-	res = yt921x_port_set_pvid(priv, port,
-				   vlan_filtering ? 0 : YT921X_VID_UNWARE);
+	res = yt921x_port_set_pvid(priv, port, 0);
 	if (res)
 		return res;
 
@@ -1188,7 +1186,7 @@ static int yt921x_userport_standalone(struct yt921x_priv *priv, int port)
 	/* Unrelated since learning is off and all packets are trapped;
 	 * set it anyway
 	 */
-	res = yt921x_port_set_pvid(priv, port, YT921X_VID_UNWARE);
+	res = yt921x_port_set_pvid(priv, port, 4095);
 	if (res)
 		return res;
 
@@ -2270,7 +2268,6 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
 {
 	struct dsa_switch *ds = &priv->ds;
 	unsigned long cpu_ports_mask;
-	u32 ctrls[2];
 	u32 ctrl;
 	int port;
 	int res;
@@ -2328,15 +2325,6 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
 	if (res)
 		return res;
 
-	/* Tagged VID 0 should be treated as untagged, which confuses the
-	 * hardware a lot
-	 */
-	ctrls[0] = YT921X_VLAN_CTRLa_LEARN_DIS | YT921X_VLAN_CTRLa_PORTS_M;
-	ctrls[1] = 0;
-	res = yt921x_reg64_write(priv, YT921X_VLANn_CTRL(0), ctrls);
-	if (res)
-		return res;
-
 	return 0;
 }
 
-- 
2.53.0

Re: [PATCH net 3/3] net: dsa: motorcomm: Use VID 0 as the unaware PVID

[netdev-bot+sashiko]

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

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

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 the most common case the fix never takes effect.
- [Medium] A port that leaves a VLAN-aware bridge ends up standalone with
  PVID 0 instead of 4095.
- [Medium] The patch removes the setup-time write in
  yt921x_chip_setup_dsa() that set the VID 0 VLAN entry to LEARN_DIS,…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  yt921x_vlan_filtering(), yt921x_dsa_port_vlan_add() and…

--

net: dsa: motorcomm: Use VID 0 as the unaware PVID

This patch changes the PVID of VLAN-unaware bridge ports in the yt921x
driver from 4095 to 0. The goal is for the switchdev FDB entries, which are
reported with VID 0, to be hit by hardware lookups. It also removes the
setup-time programming of the VID 0 VLAN entry, and standalone ports keep
PVID 4095.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index d84e1f6dad843..5cc8a21c9d55f 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -962,7 +961,7 @@ yt921x_vlan_filtering(struct yt921x_priv *priv, int port, bool vlan_filtering)
>  	bdev = dsa_port_bridge_dev_get(dp);
>  
>  	if (!bdev || !vlan_filtering)
> -		pvid = YT921X_VID_UNWARE;
> +		pvid = 0;

[Severity: High]

Does this assignment run when a standalone port joins a VLAN-unaware
bridge?

yt921x_port_setup() calls yt921x_userport_standalone(), so the port starts
at PVID 4095. The join path never touches PORTn_VLAN_CTRL:

yt921x_dsa_port_bridge_join()
  yt921x_bridge_join()
    yt921x_userport_bridge()   /* only clears YT921X_PORT_LEARN_DIS */
    yt921x_bridge()            /* only rewrites PORTn_ISOLATION */

The DSA core then calls dsa_port_vlan_filtering(dp, false) from
dsa_port_switchdev_sync_attrs(). That call returns early because the port
is already unaware, and the driver sets neither
needs_standalone_vlan_filtering nor vlan_filtering_is_global:

net/dsa/port.c:dsa_port_vlan_filtering() {
    ...
	if (dsa_port_is_vlan_filtering(dp) == vlan_filtering)
		return 0;
    ...
}

So yt921x_vlan_filtering() is never called. The replayed default_pvid VLAN
goes to yt921x_pvid_set(), which also skips the PVID write when the port is
not filtering:

	if (vlan_filtering) {
		res = yt921x_port_set_pvid(priv, port, vid);
		...
	}

In the default "ip link set lanX master br0" case with vlan_filtering=0,
does the hardware keep classifying untagged and priority-tagged frames to
VID 4095? If so, the VID 0 entries installed by yt921x_dsa_port_fdb_add()
would still never be hit, which is the problem this patch sets out to fix.

Before this patch, standalone and unaware ports both used 4095, so join did
not need a PVID write. Should yt921x_bridge_join() or
yt921x_userport_bridge() now set PVID 0 when the bridge is not VLAN
filtering?

[Severity: Medium]

The !bdev case now gets PVID 0 as well. What happens when a port leaves a
VLAN-aware bridge?

The commit message says "The standalone default of 4095 stays". However,
dsa_port_bridge_leave() runs things in this order:

dsa_port_bridge_leave()
  dsa_port_bridge_destroy()           /* dp->bridge = NULL */
  DSA_NOTIFIER_BRIDGE_LEAVE
    yt921x_bridge_leave()
      yt921x_userport_standalone()    /* PVID 4095 */
  dsa_port_switchdev_unsync_attrs()
    dsa_port_reset_vlan_filtering()
      dsa_port_vlan_filtering(dp, false)
        yt921x_vlan_filtering(priv, port, false)

At the last step dsa_port_bridge_dev_get(dp) returns NULL, so this branch
overwrites 4095 with 0. The old code wrote 4095 here.

Can the standalone port then classify into FID 0, which the VLAN-unaware
bridges now use? The comment in yt921x_chip_setup_dsa() says port
isolation is applied after the FDB lookup. A frame on the standalone port
whose DA hits a FID 0 entry for a bridged port would then be dropped by
isolation, rather than trapped to the CPU as unknown unicast.

The standalone PVID also starts to depend on history. It is 4095 after
setup or after leaving an unaware bridge, but 0 after leaving an aware
bridge. Should the !bdev case keep 4095?

>  	else
>  		br_vlan_get_pvid(bdev, &pvid);

[Severity: High]

This isn't a bug introduced by this patch, but is br_vlan_get_pvid() given
the right device here?

bdev comes from dsa_port_bridge_dev_get(dp), so it is the bridge master.
For a bridge master, br_vlan_get_pvid() returns the PVID of the bridge's
own VLAN group:

net/bridge/br_vlan.c:br_vlan_get_pvid() {
    ...
	else if (netif_is_bridge_master(dev))
		vg = br_vlan_group(netdev_priv(dev));
    ...
}

That is the host PVID, not the PVID of the port being configured. When
vlan_filtering is enabled, a port set up with pvid 10 would get the
bridge's PVID (for example 1) written into PORTn_VLAN_CTRL.CVID.

yt921x_dsa_port_vlan_add() and yt921x_dsa_port_vlan_del() make the same
comparison:

		bdev = dsa_port_bridge_dev_get(dp);
		if (bdev) {
			br_vlan_get_pvid(bdev, &pvid);
			if (pvid == vid)
				res = yt921x_pvid_clear(priv, port);
		}

Take the common sequence "bridge vlan add dev lanX vid 10 pvid untagged;
bridge vlan del dev lanX vid 1". When vid 1 is deleted, the bridge
master's PVID is still 1.

Would that call yt921x_pvid_clear(), write PVID 0 and set
CVLAN_DROP_UNTAGGED? All untagged ingress on lanX would then be dropped,
while the software bridge still thinks lanX has PVID 10.

>  	res = yt921x_port_set_pvid(priv, port, pvid);

[ ... ]

> @@ -2336,15 +2333,6 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
>  	if (res)
>  		return res;
>  
> -	/* Tagged VID 0 should be treated as untagged, which confuses the
> -	 * hardware a lot
> -	 */
> -	ctrls[0] = YT921X_VLAN_CTRLa_LEARN_DIS | YT921X_VLAN_CTRLa_PORTS_M;
> -	ctrls[1] = 0;
> -	res = yt921x_reg64_write(priv, YT921X_VLANn_CTRL(0), ctrls);
> -	if (res)
> -		return res;
> -

[Severity: Medium]

VID 0 is now the classification VLAN and FID for every VLAN-unaware
bridge. With this write removed, what programs the VID 0 entry?

The entry is left at hardware reset defaults for learning, membership and
untag. yt921x_dsa_port_vlan_add() and yt921x_dsa_port_vlan_del() have no
special case for VID 0, so the 8021q auto VID 0 can rewrite it.

8021q adds VID 0 through vlan_vid0_add() on NETDEV_UP when
NETIF_F_HW_VLAN_CTAG_FILTER is set. DSA sets that feature while the port
is VLAN filtering. The add reaches yt921x_vlan_add(priv, port, 0, false),
which sets the port and CPU member bits and clears UNTAG_PORTn.

When the bridge then switches to vlan_filtering=0:

dsa_user_manage_vlan_filtering(false)
  vlan_for_each(user, dsa_user_clear_vlan, user)
    yt921x_dsa_port_vlan_del(vid 0)
      yt921x_vlan_del(priv, port, 0)

and yt921x_vlan_del() does:

	masks[0] = YT921X_VLAN_CTRLa_PORTn(port);
	masks[1] = YT921X_VLAN_CTRLb_UNTAG_PORTn(port);

	return yt921x_reg64_clear_bits(priv, YT921X_VLANn_CTRL(vid), masks);

Does this remove the port from the VLAN entry that the unaware bridge now
forwards in?

VLAN-aware ports with no PVID also classify to VID 0, through
yt921x_pvid_clear() or yt921x_vlan_filtering() with pvid 0. LEARN_DIS is no
longer set on VID 0. Can frames accepted there, such as priority-tagged
frames if CVLAN_DROP_UNTAGGED does not cover them, be learnt into the FID 0
table that the unaware bridges use?

The comment in yt921x_dsa_port_vlan_add() also looks stale for VID 0:

	/* CPU port is supposed to be a member of every VLAN; see
	 * yt921x_vlan_add() and yt921x_port_setup()
	 */

Apart from 8021q, nothing adds the CPU port to VLAN 0 any more.

>  	return 0;
>  }

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