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

[MD Danish Anwar <danishanwar@ti.com>]

On 22/09/26 1:46 pm, netdev-bot+sashiko@kernel.org 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)
>>   {
> 
> [ ... ]
>