[PATCH net-next v4 0/8] net: dsa: motorcomm: Add BR_FLOOD and BR_MCAST_FLOOD support

[PATCH net-next v4 0/8] net: dsa: motorcomm: Add BR_FLOOD and BR_MCAST_FLOOD support

[David Yang]

Offload BR_FLOOD / BR_MCAST_FLOOD to the hardware for bridges when
feasible.

v4:
  - fix VID 0 handling
  - reset port flags on bridge leaving
  - remove local cpu_ports_mask
  - translate FDB VID for VLAN-unaware bridges
  - limit offloading to exactly one bridge
v3: https://lore.kernel.org/r/20261103140109.874443-1-mmyangfl@gmail.com
  - refactor port isolation and filter action for standalone ports
  - recompute flags when a port leaves bridge
  - honor BR_HAIRPIN
v2: https://lore.kernel.org/r/20261011030228.2761810-1-mmyangfl@gmail.com
  - fix build error
v1: https://lore.kernel.org/r/20261009005542.72860-1-mmyangfl@gmail.com

David Yang (8):
  net: dsa: motorcomm: Fix VLAN SVL and learn limit definitions
  net: dsa: motorcomm: Drop VID 0 VLAN table programming
  net: dsa: motorcomm: Reset port flags on bridge leaving
  net: dsa: motorcomm: Remove local cpu_ports_mask
  net: dsa: motorcomm: Isolate all ports during chip setup
  net: dsa: motorcomm: Rely on port isolation for conduit port
  net: dsa: motorcomm: Translate FDB VID for VLAN-unaware bridges
  net: dsa: motorcomm: Support BR_FLOOD and BR_MCAST_FLOOD offloading

 drivers/net/dsa/motorcomm/chip.c | 293 +++++++++++++++++++++----------
 drivers/net/dsa/motorcomm/chip.h |  10 +-
 2 files changed, 208 insertions(+), 95 deletions(-)

-- 
2.53.0

[PATCH net-next v4 1/8] net: dsa: motorcomm: Fix VLAN SVL and learn limit definitions

[David Yang]

Bit 3 of the VLAN table control enables shared VLAN learning (IVL/SVL),
it has nothing to do with S-VLAN tagging, so name it SVL_EN rather than
SVLAN_EN.

Widen PORT_LEARN_LIMIT_M to the 13 bits up to LEARN_DROP_ON_EXCEEDED at
bit 2. Nothing uses it, so this is a no-op.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/chip.c | 4 +---
 drivers/net/dsa/motorcomm/chip.h | 4 ++--
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 33e5fc127244..693564aeaec1 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -922,9 +922,7 @@ yt921x_vlan_aware_set(struct yt921x_priv *priv, int port, bool vlan_aware)
 {
 	u32 ctrl;
 
-	/* Abuse SVLAN for PCP parsing without polluting the FDB - it just works
-	 * despite YT921X_VLAN_CTRL_SVLAN_EN never being set
-	 */
+	/* Abuse SVLAN for PCP parsing without polluting the FDB */
 	if (!vlan_aware)
 		ctrl = YT921X_PORT_IGR_TPIDn_STAG(0);
 	else
diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h
index fc315f1d8f87..d9425afc7bce 100644
--- a/drivers/net/dsa/motorcomm/chip.h
+++ b/drivers/net/dsa/motorcomm/chip.h
@@ -240,7 +240,7 @@ enum yt921x_app_selector {
 #define  YT921X_PORT_LEARN_SUSPEND_DROP_EN	BIT(18)
 #define  YT921X_PORT_LEARN_DIS			BIT(17)
 #define  YT921X_PORT_LEARN_LIMIT_EN		BIT(16)
-#define  YT921X_PORT_LEARN_LIMIT_M		GENMASK(15, 8)
+#define  YT921X_PORT_LEARN_LIMIT_M		GENMASK(15, 3)
 #define   YT921X_PORT_LEARN_LIMIT(x)			FIELD_PREP(YT921X_PORT_LEARN_LIMIT_M, (x))
 #define  YT921X_PORT_LEARN_DROP_ON_EXCEEDED	BIT(2)
 #define  YT921X_PORT_LEARN_MODE_M		GENMASK(1, 0)
@@ -357,7 +357,7 @@ enum yt921x_app_selector {
 #define  YT921X_VLAN_CTRLb_UNTAG_PORTn(port)	BIT((port) + 8)
 #define  YT921X_VLAN_CTRLb_STP_ID_M		GENMASK(7, 4)
 #define   YT921X_VLAN_CTRLb_STP_ID(x)			FIELD_PREP(YT921X_VLAN_CTRLb_STP_ID_M, (x))
-#define  YT921X_VLAN_CTRLb_SVLAN_EN		BIT(3)
+#define  YT921X_VLAN_CTRLb_SVL_EN		BIT(3)
 #define  YT921X_VLAN_CTRLab_FID_M		GENMASK_ULL(34, 23)
 #define   YT921X_VLAN_CTRLab_FID(x)			FIELD_PREP(YT921X_VLAN_CTRLab_FID_M, (x))
 #define  YT921X_VLAN_CTRLa_LEARN_DIS		BIT(22)
-- 
2.53.0

[PATCH net-next v4 2/8] net: dsa: motorcomm: Drop VID 0 VLAN table programming

[David Yang]

The VID 0 VLAN table entry is unreachable for priority-tagged frames:
the hardware classifies them to the port's PVID before the VLAN table
lookup, just like untagged frames (measured on YT9215S), so the VID 0
entry is never looked up. Drop the dead programming.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/chip.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 693564aeaec1..47f06945b8c4 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -2263,7 +2263,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;
@@ -2321,15 +2320,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

[PATCH net-next v4 3/8] net: dsa: motorcomm: Reset port flags on bridge leaving

[David Yang]

DSA does not clear BR_HAIRPIN_MODE and BR_ISOLATED when a port leaves
a bridge, so the cached flags survive into the next bridge join and
program stale hairpin and port isolation state.

Reset them when leaving the bridge.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/chip.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 47f06945b8c4..c51491ef4dae 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -1158,10 +1158,17 @@ yt921x_dsa_port_vlan_add(struct dsa_switch *ds, int port,
 
 static int yt921x_userport_standalone(struct yt921x_priv *priv, int port)
 {
+	struct yt921x_port *pp = &priv->ports[port];
 	u32 mask;
 	u32 ctrl;
 	int res;
 
+	/* The return value got ignored by DSA core, reset flags before touching
+	 * the hardware.
+	 */
+	pp->hairpin = false;
+	pp->isolated = false;
+
 	ctrl = ~priv->cpu_ports_mask;
 	res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port), ctrl);
 	if (res)
-- 
2.53.0

[PATCH net-next v4 4/8] net: dsa: motorcomm: Remove local cpu_ports_mask

[David Yang]

Use dsa_cpu_ports() or dsa_is_cpu_port() instead for a clearer view of
code.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/chip.c | 18 +++++++++---------
 drivers/net/dsa/motorcomm/chip.h |  2 --
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index c51491ef4dae..8ab499611fbf 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -1003,7 +1003,7 @@ yt921x_vlan_add(struct yt921x_priv *priv, int port, u16 vid, bool untagged)
 	u32 ctrls[2];
 
 	masks[0] = YT921X_VLAN_CTRLa_PORTn(port) |
-		   YT921X_VLAN_CTRLa_PORTS(priv->cpu_ports_mask);
+		   YT921X_VLAN_CTRLa_PORTS(dsa_cpu_ports(&priv->ds));
 	ctrls[0] = masks[0];
 
 	masks[1] = YT921X_VLAN_CTRLb_UNTAG_PORTn(port);
@@ -1159,6 +1159,7 @@ yt921x_dsa_port_vlan_add(struct dsa_switch *ds, int port,
 static int yt921x_userport_standalone(struct yt921x_priv *priv, int port)
 {
 	struct yt921x_port *pp = &priv->ports[port];
+	struct dsa_switch *ds = &priv->ds;
 	u32 mask;
 	u32 ctrl;
 	int res;
@@ -1169,7 +1170,7 @@ static int yt921x_userport_standalone(struct yt921x_priv *priv, int port)
 	pp->hairpin = false;
 	pp->isolated = false;
 
-	ctrl = ~priv->cpu_ports_mask;
+	ctrl = ~dsa_cpu_ports(ds);
 	res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port), ctrl);
 	if (res)
 		return res;
@@ -1215,7 +1216,7 @@ static int yt921x_isolate(struct yt921x_priv *priv, int port)
 
 	mask = BIT(port);
 	for (int i = 0; i < YT921X_PORT_NUM; i++) {
-		if ((BIT(i) & priv->cpu_ports_mask) || i == port)
+		if (dsa_is_cpu_port(&priv->ds, port) || i == port)
 			continue;
 
 		res = yt921x_reg_set_bits(priv, YT921X_PORTn_ISOLATION(i),
@@ -1232,7 +1233,7 @@ static int yt921x_isolate(struct yt921x_priv *priv, int port)
  */
 static int yt921x_bridge(struct yt921x_priv *priv, u16 ports_mask)
 {
-	unsigned long targets_mask = ports_mask & ~priv->cpu_ports_mask;
+	unsigned long targets_mask = ports_mask & dsa_cpu_ports(&priv->ds);
 	u32 isolated_mask;
 	u32 ctrl;
 	int port;
@@ -1345,7 +1346,7 @@ yt921x_bridge_flags(struct yt921x_priv *priv, int port,
 			u32 ports_mask;
 
 			ports_mask = dsa_bridge_ports(ds, bdev);
-			ports_mask |= priv->cpu_ports_mask;
+			ports_mask |= dsa_cpu_ports(ds);
 			res = yt921x_bridge(priv, ports_mask);
 			if (res)
 				return res;
@@ -1417,7 +1418,7 @@ yt921x_dsa_port_bridge_join(struct dsa_switch *ds, int port,
 		return 0;
 
 	ports_mask = dsa_bridge_ports(ds, bridge.dev);
-	ports_mask |= priv->cpu_ports_mask;
+	ports_mask |= dsa_cpu_ports(ds);
 
 	mutex_lock(&priv->reg_lock);
 	res = yt921x_bridge_join(priv, port, ports_mask);
@@ -2275,10 +2276,10 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
 	int res;
 
 	/* Enable DSA */
-	priv->cpu_ports_mask = dsa_cpu_ports(ds);
+	cpu_ports_mask = dsa_cpu_ports(ds);
 
 	ctrl = YT921X_EXT_CPU_PORT_TAG_EN | YT921X_EXT_CPU_PORT_PORT_EN |
-	       YT921X_EXT_CPU_PORT_PORT(__ffs(priv->cpu_ports_mask));
+	       YT921X_EXT_CPU_PORT_PORT(__ffs(cpu_ports_mask));
 	res = yt921x_reg_write(priv, YT921X_EXT_CPU_PORT, ctrl);
 	if (res)
 		return res;
@@ -2315,7 +2316,6 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
 	/* Except for CPU ports, if any packets are sent via CPU ports without
 	 * tag, they should be dropped.
 	 */
-	cpu_ports_mask = priv->cpu_ports_mask;
 	for_each_set_bit(port, &cpu_ports_mask, YT921X_PORT_NUM) {
 		ctrl &= ~YT921X_ACT_UNK_ACTn_M(port);
 		ctrl |= YT921X_ACT_UNK_ACTn_DROP(port);
diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h
index d9425afc7bce..ab6f28d057cb 100644
--- a/drivers/net/dsa/motorcomm/chip.h
+++ b/drivers/net/dsa/motorcomm/chip.h
@@ -509,8 +509,6 @@ struct yt921x_priv {
 	const struct yt921x_info *info;
 	unsigned int meter_slot_ns;
 	unsigned int port_shape_slot_ns;
-	/* cache of dsa_cpu_ports(ds) */
-	u16 cpu_ports_mask;
 	unsigned char cycle_ns;
 
 	/* protect the access to the switch registers */
-- 
2.53.0

[PATCH net-next v4 5/8] net: dsa: motorcomm: Isolate all ports during chip setup

[David Yang]

Chip setup only blocks unknown unicast and unknown multicast frames,
leaving known multicast and broadcast open, though known multicast
cannot appear while the FDB is still empty.

Isolate every port right after the hardware reset, in preparation for
the isolation refactoring. Note that this only minimizes the window: a
reset always brings the hardware back to a dumb switch, so traffic can
leak before and during it.

Signed-off-by: David Yang <mmyangfl@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/motorcomm/chip.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 8ab499611fbf..32701f9e687f 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -2227,6 +2227,7 @@ static int yt921x_chip_reset(struct yt921x_priv *priv)
 {
 	struct device *dev = yt921x_priv_to_device(priv);
 	u16 eth_p_tag;
+	u32 ctrl;
 	u32 val;
 	int res;
 
@@ -2264,6 +2265,17 @@ static int yt921x_chip_reset(struct yt921x_priv *priv)
 		return -EINVAL;
 	}
 
+	/* Block all traffic as soon as we reset the hardware since it now acts
+	 * as a dumb switch.
+	 */
+	for (int port = 0; port < YT921X_PORT_NUM; port++) {
+		ctrl = ~(u32)0;
+		res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port),
+				       ctrl);
+		if (res)
+			return res;
+	}
+
 	return 0;
 }
 
-- 
2.53.0

[PATCH net-next v4 6/8] net: dsa: motorcomm: Rely on port isolation for conduit port

[David Yang]

With every port fully isolated during chip setup, the conduit port
already gets its one-way isolation (drop all packets without tag)
there. Skip the redundant handling elsewhere.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/chip.c | 52 +++++++++-----------------------
 1 file changed, 14 insertions(+), 38 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 32701f9e687f..1b05ac29ecd7 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -1125,7 +1125,7 @@ yt921x_dsa_port_vlan_add(struct dsa_switch *ds, int port,
 	int res;
 
 	/* CPU port is supposed to be a member of every VLAN; see
-	 * yt921x_vlan_add() and yt921x_port_setup()
+	 * yt921x_vlan_add()
 	 */
 	if (dsa_is_cpu_port(ds, port))
 		return 0;
@@ -1170,10 +1170,17 @@ static int yt921x_userport_standalone(struct yt921x_priv *priv, int port)
 	pp->hairpin = false;
 	pp->isolated = false;
 
-	ctrl = ~dsa_cpu_ports(ds);
-	res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port), ctrl);
-	if (res)
-		return res;
+	/* Conduit ports should by controlled by tagging completely, skip
+	 * setting isolation to preserve the one-way isolation installed
+	 * by yt921x_chip_reset().
+	 */
+	if (dsa_is_cpu_port(ds, port)) {
+		ctrl = ~dsa_cpu_ports(ds);
+		res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port),
+				       ctrl);
+		if (res)
+			return res;
+	}
 
 	/* Turn off FDB learning to prevent FDB pollution */
 	mask = YT921X_PORT_LEARN_DIS;
@@ -1948,8 +1955,6 @@ yt921x_dsa_phylink_get_caps(struct dsa_switch *ds, int port,
 
 static int yt921x_port_setup(struct yt921x_priv *priv, int port)
 {
-	struct dsa_switch *ds = &priv->ds;
-	u32 ctrl;
 	int res;
 
 	res = yt921x_userport_standalone(priv, port);
@@ -1963,27 +1968,6 @@ static int yt921x_port_setup(struct yt921x_priv *priv, int port)
 	if (res)
 		return res;
 
-	if (dsa_is_cpu_port(ds, port)) {
-		/* Egress of CPU port is supposed to be completely controlled
-		 * via tagging, so set to oneway isolated (drop all packets
-		 * without tag).
-		 */
-		ctrl = ~(u32)0;
-		res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port),
-				       ctrl);
-		if (res)
-			return res;
-
-		/* To simplify FDB "isolation" simulation, we also disable
-		 * learning on the CPU port, and let software identify packets
-		 * towarding CPU (either trapped or a static FDB entry is
-		 * matched, no matter which bridge that entry is for), which is
-		 * already done by yt921x_userport_standalone(). As a result,
-		 * VLAN-awareness becomes unrelated on the CPU port (set to
-		 * VLAN-unaware by the way).
-		 */
-	}
-
 	return 0;
 }
 
@@ -2282,9 +2266,8 @@ static int yt921x_chip_reset(struct yt921x_priv *priv)
 static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
 {
 	struct dsa_switch *ds = &priv->ds;
-	unsigned long cpu_ports_mask;
+	u32 cpu_ports_mask;
 	u32 ctrl;
-	int port;
 	int res;
 
 	/* Enable DSA */
@@ -2302,7 +2285,7 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
 	if (res)
 		return res;
 
-	ctrl = GENMASK(10, 0);
+	ctrl = GENMASK(10, 0) & ~cpu_ports_mask;
 	res = yt921x_reg_write(priv, YT921X_FILTER_UNK_UCAST, ctrl);
 	if (res)
 		return res;
@@ -2325,13 +2308,6 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
 	ctrl = 0;
 	for (int i = 0; i < YT921X_PORT_NUM; i++)
 		ctrl |= YT921X_ACT_UNK_ACTn_TRAP(i);
-	/* Except for CPU ports, if any packets are sent via CPU ports without
-	 * tag, they should be dropped.
-	 */
-	for_each_set_bit(port, &cpu_ports_mask, YT921X_PORT_NUM) {
-		ctrl &= ~YT921X_ACT_UNK_ACTn_M(port);
-		ctrl |= YT921X_ACT_UNK_ACTn_DROP(port);
-	}
 	res = yt921x_reg_write(priv, YT921X_ACT_UNK_UCAST, ctrl);
 	if (res)
 		return res;
-- 
2.53.0

[PATCH net-next v4 7/8] net: dsa: motorcomm: Translate FDB VID for VLAN-unaware bridges

[David Yang]

A VLAN-unaware bridge reports VID 0 for its FDB entries, but lookups run
with the unaware FID (VID 4095). Entries added through switchdev were
installed under FID 0 and could never be hit by the hardware lookups.

Translate the unaware FID and VID 0 back and forth, so the software
mapping stays symmetric with the hardware lookups.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/chip.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 1b05ac29ecd7..e41fe4859bfe 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -590,6 +590,9 @@ yt921x_fdb_dump(struct yt921x_priv *priv, u16 ports_mask,
 	res = yt921x_fdb_read(priv, addr, &vid, &pmask, &index, &status);
 	if (res)
 		return res;
+
+	if (vid == YT921X_VID_UNWARE)
+		vid = 0;
 	if ((pmask & ports_mask) && !is_multicast_ether_addr(addr)) {
 		res = cb(addr, vid,
 			 status == YT921X_FDB_ENTRY_STATUS_STATIC, data);
@@ -618,6 +621,8 @@ yt921x_fdb_dump(struct yt921x_priv *priv, u16 ports_mask,
 		if (!pmask)
 			break;
 
+		if (vid == YT921X_VID_UNWARE)
+			vid = 0;
 		if ((pmask & ports_mask) && !is_multicast_ether_addr(addr)) {
 			res = cb(addr, vid,
 				 status == YT921X_FDB_ENTRY_STATUS_STATIC,
@@ -740,6 +745,10 @@ yt921x_fdb_leave(struct yt921x_priv *priv, const unsigned char *addr,
 	u32 val;
 	int res;
 
+	/* See yt921x_fdb_join() */
+	if (!vid)
+		vid = YT921X_VID_UNWARE;
+
 	/* Check for presence */
 	res = yt921x_fdb_has(priv, addr, vid, &index);
 	if (res)
@@ -782,6 +791,13 @@ yt921x_fdb_join(struct yt921x_priv *priv, const unsigned char *addr, u16 vid,
 	u32 val2;
 	int res;
 
+	/* A vlan-unaware bridge reports vid 0, but untagged lookups on its
+	 * ports run with the unaware FID; map so that static entries match the
+	 * hardware lookups.
+	 */
+	if (!vid)
+		vid = YT921X_VID_UNWARE;
+
 	/* Check for presence */
 	res = yt921x_fdb_has(priv, addr, vid, &index);
 	if (res)
-- 
2.53.0

[PATCH net-next v4 8/8] net: dsa: motorcomm: Support BR_FLOOD and BR_MCAST_FLOOD offloading

[David Yang]

Let the hardware flood the unknown unicast / multicast for the port if:

- the switch carries only one bridge, otherwise frames hitting FDB entry
  from another bridge will be dropped;
- all other ports in the bridge have BR_FLOOD / BR_MCAST_FLOOD enabled;
- the port is not hairpined, as the hardware never floods back to its
  ingress port.

This should cover most single-bridge usage. Other data paths remain
unchanged in terms of behavior.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/chip.c | 180 +++++++++++++++++++++++++------
 drivers/net/dsa/motorcomm/chip.h |   4 +
 2 files changed, 150 insertions(+), 34 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index e41fe4859bfe..5fed5b5a2feb 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -1185,6 +1185,8 @@ static int yt921x_userport_standalone(struct yt921x_priv *priv, int port)
 	 */
 	pp->hairpin = false;
 	pp->isolated = false;
+	pp->flood = true;
+	pp->mcast_flood = true;
 
 	/* Conduit ports should by controlled by tagging completely, skip
 	 * setting isolation to preserve the one-way isolation installed
@@ -1209,13 +1211,17 @@ static int yt921x_userport_standalone(struct yt921x_priv *priv, int port)
 	if (res)
 		return res;
 
-	/* Unrelated since learning is off and all packets are trapped;
-	 * set it anyway
+	/* Unrelated since learning is off and there is only one possible
+	 * destination; set it anyway
 	 */
 	res = yt921x_port_set_pvid(priv, port, YT921X_VID_UNWARE);
 	if (res)
 		return res;
 
+	/* Also unrelated for FILTER_UNK_UCAST / FILTER_UNK_MCAST since all
+	 * packets are handled by software
+	 */
+
 	return 0;
 }
 
@@ -1232,6 +1238,96 @@ static int yt921x_userport_bridge(struct yt921x_priv *priv, int port)
 	return 0;
 }
 
+/* ports_mask is optional */
+static int yt921x_bridge_flood_set(struct yt921x_priv *priv, u16 ports_mask)
+{
+	struct dsa_bridge *bridges[YT921X_PORT_NUM];
+	struct dsa_switch *ds = &priv->ds;
+	struct dsa_port *dp;
+	unsigned int count;
+	u32 ctrl;
+	int res;
+	int ret;
+
+	/* Offloading is only enabled if there is at most 1 bridge - see the
+	 * cross-bridge FDB mismatch below. (0 does nothing, of course.)
+	 */
+	count = 0;
+	dsa_switch_for_each_user_port(dp, ds) {
+		unsigned int i;
+
+		if (!dp->bridge)
+			continue;
+
+		for (i = 0; i < count; i++)
+			if (bridges[i] == dp->bridge)
+				break;
+		if (i == count) {
+			bridges[count] = dp->bridge;
+			count++;
+		}
+	}
+
+	ret = 0;
+	if (count != 1) {
+		ctrl = ~(u32)0;
+		res = yt921x_reg_write(priv, YT921X_FILTER_UNK_UCAST, ctrl);
+		if (res)
+			ret = res;
+		res = yt921x_reg_write(priv, YT921X_FILTER_UNK_MCAST, ctrl);
+		if (res)
+			ret = res;
+		return ret;
+	}
+
+	ports_mask &= ~dsa_cpu_ports(ds);
+	if (!ports_mask)
+		ports_mask = dsa_bridge_ports(ds, bridges[0]->dev);
+
+	for (int type = 0; type < 2; type++) {
+		unsigned long targets_mask = ports_mask;
+		u16 flood_mask;
+		u32 addr;
+		int port;
+
+		addr = !type ? YT921X_FILTER_UNK_UCAST :
+		       YT921X_FILTER_UNK_MCAST;
+
+		flood_mask = 0;
+		for_each_set_bit(port, &targets_mask, YT921X_PORT_NUM) {
+			struct yt921x_port *pp = &priv->ports[port];
+
+			if (!type ? pp->flood : pp->mcast_flood)
+				flood_mask |= BIT(port);
+		}
+
+		ctrl = 0;
+		for_each_set_bit(port, &targets_mask, YT921X_PORT_NUM) {
+			struct yt921x_port *pp = &priv->ports[port];
+
+			/* Hardware never floods back to its own ingress port,
+			 * keep hairpin port trapped: the software bridge does
+			 * deliver a flood there.
+			 */
+			if (!pp->hairpin && !((flood_mask ^ ports_mask) &
+					      ~BIT(port)))
+				continue;
+
+			ctrl |= BIT(port);
+		}
+
+		/* ACT_UNK is fixed to TRAP for every port, see
+		 * yt921x_chip_setup_dsa().
+		 */
+
+		res = yt921x_reg_update_bits(priv, addr, ports_mask, ctrl);
+		if (res)
+			ret = res;
+	}
+
+	return ret;
+}
+
 static int yt921x_isolate(struct yt921x_priv *priv, int port)
 {
 	u32 mask;
@@ -1306,6 +1402,10 @@ static int yt921x_bridge_leave(struct yt921x_priv *priv, int port)
 	if (res)
 		return res;
 
+	res = yt921x_bridge_flood_set(priv, 0);
+	if (res)
+		return res;
+
 	return 0;
 }
 
@@ -1322,6 +1422,10 @@ yt921x_bridge_join(struct yt921x_priv *priv, int port, u16 ports_mask)
 	if (res)
 		return res;
 
+	res = yt921x_bridge_flood_set(priv, ports_mask);
+	if (res)
+		return res;
+
 	return 0;
 }
 
@@ -1330,7 +1434,12 @@ yt921x_bridge_flags(struct yt921x_priv *priv, int port,
 		    struct switchdev_brport_flags flags)
 {
 	struct yt921x_port *pp = &priv->ports[port];
-	bool do_flush;
+	struct dsa_switch *ds = &priv->ds;
+	bool change_isolated = false;
+	bool change_flood = false;
+	struct net_device *bdev;
+	struct dsa_port *dp;
+	u32 ports_mask;
 	u32 mask;
 	int res;
 
@@ -1344,36 +1453,48 @@ yt921x_bridge_flags(struct yt921x_priv *priv, int port,
 			return res;
 	}
 
-	/* BR_FLOOD, BR_MCAST_FLOOD: see the comment where ACT_UNK_ACTn_TRAP
-	 * is set
-	 */
-
 	/* BR_BCAST_FLOOD: we can filter bcast, but cannot trap them */
 
-	do_flush = false;
 	if (flags.mask & BR_HAIRPIN_MODE) {
 		pp->hairpin = flags.val & BR_HAIRPIN_MODE;
-		do_flush = true;
+		change_isolated = true;
+		/* Hairpin also affects flood flags */
+		change_flood = true;
 	}
 	if (flags.mask & BR_ISOLATED) {
 		pp->isolated = flags.val & BR_ISOLATED;
-		do_flush = true;
+		change_isolated = true;
 	}
-	if (do_flush) {
-		struct dsa_switch *ds = &priv->ds;
-		struct dsa_port *dp = dsa_to_port(ds, port);
-		struct net_device *bdev;
 
-		bdev = dsa_port_bridge_dev_get(dp);
-		if (bdev) {
-			u32 ports_mask;
+	if (flags.mask & BR_FLOOD) {
+		pp->flood = flags.val & BR_FLOOD;
+		change_flood = true;
+	}
+	if (flags.mask & BR_MCAST_FLOOD) {
+		pp->mcast_flood = flags.val & BR_MCAST_FLOOD;
+		change_flood = true;
+	}
 
-			ports_mask = dsa_bridge_ports(ds, bdev);
-			ports_mask |= dsa_cpu_ports(ds);
-			res = yt921x_bridge(priv, ports_mask);
-			if (res)
-				return res;
-		}
+	if (!change_isolated && !change_flood)
+		return 0;
+
+	dp = dsa_to_port(ds, port);
+	bdev = dsa_port_bridge_dev_get(dp);
+	if (!bdev)
+		return 0;
+
+	ports_mask = dsa_bridge_ports(ds, bdev);
+	ports_mask |= dsa_cpu_ports(ds);
+
+	if (change_isolated) {
+		res = yt921x_bridge(priv, ports_mask);
+		if (res)
+			return res;
+	}
+	if (change_flood) {
+		res = yt921x_bridge_flood_set(priv, ports_mask);
+		if (res)
+			return res;
 	}
 
 	return 0;
@@ -2301,14 +2422,6 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
 	if (res)
 		return res;
 
-	ctrl = GENMASK(10, 0) & ~cpu_ports_mask;
-	res = yt921x_reg_write(priv, YT921X_FILTER_UNK_UCAST, ctrl);
-	if (res)
-		return res;
-	res = yt921x_reg_write(priv, YT921X_FILTER_UNK_MCAST, ctrl);
-	if (res)
-		return res;
-
 	/* YT921x does not support native DSA port bridging, so we use port
 	 * isolation to emulate it. However, be especially careful that port
 	 * isolation takes _after_ FDB lookups, i.e. if an FDB entry (from
@@ -2317,9 +2430,8 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
 	 * the "bridged" ports, thus we need to trap and handle those packets by
 	 * software.
 	 *
-	 * If there is no more than one bridge, we might be able to drop them
-	 * directly given some conditions are met, but we trap them in all cases
-	 * for now.
+	 * The flood action is offloaded to the hardware by fixing UNK_ACT to
+	 * TRAP and toggling the FILTER_UNK on the appropriate ports.
 	 */
 	ctrl = 0;
 	for (int i = 0; i < YT921X_PORT_NUM; i++)
diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h
index ab6f28d057cb..98fe8250e99a 100644
--- a/drivers/net/dsa/motorcomm/chip.h
+++ b/drivers/net/dsa/motorcomm/chip.h
@@ -461,6 +461,10 @@ struct yt921x_port {
 	bool hairpin:1;
 	/* BR_ISOLATED */
 	bool isolated:1;
+	/* BR_FLOOD */
+	bool flood:1;
+	/* BR_MCAST_FLOOD */
+	bool mcast_flood:1;
 
 	unsigned short acl_cnt;
 
-- 
2.53.0

Re: [PATCH net-next v4 0/8] net: dsa: motorcomm: Add BR_FLOOD and BR_MCAST_FLOOD support

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net-next v4 0/8] net: dsa: motorcomm: Add BR_FLOOD and BR_MCAST_FLOOD support

[David Yang]

On Sat, Nov 7, 2026 at 8:21 PM <netdev-bot+sinfo@kernel.org> wrote:
>
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.
>
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
>
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

not a fix

Re: [PATCH net-next v4 2/8] net: dsa: motorcomm: Drop VID 0 VLAN table programming

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The commit message says the VID 0 entry is never looked up,
  but the driver itself can make VID 0 the classification VID, so the…

--

net: dsa: motorcomm: Drop VID 0 VLAN table programming

This commit removes the write of VLAN table entry 0 in
yt921x_chip_setup_dsa(). That write set LEARN_DIS and all port members
with an empty untag mask. The commit message says the hardware classifies
priority-tagged frames to the port's PVID before the VLAN table lookup, so
the VID 0 entry is never looked up.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 693564aeaec14..47f06945b8c4e 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -2321,15 +2320,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]
Is entry 0 really unreachable when the port's PVID is 0? The commit message
says:

  the hardware classifies them to the port's PVID before the VLAN table
  lookup, just like untagged frames (measured on YT9215S), so the VID 0
  entry is never looked up.

The driver itself programs a hardware PVID of 0 on VLAN-filtering ports.
In yt921x_vlan_filtering(), br_vlan_get_pvid() gives 0 when the bridge port
has no PVID:

	else
		br_vlan_get_pvid(bdev, &pvid);
	res = yt921x_port_set_pvid(priv, port, pvid);
	...
	if (vlan_filtering && !pvid)
		ctrl |= YT921X_PORT_VLAN_CTRL1_CVLAN_DROP_UNTAGGED;

yt921x_pvid_clear() does the same after the PVID VLAN is deleted, for
example via yt921x_dsa_port_vlan_del():

	res = yt921x_port_set_pvid(priv, port,
				   vlan_filtering ? 0 : YT921X_VID_UNWARE);

In both cases the only extra guard is CVLAN_DROP_UNTAGGED. Does that bit
also drop priority-tagged frames? If it does not, a priority-tagged frame
on such a port would be classified to PVID 0 and look up entry 0.

After this patch, entry 0 keeps its reset contents. The 8021q layer can
also change it at runtime by adding VID 0 automatically:

dsa_user_vlan_rx_add_vid(vid = 0)
  dsa_port_vlan_add() / dsa_port_host_vlan_add()
    yt921x_dsa_port_vlan_add()
      yt921x_vlan_add()

yt921x_vlan_add() sets only the port, CPU port and untag bits, never
LEARN_DIS:

	masks[0] = YT921X_VLAN_CTRLa_PORTn(port) |
		   YT921X_VLAN_CTRLa_PORTS(priv->cpu_ports_mask);
	ctrls[0] = masks[0];
	...
	return yt921x_reg64_update_bits(priv, YT921X_VLANn_CTRL(vid),
					masks, ctrls);

No other code in the driver sets YT921X_VLAN_CTRLa_LEARN_DIS. Could the
switch now learn source MACs and forward in hardware in VID 0 on a
filtering port with no PVID? The Linux bridge would drop those frames.

The measurement in the commit message also covers only YT9215S. The rest
of the series does not restore a LEARN_DIS write to YT921X_VLANn_CTRL(0),
and PVID 0 is still programmed in yt921x_vlan_filtering() and
yt921x_pvid_clear().

Should the commit message cover the PVID 0 case and the loss of LEARN_DIS
on VID 0? Or should the VID 0 programming be kept?

>  	return 0;
>  }
>

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

Re: [PATCH net-next v4 3/8] net: dsa: motorcomm: Reset port flags on bridge leaving

[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] This patch resets pp->isolated and pp->hairpin in
  yt921x_userport_standalone() when a port leaves a bridge, but nothing…
- [Low] The patch fixes a user-visible forwarding bug: a port that
  rejoins a bridge is programmed with stale hairpin/isolation state in…
- [Low] The new comment in the shared helper yt921x_userport_standalone()
  says "The return value got ignored by DSA core".

--

net: dsa: motorcomm: Reset port flags on bridge leaving

This patch clears the cached pp->hairpin and pp->isolated flags in
yt921x_userport_standalone() when a port leaves a bridge. The goal is to
keep stale BR_HAIRPIN_MODE and BR_ISOLATED state from being programmed
on the next bridge join.

> DSA does not clear BR_HAIRPIN_MODE and BR_ISOLATED when a port leaves
> a bridge, so the cached flags survive into the next bridge join and
> program stale hairpin and port isolation state.
>
> Reset them when leaving the bridge.
>
> Signed-off-by: David Yang <mmyangfl@gmail.com>

[Severity: Low]
Should this have a Fixes: tag? The caching of pp->hairpin and
pp->isolated in yt921x_bridge_flags(), with no reset on leave, comes
from the original driver commit:

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 47f06945b8c4e..c51491ef4dae5 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -1158,10 +1158,17 @@ yt921x_dsa_port_vlan_add(struct dsa_switch *ds, int port,
>
>  static int yt921x_userport_standalone(struct yt921x_priv *priv, int port)
>  {
> +	struct yt921x_port *pp = &priv->ports[port];
>  	u32 mask;
>  	u32 ctrl;
>  	int res;
>
> +	/* The return value got ignored by DSA core, reset flags before touching
> +	 * the hardware.
> +	 */

[Severity: Low]
This isn't a bug, but is this comment accurate for both callers of
yt921x_userport_standalone()?

It holds for yt921x_bridge_leave(), which is reached from the void
.port_bridge_leave op. yt921x_port_setup() also calls this helper and
returns its error:

yt921x_port_setup() {
    ...
	res = yt921x_userport_standalone(priv, port);
	if (res)
		return res;
    ...
}

That error goes back through .port_setup, and dsa_port_devlink_setup()
in net/dsa/devlink.c propagates it:

	if (ds->ops->port_setup) {
		err = ds->ops->port_setup(ds, dp->index);
		if (err)
			return err;

Could the reset and this comment go into yt921x_bridge_leave() instead?

> +	pp->hairpin = false;
> +	pp->isolated = false;

[Severity: High]
Now that the cached flags are cleared on leave, does anything restore
them on the next join?

dsa_port_inherit_brport_flags() in net/dsa/port.c only replays this
set:

	const unsigned long mask = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
				   BR_BCAST_FLOOD | BR_PORT_LOCKED;

So BR_ISOLATED and BR_HAIRPIN_MODE on the bridge port never reach the
driver on join. The bridge doesn't replay brport flags either.
nbp_switchdev_sync_objs() only replays VLAN, MDB and FDB objects.

This driver implements .port_lag_join. Take a bond that is a bridge
port and already has "bridge link set dev bond0 isolated on" (or
"hairpin on") applied, so yt921x_bridge_flags() has cached the flag on
each yt921x member.

Then a member leaves the bond and rejoins, or a new member is added:

dsa_port_lag_leave()
  dsa_port_bridge_leave()
    yt921x_dsa_port_bridge_leave()
      yt921x_bridge_leave()
        yt921x_userport_standalone()
          pp->hairpin = false;
          pp->isolated = false;

dsa_port_lag_join()
  dsa_port_bridge_join()
    yt921x_dsa_port_bridge_join()
      yt921x_bridge_join()
        yt921x_bridge()

yt921x_bridge() builds YT921X_PORTn_ISOLATION(port) only from the
cache:

		ctrl = ~ports_mask;
		if (pp->isolated)
			ctrl |= isolated_mask;
		if (!pp->hairpin)
			ctrl |= BIT(port);

Wouldn't this leave the port without isolation and hairpin in hardware?
It would also be dropped from isolated_mask for the other isolated
ports. Meanwhile "bridge link show" still reports bond0 as isolated, and
the hardware stays wrong until the user toggles the flag again.

Before this patch, the stale cache happened to keep a rejoining member
correct, so the rejoin case looks like a regression. The commit message
says the aim is correct hairpin and isolation state on the next bridge
join, and this LAG path still doesn't get there.

Would it be better to sync these flags on join? One way is to add
BR_HAIRPIN_MODE | BR_ISOLATED to the inherit and clear masks in
net/dsa/port.c, which would make the driver-local reset unnecessary.
Another is to read br_port_flag_is_set() on the bridge port in the
driver's bridge join path.

> +
>  	ctrl = ~priv->cpu_ports_mask;
>  	res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port), ctrl);
>  	if (res)

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

Re: [PATCH net-next v4 4/8] net: dsa: motorcomm: Remove local cpu_ports_mask

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · 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] This commit was meant to be a pure refactor, but it reverses the
  target mask in yt921x_bridge() (drivers/net/dsa/motorcomm/chip.c).
- [Medium] In yt921x_isolate() (drivers/net/dsa/motorcomm/chip.c), the
  per-iteration check `(BIT(i) & priv->cpu_ports_mask)` was replaced…

--

net: dsa: motorcomm: Remove local cpu_ports_mask

This commit drops the cached cpu_ports_mask field from struct yt921x_priv.
Its users now call dsa_cpu_ports() or dsa_is_cpu_port() instead. The
commit describes this as a readability change.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index c51491ef4dae5..8ab499611fbf7 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -1215,7 +1216,7 @@ static int yt921x_isolate(struct yt921x_priv *priv, int port)
>  
>  	mask = BIT(port);
>  	for (int i = 0; i < YT921X_PORT_NUM; i++) {
> -		if ((BIT(i) & priv->cpu_ports_mask) || i == port)
> +		if (dsa_is_cpu_port(&priv->ds, port) || i == port)
>  			continue;

[Severity: Medium]
Should this be dsa_is_cpu_port(&priv->ds, i) instead of port?

The old check tested the loop index i against the CPU port mask. The new
check tests the function argument port, which is the same on every
iteration.

The only caller path is:

yt921x_dsa_port_bridge_leave()
  yt921x_bridge_leave()
    yt921x_isolate()

yt921x_dsa_port_bridge_leave() already returns early for CPU ports:

	if (dsa_is_cpu_port(ds, port))
		return;

So the new condition is always false, and the loop no longer skips CPU
ports. On every bridge leave, BIT(port) now gets set in
YT921X_PORTn_ISOLATION() of each CPU port. The original code left those
registers alone, because port setup programs them.

If yt921x_isolate() were ever called with a CPU port, the condition would
be true on every iteration and no port would be updated.

>  
>  		res = yt921x_reg_set_bits(priv, YT921X_PORTn_ISOLATION(i),
> @@ -1232,7 +1233,7 @@ static int yt921x_isolate(struct yt921x_priv *priv, int port)
>   */
>  static int yt921x_bridge(struct yt921x_priv *priv, u16 ports_mask)
>  {
> -	unsigned long targets_mask = ports_mask & ~priv->cpu_ports_mask;
> +	unsigned long targets_mask = ports_mask & dsa_cpu_ports(&priv->ds);

[Severity: High]
Is a ~ missing here? The old code selected the bridge members that are
not CPU ports. The new code selects only the CPU ports.

Both callers add the CPU ports to ports_mask before calling
yt921x_bridge():

yt921x_dsa_port_bridge_join():
	ports_mask = dsa_bridge_ports(ds, bridge.dev);
	ports_mask |= dsa_cpu_ports(ds);

yt921x_bridge_flags():
	ports_mask = dsa_bridge_ports(ds, bdev);
	ports_mask |= dsa_cpu_ports(ds);

So targets_mask always ends up equal to dsa_cpu_ports(ds). The loops under
the "Block from non-cpu bridge ports" comment then write only the CPU
ports' YT921X_PORTn_ISOLATION() registers.

This has three effects:

The ~(u32)0 isolation value that yt921x_port_setup() installs on the CPU
port is overwritten with ~ports_mask | BIT(cpu).

The user ports in the bridge never have their isolation registers
rewritten. They keep the standalone value ~dsa_cpu_ports(ds) from
yt921x_userport_standalone(), so they stay blocked from each other.

BR_HAIRPIN_MODE and BR_ISOLATED updates from yt921x_bridge_flags() go to
the CPU port, not the user ports.

yt921x_userport_bridge() clears YT921X_PORT_LEARN_DIS on join, so the
switch learns FDB entries for bridge members. The comment in
yt921x_chip_setup_dsa() says port isolation is applied after FDB lookups.

Would known-unicast frames between two bridged user ports then be dropped
once their FDB entries are learned?

Should this be ports_mask & ~dsa_cpu_ports(&priv->ds)?

>  	u32 isolated_mask;
>  	u32 ctrl;
>  	int port;

[ ... ]

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

Re: [PATCH net-next v4 5/8] net: dsa: motorcomm: Isolate all ports during chip setup

[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] yt921x_bridge() in drivers/net/dsa/motorcomm/chip.c computes
  `targets_mask = ports_mask & dsa_cpu_ports(&priv->ds)`.
- [Low] yt921x_isolate() in drivers/net/dsa/motorcomm/chip.c loops over
  `i` but checks `dsa_is_cpu_port(&priv->ds, port)` instead of…
- [Low] The commit message says every port is isolated "right after the
  hardware reset", and the new comment says "Block all traffic as soon…

--

net: dsa: motorcomm: Isolate all ports during chip setup

This patch writes ~0 to the PORTn_ISOLATION register of every port at the
end of yt921x_chip_reset(). All traffic is then blocked once the hardware
reset has turned the chip back into a dumb switch. It prepares for the
isolation refactoring later in the series.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 8ab499611fbf7..32701f9e687fa 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -2264,6 +2265,17 @@ static int yt921x_chip_reset(struct yt921x_priv *priv)
>  		return -EINVAL;
>  	}
>  
> +	/* Block all traffic as soon as we reset the hardware since it now acts
> +	 * as a dumb switch.
> +	 */
> +	for (int port = 0; port < YT921X_PORT_NUM; port++) {

[Severity: Low]
The commit message says ports are isolated "right after the hardware
reset". The new comment says "as soon as we reset the hardware". In
yt921x_chip_reset(), though, this loop runs after the TPID check:

	res = yt921x_reg_read(priv, YT921X_CPU_TAG_TPID, &val);
	if (res)
		return res;
	eth_p_tag = FIELD_GET(YT921X_CPU_TAG_TPID_TPID_M, val);
	if (eth_p_tag != ETH_P_YT921X) {
		...
		return -EINVAL;
	}

The TPID read can fail, or the value can differ from ETH_P_YT921X. In
either case the function returns after YT921X_RST_HW has already reset
the chip, and no PORTn_ISOLATION register is written.

yt921x_dsa_setup() passes the error up, setup fails, and .port_setup
never runs. The switch keeps forwarding between all ports, including the
conduit, and there are no netdevs to manage it.

The behaviour here is the same as before the patch, so this isn't a
regression. The TPID check doesn't depend on isolation, though.

Could the loop move to directly after the yt921x_reg_wait() on
YT921X_RST? That would cover this error path. It would also make the
window on the success path one bus transaction shorter.

> +		ctrl = ~(u32)0;
> +		res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port),
> +				       ctrl);
> +		if (res)
> +			return res;
> +	}

[Severity: High]
This code isn't part of this diff, but does yt921x_bridge() undo this
isolation for the CPU port as soon as a port joins a bridge?

Earlier in the series, "net: dsa: motorcomm: Remove local cpu_ports_mask"
changed the mask computation in yt921x_bridge() to this:

drivers/net/dsa/motorcomm/chip.c:yt921x_bridge() {
	unsigned long targets_mask = ports_mask & dsa_cpu_ports(&priv->ds);
	...
	/* Block from non-cpu bridge ports ... */
	for_each_set_bit(port, &targets_mask, YT921X_PORT_NUM) {
		...
		ctrl = ~ports_mask;
		...
		res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port),
				       ctrl);
	...
}

Before that commit it was ports_mask & ~priv->cpu_ports_mask. Is a '~'
missing in front of dsa_cpu_ports()?

Both yt921x_bridge_join() and yt921x_bridge_flags() OR the CPU ports into
ports_mask. That makes targets_mask exactly the CPU port mask, so both
loops visit only the CPU port.

There are two effects. First, the CPU port's PORTn_ISOLATION is replaced
with ~ports_mask | BIT(cpu). This removes the one-way ~0 isolation set
here and in yt921x_port_setup(). Untagged frames from the conduit can
then be forwarded to bridge members.

Second, the user bridge ports are never reprogrammed. They keep the
~dsa_cpu_ports() value written by yt921x_userport_standalone().
Hardware forwarding between bridge members is never enabled, and
pp->isolated and pp->hairpin are never applied to these ports.

yt921x_userport_bridge() clears LEARN_DIS on join, so the switch learns
MACs. Known unicast is then sent toward the peer port, where the source
port's isolation blocks it.

The tagger only sets offload_fwd_mark for frames the hardware has already
forwarded. So the software bridge doesn't reliably recover these frames
either.

Running "ip link set lanX master br0" on any YT921x switch takes this
path:

  yt921x_dsa_port_bridge_join()
    yt921x_bridge_join()
      yt921x_bridge()

Bridge flag changes reach the same code through yt921x_bridge_flags().
The line is still unchanged at the end of the series.

[Severity: Low]
This one also comes from "net: dsa: motorcomm: Remove local
cpu_ports_mask". Is the CPU port check in yt921x_isolate() testing the
wrong loop variable?

drivers/net/dsa/motorcomm/chip.c:yt921x_isolate() {
	...
	for (int i = 0; i < YT921X_PORT_NUM; i++) {
		if (dsa_is_cpu_port(&priv->ds, port) || i == port)
			continue;
	...
}

The original check was BIT(i) & priv->cpu_ports_mask, so this should
probably be dsa_is_cpu_port(&priv->ds, i). As written, the result is the
same on every iteration.

yt921x_dsa_port_bridge_leave() returns early for CPU ports, so the
condition is always false. The CPU port's PORTn_ISOLATION then gets
BIT(port) set on every bridge leave.

By itself, that matches the ~0 policy this patch sets up. Combined with
the yt921x_bridge() problem above, the CPU port's isolation state ends up
depending on the order of bridge joins and leaves. This check is also
unchanged at the end of the series.

>  
>  	return 0;
>  }

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

Re: [PATCH net-next v4 6/8] net: dsa: motorcomm: Rely on port isolation for conduit port

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · 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] This patch inverts the isolation logic in
  yt921x_userport_standalone()…
- [High] This patch makes the CPU port's PORTn_ISOLATION register the
  only hardware guard against untagged conduit traffic.
- [Low] yt921x_isolate() (drivers/net/dsa/motorcomm/chip.c:1225-1227)
  skips with `if (dsa_is_cpu_port(&priv->ds, port) || i == port)…

--

net: dsa: motorcomm: Rely on port isolation for conduit port

This patch takes the ~0 isolation that yt921x_chip_reset() writes to the
conduit port and makes it the only drop mechanism for untagged traffic
sent on the conduit. To do that it removes the explicit CPU port
isolation rewrite from yt921x_port_setup(), drops the ACT_UNK drop action
for CPU ports, and takes CPU ports out of FILTER_UNK_UCAST/MCAST.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 32701f9e687fa..1b05ac29ecd76 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -1170,10 +1170,17 @@ static int yt921x_userport_standalone(struct yt921x_priv *priv, int port)
>  	pp->hairpin = false;
>  	pp->isolated = false;
>  
> -	ctrl = ~dsa_cpu_ports(ds);
> -	res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port), ctrl);
> -	if (res)
> -		return res;
> +	/* Conduit ports should by controlled by tagging completely, skip
> +	 * setting isolation to preserve the one-way isolation installed
> +	 * by yt921x_chip_reset().
> +	 */
> +	if (dsa_is_cpu_port(ds, port)) {
> +		ctrl = ~dsa_cpu_ports(ds);
> +		res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port),
> +				       ctrl);
> +		if (res)
> +			return res;
> +	}

[Severity: High]
Is this condition inverted? The comment says conduit ports are skipped so
they keep the isolation from yt921x_chip_reset(). With
if (dsa_is_cpu_port(ds, port)), though, only CPU ports get written with
~dsa_cpu_ports(ds), and every user port is skipped.

At probe, yt921x_chip_reset() writes ~0 to every PORTn_ISOLATION register.
yt921x_port_setup()->yt921x_userport_standalone() then runs for each port:

  - CPU ports are overwritten with ~dsa_cpu_ports(). That clears their
    block bits toward themselves and toward other CPU ports. This patch
    also removes the ~0 rewrite from yt921x_port_setup(), so nothing
    restores the one-way isolation.

  - User ports are never written and stay at ~0. That also blocks them
    toward the CPU port.

Unknown unicast and multicast are still trapped. However, the driver notes
in yt921x_bridge_flags() that broadcast can be filtered but not trapped,
and isolation is applied after the FDB lookup. Would broadcast frames
received on standalone user ports, such as ARP requests or DHCP, be
dropped before they reach the host?

This patch also removes the ACT_UNK drop action and the FILTER_UNK
coverage for CPU ports in yt921x_chip_setup_dsa(). The CPU port isolation
is now the only guard for untagged conduit traffic, and it is no longer
~0. In multi-CPU setups, could untagged frames from one conduit be
forwarded to another CPU port, or hairpinned back?

yt921x_bridge_leave() also relies on this function to reset the
isolation register of the leaving port:

yt921x_bridge_leave()
    yt921x_userport_standalone(priv, port);
    yt921x_isolate(priv, port);

That reset no longer happens for user ports. This is masked for now
because yt921x_bridge() never programs user port registers (see below).
Once that is fixed, wouldn't a port leaving a bridge keep its bridge-time
value? It could then still forward to its former peers, because
yt921x_isolate() only blocks the peer->port direction.

The condition is unchanged at the end of the series, in "net: dsa:
motorcomm: Support BR_FLOOD and BR_MCAST_FLOOD offloading". Was the
intended test if (!dsa_is_cpu_port(ds, port))?

>  
>  	/* Turn off FDB learning to prevent FDB pollution */
>  	mask = YT921X_PORT_LEARN_DIS;

[ ... ]

> @@ -2302,7 +2285,7 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
>  	if (res)
>  		return res;
>  
> -	ctrl = GENMASK(10, 0);
> +	ctrl = GENMASK(10, 0) & ~cpu_ports_mask;
>  	res = yt921x_reg_write(priv, YT921X_FILTER_UNK_UCAST, ctrl);
>  	if (res)
>  		return res;
> @@ -2325,13 +2308,6 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
>  	ctrl = 0;
>  	for (int i = 0; i < YT921X_PORT_NUM; i++)
>  		ctrl |= YT921X_ACT_UNK_ACTn_TRAP(i);
> -	/* Except for CPU ports, if any packets are sent via CPU ports without
> -	 * tag, they should be dropped.
> -	 */
> -	for_each_set_bit(port, &cpu_ports_mask, YT921X_PORT_NUM) {
> -		ctrl &= ~YT921X_ACT_UNK_ACTn_M(port);
> -		ctrl |= YT921X_ACT_UNK_ACTn_DROP(port);
> -	}

[Severity: High]
This removes the drop action for CPU ports, and the hunk above takes CPU
ports out of FILTER_UNK_UCAST/MCAST. Both changes assume the conduit keeps
the ~0 isolation from yt921x_chip_reset(). Does that still hold after a
bridge join?

yt921x_bridge() has:

static int yt921x_bridge(struct yt921x_priv *priv, u16 ports_mask)
{
	unsigned long targets_mask = ports_mask & dsa_cpu_ports(&priv->ds);
	...
	/* Block from non-cpu bridge ports ... */
	for_each_set_bit(port, &targets_mask, YT921X_PORT_NUM) {
		...
		/* to non-bridge ports */
		ctrl = ~ports_mask;
		...
		res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port),
				       ctrl);

In the baseline this mask was ports_mask & ~priv->cpu_ports_mask, which
matches the "Block from non-cpu bridge ports" comment. The ~ was lost in
the earlier "net: dsa: motorcomm: Remove local cpu_ports_mask" commit in
this series.

Both yt921x_dsa_port_bridge_join() and yt921x_bridge_flags() (on
BR_HAIRPIN_MODE or BR_ISOLATED changes) OR dsa_cpu_ports() into
ports_mask. The loop therefore rewrites only the CPU port register, with
~ports_mask | BIT(cpu), which unblocks the conduit toward every bridge
member.

With the drop action and FILTER_UNK coverage gone, can untagged
broadcast and unknown unicast/multicast sent on the conduit now reach the
bridged user ports without going through the DSA tag? IPv6 link-local
NDP/MLD/RS that the host sends on the conduit netdev would be one source
of such frames.

The isolation registers of the bridged user ports themselves are never
programmed, so they stay at ~0. Would learned unicast between bridge
members then be dropped by isolation after the FDB lookup? And would
broadcast from bridged ports be unable to reach the CPU?

This mask is also unchanged at the end of the series, in "net: dsa:
motorcomm: Support BR_FLOOD and BR_MCAST_FLOOD offloading".

[Severity: Low]
On the same subject of CPU port isolation registers, yt921x_isolate() has:

	for (int i = 0; i < YT921X_PORT_NUM; i++) {
		if (dsa_is_cpu_port(&priv->ds, port) || i == port)
			continue;

Should this test i instead of port? The baseline was
(BIT(i) & priv->cpu_ports_mask) || i == port.

yt921x_dsa_port_bridge_leave() returns early for CPU ports, so port is
always a user port here. As a result the CPU port registers are no longer
skipped, and BIT(port) gets set in them. Those are the registers this
patch says should stay as yt921x_chip_reset() left them. The extra bit
moves the conduit toward ~0, so there is no visible forwarding regression.

The wrong variable came in with "net: dsa: motorcomm: Remove local
cpu_ports_mask" and is still there at the end of the series.

>  	res = yt921x_reg_write(priv, YT921X_ACT_UNK_UCAST, ctrl);
>  	if (res)
>  		return res;

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

Re: [PATCH net-next v4 7/8] net: dsa: motorcomm: Translate FDB VID for VLAN-unaware bridges

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] Static entries for VLAN-unaware bridges now share FID 4095
  with hardware-learned dynamic entries.
- [Low] The commit fixes a functional bug that has existed since the
  driver was added, but it carries no Fixes: tag.
- [Low] yt921x_fdb_dump() declares `u16 vid;` (line 582) with no
  initializer.

--

net: dsa: motorcomm: Translate FDB VID for VLAN-unaware bridges

This patch maps VID 0 from a VLAN-unaware bridge to the unaware FID
(YT921X_VID_UNWARE, 4095) in yt921x_fdb_join() and yt921x_fdb_leave().
It also maps FID 4095 back to VID 0 in yt921x_fdb_dump(). The goal is
that static FDB entries installed through switchdev match the hardware
lookups.

> A VLAN-unaware bridge reports VID 0 for its FDB entries, but lookups run
> with the unaware FID (VID 4095). Entries added through switchdev were
> installed under FID 0 and could never be hit by the hardware lookups.

[Severity: Low]
This describes a user-visible bug that has been there since the driver
was added. Should this carry a Fixes: tag?

YT921X_VID_UNWARE was added in the same commit as the PVID 4095 setup in
yt921x_vlan_filtering(). So were yt921x_fdb_join() and yt921x_fdb_leave(),
which write the caller's VID straight into FDB_IO1_FID. That suggests:

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 1b05ac29ecd76..e41fe4859bfe4 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -590,6 +590,9 @@ yt921x_fdb_dump(struct yt921x_priv *priv, u16 ports_mask,
>  	res = yt921x_fdb_read(priv, addr, &vid, &pmask, &index, &status);
>  	if (res)
>  		return res;
> +
> +	if (vid == YT921X_VID_UNWARE)
> +		vid = 0;

[Severity: Low]
Can this read vid before it has been initialized?

In yt921x_fdb_dump(), vid has no initializer. yt921x_fdb_read() returns 0
without writing *vidp when the slot is NOTFOUND or has INVALID status:

yt921x_fdb_read() {
    ...
	if (val & YT921X_FDB_RESULT_NOTFOUND) {
		*ports_maskp = 0;
		return 0;
	}
    ...
	if ((data1 & YT921X_FDB_IO1_STATUS_M) ==
	    YT921X_FDB_IO1_STATUS_INVALID) {
		*ports_maskp = 0;
		return 0;
	}
    ...
}

When FDB index 0 is empty, the new comparison therefore runs on an
uninitialized stack value. The result is thrown away because cb() is
skipped when pmask is 0. KMSAN or -Wmaybe-uninitialized could still flag
it.

Before this patch, vid was only read inside the (pmask & ports_mask)
block.

The copy inside the GET_NEXT loop is fine because the !pmask break runs
first. Could this remap move inside the pmask check, or could vid be
initialized?

>  	if ((pmask & ports_mask) && !is_multicast_ether_addr(addr)) {
>  		res = cb(addr, vid,
>  			 status == YT921X_FDB_ENTRY_STATUS_STATIC, data);

[ ... ]

> @@ -782,6 +791,13 @@ yt921x_fdb_join(struct yt921x_priv *priv, const unsigned char *addr, u16 vid,
>  	u32 val2;
>  	int res;
>  
> +	/* A vlan-unaware bridge reports vid 0, but untagged lookups on its
> +	 * ports run with the unaware FID; map so that static entries match the
> +	 * hardware lookups.
> +	 */
> +	if (!vid)
> +		vid = YT921X_VID_UNWARE;
> +
>  	/* Check for presence */
>  	res = yt921x_fdb_has(priv, addr, vid, &index);
>  	if (res)

[Severity: Medium]
With this mapping, static entries for a VLAN-unaware bridge now share FID
4095 with entries the hardware learns. Before, they were in FID 0, which
learning never touched. What happens when yt921x_fdb_has() finds an
already learned entry for the same MAC?

Further down in yt921x_fdb_join(), the existing entry is not replaced.
It is forced to static and the new port is ORed into its egress mask:

	ctrl1 = val1 & ~YT921X_FDB_IO1_STATUS_M;
	ctrl1 |= YT921X_FDB_IO1_STATUS_STATIC;
	ctrl2 = val2 | YT921X_FDB_IO2_EGR_PORTS(ports_mask);

A learned entry on port P thus becomes a static entry for both P and the
new port, so unicast to that MAC is also sent to P.

On delete, yt921x_fdb_leave() only clears the departing port's bit:

	ctrl2 = val2 & ~YT921X_FDB_IO2_EGR_PORTS(ports_mask);

That leaves a static entry on P that never ages out. It is not cleared by
yt921x_dsa_port_fast_age() either, because that calls
yt921x_fdb_flush_port(priv, port, false).

Assisted learning on the CPU port is one way to get there:

  station X is learned in hardware on switch port P
  X moves to a foreign bridge port, e.g. Wi-Fi
  port_fdb_add(cpu, X, 0)  -> static {P, CPU}
  bridge entry ages out
  port_fdb_del(cpu, X, 0)  -> static {P}

If X later shows up on another switch port, hardware learning cannot
override the static entry. DSA also ignores bridge-learned entries on
offloaded ports, so traffic to X keeps going to P.

The same stale state can come from a user "bridge fdb add X dev swpA
master static" for a MAC currently learned on swpB, followed by
"bridge fdb del".

The merge logic is older than this patch and already affects VLAN-aware
bridges. This mapping makes it reachable for VLAN-unaware bridges too.
Can this leave static entries in FID 4095 that the software bridge does
not know about?

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

Re: [PATCH net-next v4 8/8] net: dsa: motorcomm: Support BR_FLOOD and BR_MCAST_FLOOD offloading

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 4 · 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 flood offload stops trapping unknown unicast/multicast on
  bridge ports and relies on hardware flooding, which obeys…
- [High] net: dsa: motorcomm: inverted dsa_is_cpu_port() check in
  yt921x_userport_standalone()
- [Medium] The patch deletes the setup-time programming of
  YT921X_FILTER_UNK_UCAST/YT921X_FILTER_UNK_MCAST from…
- [Medium] When the switch has zero or several bridges,
  yt921x_bridge_flood_set() writes ~(u32)0 to both…
- [Medium] Before this patch, all unknown multicast from user ports was
  trapped to the CPU (FILTER_UNK_MCAST bit set, ACT_UNK=TRAP).
- [Medium] net: dsa: motorcomm: wrong port index passed to
  dsa_is_cpu_port() in yt921x_isolate()
- [Low] yt921x_bridge_join() adds yt921x_bridge_flood_set() as a new
  failing step after yt921x_userport_bridge() (learning enabled) and…

--

net: dsa: motorcomm: Support BR_FLOOD and BR_MCAST_FLOOD offloading

This commit lets the switch flood unknown unicast and multicast in
hardware by clearing a bridge port's FILTER_UNK bits. That happens only
when the switch carries a single bridge, all other members have flooding
enabled, and the port is not in hairpin mode. The setup-time FILTER_UNK
programming in yt921x_chip_setup_dsa() is removed, and the registers are
now recomputed on bridge join, bridge leave and port flag changes.

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

[ ... ]

> @@ -1232,6 +1238,96 @@ static int yt921x_userport_bridge(struct yt921x_priv *priv, int port)
>  	return 0;
>  }
>  
> +/* ports_mask is optional */
> +static int yt921x_bridge_flood_set(struct yt921x_priv *priv, u16 ports_mask)
> +{

[ ... ]

> +	ret = 0;
> +	if (count != 1) {
> +		ctrl = ~(u32)0;
> +		res = yt921x_reg_write(priv, YT921X_FILTER_UNK_UCAST, ctrl);
> +		if (res)
> +			ret = res;
> +		res = yt921x_reg_write(priv, YT921X_FILTER_UNK_MCAST, ctrl);
> +		if (res)
> +			ret = res;
> +		return ret;
> +	}
> +
> +	ports_mask &= ~dsa_cpu_ports(ds);

[Severity: Medium]
This isn't new in this patch, but yt921x_bridge_leave() still runs it
on every leave. The loop in yt921x_isolate() is meant to skip the CPU
ports, but it tests the function argument instead of the loop index:

	if (dsa_is_cpu_port(&priv->ds, port) || i == port)
		continue;

yt921x_dsa_port_bridge_leave() returns early for CPU ports, so port is
always a user port here and dsa_is_cpu_port(&priv->ds, port) is always
false. The loop therefore also sets BIT(port) in the CPU port's
YT921X_PORTn_ISOLATION register.

With the code as it stands, that bit is not a no-op. The CPU port's
isolation is no longer the ~(u32)0 from yt921x_chip_reset():
yt921x_userport_standalone() writes ~dsa_cpu_ports(ds) to it, and
yt921x_bridge() rewrites it from ~ports_mask. Setting BIT(port) there
blocks forwarding from the CPU port to the departing port until the
next yt921x_bridge() call rewrites the register. It also goes against
the comment saying conduit ports should be controlled by tagging
completely.

Was dsa_is_cpu_port(&priv->ds, i) intended here? Or is a blocked
CPU-to-port path harmless because everything sent from the conduit is
steered by the tag?

[Severity: High]
This isn't introduced by this patch, but the patch starts depending on
it. The comment in yt921x_userport_standalone() says conduit ports are
skipped so they keep the one-way isolation from yt921x_chip_reset(),
but the test is the other way round:

	if (dsa_is_cpu_port(ds, port)) {
		ctrl = ~dsa_cpu_ports(ds);

Shouldn't that be !dsa_is_cpu_port(ds, port)? As written, the CPU
port's YT921X_PORTn_ISOLATION is overwritten with ~dsa_cpu_ports(ds),
while every user port keeps the ~(u32)0 from yt921x_chip_reset(). A
standalone user port is then isolated from the CPU port as well as
from its peers.

The comment this patch adds here says FILTER_UNK_UCAST and
FILTER_UNK_MCAST don't matter for standalone ports since all packets
are handled by software. That held while yt921x_chip_setup_dsa() set
FILTER_UNK on every user port, because unknown frames were trapped via
ACT_UNK regardless of isolation. With that write gone, a standalone
port whose FILTER_UNK bit is not set (the reset default at probe, or
the value left behind after leaving a bridge) passes unknown unicast
and multicast to hardware forwarding. The ~(u32)0 isolation then drops
them before they reach the CPU port.

Bridge leave has the same gap. yt921x_bridge_leave() goes through
yt921x_userport_standalone(), which never writes the departing user
port's isolation register, so nothing resets it to ~dsa_cpu_ports(ds).
Once yt921x_bridge() programs user port isolation the way its comment
describes, a port that leaves the bridge would keep forwarding in
hardware to its former bridge peers.

Should the condition be fixed earlier in the series, so the FILTER_UNK
handling here sits on top of working standalone isolation?

[Severity: Medium]
With zero bridges, or more than one, this writes ~0 to both FILTER_UNK
registers. The setup code removed further down in this patch wrote:

    ctrl = GENMASK(10, 0) & ~cpu_ports_mask;

Doesn't ~0 also set the reserved bits 31:11, which lie outside
YT921X_FILTER_PORTS_M, along with the CPU port's bit?

ACT_UNK is TRAP for every port, including the CPU port. Could untagged
frames with an unknown DA that enter the conduit port then be trapped
back to the CPU? yt921x_tag_rcv() can't map the CPU port index to a user
port, so it would drop them with the ratelimited "Couldn't decode source
port" warning.

In the single-bridge path, ports_mask &= ~dsa_cpu_ports(ds) keeps the
CPU bit out of the update mask. So its value depends on history: the
reset default at boot, or 1 after any zero-bridge or multi-bridge event.

Would YT921X_FILTER_PORTS(~dsa_cpu_ports(ds)) be a better value to write
here?

[ ... ]

> +		ctrl = 0;
> +		for_each_set_bit(port, &targets_mask, YT921X_PORT_NUM) {
> +			struct yt921x_port *pp = &priv->ports[port];
> +
> +			/* Hardware never floods back to its own ingress port,
> +			 * keep hairpin port trapped: the software bridge does
> +			 * deliver a flood there.
> +			 */
> +			if (!pp->hairpin && !((flood_mask ^ ports_mask) &
> +					      ~BIT(port)))
> +				continue;

[Severity: High]
With one bridge and the default BR_FLOOD | BR_MCAST_FLOOD, this clears
FILTER_UNK for every non-hairpin member. Unknown traffic is then flooded
by hardware, which obeys PORTn_ISOLATION. At this point in the series,
does anything open isolation between bridged user ports?

yt921x_bridge() selects only the CPU ports:

    unsigned long targets_mask = ports_mask & dsa_cpu_ports(&priv->ds);

Its own comment says "Block from non-cpu bridge ports ...". The ~ seems
to have been lost in the earlier patch "net: dsa: motorcomm: Remove local
cpu_ports_mask". Before that patch the line was
ports_mask & ~priv->cpu_ports_mask.

yt921x_userport_standalone() also rewrites isolation only when
dsa_is_cpu_port(ds, port) is true. So user ports keep the ~(u32)0
written by yt921x_chip_reset(). Separately, yt921x_isolate() tests
dsa_is_cpu_port(&priv->ds, port) where it looks like it should test i.

Before this patch every unknown frame was trapped, so the software bridge
still flooded it. Now the frame is flooded by hardware from a source port
whose isolation blocks all peers. Any copy that reaches the CPU carries
YT921X_TAG_CODE_FORWARD, and yt921x_tag_rcv() calls
dsa_default_offload_fwd_mark() on it, so the software bridge won't flood
it again.

After "ip link set lan1 master br0; ip link set lan2 master br0", is
unknown unicast and multicast still delivered between lan1 and lan2?

> +
> +			ctrl |= BIT(port);
> +		}
> +
> +		/* ACT_UNK is fixed to TRAP for every port, see
> +		 * yt921x_chip_setup_dsa().
> +		 */
> +
> +		res = yt921x_reg_update_bits(priv, addr, ports_mask, ctrl);

[Severity: Medium]
Before this patch, all unknown multicast from user ports was trapped. The
software bridge therefore received IEEE link-local groups
(01:80:C2:00:00:0x: STP BPDUs, LACPDUs, LLDP, EAPOL) and consumed them
without forwarding.

Clearing FILTER_UNK_MCAST here hands unknown multicast to hardware
flooding. I couldn't find a reserved multicast (RMA) trap programmed
anywhere in drivers/net/dsa/motorcomm/.

chip.h defines YT921X_ACT_UNK_MCAST_BYPASS_DROP_RMA inside the
ACT_UNK_MCAST register. That suggests RMA frames go through the unknown
multicast action path.

If so, are BPDUs and LACPDUs now flooded by hardware instead of trapped?
With isolation open they would be forwarded between bridge ports. With
the current all-blocked user port isolation they may not reach the CPU
at all. Copies that do reach the CPU would carry the FORWARD code and get
offload_fwd_mark.

> +		if (res)
> +			ret = res;
> +	}

[ ... ]

> @@ -1322,6 +1422,10 @@ yt921x_bridge_join(struct yt921x_priv *priv, int port, u16 ports_mask)
>  	if (res)
>  		return res;
>  
> +	res = yt921x_bridge_flood_set(priv, ports_mask);
> +	if (res)
> +		return res;
> +

[Severity: Low]
If yt921x_bridge_flood_set() fails here, yt921x_userport_bridge() has
already enabled learning and yt921x_bridge() has already programmed
isolation. DSA doesn't send BRIDGE_LEAVE when the join notifier fails:

net/dsa/port.c:dsa_port_bridge_join() {
    ...
	err = dsa_broadcast(DSA_NOTIFIER_BRIDGE_JOIN, &info);
	if (err)
		goto out_rollback;
    ...
out_rollback:
	dsa_port_bridge_destroy(dp, br);
	return err;
}

Would this leave learning enabled on a port that DSA now treats as
standalone?

yt921x_bridge_flood_set() also keeps going after a failed write. So one
FILTER_UNK register might be updated while the other is not, and both
updates were computed with the joining port counted as a member.

[ ... ]

> @@ -2301,14 +2422,6 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
>  	if (res)
>  		return res;
>  
> -	ctrl = GENMASK(10, 0) & ~cpu_ports_mask;
> -	res = yt921x_reg_write(priv, YT921X_FILTER_UNK_UCAST, ctrl);
> -	if (res)
> -		return res;
> -	res = yt921x_reg_write(priv, YT921X_FILTER_UNK_MCAST, ctrl);
> -	if (res)
> -		return res;
> -

[Severity: Medium]
With this write removed, yt921x_bridge_flood_set() is the only writer of
YT921X_FILTER_UNK_UCAST and YT921X_FILTER_UNK_MCAST. It runs only on
bridge join, leave and flags events.

Are standalone ports left on the hardware reset default from probe until
the first bridge event? Ports that never join the bridge would stay on
it for good.

There is also the leave case. dsa_port_bridge_leave() clears dp->bridge
before it broadcasts:

net/dsa/port.c:dsa_port_bridge_leave() {
    ...
	dsa_port_bridge_destroy(dp, br);

	err = dsa_broadcast(DSA_NOTIFIER_BRIDGE_LEAVE, &info);
    ...
}

Take "ip link set lan1 master br0; ip link set lan2 master br0; ip link
set lan1 nomaster". yt921x_bridge_leave()->yt921x_bridge_flood_set(priv,
0) sees count == 1, and ports_mask covers only lan2, so the cleared
FILTER_UNK bits of lan1 are never restored.

The flag reset that follows, through yt921x_bridge_flags(), returns
early here:

	bdev = dsa_port_bridge_dev_get(dp);
	if (!bdev)
		return 0;

yt921x_userport_standalone() only resets pp->flood and pp->mcast_flood
in software.

The new comment in yt921x_userport_standalone() says FILTER_UNK is
unrelated "since all packets are handled by software". What makes sure
standalone traffic is still trapped to the CPU in these cases?

The same zero-bridge configuration also ends up with different hardware
state at boot (reset default) and after the last bridge goes away (~0,
all trapped).

The commit message says:

  This should cover most single-bridge usage. Other data paths remain
  unchanged in terms of behavior.

Does that still hold for standalone ports?

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