Re: [PATCH net-next 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 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · 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] The commit message says the change "does not affect userspace
  tools: .port_fdb_add() is asynchronous and DSA core always returns 0…
- [Medium] The patch reads YT921X_FDB_RESULT_NOTFOUND (BIT(14),
  chip.h:284) after an OP_ADD as "table full" and returns -ENOSPC from…
- [Low] The patch fixes a real bug, but it has no Fixes: tag.

--

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

This patch makes yt921x_fdb_add() return -ENOSPC when the FDB result
register reports YT921X_FDB_RESULT_NOTFOUND after an add operation.
Before the patch, the hardware status was ignored. The commit message
says userspace is not affected because .port_fdb_add() is asynchronous.

> Note that this does not affect userspace tools: .port_fdb_add() is
> asynchronous and DSA core always returns 0 for that.

[Severity: Medium]
Is this accurate? yt921x_fdb_add() is also reached from the MDB path, and
DSA returns .port_mdb_add() errors synchronously:

yt921x_dsa_port_mdb_add()
  yt921x_fdb_join()
    yt921x_fdb_add()
      return -ENOSPC;

When a port joins a bridge, host-joined MDB groups are replayed as
SWITCHDEV_OBJ_ID_HOST_MDB. nbp_switchdev_sync_objs() then returns the
error unchanged:

net/bridge/br_switchdev.c:nbp_switchdev_sync_objs() {
    ...
	err = br_switchdev_mdb_replay(br_dev, dev, ctx, true, blocking_nb,
				      extack);
	if (err) {
		/* -EOPNOTSUPP not propagated from MDB replay. */
		return err;
	}
    ...
}

If the hardware table is full, will "ip link set lanX master br0" now
fail with -ENOSPC? Before this patch it succeeded.

For deferred MDB adds, br_switchdev_mdb_complete() now calls
br_multicast_set_pg_offload_flags(p, !err) with an error. Userspace then
gets an RTM_NEWMDB notification and sees offload_failed in
"bridge mdb show".

On the FDB path, dsa_user_switchdev_event_work() now logs the error and
skips dsa_fdb_offload_notify():

net/dsa/user.c:dsa_user_switchdev_event_work() {
    ...
		if (err) {
			dev_err(ds->dev,
				"port %d failed to add %pM vid %d to fdb: %d\n",
				dp->index, addr, vid, err);
			break;
		}
		dsa_fdb_offload_notify(switchdev_work);
    ...
}

As a result, "bridge fdb show" no longer marks these entries as
offloaded. The driver sets ds->assisted_learning_on_cpu_port = true, so
every learned station MAC takes this path. A full table would then log
one dev_err() per address, with no rate limit.

The new behavior looks more correct. Could the commit message describe
these changes that userspace can see, especially that bridge join can
now fail?

> Signed-off-by: David Yang <mmyangfl@gmail.com>

[Severity: Low]
Should this have a Fixes: tag?

Before this patch, a static FDB or MDB add that the hardware rejected
still returned 0. DSA then called dsa_fdb_offload_notify(), and the
bridge set MDB offload flags for entries the switch never installed.
Traffic to those addresses could be flooded or forwarded wrongly.

The ignored result goes back to the original driver submission:

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

Could this also go to net instead of being bundled with net-next feature
work?

> 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]
Does the hardware set YT921X_FDB_RESULT_NOTFOUND when an OP_ADD finds no
free slot?

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

	if (val & YT921X_FDB_RESULT_NOTFOUND) {
		*indexp = YT921X_FDB_NUM;
		return 0;
	}

The neighbouring bit is defined in chip.h but never checked:

#define  YT921X_FDB_RESULT_OVERWRITED		BIT(13)

If the hardware reports a full table some other way, this check would
miss the case the commit message describes. Static FDB and MDB entries
could then still be lost silently while being marked as offloaded.

If OVERWRITED means the ADD evicted another entry from a full bucket,
would that other entry also be lost silently?

Could the datasheet meaning of these bits for OP_ADD be confirmed?

Separately, yt921x_fdb_add_index_in12() still ignores every result bit.
It is used by the join and leave update paths. Should it check them too?

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

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