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

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

On 23/09/26 1:57 pm, MD Danish Anwar wrote:
> 
> 
> 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)
>>>   {
>>
>> [ ... ]
>>
> 

pw-bot: cr