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

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

[David Yang]

Add basic devlink support: expose the ASIC id through .devlink_info_get,
and register devlink resources for the hardware blocks, reporting ACL,
FDB (with unicast/mcast/static occupancy) and meter usage.

v2:
 - fix issues reported by sashiko
v1: https://lore.kernel.org/r/20261106184811.350700-1-mmyangfl@gmail.com

David Yang (2):
  net: dsa: motorcomm: Report error when FDB fails to add
  net: dsa: motorcomm: Add devlink info and resources

 drivers/net/dsa/motorcomm/Makefile  |   1 +
 drivers/net/dsa/motorcomm/chip.c    |  63 +++++++-
 drivers/net/dsa/motorcomm/chip.h    |   4 +
 drivers/net/dsa/motorcomm/devlink.c | 216 ++++++++++++++++++++++++++++
 drivers/net/dsa/motorcomm/devlink.h |  18 +++
 5 files changed, 295 insertions(+), 7 deletions(-)
 create mode 100644 drivers/net/dsa/motorcomm/devlink.c
 create mode 100644 drivers/net/dsa/motorcomm/devlink.h

-- 
2.53.0

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

[David Yang]

fdb_add() ignored any errors returned by the hardware. Return error for
that case.

In practice the 4096-entry table is hardly ever filled, and since
fdb_add() will automatically expel dynamic entries, something bad will
definitively happen before you fill it up with static entries. This,
however, is in preparation for the upcoming static entry counters.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/chip.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 33e5fc127244..a7a9a7439304 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;
 }
 
 static int
-- 
2.53.0

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

[David Yang]

Expose the ASIC model via devlink info (ASIC_ID), and register devlink
resources for ACLs, FDB and meters. Resource occupancy is read back from
the hardware counters for learned FDB entries; static FDB entries are
counted in software.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/Makefile  |   1 +
 drivers/net/dsa/motorcomm/chip.c    |  55 ++++++-
 drivers/net/dsa/motorcomm/chip.h    |   4 +
 drivers/net/dsa/motorcomm/devlink.c | 216 ++++++++++++++++++++++++++++
 drivers/net/dsa/motorcomm/devlink.h |  18 +++
 5 files changed, 288 insertions(+), 6 deletions(-)
 create mode 100644 drivers/net/dsa/motorcomm/devlink.c
 create mode 100644 drivers/net/dsa/motorcomm/devlink.h

diff --git a/drivers/net/dsa/motorcomm/Makefile b/drivers/net/dsa/motorcomm/Makefile
index 9eaa59f699e8..0b31d815ce43 100644
--- a/drivers/net/dsa/motorcomm/Makefile
+++ b/drivers/net/dsa/motorcomm/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_NET_DSA_YT921X) += yt921x.o
 yt921x-objs := chip.o
 yt921x-$(CONFIG_NET_DSA_YT921X_LEDS) += leds.o
+yt921x-objs += devlink.o
 yt921x-objs += mdio_bus.o
 yt921x-objs += mib.o
 yt921x-objs += pcs-921x.o
diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index a7a9a7439304..95d57e74d64c 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -26,6 +26,7 @@
 #include <net/ieee8021q.h>
 
 #include "chip.h"
+#include "devlink.h"
 #include "leds.h"
 #include "mdio_bus.h"
 #include "mib.h"
@@ -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;
 }
 
@@ -761,18 +766,35 @@ yt921x_fdb_leave(struct yt921x_priv *priv, const unsigned char *addr,
 	ctrl2 = val2 & ~YT921X_FDB_IO2_EGR_PORTS(ports_mask);
 	if (ctrl2 == val2)
 		return 0;
+
+	res = yt921x_reg_read(priv, YT921X_FDB_OUT1, &ctrl1);
+	if (res)
+		return res;
+
 	if (!(ctrl2 & YT921X_FDB_IO2_EGR_PORTS_M)) {
 		ctrl = YT921X_FDB_OP_OP_DEL | YT921X_FDB_OP_START;
 		res = yt921x_reg_write(priv, YT921X_FDB_OP, ctrl);
 		if (res)
 			return res;
 
-		return yt921x_fdb_wait(priv, &val);
-	}
+		res = yt921x_fdb_wait(priv, &val);
+		if (res)
+			return res;
 
-	res = yt921x_reg_read(priv, YT921X_FDB_OUT1, &ctrl1);
-	if (res)
-		return res;
+		if ((ctrl1 & YT921X_FDB_IO1_STATUS_M) ==
+		    YT921X_FDB_IO1_STATUS_STATIC) {
+			unsigned short *cnt;
+
+			if (is_multicast_ether_addr(addr))
+				cnt = &priv->fdb_static_mcast_cnt;
+			else
+				cnt = &priv->fdb_static_ucast_cnt;
+
+			if (!WARN_ON(!*cnt))
+				(*cnt)--;
+		}
+		return 0;
+	}
 
 	return yt921x_fdb_add_index_in12(priv, index, ctrl1, ctrl2);
 }
@@ -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++;
+	}
+	return 0;
 }
 
 static int
@@ -2154,6 +2187,8 @@ static void yt921x_dsa_teardown(struct dsa_switch *ds)
 
 	yt921x_leds_remove(priv);
 #endif
+
+	dsa_devlink_resources_unregister(ds);
 }
 
 static int yt921x_chip_detect(struct yt921x_priv *priv)
@@ -2481,6 +2516,9 @@ static int yt921x_dsa_setup(struct dsa_switch *ds)
 			  pp->pcs.supported_interfaces);
 	}
 
+	priv->fdb_static_ucast_cnt = 0;
+	priv->fdb_static_mcast_cnt = 0;
+
 	mutex_lock(&priv->reg_lock);
 	res = yt921x_chip_setup(priv);
 	mutex_unlock(&priv->reg_lock);
@@ -2488,6 +2526,10 @@ static int yt921x_dsa_setup(struct dsa_switch *ds)
 	if (res)
 		return res;
 
+	res = yt921x_devlink_setup(priv);
+	if (res)
+		return res;
+
 #if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
 	res = yt921x_leds_setup(priv);
 	if (res)
@@ -2576,6 +2618,7 @@ static const struct dsa_switch_ops yt921x_dsa_switch_ops = {
 	.port_add_dscp_prio	= yt921x_dsa_port_add_dscp_prio,
 #endif
 	/* chip */
+	.devlink_info_get	= yt921x_dsa_devlink_info_get,
 	.teardown		= yt921x_dsa_teardown,
 	.setup			= yt921x_dsa_setup,
 };
diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h
index fc315f1d8f87..d3f2a86feb22 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_PORT_LEARN_MODE_AUTO			YT921X_PORT_LEARN_MODE(0)
 #define   YT921X_PORT_LEARN_MODE_AUTO_AND_COPY		YT921X_PORT_LEARN_MODE(1)
 #define   YT921X_PORT_LEARN_MODE_CPU_CONTROL		YT921X_PORT_LEARN_MODE(2)
+#define YT921X_LEARN_UCAST_CNT		0x18043c
 #define YT921X_AGEING			0x180440
 #define  YT921X_AGEING_INTERVAL_M		GENMASK(15, 0)
 #define YT921X_FDB_IN0			0x180454
@@ -312,6 +313,7 @@ enum yt921x_app_selector {
 #define   YT921X_FDB_IO2_PRIO(x)			FIELD_PREP(YT921X_FDB_IO2_PRIO_M, (x))
 #define  YT921X_FDB_IO2_NEW_VID_M		GENMASK(11, 0)
 #define   YT921X_FDB_IO2_NEW_VID(x)			FIELD_PREP(YT921X_FDB_IO2_NEW_VID_M, (x))
+#define YT921X_LEARN_MCAST_CNT		0x180504
 #define YT921X_FILTER_UNK_UCAST		0x180508
 #define YT921X_FILTER_UNK_MCAST		0x18050c
 #define YT921X_FILTER_MCAST		0x180510
@@ -525,6 +527,8 @@ struct yt921x_priv {
 	struct yt921x_port ports[YT921X_PORT_NUM];
 
 	u16 eee_ports_mask;
+	unsigned short fdb_static_ucast_cnt;
+	unsigned short fdb_static_mcast_cnt;
 
 	DECLARE_BITMAP(meters_map, YT921X_METER_NUM);
 
diff --git a/drivers/net/dsa/motorcomm/devlink.c b/drivers/net/dsa/motorcomm/devlink.c
new file mode 100644
index 000000000000..a9a611ef5571
--- /dev/null
+++ b/drivers/net/dsa/motorcomm/devlink.c
@@ -0,0 +1,216 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2026 David Yang
+ */
+
+#include "chip.h"
+#include "devlink.h"
+#include "smi.h"
+#include "tc.h"
+
+int
+yt921x_dsa_devlink_info_get(struct dsa_switch *ds, struct devlink_info_req *req,
+			    struct netlink_ext_ack *extack)
+{
+	struct yt921x_priv *priv = dsa_to_yt921x_priv(ds);
+	int res;
+
+	if (!priv->info)
+		return -ENODEV;
+
+	res = devlink_info_version_fixed_put(req,
+					     DEVLINK_INFO_VERSION_GENERIC_ASIC_ID,
+					     priv->info->name);
+	if (res)
+		return res;
+
+	return 0;
+}
+
+static u64 yt921x_devlink_occ_get_acl(void *ctx)
+{
+	struct yt921x_priv *priv = ctx;
+	u64 count = 0;
+
+	mutex_lock(&priv->reg_lock);
+	for (unsigned int i = 0; i < YT921X_ACL_BLK_NUM; i++)
+		count += hweight8(priv->acl_masks[i]);
+	mutex_unlock(&priv->reg_lock);
+
+	return count;
+}
+
+static u64 yt921x_devlink_occ_get_fdb_ucast(void *ctx)
+{
+	struct yt921x_priv *priv = ctx;
+	u32 val;
+	int res;
+
+	mutex_lock(&priv->reg_lock);
+	res = yt921x_reg_read(priv, YT921X_LEARN_UCAST_CNT, &val);
+	if (!res)
+		val += priv->fdb_static_ucast_cnt;
+	mutex_unlock(&priv->reg_lock);
+
+	if (res)
+		return 0;
+	return val;
+}
+
+static u64 yt921x_devlink_occ_get_fdb_mcast(void *ctx)
+{
+	struct yt921x_priv *priv = ctx;
+	u32 val;
+	int res;
+
+	mutex_lock(&priv->reg_lock);
+	res = yt921x_reg_read(priv, YT921X_LEARN_MCAST_CNT, &val);
+	if (!res)
+		val += priv->fdb_static_mcast_cnt;
+	mutex_unlock(&priv->reg_lock);
+
+	if (res)
+		return 0;
+	return val;
+}
+
+static u64 yt921x_devlink_occ_get_fdb_static(void *ctx)
+{
+	struct yt921x_priv *priv = ctx;
+	u64 count;
+
+	mutex_lock(&priv->reg_lock);
+	count = priv->fdb_static_ucast_cnt + priv->fdb_static_mcast_cnt;
+	mutex_unlock(&priv->reg_lock);
+
+	return count;
+}
+
+static u64 yt921x_devlink_occ_get_fdb(void *ctx)
+{
+	struct yt921x_priv *priv = ctx;
+	u64 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_ucast_cnt;
+	count += priv->fdb_static_mcast_cnt;
+
+end:
+	mutex_unlock(&priv->reg_lock);
+
+	if (res)
+		return 0;
+	return count;
+}
+
+static u64 yt921x_devlink_occ_get_meter(void *ctx)
+{
+	struct yt921x_priv *priv = ctx;
+	u64 count;
+
+	mutex_lock(&priv->reg_lock);
+	count = bitmap_weight(priv->meters_map, YT921X_METER_NUM);
+	mutex_unlock(&priv->reg_lock);
+
+	return count;
+}
+
+enum yt921x_devlink_resource_id {
+	YT921X_DEVLINK_RESOURCE_ID_NONE,  /* DEVLINK_RESOURCE_ID_PARENT_TOP */
+	YT921X_DEVLINK_RESOURCE_ID_ACL,
+	YT921X_DEVLINK_RESOURCE_ID_FDB,
+	YT921X_DEVLINK_RESOURCE_ID_FDB_UCAST,
+	YT921X_DEVLINK_RESOURCE_ID_FDB_MCAST,
+	YT921X_DEVLINK_RESOURCE_ID_FDB_STATIC,
+	YT921X_DEVLINK_RESOURCE_ID_METER,
+};
+
+struct yt921x_devlink_resource {
+	const char *name;
+	devlink_resource_occ_get_t *occ_get;
+	unsigned short size;
+	unsigned char id;
+	unsigned char parent_id;
+};
+
+static const struct yt921x_devlink_resource yt921x_devlink_resources[] = {
+	{
+		"ACLs", yt921x_devlink_occ_get_acl,
+		YT921X_ACL_NUM,
+		YT921X_DEVLINK_RESOURCE_ID_ACL,
+		DEVLINK_RESOURCE_ID_PARENT_TOP,
+	},
+	{
+		"FDBs", yt921x_devlink_occ_get_fdb,
+		YT921X_FDB_NUM,
+		YT921X_DEVLINK_RESOURCE_ID_FDB,
+		DEVLINK_RESOURCE_ID_PARENT_TOP,
+	},
+	{
+		"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,
+	},
+	{
+		"meters", yt921x_devlink_occ_get_meter,
+		YT921X_METER_NUM,
+		YT921X_DEVLINK_RESOURCE_ID_METER,
+		DEVLINK_RESOURCE_ID_PARENT_TOP,
+	},
+};
+
+int yt921x_devlink_setup(struct yt921x_priv *priv)
+{
+	struct dsa_switch *ds = &priv->ds;
+	int res;
+
+	for (unsigned int i = 0; i < ARRAY_SIZE(yt921x_devlink_resources);
+	     i++) {
+		struct devlink_resource_size_params size_params;
+		const struct yt921x_devlink_resource *r;
+
+		r = &yt921x_devlink_resources[i];
+
+		devlink_resource_size_params_init(&size_params, r->size,
+						  r->size, 1,
+						  DEVLINK_RESOURCE_UNIT_ENTRY);
+
+		res = dsa_devlink_resource_register(ds, r->name, r->size, r->id,
+						    r->parent_id, &size_params);
+		if (res) {
+			dsa_devlink_resources_unregister(ds);
+			return res;
+		}
+
+		dsa_devlink_resource_occ_get_register(ds, r->id, r->occ_get,
+						      priv);
+	}
+
+	return 0;
+}
diff --git a/drivers/net/dsa/motorcomm/devlink.h b/drivers/net/dsa/motorcomm/devlink.h
new file mode 100644
index 000000000000..184b9f05742e
--- /dev/null
+++ b/drivers/net/dsa/motorcomm/devlink.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (c) 2026 David Yang
+ */
+
+#ifndef _YT_DEVLINK_H
+#define _YT_DEVLINK_H
+
+#include <net/dsa.h>
+
+struct yt921x_priv;
+
+int
+yt921x_dsa_devlink_info_get(struct dsa_switch *ds, struct devlink_info_req *req,
+			    struct netlink_ext_ack *extack);
+int yt921x_devlink_setup(struct yt921x_priv *priv);
+
+#endif
-- 
2.53.0

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

[netdev-bot+sashiko]

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

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

[netdev-bot+sashiko]

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