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

[PATCH net-next v5 0/7] 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.

v5:
  - drop patch 3 of preexisting issue
  - fix issues found by sashiko
  - program RMA registers
v4: https://lore.kernel.org/r/20261107121631.1040612-1-mmyangfl@gmail.com
  - 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 (7):
  net: dsa: motorcomm: Fix VLAN SVL and learn limit definitions
  net: dsa: motorcomm: Drop VID 0 VLAN table programming
  net: dsa: motorcomm: Remove local cpu_ports_mask
  net: dsa: motorcomm: Isolate all ports during chip setup
  net: dsa: motorcomm: Translate FDB VID for VLAN-unaware bridges
  net: dsa: motorcomm: Report inconsistency on bridge join failure
  net: dsa: motorcomm: Support BR_FLOOD and BR_MCAST_FLOOD offloading

 drivers/net/dsa/motorcomm/chip.c | 242 +++++++++++++++++++++++++------
 drivers/net/dsa/motorcomm/chip.h |  21 ++-
 2 files changed, 213 insertions(+), 50 deletions(-)

-- 
2.53.0

[PATCH net-next v5 1/7] 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 v5 2/7] 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 v5 3/7] 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 47f06945b8c4..7686995b803f 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);
@@ -1158,11 +1158,12 @@ yt921x_dsa_port_vlan_add(struct dsa_switch *ds, int port,
 
 static int yt921x_userport_standalone(struct yt921x_priv *priv, int port)
 {
+	struct dsa_switch *ds = &priv->ds;
 	u32 mask;
 	u32 ctrl;
 	int res;
 
-	ctrl = ~priv->cpu_ports_mask;
+	ctrl = ~dsa_cpu_ports(ds);
 	res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port), ctrl);
 	if (res)
 		return res;
@@ -1208,7 +1209,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, i) || i == port)
 			continue;
 
 		res = yt921x_reg_set_bits(priv, YT921X_PORTn_ISOLATION(i),
@@ -1225,7 +1226,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;
@@ -1338,7 +1339,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;
@@ -1410,7 +1411,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);
@@ -2268,10 +2269,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;
@@ -2308,7 +2309,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 v5 4/7] 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 7686995b803f..fbf3a0cba49b 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -2220,6 +2220,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;
 
@@ -2257,6 +2258,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 v5 5/7] 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 fbf3a0cba49b..46534127d070 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -590,7 +590,10 @@ 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 ((pmask & ports_mask) && !is_multicast_ether_addr(addr)) {
+		if (vid == YT921X_VID_UNWARE)
+			vid = 0;
 		res = cb(addr, vid,
 			 status == YT921X_FDB_ENTRY_STATUS_STATIC, data);
 		if (res)
@@ -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 v5 6/7] net: dsa: motorcomm: Report inconsistency on bridge join failure

[David Yang]

DSA does not send bridge_leave when port_bridge_join fails, so a join
that fails midway leaves the port half configured. Rolling back from the
driver would just issue more register accesses that can fail the same
way. Set an extack message on failure instead.

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

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 46534127d070..3d4aa79ac026 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -1433,6 +1433,11 @@ yt921x_dsa_port_bridge_join(struct dsa_switch *ds, int port,
 	res = yt921x_bridge_join(priv, port, ports_mask);
 	mutex_unlock(&priv->reg_lock);
 
+	/* DSA does not send bridge_leave when join fails */
+	if (res)
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Fatal IO error; hardware no longer consistent");
+
 	return res;
 }
 
-- 
2.53.0

[PATCH net-next v5 7/7] 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 | 181 ++++++++++++++++++++++++++-----
 drivers/net/dsa/motorcomm/chip.h |  15 +++
 2 files changed, 170 insertions(+), 26 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 3d4aa79ac026..480203ac7c8e 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -1195,13 +1195,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;
 }
 
@@ -1218,6 +1222,97 @@ 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 = YT921X_FILTER_PORTS_M;
+		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;
+	}
+
+	if (ports_mask)
+		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;
@@ -1292,6 +1387,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;
 }
 
@@ -1308,6 +1407,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;
 }
 
@@ -1316,7 +1419,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;
 
@@ -1330,36 +1438,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;
@@ -2332,9 +2452,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++)
@@ -2353,6 +2472,16 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
 	if (res)
 		return res;
 
+	/* Capture RMA when offloading mcast flood to hardware */
+	for (int i = 0; i < 0x10; i++) {
+		/* Reuse unknown mcast code */
+		ctrl = YT921X_RMA_CTRL_CPU_CODE(0x1a) |
+		       YT921X_RMA_CTRL_COPY_TO_CPU | YT921X_RMA_CTRL_DROP;
+		res = yt921x_reg_write(priv, YT921X_RMA_CTRLn(i), ctrl);
+		if (res)
+			return res;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h
index ab6f28d057cb..12a7d1bf9f1d 100644
--- a/drivers/net/dsa/motorcomm/chip.h
+++ b/drivers/net/dsa/motorcomm/chip.h
@@ -329,6 +329,17 @@ enum yt921x_app_selector {
 #define YT921X_LAG_MEMBERnm(n, m)	(0x1805b0 + 4 * (4 * (n) + (m)))
 #define  YT921X_LAG_MEMBER_PORT_M		GENMASK(3, 0)
 #define   YT921X_LAG_MEMBER_PORT(x)			FIELD_PREP(YT921X_LAG_MEMBER_PORT_M, (x))
+#define YT921X_RMA_CTRLn(n)		(0x1805d0 + 4 * (n))
+#define  YT921X_RMA_CTRL_BYPASS_STORM		BIT(12)
+#define  YT921X_RMA_CTRL_BYPASS_FLOW_METER	BIT(11)
+#define  YT921X_RMA_CTRL_BYPASS_PORT_METER	BIT(10)
+#define  YT921X_RMA_CTRL_DROP			BIT(9)
+#define  YT921X_RMA_CTRL_COPY_TO_CPU		BIT(8)
+#define  YT921X_RMA_CTRL_IGNORE_VLAN		BIT(7)
+#define  YT921X_RMA_CTRL_IGNORE_PORT_ISOLATE	BIT(6)
+#define  YT921X_RMA_CTRL_CPU_CODE_M		GENMASK(5, 0)
+#define   YT921X_RMA_CTRL_CPU_CODE(x)			FIELD_PREP(YT921X_RMA_CTRL_CPU_CODE_M, (x))
+#define   YT921X_RMA_CTRL_CPU_CODE_DEFAULT		0x3f
 #define YT921X_CPU_COPY			0x180690
 #define  YT921X_CPU_COPY_FORCE_INT_PORT		BIT(2)
 #define  YT921X_CPU_COPY_TO_INT_CPU		BIT(1)
@@ -461,6 +472,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 v5 2/7] net: dsa: motorcomm: Drop VID 0 VLAN table programming

[netdev-bot+sashiko]

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

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

- [Medium] The commit message says the VID 0 VLAN table entry is never
  looked up because the hardware gives priority-tagged frames the port's…
- [Medium] The commit removes the VID 0 VLAN table programming for every
  supported chip, but its only evidence is a measurement on YT9215S.

--

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

This commit removes the setup-time write of the VID 0 VLAN table entry
from yt921x_chip_setup_dsa(). The commit message says the entry is
unreachable because priority-tagged frames are classified to the port's
PVID before the VLAN table lookup, as measured on YT9215S.

> 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);

[Severity: Medium]
Is the VID 0 entry still unreachable when the port's PVID is itself 0?

The commit message's reasoning only works if the PVID is non-zero. The
driver sets PVID 0 on VLAN-filtering ports in two places.

yt921x_pvid_clear():

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

yt921x_vlan_filtering(), where br_vlan_get_pvid() returns 0 when the
bridge port has no PVID:

	br_vlan_get_pvid(bdev, &pvid);
	res = yt921x_port_set_pvid(priv, port, pvid);
	...
	/* Do not drop tagged frames here; let VLAN_IGR_FILTER do it */
	if (vlan_filtering && !pvid)
		ctrl |= YT921X_PORT_VLAN_CTRL1_CVLAN_DROP_UNTAGGED;

In both cases only CVLAN_DROP_UNTAGGED is set. Does CVLAN_DROP_UNTAGGED
also drop priority-tagged frames?

If it does not, then by the classification model in the commit message,
a priority-tagged frame on such a port gets VID 0. The hardware would
then read YT921X_VLANn_CTRL(0) for ingress membership, learning and the
untag bitmap.

Before this patch that entry had a known state: all ports were members,
LEARN_DIS was set and no ports were untagged. After it, the entry holds
either its reset default or whatever yt921x_vlan_add() writes when 8021q
adds VID 0 through dsa_user_vlan_rx_add_vid():

yt921x_vlan_add():

	masks[0] = YT921X_VLAN_CTRLa_PORTn(port) |
		   YT921X_VLAN_CTRLa_PORTS(priv->cpu_ports_mask);
	ctrls[0] = masks[0];

That write never sets LEARN_DIS.

Could this change how priority-tagged frames are forwarded or learned on
VLAN-aware ports that have no PVID?

Later patches in the series do not seem to restore any VID 0
programming. That leaves yt921x_vlan_add() and yt921x_vlan_del() as the
only code that writes VLANn_CTRL(0).

[Severity: Medium]
The commit message says this behaviour was "measured on YT9215S", but
the removal applies to every chip in yt921x_infos. That list includes
YT9215SC, YT9215RB, YT9214NB and YT9213NB. It also includes YT9218N and
YT9218MB, which have a different chip ID major:

	{
		"YT9218N", YT9218_MAJOR, 0, 0,
	...
	{
		"YT9218MB", YT9218_MAJOR, 1, 0,

Has the same PVID classification of priority-tagged frames been
confirmed on the YT9218 parts? The removed comment noted that tagged
VID 0 "confuses the hardware a lot".

If a variant does look up the VID 0 entry for priority-tagged frames,
that entry now stays at its reset default. After 8021q adds VID 0, it
holds a partial state with learning enabled instead.

On those chips, could priority-tagged traffic then be dropped by
VLAN_IGR_FILTER, forwarded to the wrong ports, or learned into the
VID 0 FID?

yt921x_chip_setup_dsa() does not check priv->info->major before
skipping this programming.

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

Re: [PATCH net-next v5 4/7] net: dsa: motorcomm: Isolate all ports during chip setup

[netdev-bot+sashiko]

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

Low priority / nit picks for your consideration:

- [Low] In yt921x_chip_reset() (drivers/net/dsa/motorcomm/chip.c), the
  new port-isolation loop does not run right after the hardware reset,…

--

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

This commit writes all-ones to YT921X_PORTn_ISOLATION for every port in
yt921x_chip_reset(), so that all ports are isolated after the hardware
reset. It prepares for a later isolation refactoring. The commit message
notes that traffic can still leak before and during the reset.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 7686995b803f3..fbf3a0cba49b3 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -2257,6 +2258,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;
> +	}
> +

[Severity: Low]
The commit message says ports are isolated "right after the hardware
reset". Does the placement of this loop match that?

In yt921x_chip_reset() the loop comes after the CPU tag TPID check, and
that check can return early once the reset has already completed:

    res = yt921x_reg_write(priv, YT921X_RST, YT921X_RST_HW);
    ...
    res = yt921x_reg_wait(priv, YT921X_RST, ~0, &val);
    if (res)
        return res;

    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;
    }

If the TPID read fails, or the TPID does not match ETH_P_YT921X, the
function returns before any isolation register is written. The chip has
already been reset at that point, so it forwards between all ports.

yt921x_dsa_setup() then returns the error:

    mutex_lock(&priv->reg_lock);
    res = yt921x_chip_reset(priv);
    mutex_unlock(&priv->reg_lock);

    if (res)
        return res;

Probe fails and the hardware is not touched again. The switch keeps
bridging every front port.

yt921x_chip_detect() has already checked the chip ID, mode and extmode
before the reset. Would it be safe to move this loop to just after
yt921x_reg_wait() and before the TPID check? That would also match the
commit message.

The code is the same at the end of the series, in "net: dsa: motorcomm:
Support BR_FLOOD and BR_MCAST_FLOOD offloading", where the TPID check
still runs before the isolation loop.

>  	return 0;
>  }

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

Re: [PATCH net-next v5 5/7] 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: 1 · Medium: 0 · Low: 2

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

- [High] In drivers/net/dsa/motorcomm/chip.c, yt921x_fdb_join() (line
  798) and yt921x_fdb_leave() (line 749) now rewrite every vid-0 FDB/MDB…
- [Low] The patch fixes a defect that has existed since the driver was
  added in commit 186623f4aa72 ("net: dsa: yt921x: Add support for…
- [Low] The commit message describes only FDB entries ('A VLAN-unaware
  bridge reports VID 0 for its FDB entries').

--

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

This patch remaps VID 0 to the VLAN-unaware FID (YT921X_VID_UNWARE, 4095)
when adding or removing entries in yt921x_fdb_join() and
yt921x_fdb_leave(). It also maps FID 4095 back to VID 0 when unicast
entries are dumped in yt921x_fdb_dump().

> 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]
The commit message only mentions FDB entries. However,
yt921x_fdb_join() and yt921x_fdb_leave() are also the backends for
yt921x_dsa_port_mdb_add() and yt921x_dsa_port_mdb_del():

yt921x_dsa_port_mdb_add()
    res = yt921x_fdb_join(priv, addr, vid, BIT(port));

yt921x_dsa_port_mdb_del()
    res = yt921x_fdb_leave(priv, addr, vid, BIT(port));

That means every vid 0 MDB entry on a VLAN-unaware bridge, from IGMP/MLD
snooping or static mdb entries, also moves from the unused FID 0 into
the live FID 4095.

Before this patch, the hardware treated known-group multicast as unknown
and trapped it to the CPU. The software bridge then forwarded it to the
group members and to the multicast router ports.

Now the hardware forwards it only to the MDB member ports. When the CPU
port is in the mask, the frame arrives with code FORWARD and
offload_fwd_mark set, so software does not forward it again. net/dsa has
no mrouter handling, so router ports that are not group members appear
to stop receiving this traffic.

Could the commit message describe this change in multicast forwarding?
The "back and forth" translation on the dump side also skips multicast
entries.

[ ... ]

>     Signed-off-by: David Yang <mmyangfl@gmail.com>

[Severity: Low]
This isn't a bug, but should this carry a Fixes: tag? Since the driver
was added, yt921x_fdb_join() and yt921x_fdb_leave() have passed vid 0
straight through to FID 0:

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

Static entries were never offloaded. Host FDB entries installed through
assisted_learning_on_cpu_port also never overrode the hardware-learned
FID 4095 entry. So a station that moved from a switch port to a CPU-side
interface kept being forwarded to its old port until ageing.

Note that backporting this patch as written would also backport the
cross-domain FID 4095 issue raised below.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index fbf3a0cba49b3..46534127d070e 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -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;

[Severity: High]
Is this remap safe when more than one forwarding domain uses FID 4095,
or when a vid 0 entry doesn't come from a VLAN-unaware bridge?

Both here and in yt921x_fdb_leave(), every vid 0 add or delete is
remapped. The code ignores the dsa_db argument and never checks the
bridge's vlan_filtering state. Every VLAN-unaware bridge port and every
standalone port seems to share FID 4095:

yt921x_vlan_filtering()
	if (!bdev || !vlan_filtering)
		pvid = YT921X_VID_UNWARE;

yt921x_userport_standalone()
	res = yt921x_port_set_pvid(priv, port, YT921X_VID_UNWARE);

The driver doesn't set ds->fdb_isolation or program a separate FID per
bridge.

Before this patch, vid 0 entries landed in FID 0, which nothing looked
up. Now they are live STATIC entries in the shared FID. The comment in
yt921x_chip_setup_dsa() describes what happens on a cross-domain hit:

	 * isolation takes _after_ FDB lookups, i.e. if an FDB entry (from
	 * another bridge) is matched and the destination port (in another
	 * bridge) is blocked, the packet will be dropped instead of flooding to

Take a VLAN-unaware LAN bridge where a host on swp1 has joined multicast
group G, plus a standalone WAN port:

yt921x_dsa_port_mdb_add(swp1, G, vid 0)
  yt921x_fdb_join()
    vid = YT921X_VID_UNWARE
    STATIC entry {G, FID 4095, egress swp1}

A frame for G that arrives on the standalone WAN port is looked up in
FID 4095 and matches the LAN entry. The standalone isolation mask
(~cpu ports) then removes swp1, so the frame is dropped. Before this
patch, the same frame was unknown multicast and was trapped to the CPU.

Two VLAN-unaware bridges behave the same way. br1 traffic for a MAC or
group known only in br0 is dropped instead of being trapped. If the CPU
port is in the entry, the frame arrives with code FORWARD and
yt921x_tag_rcv() calls dsa_default_offload_fwd_mark(). Then the br1
software path won't forward it to br1 switch ports either.

VLAN-aware bridges send vid 0 entries down too, because br_fdb_add()
adds a vid 0 entry when no VLAN is given. VLAN-aware ports fall back to
pvid 0 rather than 4095:

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

So in FID 4095 those entries would only affect other domains.

The entries are STATIC, so hardware learning can't correct this.

Could the remap be limited to entries that really belong to a
VLAN-unaware bridge, for example based on db.bridge or the port's
VLAN-filtering state, with care for vlan_filtering toggles? The other
option would be for the driver to separate FIDs per bridge first.

Later patches in the series don't touch yt921x_fdb_join() or
yt921x_fdb_leave(). The flood offload patch ("Support BR_FLOOD and
BR_MCAST_FLOOD offloading") mentions that cross-bridge FDB hits are
dropped, but it only restricts flooding of unknown traffic.

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

Re: [PATCH net-next v5 6/7] net: dsa: motorcomm: Report inconsistency on bridge join failure

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · 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] When yt921x_bridge_join() fails partway, the patch knowingly
  leaves the switch half-programmed and only sets an extack message.
- [Medium] In yt921x_dsa_port_bridge_join()
  (drivers/net/dsa/motorcomm/chip.c:1436-1439), a failed join is…
- [Low] The new extack text "Fatal IO error; hardware no longer
  consistent" is set for every non-zero return from…

--

net: dsa: motorcomm: Report inconsistency on bridge join failure

This commit sets an extack error message in yt921x_dsa_port_bridge_join()
when yt921x_bridge_join() fails, because DSA does not call port_bridge_leave
after a failed join. The commit message says that rolling back from the
driver would only issue more register accesses that could fail the same way.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 46534127d070e..3d4aa79ac0268 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -1433,6 +1433,11 @@ yt921x_dsa_port_bridge_join(struct dsa_switch *ds, int port,
>  	res = yt921x_bridge_join(priv, port, ports_mask);
>  	mutex_unlock(&priv->reg_lock);
>  
> +	/* DSA does not send bridge_leave when join fails */
> +	if (res)

[Severity: High]
What state is the switch left in when yt921x_bridge_join() fails partway?
In net/dsa/port.c, dsa_port_bridge_join() does only this:

	err = dsa_broadcast(DSA_NOTIFIER_BRIDGE_JOIN, &info);
	if (err)
		goto out_rollback;
    ...
out_rollback:
	dsa_port_bridge_destroy(dp, br);

So software treats the port as standalone, but the hardware may still have
the following.

YT921X_PORT_LEARN_DIS may be cleared in YT921X_PORTn_LEARN(port) by
yt921x_userport_bridge(). The driver does not set fdb_isolation, so the
standalone port would learn into the shared FDB. yt921x_userport_standalone()
normally prevents that.

Some YT921X_PORTn_ISOLATION() registers may already have been rewritten by
the loop in yt921x_bridge():

		res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port),
				       ctrl);
		if (res)
			return res;

ports_mask includes the joining port. So the lower-indexed members would no
longer block the joining port, and the joining port's own register may
already allow forwarding to the bridge.

Can the switch then forward and flood traffic between a port the kernel
thinks is standalone and the bridge members, until another join/leave
cycle?

The commit message says:

  "Rolling back from the driver would just issue more register accesses
  that can fail the same way."

yt921x_reg_mdio_read() and yt921x_reg_mdio_write() each send the full SMI
address/data sequence by themselves. So an MDIO failure looks
per-transaction and may be transient. The driver also keeps doing register
I/O after this failure.

Would a best-effort call to yt921x_bridge_leave() on this error path work,
reporting the inconsistency only if that call also fails?
yt921x_bridge_leave() already exists. It runs yt921x_userport_standalone()
and then yt921x_isolate() to restore the standalone configuration.

Later in the series, "net: dsa: motorcomm: Support BR_FLOOD and
BR_MCAST_FLOOD offloading" adds yt921x_bridge_flood_set() as a third join
step. It writes the global YT921X_FILTER_UNK_UCAST/MCAST registers, and the
error path is still not rolled back there.

> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Fatal IO error; hardware no longer consistent");

[Severity: Medium]
Will this message reach anyone when extack is NULL? NL_SET_ERR_MSG_MOD()
does nothing in that case, and the legacy bridge ioctl passes NULL:

net/bridge/br_ioctl.c:add_del_if() {
    ...
		ret = br_add_if(br, dev, NULL);
    ...
}

That NULL is passed down this chain:

netdev_upper_dev_link()->dsa_user_changeupper()->dsa_port_bridge_join()
    ->DSA_NOTIFIER_BRIDGE_JOIN->ds->ops->port_bridge_join()

So brctl addif (SIOCBRADDIF) on a yt921x port would still report nothing
beyond the errno.

On the netlink path, the message goes only to the requesting socket. The
hardware state it describes stays after the reply.

The matching leave path in this driver already logs failures in
yt921x_dsa_port_bridge_leave():

	if (res)
		dev_err(dev, "Failed to %s port %d: %i\n", "unbridge",
			port, res);

Should the join path also log with dev_err(), in addition to setting the
extack message?

[Severity: Low]
Is this message accurate for every non-zero return from
yt921x_bridge_join()?

The first hardware access is this chain:

yt921x_userport_bridge()->yt921x_reg_clear_bits()->yt921x_reg_update_bits()

yt921x_reg_update_bits() does this before any write:

	res = yt921x_reg_read(priv, reg, &v);
	if (res)
		return res;

If that read fails, no register has been changed. After
dsa_port_bridge_destroy(), hardware and software still agree, yet the user
is told the hardware is "no longer consistent".

"Fatal" also seems inaccurate. The driver records no dead state, and each
SMI access is self-contained.

A more speculative case: if a register op returned -EOPNOTSUPP,
dsa_user_changeupper() would turn the join into a success:

			if (err == -EOPNOTSUPP) {
				NL_SET_ERR_MSG_WEAK_MOD(extack,
							"Offloading not supported");
				err = 0;
			}

A weak message does not replace the driver's text. The successful join
would then carry the "Fatal IO error" message.

Could the message be set only when a write has actually happened, or be
softened to something like "may be inconsistent"?

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · 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] yt921x_bridge_flood_set() (chip.c lines 1281-1308) clears a
  port's FILTER_UNK_MCAST bit when every other bridge port has…
- [Medium] yt921x_bridge_flood_set() (chip.c line 1297) relies on
  pp->hairpin to keep hairpin ports trapped, because the hardware never…
- [Medium] In the type == 1 (multicast) pass, yt921x_bridge_flood_set()
  (chip.c lines 1281-1308) builds flood_mask only from pp->mcast_flood.
- [Low] yt921x_chip_setup_dsa() (drivers/net/dsa/motorcomm/chip.c, lines
  2475-2483) now reprograms all 16 Reserved Multicast Address entries,…
- [Low] yt921x_bridge_leave() (chip.c line 1390) now calls
  yt921x_bridge_flood_set(priv, 0).

--

net: dsa: motorcomm: Support BR_FLOOD and BR_MCAST_FLOOD offloading

This commit lets the hardware flood unknown unicast and multicast for a
bridge port when the switch carries only one bridge, all other bridge
ports have BR_FLOOD / BR_MCAST_FLOOD set, and the port is not in hairpin
mode. It caches BR_FLOOD and BR_MCAST_FLOOD per port and reprograms
FILTER_UNK_UCAST/MCAST on bridge join, bridge leave and flag changes. It
also sets up the reserved multicast address (RMA) entries to trap to
the CPU.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 3d4aa79ac0268..480203ac7c8ea 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -1218,6 +1222,97 @@ 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)
> +{

[ ... ]

> +	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);
> +		}

[Severity: Medium]
In the multicast pass, is BR_MCAST_FLOOD alone enough to decide that the
hardware may flood unregistered multicast?

When multicast_snooping is enabled (the default) and a querier is present,
the software bridge does not br_flood() unregistered non-link-local IP
multicast. br_multicast_ipv4_rcv()/br_multicast_ipv6_rcv() set
mrouters_only, and br_handle_frame_finish() then calls
br_multicast_flood(), which only reaches MDB ports and router ports.

Once FILTER_UNK_MCAST is cleared here, a frame whose DA is not in the
hardware MDB is flooded to every bridge port that port isolation allows.
The CPU copy comes back with YT921X_TAG_CODE_FORWARD, so yt921x_tag_rcv()
calls dsa_default_offload_fwd_mark() and the bridge can no longer narrow
the flood.

Before this patch these frames were trapped, and software sent them only
to router ports. Should the snooping and router port state be taken into
account before the FILTER_UNK_MCAST bits are cleared?

> +
> +		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: Medium]
Can pp->hairpin disagree with the bridge port's actual hairpin flag here?

pp->hairpin is only written in yt921x_bridge_flags() when the bridge sends
an explicit BR_HAIRPIN_MODE change. DSA core does not sync this flag on
join and does not reset it on leave. dsa_port_inherit_brport_flags() and
dsa_port_clear_brport_flags() both use this mask:

net/dsa/port.c:
	const unsigned long mask = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
				   BR_BCAST_FLOOD | BR_PORT_LOCKED;

Take bond0 as an existing bridge port with hairpin on, and enslave a
yt921x port to it afterwards. dsa_port_lag_join()->dsa_port_bridge_join()
inherits the flood flags but not hairpin. pp->hairpin stays false, so the
FILTER_UNK_UCAST/MCAST bits for that port get cleared.

The hardware then floods unknown unicast and multicast from that port with
offload_fwd_mark set. nbp_switchdev_allowed_egress() keeps the software
bridge from doing the hairpin flood back to bond0. Before this patch these
frames were trapped and software delivered the hairpin flood.

The opposite case also looks possible. A port that leaves a hairpin bridge
and joins a non-hairpin one keeps pp->hairpin true, so it stays trapped
and loses the offload for no reason.

> +
> +			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]
What happens to IGMPv1/v2 and MLDv1 membership reports once
FILTER_UNK_MCAST is cleared?

These reports are sent to the group address. For an unregistered group
they count as unknown multicast, so the hardware floods them to all bridge
ports. The CPU copy carries code FORWARD, so offload_fwd_mark is set.

Before this patch they were trapped with code 0x1a. With snooping on and a
querier present, the bridge set mrouters_only on them, and
br_handle_frame_finish() forwarded them only to multicast router ports.

Now that the reports reach every port, other IGMPv2/MLDv1 hosts in the
same group may suppress their own reports. The bridge would then learn
only the first reporter. MDB entries are offloaded through
yt921x_dsa_port_mdb_add(), so the hardware would forward the group only
to that one port. Could the hosts that suppressed their reports lose the
stream?

The patch adds a trap for the 01:80:C2:00:00:0X range in
yt921x_chip_setup_dsa(), but nothing for IGMP/MLD. The chip appears to
have IGMP-related controls (YT921X_ACT_UNK_MCAST_BYPASS_DROP_IGMP,
YT921X_VLAN_IGR_FILTER_PORTn_BYPASS_IGMP). Should an IGMP/MLD trap be set
up before hardware multicast flooding is enabled? Or does the hardware
already trap these frames by default?

> +		if (res)
> +			ret = res;
> +	}
> +
> +	return ret;
> +}
> +
>  static int yt921x_isolate(struct yt921x_priv *priv, int port)
>  {
>  	u32 mask;
> @@ -1292,6 +1387,10 @@ static int yt921x_bridge_leave(struct yt921x_priv *priv, int port)
>  	if (res)
>  		return res;
>  
> +	res = yt921x_bridge_flood_set(priv, 0);

[Severity: Low]
Does this call assume that dp->bridge is already cleared for the leaving
port?

yt921x_bridge_flood_set() counts bridges from dp->bridge and takes its
fallback mask from dsa_bridge_ports(). That works for
dsa_port_bridge_leave(), which calls dsa_port_bridge_destroy() before it
broadcasts DSA_NOTIFIER_BRIDGE_LEAVE. The join rollback runs in the other
order:

net/dsa/port.c:dsa_port_bridge_join() {
    ...
out_rollback_unbridge:
	dsa_broadcast(DSA_NOTIFIER_BRIDGE_LEAVE, &info);
out_rollback:
	dsa_port_bridge_destroy(dp, br);
	return err;
}

Suppose the failed join created a second bridge, and then
switchdev_bridge_port_offload() or dsa_port_switchdev_sync_attrs() fails.
count is still 2 during the rollback leave, so FILTER_UNK_UCAST/MCAST are
reset to YT921X_FILTER_PORTS_M. Nothing recomputes them after
dsa_port_bridge_destroy(). The one remaining bridge then has no flood
offload until the next flag change or join/leave.

If the failed port joined an existing bridge instead, the bits for the
remaining members are computed with the failed port's stale
pp->flood/pp->mcast_flood still counted.

Separately, the existing comment in yt921x_dsa_port_bridge_join(), "DSA
does not send bridge_leave when join fails", does not seem to match these
rollback paths.

> +	if (res)
> +		return res;
> +
>  	return 0;
>  }

[ ... ]

> @@ -2353,6 +2472,16 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
>  	if (res)
>  		return res;
>  
> +	/* Capture RMA when offloading mcast flood to hardware */
> +	for (int i = 0; i < 0x10; i++) {
> +		/* Reuse unknown mcast code */
> +		ctrl = YT921X_RMA_CTRL_CPU_CODE(0x1a) |
> +		       YT921X_RMA_CTRL_COPY_TO_CPU | YT921X_RMA_CTRL_DROP;
> +		res = yt921x_reg_write(priv, YT921X_RMA_CTRLn(i), ctrl);

[Severity: Low]
This loop programs all 16 RMA entries (01:80:C2:00:00:00 to 0F) on every
setup and for every port. That includes standalone ports and multi-bridge
setups, where flood offload is never enabled. The commit message says:

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

Link-local control frames (STP, LACP, 802.1X, LLDP and so on) are now
classified by the RMA engine rather than by the unknown multicast trap.
All the bypass bits (BYPASS_STORM, BYPASS_PORT_METER, BYPASS_FLOW_METER,
IGNORE_VLAN, IGNORE_PORT_ISOLATE) are left clear.

Could the commit message mention this change? It would also help to say
whether metering, storm control or VLAN handling of these frames changes.

Also, 0x1a has to match YT921X_TAG_CODE_UNK_MCAST in net/dsa/tag_yt921x.c.
That is what makes yt921x_tag_rcv() treat these frames as trapped and
leave offload_fwd_mark clear. Would a named constant, or at least a
comment pointing to the tagger, be better than the literal?

> +		if (res)
> +			return res;
> +	}
> +
>  	return 0;
>  }

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