Re: [RFC PATCH 02/10] net: dsa: b53: prevent FAST_AGE access on BCM5325

[Vladimir Oltean <olteanv@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1616 bytes --]

On Mon, Jun 02, 2025 at 11:01:40AM -0700, Florian Fainelli wrote:
> On 6/2/25 02:37, Vladimir Oltean wrote:
> > Hello,
> > 
> > On Sat, May 31, 2025 at 12:13:00PM +0200, Álvaro Fernández Rojas wrote:
> > > BCM5325 doesn't implement FAST_AGE registers so we should avoid reading or
> > > writing them.
> > > 
> > > Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
> > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> > > ---
> > 
> > How about implementing a "slow age" procedure instead? Walk through the
> > FDB, and delete the dynamically learned entries for the port?
> > 
> > Address aging is important for STP state transitions.
> 
> That's a good suggestion, I suppose for now this can be b53 specific until
> we encounter another 20 year old switch and then we move that logic within
> the DSA framework?
> -- 
> Florian

Hmm, thank you for saying that, I didn't even consider consolidating the
logic in the DSA framework, but it sure makes sense and we already have
almost all API required to do that.

So I now have a WIP patch attached, but the consolidation into the
framework is definitely a net-next activity, since it will also affect
mt7530, hellcreek and vsc73xx, and those need testing. Also, for b53 it
further requires changing the port_fast_age() prototype.

The question further becomes whether for stable kernels we should
implement a local slow age procedure in b53 like the one from sja1105,
only to delete it in net-next, or to skip it altogether, go with
Álvaro's patch as is and concentrate on the net-next implementation
directly. I'm ok both ways.

[-- Attachment #2: 0001-net-dsa-centralize-the-slow-aging-procedure-from-sja.patch --]
[-- Type: text/x-diff, Size: 7257 bytes --]

From ceb0f0b25ecd3bae6629bbb74fe30f030ddd3a0a Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Tue, 3 Jun 2025 12:40:30 +0300
Subject: [PATCH] net: dsa: centralize the slow aging procedure from sja1105
 (WIP)

With more hardware drivers which are unable to perform a dynamic FDB
flush on a port (see
https://lore.kernel.org/netdev/20250531101308.155757-3-noltari@gmail.com/),
it makes sense to move the sja1105 logic to the framework level, so that
more drivers can flush out dynamically learned entries (relevant when
transitioning to a bridge port STP state incompatible with learning).

The drivers which have .port_fdb_dump() and .port_fdb_del() but not
.port_fast_age() are mt7530, hellcreek, vsc73xx. These will go through
dsa_port_slow_age() now.

TODO: multi-generational drivers like b53 cannot signal that old
hardware cannot do fast ageing and should fall back to
dsa_port_slow_age(), because they offer a single ds->ops->port_fast_age()
to the framework (which returns void), and the framework relies purely
on the presence of the function pointer to determine that the function
is implemented. We should change ds->ops->port_fast_age() to return int,
and treat -EOPNOTSUPP, so as to fall back to slow aging even in that
case.

Also, this change also has squashed the conversion of some function
prototypes from "struct dsa_port *" to "const struct dsa_port *", to be
compatible with the dsa_port_fast_age() caller where dp is a const
pointer. Eventualy, these changes should be split out into a preparatory
change.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 53 --------------------------
 net/dsa/port.c                         | 50 +++++++++++++++++++++---
 net/dsa/port.h                         |  5 ++-
 3 files changed, 48 insertions(+), 60 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index f8454f3b6f9c..77faa43880ed 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1910,58 +1910,6 @@ static int sja1105_fdb_dump(struct dsa_switch *ds, int port,
 	return 0;
 }
 
-static void sja1105_fast_age(struct dsa_switch *ds, int port)
-{
-	struct dsa_port *dp = dsa_to_port(ds, port);
-	struct sja1105_private *priv = ds->priv;
-	struct dsa_db db = {
-		.type = DSA_DB_BRIDGE,
-		.bridge = {
-			.dev = dsa_port_bridge_dev_get(dp),
-			.num = dsa_port_bridge_num_get(dp),
-		},
-	};
-	int i;
-
-	mutex_lock(&priv->fdb_lock);
-
-	for (i = 0; i < SJA1105_MAX_L2_LOOKUP_COUNT; i++) {
-		struct sja1105_l2_lookup_entry l2_lookup = {0};
-		u8 macaddr[ETH_ALEN];
-		int rc;
-
-		rc = sja1105_dynamic_config_read(priv, BLK_IDX_L2_LOOKUP,
-						 i, &l2_lookup);
-		/* No fdb entry at i, not an issue */
-		if (rc == -ENOENT)
-			continue;
-		if (rc) {
-			dev_err(ds->dev, "Failed to read FDB: %pe\n",
-				ERR_PTR(rc));
-			break;
-		}
-
-		if (!(l2_lookup.destports & BIT(port)))
-			continue;
-
-		/* Don't delete static FDB entries */
-		if (l2_lookup.lockeds)
-			continue;
-
-		u64_to_ether_addr(l2_lookup.macaddr, macaddr);
-
-		rc = __sja1105_fdb_del(ds, port, macaddr, l2_lookup.vlanid, db);
-		if (rc) {
-			dev_err(ds->dev,
-				"Failed to delete FDB entry %pM vid %lld: %pe\n",
-				macaddr, l2_lookup.vlanid, ERR_PTR(rc));
-			break;
-		}
-	}
-
-	mutex_unlock(&priv->fdb_lock);
-}
-
 static int sja1105_mdb_add(struct dsa_switch *ds, int port,
 			   const struct switchdev_obj_port_mdb *mdb,
 			   struct dsa_db db)
@@ -3222,7 +3170,6 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
 	.port_fdb_dump		= sja1105_fdb_dump,
 	.port_fdb_add		= sja1105_fdb_add,
 	.port_fdb_del		= sja1105_fdb_del,
-	.port_fast_age		= sja1105_fast_age,
 	.port_bridge_join	= sja1105_bridge_join,
 	.port_bridge_leave	= sja1105_bridge_leave,
 	.port_pre_bridge_flags	= sja1105_port_pre_bridge_flags,
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 082573ae6864..2875bda2603f 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -52,14 +52,53 @@ static void dsa_port_notify_bridge_fdb_flush(const struct dsa_port *dp, u16 vid)
 				 brport_dev, &info.info, NULL);
 }
 
+struct dsa_port_slow_age_ctx {
+	const struct dsa_port *dp;
+};
+
+static int
+dsa_port_slow_age_entry(const unsigned char *addr, u16 vid,
+			bool is_static, void *data)
+{
+	struct dsa_port_slow_age_ctx *ctx = data;
+	const struct dsa_port *dp = ctx->dp;
+
+	if (is_static)
+		return 0;
+
+	dev_dbg(dp->ds->dev,
+		"Flushing dynamic FDB entry %pM vid %u on port %d\n",
+		addr, vid, dp->index);
+
+	return dsa_port_fdb_del(dp, addr, vid);
+}
+
+static int dsa_port_slow_age(const struct dsa_port *dp)
+{
+	struct dsa_port_slow_age_ctx ctx = {
+		.dp = dp,
+	};
+
+	return dsa_port_fdb_dump(dp, dsa_port_slow_age_entry, &ctx);
+}
+
 static void dsa_port_fast_age(const struct dsa_port *dp)
 {
 	struct dsa_switch *ds = dp->ds;
+	int err = 0;
 
-	if (!ds->ops->port_fast_age)
-		return;
+	if (ds->ops->port_fast_age)
+		ds->ops->port_fast_age(ds, dp->index);
+	else
+		err = dsa_port_slow_age(dp);
 
-	ds->ops->port_fast_age(ds, dp->index);
+	if (err && err != -EOPNOTSUPP) {
+		dev_err(ds->dev,
+			"Port %d failed to age dynamic FDB entries: %pe\n",
+			dp->index, ERR_PTR(err));
+	}
+	if (err)
+		return;
 
 	/* flush all VLANs */
 	dsa_port_notify_bridge_fdb_flush(dp, 0);
@@ -996,7 +1035,7 @@ int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 	return dsa_port_notify(dp, DSA_NOTIFIER_FDB_ADD, &info);
 }
 
-int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
+int dsa_port_fdb_del(const struct dsa_port *dp, const unsigned char *addr,
 		     u16 vid)
 {
 	struct dsa_notifier_fdb_info info = {
@@ -1151,7 +1190,8 @@ int dsa_port_lag_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 	return dsa_port_notify(dp, DSA_NOTIFIER_LAG_FDB_DEL, &info);
 }
 
-int dsa_port_fdb_dump(struct dsa_port *dp, dsa_fdb_dump_cb_t *cb, void *data)
+int dsa_port_fdb_dump(const struct dsa_port *dp, dsa_fdb_dump_cb_t *cb,
+		      void *data)
 {
 	struct dsa_switch *ds = dp->ds;
 	int port = dp->index;
diff --git a/net/dsa/port.h b/net/dsa/port.h
index 6bc3291573c0..ea20ed6d706e 100644
--- a/net/dsa/port.h
+++ b/net/dsa/port.h
@@ -48,7 +48,7 @@ int dsa_port_vlan_msti(struct dsa_port *dp,
 int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu);
 int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 		     u16 vid);
-int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
+int dsa_port_fdb_del(const struct dsa_port *dp, const unsigned char *addr,
 		     u16 vid);
 int dsa_port_standalone_host_fdb_add(struct dsa_port *dp,
 				     const unsigned char *addr, u16 vid);
@@ -62,7 +62,8 @@ int dsa_port_lag_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 			 u16 vid);
 int dsa_port_lag_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 			 u16 vid);
-int dsa_port_fdb_dump(struct dsa_port *dp, dsa_fdb_dump_cb_t *cb, void *data);
+int dsa_port_fdb_dump(const struct dsa_port *dp, dsa_fdb_dump_cb_t *cb,
+		      void *data);
 int dsa_port_mdb_add(const struct dsa_port *dp,
 		     const struct switchdev_obj_port_mdb *mdb);
 int dsa_port_mdb_del(const struct dsa_port *dp,
-- 
2.43.0