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

[PATCH net v2] 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.

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. To solve this use a new
cpsw_ale_del_ucast_dynamic_by_port() that matches by (addr, port). If vid
is 0, it clears every dynamic row for that MAC on that port regardless of
vid. If vid!=0 it only clears the entry matching both MAC and vid on
that port.

Handling of the user added entries remains unaffected as the changes
made are only for dynamic learned entries.

Fixes: 86e8b070b25e ("net: ti: am65-cpsw-nuss: Add switchdev support")
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
v2 - v1:
Address comments recieved from Sashiko [1]

Sashiko had 1 High and 2 Medium comments on v1. 1 High and 1 medium is addressed
in this patch. 1 Medium is a acceptable behaviour and not an actual issue.
Sashiko also had 1 Low and 1 Medium pre-existing issues. Those two
pre-existing issues are still there and can be planned to fix later but
not as part of this patch.

Addressed comments
1) Dynamic delete with non zero vid was going through cpsw_ale_del_ucast()
doesn't use port based matching 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. This is fixed by calling
cpsw_ale_del_ucast_dynamic_by_port() for all dynamic entries. vid handling
is taken care by this API.

2) Added EXPORT_SYMBOL_GPL() for cpsw_ale_del_ucast_dynamic_by_port()

3) There was a comment about cost associated with full ALE walk for each
dynamic delete. The ALE size is small (max 512 entries on the largest
supported device), so no change is done here.

[1] https://lore.kernel.org/all/179006497666.2160803.14768308117153644313@kernel.org/
v1 https://lore.kernel.org/all/20260918075926.3616434-1-danishanwar@ti.com/

 drivers/net/ethernet/ti/am65-cpsw-switchdev.c | 12 +++--
 drivers/net/ethernet/ti/cpsw_ale.c            | 44 ++++++++++++++++++-
 drivers/net/ethernet/ti/cpsw_ale.h            |  2 +
 3 files changed, 54 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..0dc681748b0e3 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;
 		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);
+		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..86bcea5744e01 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,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);
+		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;
+		entry_vid = cpsw_ale_get_vlan_id(ale_entry);
+		if (vid && entry_vid != vid)
+			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++;
+
+		if (vid)
+			return 0;
+	}
+
+	return deleted ? 0 : -ENOENT;
+}
+EXPORT_SYMBOL_GPL(cpsw_ale_del_ucast_dynamic_by_port);
+
 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..3c48e4da18202 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, u16 vid);
 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 v2] 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 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.

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.

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?

>  		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?

> +		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?

> 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?

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

[ ... ]

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