Re: [PATCH net] net: ti: am65-cpsw-switchdev: flush dynamic FDB entries by port on delete
From: MD Danish Anwar
Date: Wed Sep 23 2026 - 04:35:33 EST
On 22/09/26 1:46 pm, netdev-bot+sashiko@xxxxxxxxxx wrote:
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?
This is a valid issue and I will fix this in v2. The fix would be to pass vid also to cpsw_ale_del_ucast_dynamic_by_port and if vid=0 delete all entries of a mac. if vid!=0 only delete the entry matching the vid.
The else path would remain unchanged as it will be only hit for user added entries.
if (!fdb->added_by_user)
cpsw_ale_del_ucast_dynamic_by_port(cpsw>ale, (u8 *)fdb->addr, port_id, fdb->vid);
else
cpsw_ale_del_ucast(cpsw->ale, (u8 *)fdb->addr, port_id,
fdb->vid ? ALE_VLAN : 0, fdb->vid);
[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?
The cost associated with full ALE walk is accepetable here. The ALE size is small (max 512 entries on the largest supported device) and the event rate driven by normal bridge learning/ageing/roaming is modest, so the added MMIO load and lock hold time are acceptable.
This is acceptable and doesn't need to be addressed in v2.
[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.
This is intentional and unrelated to this patch. Can be added later if needed.
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.
This will be fixed in v2.
static void cpsw_ale_flush_mcast(struct cpsw_ale *ale, u32 *ale_entry,
int port_mask)
{
[ ... ]