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

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

On 28/09/26 11:20 am, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (1) · Medium: 2 (1) · 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 patch removes the `!fdb->added_by_user` gate from the
>    SWITCHDEV_FDB_DEL_TO_DEVICE branch of am65_cpsw_switchdev_event_work().
> - [Medium] Before this patch, dynamic DEL_TO_DEVICE events hit the
>    `!fdb->added_by_user` break and never touched hardware.
> 
> Pre-existing issues:
> - [High] cpsw_ale has no internal lock (no spinlock, mutex or lockdep
>    assert in cpsw_ale.c).
> - [Medium] This problem wasn't introduced by this patch, but the older
>    CPSW switchdev driver has the same flush defect.
> 
> --
> 
> net: ti: am65-cpsw-switchdev: flush dynamic FDB entries by port on delete
> 
> This patch drops the added_by_user gate on SWITCHDEV_FDB_DEL_TO_DEVICE in
> am65-cpsw-switchdev, so dynamic FDB deletes now reach the hardware. It adds
> cpsw_ale_del_ucast_dynamic_by_port(), which walks the ALE and frees
> non-persistent unicast rows that match (addr, port). When vid is 0 it frees
> matching rows in every vid.
> 
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
>> index 53cdac272b58..0dc681748b0e 100644
>> --- a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
>> +++ b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
>> @@ -397,13 +397,19 @@ 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;
> 
> [Severity: Medium]
> 
> With the added_by_user gate removed, does routine bridge software ageing
> now remove live ALE rows as well?
> 
> br_fdb_cleanup() ages out dynamic entries like this:
> 
> br_fdb_cleanup()
>    fdb_delete(br, f, true)
>      fdb_notify(br, f, RTM_DELNEIGH, true)
>        br_switchdev_fdb_notify()
>          -> SWITCHDEV_FDB_DEL_TO_DEVICE, added_by_user = 0
> 
> This driver never reports hardware-learned entries to the bridge (there is
> no SWITCHDEV_FDB_ADD_TO_BRIDGE). Also, am65_cpsw_port_attr_set() does not
> handle SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME. As a result, the bridge only
> refreshes an entry when a frame from that station reaches the CPU.
> 

CPSW driver doesn't support any mechanism to indicate that an entry was 
learned by ALE so SWITCHDEV_FDB_ADD_TO_BRIDGE can't be implemented.

> Take a station whose unicast traffic is switched only in hardware. Its
> bridge entry expires after the default 300 seconds, while traffic keeps
> refreshing its ALE row. The ALE ages out untouched rows after about 30
> seconds (AM65_CPSW_ALE_AGEOUT_DEFAULT with CPSW_ALE_F_HW_AUTOAGING), so any
> row still present at that point is in use.
> 
> cpsw_ale_del_ucast_dynamic_by_port() does not check touched/untouched
> state. With vid == 0 it frees every matching row for that MAC on the port.
> 
> Would traffic to that station then be flooded as unknown unicast until the
> station transmits again? That would happen once per bridge ageing interval,
> with a full ALE walk each time.

Yes, for stations whose traffic never reaches the CPU (pure hardware 
forwarding). The flush fix is the primary goal of this patch; the ageing 
side-effect is a trade-off.

> 
> The commit message only describes this as a flush fix. Should it mention
> the change in ageing behaviour, or should the ageing path be filtered out?
> 

I will update the commit message to mention this change in ageing 
behavior. No code change regarding this in v3.

>>   		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)
>> +			cpsw_ale_del_ucast_dynamic_by_port(cpsw->ale,
>> +							   (u8 *)fdb->addr,
>> +							   port_id,
>> +							   fdb->vid);
> 
> [Severity: Medium]
> 
> Before this patch, dynamic DEL_TO_DEVICE events hit the break before any
> hardware access. Now each one does a full ALE MMIO walk. Does this path
> need a runtime PM reference?
> 
> On port-down the sequence looks like this:
> 
> am65_cpsw_nuss_ndo_slave_stop()
>    am65_cpsw_nuss_common_stop()
>      cpsw_ale_stop()
>    pm_runtime_put(common->dev)
> 
> NETDEV_DOWN then triggers:
> 
> br_stp_disable_port()
>    br_fdb_delete_by_port()
>      -> one SWITCHDEV_FDB_DEL_TO_DEVICE per dynamic entry, added_by_user = 0
> 
> am65_cpsw_switchdev_event_work()
>    cpsw_ale_del_ucast_dynamic_by_port()
>      cpsw_ale_read()   /* writel_relaxed/readl_relaxed of ALE regs */
> 
> When the last port goes down, these work items access the ALE registers
> after the last runtime PM reference has been dropped. Other ALE users in
> am65-cpsw-nuss.c take pm_runtime_resume_and_get() first, for example the
> VLAN add/kill and set_mac_address paths.
> 
> With CONFIG_TI_K3_AM65_CPTS disabled, am65_cpsw_init_cpts() returns before
> it calls pm_runtime_forbid(), so the device can runtime suspend. The comment
> there notes that K3 CPSWxG modules may lose context on ON->OFF transitions.
> 
> Could this access a suspended module?
> 

I will add runtime PM reference in v3.

>> +		else
>> +			cpsw_ale_del_ucast(cpsw->ale, (u8 *)fdb->addr, port_id,
>> +					   fdb->vid ? ALE_VLAN : 0, fdb->vid);
>>   		break;
> 
> [Severity: Medium]
> 
> This is a pre-existing issue that was not introduced by this patch. The
> older CPSW switchdev driver seems to have the same flush problem.
> cpsw_switchdev_event_work() in drivers/net/ethernet/ti/cpsw_switchdev.c
> still has:
> 
> 	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_init_host_port() in cpsw_new.c enables ALE_VLAN_AWARE, so the nonzero
> vid mismatch described in the commit message applies there too.
> cpsw_switchdev_event() queues DEL_TO_DEVICE work without any filtering.
> 
> Should cpsw_switchdev.c also use the new
> cpsw_ale_del_ucast_dynamic_by_port() helper, or is that planned as a
> separate fix?
> 

Sure, I will extend the fix to cpsw_switchdev.c driver as well. This 
will be fixed in v3.

>> diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
>> index e202bba49480..86bcea5744e0 100644
>> --- a/drivers/net/ethernet/ti/cpsw_ale.c
>> +++ b/drivers/net/ethernet/ti/cpsw_ale.c
> 
> [ ... ]
> 
>> @@ -441,6 +441,48 @@ 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, u16 vid)
>> +{
>> +	u32 ale_entry[ALE_ENTRY_WORDS];
>> +	int type, ucast_type, idx;
>> +	u8 entry_addr[6];
>> +	int deleted = 0;
>> +	int entry_vid;
>> +
>> +	for (idx = 0; idx < ale->params.ale_entries; idx++) {
>> +		cpsw_ale_read(ale, idx, ale_entry);
> 
> [ ... ]
> 
>> +		memset(ale_entry, 0, sizeof(ale_entry));
>> +		cpsw_ale_set_entry_type(ale_entry, ALE_TYPE_FREE);
>> +		cpsw_ale_write(ale, idx, ale_entry);
> 
> [Severity: High]
> 
> This is a pre-existing issue: the existing switchdev ucast add/del and VLAN
> ops can already race here. This patch adds a new full-table read/write
> walker and runs it much more often.
> 
> ALE table accesses go through a shared indirect window: the index goes into
> ALE_TABLE_CONTROL, then the ALE_TABLE words are read or written.
> cpsw_ale.c has no internal lock. The only thing serializing this loop is
> rtnl_lock() taken by the switchdev work.
> 
> am65_cpsw_nuss_ndo_slave_set_rx_mode() is called on the legacy
> ndo_set_rx_mode path in __dev_set_rx_mode(). It runs under
> netif_addr_lock_bh() without rtnl. It calls cpsw_ale_flush_multicast(),
> which reads every ALE_TYPE_ADDR/ALE_TYPE_VLAN_ADDR row, including unicast
> rows, and always writes it back:
> 
> 	for (idx = 0; idx < ale->params.ale_entries; idx++) {
> 		cpsw_ale_read(ale, idx, ale_entry);
> 		...
> 		cpsw_ale_write(ale, idx, ale_entry);
> 	}
> 
> An unprivileged user can reach that path without rtnl:
> 
> setsockopt(IPV6_ADD_MEMBERSHIP)
>    __ipv6_sock_mc_join()
>      __ipv6_dev_mc_inc()
>        igmp6_group_added()
>          dev_mc_add()
>            __dev_mc_add()
>              __dev_set_rx_mode()
>                am65_cpsw_nuss_ndo_slave_set_rx_mode()
>                  cpsw_ale_flush_multicast()
> 
> Suppose cpsw_ale_flush_multicast() reads row N, this function then frees
> row N, and cpsw_ale_flush_multicast() writes its stale copy back. Doesn't
> the deleted dynamic entry come back?
> 
> The ALE_TABLE_CONTROL writes from the two paths could also interleave.
> Could one path then read another row's contents, or write its words to the
> wrong index? That would free or overwrite an unrelated VLAN, persistent or
> multicast row.
> 
> Should ALE table accesses be serialized by a lock inside cpsw_ale?
> 

This is a valid pre-existing race. Adding a mutex to cpsw_ale would be a 
separate cleanup. The current patch scope is limited to the flush fix 
and pm_runtime. The ALE locking can be addressed in a follow-up patch 
since the race exists independently of this change.

This will not be fixed in v3.

pw-bot: cr

>> +		deleted++;
>> +
>> +		if (vid)
>> +			return 0;
>> +	}
>> +
>> +	return deleted ? 0 : -ENOENT;
>> +}
>> +EXPORT_SYMBOL_GPL(cpsw_ale_del_ucast_dynamic_by_port);
> 
> [ ... ]
>