Re: [PATCH net-next v2 2/2] net: dsa: motorcomm: Add devlink info and resources

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 3

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 devlink resource tree registered in yt921x_devlink_setup()
  does not match how devlink resources work.
- [Medium] The FDB occupancy callbacks
  (yt921x_devlink_occ_get_fdb_ucast(),…
- [Low] The patch adds devlink info (DEVLINK_INFO_VERSION_GENERIC_ASIC_ID
  via yt921x_dsa_devlink_info_get()) and six devlink resources (ACLs,…
- [Low] YT921X_LEARN_UCAST_CNT (0x18043c) and YT921X_LEARN_MCAST_CNT
  (0x180504) are added to chip.h as bare addresses with no count-field…
- [Low] yt921x_fdb_add() issues a hash-mode FDB_OP_OP_ADD and checks only
  YT921X_FDB_RESULT_NOTFOUND.

--

net: dsa: motorcomm: Add devlink info and resources

This adds a devlink info callback that reports the ASIC name as
asic.id. It also registers devlink resources for ACLs, the FDB (with
ucast, mcast and static children) and meters. FDB occupancy combines the
hardware learned-entry counters with software counters of static entries,
which yt921x_fdb_add(), yt921x_fdb_join() and yt921x_fdb_leave() maintain.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index a7a9a74393045..95d57e74d64c2 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -731,6 +732,10 @@ yt921x_fdb_add(struct yt921x_priv *priv, const unsigned char *addr, u16 vid,
>  	if (val & YT921X_FDB_RESULT_NOTFOUND)
>  		return -ENOSPC;
>  
> +	if (is_multicast_ether_addr(addr))
> +		priv->fdb_static_mcast_cnt++;
> +	else
> +		priv->fdb_static_ucast_cnt++;
>  	return 0;
>  }

[Severity: Low]
Only YT921X_FDB_RESULT_NOTFOUND is checked before the static counters are
incremented. What happens if the hardware reports
YT921X_FDB_RESULT_OVERWRITED here?

yt921x_fdb_join() calls yt921x_fdb_add() only after yt921x_fdb_has() has
found no entry for this MAC+FID. An overwrite would therefore mean that
some other entry in the hash bucket was replaced. If that entry was
static, the switch has dropped it, but fdb_static_*_cnt still counts it.

A later port_fdb_del/port_mdb_del for the evicted address then takes the
early return in yt921x_fdb_leave():

	if (index >= YT921X_FDB_NUM)
		return 0;

so the counter stays wrong until the next setup.

Ignoring OVERWRITED is not new to this patch. The new counting, though,
assumes that every successful ADD adds exactly one static entry and
removes none. Can that be checked here, or does the hardware never evict
static entries on a hash ADD?

[ ... ]

> @@ -809,7 +831,18 @@ yt921x_fdb_join(struct yt921x_priv *priv, const unsigned char *addr, u16 vid,
>  	if (ctrl1 == val1 && ctrl2 == val2)
>  		return 0;
>  
> -	return yt921x_fdb_add_index_in12(priv, index, ctrl1, ctrl2);
> +	res = yt921x_fdb_add_index_in12(priv, index, ctrl1, ctrl2);
> +	if (res)
> +		return res;
> +
> +	if ((val1 & YT921X_FDB_IO1_STATUS_M) !=
> +	    YT921X_FDB_IO1_STATUS_STATIC) {
> +		if (is_multicast_ether_addr(addr))
> +			priv->fdb_static_mcast_cnt++;
> +		else
> +			priv->fdb_static_ucast_cnt++;
> +	}

[Severity: Medium]
If the existing slot was learned dynamically (for example, bridge fdb add
on a MAC that has already been learned), this rewrites it as STATIC with
an index-mode ADD and increments fdb_static_ucast_cnt. Does
YT921X_LEARN_UCAST_CNT go down when the slot is converted this way?

If it doesn't, yt921x_devlink_occ_get_fdb_ucast() and
yt921x_devlink_occ_get_fdb() count this entry twice. When
yt921x_fdb_leave() later deletes the entry, only the software counter is
decremented.

Multicast raises a similar question. The hardware doesn't learn
multicast source MACs, and the driver doesn't set up hardware IGMP/MLD
snooping. So YT921X_LEARN_MCAST_CNT is either always zero, or it counts
the MDB entries the CPU programs, which fdb_static_mcast_cnt also counts.

In either case, can the reported FDB occupancy exceed YT921X_FDB_NUM?
Is there anything that confirms these hardware counters leave out entries
added with YT921X_FDB_OP_OP_ADD?

[ ... ]

> diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h
> index fc315f1d8f87b..d3f2a86feb228 100644
> --- a/drivers/net/dsa/motorcomm/chip.h
> +++ b/drivers/net/dsa/motorcomm/chip.h
> @@ -248,6 +248,7 @@ enum yt921x_app_selector {

[ ... ]

> +#define YT921X_LEARN_UCAST_CNT		0x18043c
>  #define YT921X_AGEING			0x180440
>  #define  YT921X_AGEING_INTERVAL_M		GENMASK(15, 0)

[ ... ]

> @@ -312,6 +313,7 @@ enum yt921x_app_selector {

[ ... ]

> +#define YT921X_LEARN_MCAST_CNT		0x180504

[Severity: Low]
Both counter registers are defined without a field mask. The devlink
occ_get callbacks use the whole 32-bit value as an entry count, for
example in yt921x_devlink_occ_get_fdb():

	res = yt921x_reg_read(priv, YT921X_LEARN_UCAST_CNT, &val);
	if (res)
		goto end;
	count += val;

The table holds only YT921X_FDB_NUM (4096) entries. Are the other bits
of these registers guaranteed to read as zero?

The neighbouring registers define their fields with GENMASK(). Would a
count mask used with FIELD_GET() be safer here?

[ ... ]

> diff --git a/drivers/net/dsa/motorcomm/devlink.c b/drivers/net/dsa/motorcomm/devlink.c
> new file mode 100644
> index 0000000000000..a9a611ef55713
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/devlink.c
> @@ -0,0 +1,216 @@

[ ... ]

> +int
> +yt921x_dsa_devlink_info_get(struct dsa_switch *ds, struct devlink_info_req *req,
> +			    struct netlink_ext_ack *extack)
> +{

[ ... ]

> +	res = devlink_info_version_fixed_put(req,
> +					     DEVLINK_INFO_VERSION_GENERIC_ASIC_ID,
> +					     priv->info->name);

[Severity: Low]
This isn't a bug, but Documentation/networking/devlink/index.rst says:

  Each driver that implements devlink is expected to document what
  parameters, info versions, and other features it supports.

There is no yt921x file under Documentation/networking/devlink/. Nothing
documents the asic.id info version or the ACLs, FDBs, FDBs/ucast,
FDBs/mcast, FDBs/static and meters resources. Could one be added?

Two things are not obvious from the resource names. The static child
overlaps ucast/mcast. The ucast/mcast occupancy mixes hardware learned
counts with software static counts.

Among the existing DSA drivers that use devlink, only mv88e6xxx has such
a file. The documented expectation still applies to new features.

[ ... ]

> +static const struct yt921x_devlink_resource yt921x_devlink_resources[] = {

[ ... ]

> +	{
> +		"ucast", yt921x_devlink_occ_get_fdb_ucast,
> +		0,
> +		YT921X_DEVLINK_RESOURCE_ID_FDB_UCAST,
> +		YT921X_DEVLINK_RESOURCE_ID_FDB,
> +	},
> +	{
> +		"mcast", yt921x_devlink_occ_get_fdb_mcast,
> +		0,
> +		YT921X_DEVLINK_RESOURCE_ID_FDB_MCAST,
> +		YT921X_DEVLINK_RESOURCE_ID_FDB,
> +	},
> +	{
> +		"static", yt921x_devlink_occ_get_fdb_static,
> +		0,
> +		YT921X_DEVLINK_RESOURCE_ID_FDB_STATIC,
> +		YT921X_DEVLINK_RESOURCE_ID_FDB,
> +	},

[Severity: Medium]
These three children are registered with size 0, and
yt921x_devlink_setup() passes r->size as both size_min and size_max.
Their occ_get callbacks can still return non-zero counts, up to
YT921X_FDB_NUM. So devlink resource show would print something like
"ucast size 0 occ 1200". Is that how sub-resources are meant to be used?

The devlink core treats children as partitions of the parent's size.
devlink_resource_validate_children() adds up the children's size_new and
checks the total against the parent:

	list_for_each_entry(child_resource, &resource->resource_list, list)
		parts_size += child_resource->size_new;

The example in Documentation/networking/devlink/devlink-resource.rst
also shows children with real sizes that partition the parent.

The children also overlap. yt921x_devlink_occ_get_fdb_ucast() and
yt921x_devlink_occ_get_fdb_mcast() already add fdb_static_ucast_cnt and
fdb_static_mcast_cnt. yt921x_devlink_occ_get_fdb_static() then returns:

	count = priv->fdb_static_ucast_cnt + priv->fdb_static_mcast_cnt;

so the children's occupancy adds up to HW_U + HW_M + 2S, while the
parent FDBs reports HW_U + HW_M + S. Should static be split out of
ucast/mcast, or removed as a child?

The /FDBs/ucast, /FDBs/mcast and /FDBs/static names are visible to
userspace and will be hard to change later. The commit message also
doesn't describe this layout. Could the hierarchy be reworked before it
lands?

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