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

[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: 1 · Low: 2

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 FDB occupancy that yt921x_devlink_occ_get_fdb()
  (drivers/net/dsa/motorcomm/devlink.c) reports for the "FDB" devlink…
- [Low] The patch adds devlink info (DEVLINK_INFO_VERSION_GENERIC_ASIC_ID
  from yt921x_dsa_devlink_info_get()) and three driver-specific devlink…
- [Low] The commit message says only "Expose the ASIC model via devlink
  info (ASIC_ID), and register devlink resources for ACLs, FDB and…

--

net: dsa: motorcomm: Add devlink info and resources

This commit adds devlink info to the yt921x DSA driver, reporting the ASIC
model as ASIC_ID. It also registers devlink resources for the ACL, FDB and
meter tables. To compute FDB occupancy it adds a software counter of
static FDB entries.

> Expose the ASIC model via devlink info (ASIC_ID), and register devlink
> resources for ACLs, FDB and meters.

[Severity: Low]
Could the commit message also describe the changes to the FDB programming
paths?

The patch adds priv->fdb_static_cnt and updates it in yt921x_fdb_add(),
yt921x_fdb_join() and yt921x_fdb_leave(). These run for every bridge
fdb/mdb add and delete.

yt921x_fdb_leave() is also restructured. YT921X_FDB_OUT1 is now read
before the DEL operation, not only on the rewrite path. A
WARN_ON(!priv->fdb_static_cnt) is also added on the delete path.

It would help to explain why FDB occupancy is computed as
LEARN_UCAST_CNT + LEARN_MCAST_CNT + a software static count. That formula
assumes the hardware counters only include learned entries.

> diff --git a/drivers/net/dsa/motorcomm/devlink.c b/drivers/net/dsa/motorcomm/devlink.c
> new file mode 100644
> index 0000000000000..529d2decc4036
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/devlink.c

[ ... ]

> +static u64 yt921x_devlink_occ_get_fdb(void *ctx)
> +{
> +	struct yt921x_priv *priv = ctx;
> +	unsigned int count = 0;
> +	u32 val;
> +	int res;
> +
> +	mutex_lock(&priv->reg_lock);
> +
> +	res = yt921x_reg_read(priv, YT921X_LEARN_UCAST_CNT, &val);
> +	if (res)
> +		goto end;
> +	count += val;
> +
> +	res = yt921x_reg_read(priv, YT921X_LEARN_MCAST_CNT, &val);
> +	if (res)
> +		goto end;
> +	count += val;
> +
> +	count += priv->fdb_static_cnt;

[Severity: Medium]
Can this report more FDB occupancy than actually exists, possibly even
more than the advertised size of YT921X_FDB_NUM?

The new YT921X_LEARN_UCAST_CNT and YT921X_LEARN_MCAST_CNT definitions in
chip.h have no field mask. Neighbouring registers do, for example
YT921X_AGEING_INTERVAL_M. The full 32-bit value is added here, so any
reserved, limit or flag bits next to the count would end up in the
reported occupancy. Is the whole register the counter?

The sum also assumes the hardware learn counters and fdb_static_cnt never
overlap. In yt921x_fdb_join(), an existing hardware-learned entry is
rewritten as static with an index-mode ADD:

	ctrl1 = val1 & ~YT921X_FDB_IO1_STATUS_M;
	ctrl1 |= YT921X_FDB_IO1_STATUS_STATIC;
	...
	if (ctrl1 != val1)
		priv->fdb_static_cnt++;

Is that entry counted twice if the hardware learn counter does not drop
when the CPU rewrites the status?

The same could happen in yt921x_fdb_add():

1. yt921x_fdb_has() returns not found.
2. The learning engine then learns the same MAC/FID. reg_lock only
   serializes CPU access, not hardware learning.
3. The hash-mode ADD overwrites that entry, but fdb_static_cnt++ still
   runs.

MDB entries are also added as static through yt921x_fdb_join() from
port_mdb_add, so they are counted in fdb_static_cnt. Source MAC learning
never creates multicast entries. If LEARN_MCAST_CNT counts all multicast
table entries, would every MDB entry be counted twice?

> +
> +end:
> +	mutex_unlock(&priv->reg_lock);

[ ... ]

> +static const struct yt921x_devlink_resource yt921x_devlink_resources[] = {
> +	{
> +		"ACL", yt921x_devlink_occ_get_acl,
> +		YT921X_ACL_NUM,
> +		YT921X_DEVLINK_RESOURCE_ID_ACL,
> +		DEVLINK_RESOURCE_ID_PARENT_TOP,
> +	},
> +	{
> +		"FDB", yt921x_devlink_occ_get_fdb,
> +		YT921X_FDB_NUM,
> +		YT921X_DEVLINK_RESOURCE_ID_FDB,
> +		DEVLINK_RESOURCE_ID_PARENT_TOP,
> +	},
> +	{
> +		"meters", yt921x_devlink_occ_get_meter,
> +		YT921X_METER_NUM,
> +		YT921X_DEVLINK_RESOURCE_ID_METER,
> +		DEVLINK_RESOURCE_ID_PARENT_TOP,
> +	},
> +};

[Severity: Low]
Should this come with a driver page under Documentation/networking/devlink/?

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 entry there yet. For comparison, mv88e6xxx.rst
documents the devlink features of another DSA driver.

The "ACL", "FDB" and "meters" resource names and the ASIC_ID info version
are new user-visible interfaces. For example, "FDB" also includes MDB
entries because the hardware table is shared.

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