Re: [PATCH net-next v3 1/2] net: dsa: motorcomm: Report error when FDB fails to add

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · 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 code change is right, but the commit message gives a wrong
  reason for leaving out the Fixes: tag.
- [Medium] yt921x_fdb_add() in drivers/net/dsa/motorcomm/chip.c now
  treats only YT921X_FDB_RESULT_NOTFOUND (bit 14) as a failure of the…

--

net: dsa: motorcomm: Report error when FDB fails to add

This patch changes yt921x_fdb_add() to check the FDB result register
after the ADD operation finishes. If the hardware reports
YT921X_FDB_RESULT_NOTFOUND, the function now returns -ENOSPC instead of
success.

> Not carrying a fix tag: filling up 4096-entry table with static entries
> is just asking for trouble.

[Severity: Medium]
Is this reasoning accurate? Before this patch, a hardware ADD failure was
reported as success. DSA and the bridge then treated the entry as
offloaded even though the switch never installed it. That looks like it
dates back to 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm
YT921x").

Admin-added static FDB entries are not the only way to fill the table.
yt921x_dsa_port_mdb_add() goes through the same path, and every entry it
writes is YT921X_FDB_IO1_STATUS_STATIC:

yt921x_dsa_port_mdb_add()
    yt921x_fdb_join()
        yt921x_fdb_add()

Bridge IGMP/MLD snooping is enabled by default. It allows up to
BR_MULTICAST_DEFAULT_HASH_MAX MDB entries, and that limit is also 4096:

net/bridge/br_private.h:
#define BR_MULTICAST_DEFAULT_HASH_MAX 4096

Those MDB entries are offloaded via switchdev into port_mdb_add. So it
seems hosts on the LAN could fill the shared hardware table just by
sending group joins. The same table also holds unicast static FDB
entries and host entries.

Also, the ADD is issued without YT921X_FDB_OP_MODE_INDEX, so the
hardware places the entry by MAC+FID. If the table uses
limited-associativity hash buckets, could ADD fail before all 4096
entries are in use? This depends on the datasheet.

Should this carry:

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")

so that stable kernels pick it up? If not, could the commit message
describe the failure conditions more precisely, and mention that MDB
entries are affected as well as unicast FDB?

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 33e5fc127244d..a7a9a74393045 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -725,7 +725,13 @@ yt921x_fdb_add(struct yt921x_priv *priv, const unsigned char *addr, u16 vid,
>  	if (res)
>  		return res;
>  
> -	return yt921x_fdb_wait(priv, &val);
> +	res = yt921x_fdb_wait(priv, &val);
> +	if (res)
> +		return res;
> +	if (val & YT921X_FDB_RESULT_NOTFOUND)
> +		return -ENOSPC;
> +
> +	return 0;

[Severity: Medium]
What should happen when YT921X_FDB_RESULT_OVERWRITED is set here? The
result register defines it in chip.h next to NOTFOUND:

#define  YT921X_FDB_RESULT_NOTFOUND		BIT(14)
#define  YT921X_FDB_RESULT_OVERWRITED		BIT(13)

Nothing in the driver reads it.

yt921x_fdb_join() only calls yt921x_fdb_add() after yt921x_fdb_has() has
reported the {MAC, FID} key absent, with reg_lock held:

	res = yt921x_fdb_has(priv, addr, vid, &index);
	if (res)
		return res;
	if (index >= YT921X_FDB_NUM)
		return yt921x_fdb_add(priv, addr, vid, ports_mask);

So an overwrite here probably means some other entry in the same bucket
was replaced. The exception would be a dynamic entry for this key being
learned between the two operations.

If the hardware can evict an earlier static FDB or MDB entry this way,
this would return 0 while that entry silently disappears from the
switch. A later yt921x_fdb_leave() for it would then see NOTFOUND and
also return 0.

Could OVERWRITED mean a static entry was evicted? If it only ever means
a dynamic entry was replaced, a short comment saying why it is safe to
ignore might help.

>  }
>  
>  static int

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