Re: [PATCH net] net: dsa: motorcomm: Do not inherit ports from dynamic FDB entries

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] This patch replaces the egress port mask in yt921x_fdb_join()
  (drivers/net/dsa/motorcomm/chip.c) only when the existing hardware…

--

net: dsa: motorcomm: Do not inherit ports from dynamic FDB entries

This patch changes yt921x_fdb_join() so that when the existing hardware
FDB entry is not static, its egress port mask is replaced instead of
merged. The goal is to avoid converting hardware-learned ports into a
static entry that DSA does not track and can never delete.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 33e5fc127244d..150c51f1aa358 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -799,7 +799,10 @@ yt921x_fdb_join(struct yt921x_priv *priv, const unsigned char *addr, u16 vid,
>  
>  	ctrl1 = val1 & ~YT921X_FDB_IO1_STATUS_M;
>  	ctrl1 |= YT921X_FDB_IO1_STATUS_STATIC;
> -	ctrl2 = val2 | YT921X_FDB_IO2_EGR_PORTS(ports_mask);
> +	ctrl2 = val2;
> +	if (ctrl1 != val1)
> +		ctrl2 &= ~YT921X_FDB_IO2_EGR_PORTS_M;
> +	ctrl2 |= YT921X_FDB_IO2_EGR_PORTS(ports_mask);

[Severity: High]
If the existing entry is already static, this path still ORs the new port
into the mask, and it does so for unicast addresses too. Can a static
unicast entry still be "silently expanded and can never be deleted"
when it moves between ports?

One sequence that seems to reach this:

  bridge fdb add <mac> dev swp0 master static
  bridge fdb replace <mac> dev swp1 master static

In net/bridge/br_fdb.c:fdb_add_entry(), fdb_update_dst() sees that the
port changed and returns true. The bridge then calls only
fdb_notify(br, fdb, RTM_NEWNEIGH, true). As a result,
br_switchdev_fdb_notify() sends SWITCHDEV_FDB_ADD_TO_DEVICE for swp1
and sends no delete for swp0.

For user ports, dsa_port_do_fdb_add() in net/dsa/switch.c calls
ds->ops->port_fdb_add() directly. That ends up in
yt921x_fdb_join(priv, mac, vid, BIT(swp1)).

The hardware entry is already STATIC there, so ctrl1 == val1. The new
clear of YT921X_FDB_IO2_EGR_PORTS_M is skipped, and EGR_PORTS becomes
swp0 | swp1. Unicast frames to <mac> would then go out both ports, while
the bridge thinks the address is only on swp1.

A later delete on swp1 goes through yt921x_fdb_leave(), which clears only
the departing port's bit:

	ctrl2 = val2 & ~YT921X_FDB_IO2_EGR_PORTS(ports_mask);
	if (ctrl2 == val2)
		return 0;
	if (!(ctrl2 & YT921X_FDB_IO2_EGR_PORTS_M)) {

That leaves a static entry pointing at swp0 in hardware. The driver only
flushes with flush_static=false, so nothing would remove that entry
later.

A host FDB entry on the CPU port, installed with
assisted_learning_on_cpu_port, looks like it can reach the same path
when a replace moves the address to a user port.

For comparison, mv88e6xxx_port_db_load_purge() replaces the port vector
for static unicast and merges only otherwise:

	if (state == MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC)
		entry.portvec = BIT(port);
	else
		entry.portvec |= BIT(port);

Hardware-learned entries can only be unicast, because source MACs are
never multicast. Would it work to decide based on the address instead,
for example by replacing the mask when !is_multicast_ether_addr(addr)
and merging only for MDB entries? That would cover both the dynamic
case this patch targets and the static port-move case above.

>  	if (ctrl1 == val1 && ctrl2 == val2)
>  		return 0;
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109153407.1761032-1-mmyangfl%40gmail.com