[PATCH net] net: ti: am65-cpsw-switchdev: flush dynamic FDB entries by port on delete

[PATCH net] net: ti: am65-cpsw-switchdev: flush dynamic FDB entries by port on delete

[MD Danish Anwar]

On an FDB flush, dynamic ALE entries don't get deleted. They stay stale
until the ALE ages them out on its own ~30 seconds later. Two issues in the
DEL_TO_DEVICE handling cause this.

First, the delete was gated on the same "added_by_user" check used for
ADD, so every dynamic delete was dropped before reaching
cpsw_ale_del_ucast() at all. Drop that gate. Dynamic deletes are the only
way to remove a hardware-learned entry early; the ALE never reports upward
when it ages one out.

Second, dropping the gate alone isn't enough: ALE_VLAN_AWARE is always
on in switch mode, so a dynamic entry is stored under a real, nonzero
vid (possibly several, if the same MAC was learned on more than one
vid on a trunk port). With the bridge's vlan_filtering off, the bridge
core never learns the real vid and reports vid=0 on delete, so
cpsw_ale_del_ucast()'s exact (addr, vid) match never finds the row it
returns -ENOENT and the entry is left in place. For this vid=0-ambiguous
case, use a new cpsw_ale_del_ucast_dynamic_by_port() that matches by (addr,
port) instead of guessing one vid, clearing every dynamic row for that MAC
on that port regardless of vid.

Fixes: 86e8b070b25e ("net: ti: am65-cpsw-nuss: Add switchdev support")
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/net/ethernet/ti/am65-cpsw-switchdev.c | 16 +++++++--
 drivers/net/ethernet/ti/cpsw_ale.c            | 36 ++++++++++++++++++-
 drivers/net/ethernet/ti/cpsw_ale.h            |  2 ++
 3 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
index 53cdac272b583..8b4640809cf5b 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
@@ -397,13 +397,23 @@ static void am65_cpsw_switchdev_event_work(struct work_struct *work)
 			   fdb->addr, fdb->vid, fdb->added_by_user,
 			   fdb->offloaded, port_id);
 
-		if (!fdb->added_by_user || fdb->is_local)
+		if (fdb->is_local)
 			break;
 		if (memcmp(port->slave.mac_addr, (u8 *)fdb->addr, ETH_ALEN) == 0)
 			port_id = HOST_PORT_NUM;
 
-		cpsw_ale_del_ucast(cpsw->ale, (u8 *)fdb->addr, port_id,
-				   fdb->vid ? ALE_VLAN : 0, fdb->vid);
+		if (!fdb->added_by_user && !fdb->vid)
+			/* vid=0 here just means "bridge doesn't know the
+			 * real vid" (vlan_filtering=0) -- the dynamic entry
+			 * may be stored under any nonzero vid, or several.
+			 * Delete by (addr, port) instead of guessing a vid.
+			 */
+			cpsw_ale_del_ucast_dynamic_by_port(cpsw->ale,
+							   (u8 *)fdb->addr,
+							   port_id);
+		else
+			cpsw_ale_del_ucast(cpsw->ale, (u8 *)fdb->addr, port_id,
+					   fdb->vid ? ALE_VLAN : 0, fdb->vid);
 		break;
 	default:
 		break;
diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
index e202bba494807..1a2aceda3dbe3 100644
--- a/drivers/net/ethernet/ti/cpsw_ale.c
+++ b/drivers/net/ethernet/ti/cpsw_ale.c
@@ -249,7 +249,7 @@ DEFINE_ALE_FIELD_SET(mcast_state,	62,	2)
 DEFINE_ALE_FIELD1(port_mask,		66)
 DEFINE_ALE_FIELD(super,			65,	1)
 DEFINE_ALE_FIELD(ucast_type,		62,     2)
-DEFINE_ALE_FIELD1_SET(port_num,		66)
+DEFINE_ALE_FIELD1(port_num,		66)
 DEFINE_ALE_FIELD_SET(blocked,		65,     1)
 DEFINE_ALE_FIELD_SET(secure,		64,     1)
 DEFINE_ALE_FIELD_GET(mcast,		40,	1)
@@ -441,6 +441,40 @@ static int cpsw_ale_find_ageable(struct cpsw_ale *ale)
 	return -ENOENT;
 }
 
+int cpsw_ale_del_ucast_dynamic_by_port(struct cpsw_ale *ale, const u8 *addr,
+				       int port)
+{
+	u32 ale_entry[ALE_ENTRY_WORDS];
+	int type, ucast_type, idx;
+	u8 entry_addr[6];
+	int deleted = 0;
+
+	for (idx = 0; idx < ale->params.ale_entries; idx++) {
+		cpsw_ale_read(ale, idx, ale_entry);
+		type = cpsw_ale_get_entry_type(ale_entry);
+		if (type != ALE_TYPE_ADDR && type != ALE_TYPE_VLAN_ADDR)
+			continue;
+		if (cpsw_ale_get_mcast(ale_entry))
+			continue;
+		ucast_type = cpsw_ale_get_ucast_type(ale_entry);
+		if (ucast_type == ALE_UCAST_PERSISTANT ||
+		    ucast_type == ALE_UCAST_OUI)
+			continue;
+		if (cpsw_ale_get_port_num(ale_entry, ale->port_num_bits) != port)
+			continue;
+		cpsw_ale_get_addr(ale_entry, entry_addr);
+		if (!ether_addr_equal(entry_addr, addr))
+			continue;
+
+		memset(ale_entry, 0, sizeof(ale_entry));
+		cpsw_ale_set_entry_type(ale_entry, ALE_TYPE_FREE);
+		cpsw_ale_write(ale, idx, ale_entry);
+		deleted++;
+	}
+
+	return deleted ? 0 : -ENOENT;
+}
+
 static void cpsw_ale_flush_mcast(struct cpsw_ale *ale, u32 *ale_entry,
 				 int port_mask)
 {
diff --git a/drivers/net/ethernet/ti/cpsw_ale.h b/drivers/net/ethernet/ti/cpsw_ale.h
index 87b7d1b3a34a9..69b004cbb1c76 100644
--- a/drivers/net/ethernet/ti/cpsw_ale.h
+++ b/drivers/net/ethernet/ti/cpsw_ale.h
@@ -166,6 +166,8 @@ int cpsw_ale_add_ucast(struct cpsw_ale *ale, const u8 *addr, int port,
 		       int flags, u16 vid);
 int cpsw_ale_del_ucast(struct cpsw_ale *ale, const u8 *addr, int port,
 		       int flags, u16 vid);
+int cpsw_ale_del_ucast_dynamic_by_port(struct cpsw_ale *ale, const u8 *addr,
+				       int port);
 int cpsw_ale_add_mcast(struct cpsw_ale *ale, const u8 *addr, int port_mask,
 		       int flags, u16 vid, int mcast_state);
 int cpsw_ale_del_mcast(struct cpsw_ale *ale, const u8 *addr, int port_mask,

base-commit: c9151088f1674fd29ff26a20f5fc687acf53a2f0
-- 
2.34.1

Re: [PATCH net] net: ti: am65-cpsw-switchdev: flush dynamic FDB entries by port on delete

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 (1) · Low: 0 (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] New cross-module ALE helper
  `cpsw_ale_del_ucast_dynamic_by_port()` is added to…
- [Medium] In am65_cpsw_switchdev_event_work() the `!fdb->added_by_user`
  gate is dropped for SWITCHDEV_FDB_DEL_TO_DEVICE, but only the `vid ==…
- [Medium] Dropping the `!fdb->added_by_user` gate makes every hardware-
  learned (dynamic) bridge FDB delete run an unbounded full ALE table…

Pre-existing issues:
- [Medium] cpsw_ale_del_ucast_dynamic_by_port() performs a full-table
  read-modify-write walk over the ALE indirect access window…
- [Low] The identical stale-dynamic-FDB defect remains in the sibling
  legacy switchdev driver drivers/net/ethernet/ti/cpsw_switchdev.c…

--

net: ti: am65-cpsw-switchdev: flush dynamic FDB entries by port on delete

The patch drops the added_by_user gate in the SWITCHDEV_FDB_DEL_TO_DEVICE
arm of am65_cpsw_switchdev_event_work() so dynamic FDB deletes reach the
ALE, and adds cpsw_ale_del_ucast_dynamic_by_port() to clear dynamic rows
for a MAC on a port regardless of vid when the bridge reports vid=0.

> diff --git a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
> index 53cdac272b58..8b4640809cf5 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
> @@ -397,13 +397,23 @@ static void am65_cpsw_switchdev_event_work(struct work_struct *work)
>  			   fdb->addr, fdb->vid, fdb->added_by_user,
>  			   fdb->offloaded, port_id);
>  
> -		if (!fdb->added_by_user || fdb->is_local)
> +		if (fdb->is_local)
>  			break;
>  		if (memcmp(port->slave.mac_addr, (u8 *)fdb->addr, ETH_ALEN) == 0)
>  			port_id = HOST_PORT_NUM;
>  
> -		cpsw_ale_del_ucast(cpsw->ale, (u8 *)fdb->addr, port_id,
> -				   fdb->vid ? ALE_VLAN : 0, fdb->vid);
> +		if (!fdb->added_by_user && !fdb->vid)
> +			/* vid=0 here just means "bridge doesn't know the
> +			 * real vid" (vlan_filtering=0) -- the dynamic entry
> +			 * may be stored under any nonzero vid, or several.
> +			 * Delete by (addr, port) instead of guessing a vid.
> +			 */
> +			cpsw_ale_del_ucast_dynamic_by_port(cpsw->ale,
> +							   (u8 *)fdb->addr,
> +							   port_id);
> +		else
> +			cpsw_ale_del_ucast(cpsw->ale, (u8 *)fdb->addr, port_id,
> +					   fdb->vid ? ALE_VLAN : 0, fdb->vid);

[Severity: Medium]
What happens to dynamic deletes that carry a nonzero vid (bridge with
vlan_filtering=1)?  They now land in the else arm, and
cpsw_ale_del_ucast() resolves the row through cpsw_ale_match_addr(),
which compares only entry type, vlan_id and MAC:

drivers/net/ethernet/ti/cpsw_ale.c:cpsw_ale_match_addr() {
	...
		if (cpsw_ale_get_vlan_id(ale_entry) != vid)
			continue;
		cpsw_ale_get_addr(ale_entry, entry_addr);
		if (ether_addr_equal(entry_addr, addr))
			return idx;
	...
}

The port argument of cpsw_ale_del_ucast() is never used, and no
ucast_type filtering happens, so the row that gets blanked may belong to
a different port or be an ALE_UCAST_PERSISTANT row.

The MAC roaming case looks reachable: br_fdb_update() emits a dynamic
RTM_DELNEIGH for the old port while the ALE has already re-learned the
MAC on the new port:

net/bridge/br_fdb.c:br_fdb_update() {
	...
			if (unlikely(source != READ_ONCE(fdb->dst) &&
				     !test_bit(BR_FDB_STICKY, &fdb->flags))) {
				br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH);
				WRITE_ONCE(fdb->dst, source);
	...
}

The ADD arm still keeps its added_by_user gate, so nothing reprograms the
row afterwards.  Before this patch the gate made that path unreachable
for dynamic entries.

Should the vid != 0 dynamic case also go through a port-aware and
ucast_type-aware delete, so both arms of the new conditional agree on
which rows a dynamic delete may clear?

[Severity: Medium]
With the added_by_user gate gone, every hardware-learned delete now runs
a full ALE table walk in cpsw_ale_del_ucast_dynamic_by_port() while the
rtnl_lock() taken at the top of am65_cpsw_switchdev_event_work() is held.
The helper has no early exit, since it deliberately clears rows under all
vids, so each event costs ale_entries times six MMIO accesses (64 entries
on cpsw2g, 512 on am64-cpswxg, more on j721e-cpswxg).

The event rate is driven by network traffic: learning, ageing, flush and
especially roaming, where one RTM_DELNEIGH is emitted per roam.  An
off-box host can keep this going by sourcing frames with the MAC of a
host known on another bridge port.

Is the added rtnl hold time and MMIO load per untrusted FDB event
acceptable here, or would an early exit / bounded search be preferable?

[Severity: Low]
This isn't a bug introduced by this patch, but the legacy sibling driver
drivers/net/ethernet/ti/cpsw_switchdev.c still has the exact code this
patch replaces in its SWITCHDEV_FDB_DEL_TO_DEVICE arm:

drivers/net/ethernet/ti/cpsw_switchdev.c:cpsw_switchdev_event_work() {
	...
		if (!fdb->added_by_user || fdb->is_local)
			break;
	...
		cpsw_ale_del_ucast(cpsw->ale, (u8 *)fdb->addr, port,
				   fdb->vid ? ALE_VLAN : 0, fdb->vid);
	...
}

cpsw_new.c also sets ALE_VLAN_AWARE unconditionally in switch mode, so
both premises stated in the changelog seem to hold there as well.  Since
the new helper is declared in the shared cpsw_ale.h consumed by both
drivers, was leaving cpsw_switchdev.c unconverted intentional?  A note in
the changelog about the scope would help.

>  		break;
>  	default:
>  		break;
> diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
> index e202bba49480..1a2aceda3dbe 100644
> --- a/drivers/net/ethernet/ti/cpsw_ale.c
> +++ b/drivers/net/ethernet/ti/cpsw_ale.c

[ ... ]

> @@ -441,6 +441,40 @@ static int cpsw_ale_find_ageable(struct cpsw_ale *ale)
>  	return -ENOENT;
>  }
>  
> +int cpsw_ale_del_ucast_dynamic_by_port(struct cpsw_ale *ale, const u8 *addr,
> +				       int port)
> +{
> +	u32 ale_entry[ALE_ENTRY_WORDS];
> +	int type, ucast_type, idx;
> +	u8 entry_addr[6];
> +	int deleted = 0;
> +
> +	for (idx = 0; idx < ale->params.ale_entries; idx++) {
> +		cpsw_ale_read(ale, idx, ale_entry);

[Severity: Medium]
This is a pre-existing property of cpsw_ale.c rather than something the
patch introduces, but the new helper adds one more unlocked
read-modify-write walk over the shared ALE indirect access window:

drivers/net/ethernet/ti/cpsw_ale.c:cpsw_ale_read() {
	writel_relaxed(idx, ale->params.ale_regs + ALE_TABLE_CONTROL);
	for (i = 0; i < ALE_ENTRY_WORDS; i++)
		ale_entry[i] = readl_relaxed(ale->params.ale_regs +
					     ALE_TABLE + 4 * i);
	...
}

There are no locks anywhere in cpsw_ale.c, and other table transactions
such as am65_cpsw_nuss_ndo_slave_set_rx_mode() ->
cpsw_ale_flush_multicast() run under netif_addr_lock_bh() rather than the
rtnl_lock held by the switchdev worker.  Should anything ever interleave,
one transaction's staged words can be committed under the other's index,
and cpsw_ale_flush_multicast() writes back every ADDR/VLAN_ADDR row it
reads, which could restore a row this helper just cleared.

Is there an assumption documented somewhere that all ALE table accessors
run under rtnl?

> +		type = cpsw_ale_get_entry_type(ale_entry);
> +		if (type != ALE_TYPE_ADDR && type != ALE_TYPE_VLAN_ADDR)
> +			continue;
> +		if (cpsw_ale_get_mcast(ale_entry))
> +			continue;
> +		ucast_type = cpsw_ale_get_ucast_type(ale_entry);
> +		if (ucast_type == ALE_UCAST_PERSISTANT ||
> +		    ucast_type == ALE_UCAST_OUI)
> +			continue;
> +		if (cpsw_ale_get_port_num(ale_entry, ale->port_num_bits) != port)
> +			continue;
> +		cpsw_ale_get_addr(ale_entry, entry_addr);
> +		if (!ether_addr_equal(entry_addr, addr))
> +			continue;
> +
> +		memset(ale_entry, 0, sizeof(ale_entry));
> +		cpsw_ale_set_entry_type(ale_entry, ALE_TYPE_FREE);
> +		cpsw_ale_write(ale, idx, ale_entry);
> +		deleted++;
> +	}
> +
> +	return deleted ? 0 : -ENOENT;
> +}

[Severity: High]
Is an EXPORT_SYMBOL_GPL() missing here?  The neighbouring ALE entry
points used across module boundaries all have one:

drivers/net/ethernet/ti/cpsw_ale.c
	EXPORT_SYMBOL_GPL(cpsw_ale_add_ucast);
	...
	EXPORT_SYMBOL_GPL(cpsw_ale_del_ucast);

The only caller, am65_cpsw_switchdev_event_work() in
am65-cpsw-switchdev.c, is linked into a different module than
cpsw_ale.o:

drivers/net/ethernet/ti/Makefile
	ti-cpsw-ale-y += cpsw_ale.o
	...
	obj-$(CONFIG_TI_K3_AM65_CPSW_NUSS) += ti-am65-cpsw-nuss.o ti-cpsw-sl.o ti-cpsw-ale.o
	ti-am65-cpsw-nuss-y := am65-cpsw-nuss.o am65-cpsw-ethtool.o
	ti-am65-cpsw-nuss-$(CONFIG_TI_K3_AM65_CPSW_SWITCHDEV) += am65-cpsw-switchdev.o

With CONFIG_TI_K3_AM65_CPSW_NUSS=m and
CONFIG_TI_K3_AM65_CPSW_SWITCHDEV=y, does modpost report
"cpsw_ale_del_ucast_dynamic_by_port" [drivers/net/ethernet/ti/ti-am65-cpsw-nuss.ko]
undefined?  Built-in configurations would hide this.

>  
>  static void cpsw_ale_flush_mcast(struct cpsw_ale *ale, u32 *ale_entry,
>  				 int port_mask)
>  {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918075926.3616434-1-danishanwar%40ti.com