Re: [PATCH net-next v2 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: 1 · Low: 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).

- [Medium] Bit 14 of YT921X_FDB_RESULT is defined in chip.h:284 as
  YT921X_FDB_RESULT_NOTFOUND.
- [Low] This patch fixes a real silent-failure bug but has no Fixes: tag.

--

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

This patch makes yt921x_fdb_add() check the result word after the hardware
OP_ADD operation. It now returns -ENOSPC when the result reports a failure,
instead of always returning success. The commit message describes this as
preparation for the upcoming static entry counters.

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

[Severity: Medium]
Is YT921X_FDB_RESULT_NOTFOUND the right bit to treat as a capacity failure
after an OP_ADD?

Everywhere else in the driver this bit means a lookup miss. For example,
yt921x_fdb_has() checks it after a hash-mode GET_ONE:

drivers/net/dsa/motorcomm/chip.c:yt921x_fdb_has() {
    ...
	res = yt921x_fdb_wait(priv, &val);
	if (res)
		return res;
	if (val & YT921X_FDB_RESULT_NOTFOUND) {
		*indexp = YT921X_FDB_NUM;
		return 0;
	}
    ...
}

The only caller of yt921x_fdb_add() is yt921x_fdb_join(), and it calls it
only after yt921x_fdb_has() has reported the key absent. In that case a
"not found" flag would naturally be set.

Suppose the hardware sets this bit on ADD to say the key was not present
before the insert. Wouldn't every new static FDB/MDB entry then fail with
-ENOSPC? That would include the host addresses DSA installs on the CPU
port:

yt921x_dsa_port_fdb_add() / yt921x_dsa_port_mdb_add()
  -> yt921x_fdb_join()
    -> yt921x_fdb_has()   /* key absent */
    -> yt921x_fdb_add()   /* OP_ADD, NOTFOUND set -> -ENOSPC */

If the bit instead means a generic lookup or insert failure (for example,
no free slot in the hash bucket), could a comment or an alias macro be
added so the ADD-time meaning is written down?

YT921X_FDB_RESULT_OVERWRITED is also defined, but nothing reads it.

> +
> +	return 0;
>  }
>  
>  static int

[Severity: Low]
Should this patch carry a Fixes: tag?

Before this patch, yt921x_fdb_add() returned 0 even when the hardware
rejected a hash-mode OP_ADD. DSA and the bridge then counted the static FDB
or MDB entry as offloaded, although the switch had never programmed it.

The ignored result seems to come from the original driver commit:

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

The commit message says:

  In practice the 4096-entry table is hardly ever filled

The ADD is hash keyed and not index based (no YT921X_FDB_OP_MODE_INDEX).
Could the hardware reject it once a single hash bucket is full, well before
all 4096 entries are used?

If the patch is not meant for stable, could the commit message say why?

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