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

[PATCH net-next 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.

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    |  45 +++++-
 drivers/net/dsa/motorcomm/chip.h    |   3 +
 drivers/net/dsa/motorcomm/devlink.c | 208 ++++++++++++++++++++++++++++
 drivers/net/dsa/motorcomm/devlink.h |  16 +++
 5 files changed, 266 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 1/2] net: dsa: motorcomm: Report error when FDB fails to add

[David Yang]

fdb_add() ignored any errors returned by the hardware (for example,
table is full). Return error for that case.

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

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

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

[netdev-bot+sashiko]

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

[PATCH net-next 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    |  37 ++++-
 drivers/net/dsa/motorcomm/chip.h    |   3 +
 drivers/net/dsa/motorcomm/devlink.c | 208 ++++++++++++++++++++++++++++
 drivers/net/dsa/motorcomm/devlink.h |  16 +++
 5 files changed, 259 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..c3c555cb8941 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,7 @@ yt921x_fdb_add(struct yt921x_priv *priv, const unsigned char *addr, u16 vid,
 	if (val & YT921X_FDB_RESULT_NOTFOUND)
 		return -ENOSPC;
 
+	priv->fdb_static_cnt++;
 	return 0;
 }
 
@@ -761,18 +763,27 @@ 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 &&
+		    !WARN_ON(!priv->fdb_static_cnt))
+			priv->fdb_static_cnt--;
+		return 0;
+	}
 
 	return yt921x_fdb_add_index_in12(priv, index, ctrl1, ctrl2);
 }
@@ -809,7 +820,14 @@ 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)
+		priv->fdb_static_cnt++;
+	return 0;
 }
 
 static int
@@ -2154,6 +2172,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)
@@ -2488,6 +2508,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 +2600,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..5408ec268ba0 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,7 @@ struct yt921x_priv {
 	struct yt921x_port ports[YT921X_PORT_NUM];
 
 	u16 eee_ports_mask;
+	unsigned short fdb_static_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..cddc77a6fd11
--- /dev/null
+++ b/drivers/net/dsa/motorcomm/devlink.c
@@ -0,0 +1,208 @@
+// 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"
+
+#define to_yt921x_priv(_ds) container_of_const(_ds, struct yt921x_priv, ds)
+
+int
+yt921x_dsa_devlink_info_get(struct dsa_switch *ds, struct devlink_info_req *req,
+			    struct netlink_ext_ack *extack)
+{
+	struct yt921x_priv *priv = 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);
+	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);
+	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;
+
+	return READ_ONCE(priv->fdb_static_cnt);
+}
+
+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_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,
+		YT921X_FDB_NUM,
+		YT921X_DEVLINK_RESOURCE_ID_FDB_UCAST,
+		YT921X_DEVLINK_RESOURCE_ID_FDB,
+	},
+	{
+		"mcast", yt921x_devlink_occ_get_fdb_mcast,
+		YT921X_FDB_NUM,
+		YT921X_DEVLINK_RESOURCE_ID_FDB_MCAST,
+		YT921X_DEVLINK_RESOURCE_ID_FDB,
+	},
+	{
+		"static", yt921x_devlink_occ_get_fdb_static,
+		YT921X_FDB_NUM,
+		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..e13fbe320bca
--- /dev/null
+++ b/drivers/net/dsa/motorcomm/devlink.h
@@ -0,0 +1,16 @@
+/* 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>
+
+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 2/2] net: dsa: motorcomm: Add devlink info and resources

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 4 · Low: 4

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 FDB resource tree breaks devlink's rule for parent
  and child sizes.
- [Medium] yt921x_devlink_occ_get_fdb() computes FDB occupancy as
  LEARN_UCAST_CNT + LEARN_MCAST_CNT + priv->fdb_static_cnt.
- [Medium] priv->fdb_static_cnt is changed only after the whole FDB
  sequence reports success, so it can drift from the hardware when the…
- [Medium] yt921x_dsa_setup() calls yt921x_chip_reset(), which writes
  YT921X_RST_HW (chip.c:2258), described in a comment as 'almost same as…
- [Low] The uAPI resource names "ucast" and "mcast" under /FDBs do not
  describe what they report.
- [Low] devlink.h declares `int yt921x_devlink_setup(struct yt921x_priv
  *priv);` but includes only <net/dsa.h>.
- [Low] devlink.c defines its own macro `#define to_yt921x_priv(_ds)
  container_of_const(_ds, struct yt921x_priv, ds)` at line 11.
- [Low] priv->fdb_static_cnt is accessed in two inconsistent ways.

--

net: dsa: motorcomm: Add devlink info and resources

This commit reports the ASIC model through devlink info. It also registers
devlink resources for ACLs, FDB entries (with ucast, mcast and static
children) and meters. FDB occupancy adds the hardware learn counters to a
software count of static entries, which is updated in the FDB add, join
and leave paths.

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

[ ... ]

> @@ -731,6 +732,7 @@ yt921x_fdb_add(struct yt921x_priv *priv, const unsigned char *addr, u16 vid,
>  	if (val & YT921X_FDB_RESULT_NOTFOUND)
>  		return -ENOSPC;
>  
> +	priv->fdb_static_cnt++;
>  	return 0;
>  }
>  
> @@ -761,18 +763,27 @@ 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 &&
> +		    !WARN_ON(!priv->fdb_static_cnt))
> +			priv->fdb_static_cnt--;
> +		return 0;
> +	}

[Severity: Medium]
Can fdb_static_cnt drift from the hardware table when an FDB command
takes effect but a later step reports an error?

In yt921x_fdb_add(), OP_ADD|START is written to YT921X_FDB_OP before
yt921x_fdb_wait() runs. Two failures can happen after that write:

  - the wait times out ("FDB probably stuck")
  - the MDIO write was applied but still returned an error

In either case the function returns before fdb_static_cnt++, but the
entry may already be static in hardware. yt921x_fdb_join() has the same
problem through yt921x_fdb_add_index_in12().

If the same {addr, vid, port} is added again later, yt921x_fdb_join()
sees STATUS_STATIC with identical ctrl1/ctrl2 and returns 0 without
counting it. When the entry is finally removed, yt921x_fdb_leave()
reaches this check:

    if ((ctrl1 & YT921X_FDB_IO1_STATUS_M) ==
        YT921X_FDB_IO1_STATUS_STATIC &&
        !WARN_ON(!priv->fdb_static_cnt))

If there are no other static entries, the WARN_ON fires.

The opposite can happen here as well. If yt921x_fdb_wait() fails after the
DEL has completed, the decrement is skipped. The count then stays too high
for as long as the device exists, because nothing re-syncs the counter
with the hardware table.

>  
>  	return yt921x_fdb_add_index_in12(priv, index, ctrl1, ctrl2);
>  }

[ ... ]

> @@ -2154,6 +2172,8 @@ static void yt921x_dsa_teardown(struct dsa_switch *ds)
>  
>  	yt921x_leds_remove(priv);
>  #endif
> +
> +	dsa_devlink_resources_unregister(ds);
>  }

[Severity: Medium]
Should fdb_static_cnt be reset somewhere in the setup or teardown path?

yt921x_dsa_setup() calls yt921x_chip_reset(), which does:

    /* Reset */
    res = yt921x_reg_write(priv, YT921X_RST, YT921X_RST_HW);
    ...
    /* RST_HW is almost same as GPIO hard reset, so we need this delay. */

That reset clears the hardware FDB, static entries included. However,
fdb_static_cnt is only zeroed once, when priv is allocated with
devm_kzalloc() at probe time.

Setup can run again on the same priv if the DSA tree is torn down and set
up again while this switch stays probed. Examples are another member of a
multi-switch tree unbinding and rebinding, or a failed tree setup being
retried. If the counter was non-zero at teardown, the re-registered
"static" and "FDBs" resources would then count entries that no longer
exist.

Entries can still be present at teardown in two cases:

  - entries installed through ndo_dflt_fdb_add() that upper layers never
    remove; dsa_switch_release_ports() cleans these up, but it only runs
    at switch removal
  - entries left behind by the error-path drift described above

When those addresses are deleted later, yt921x_fdb_leave() finds no
hardware entry and returns early:

    if (index >= YT921X_FDB_NUM)
        return 0;

So the inflated count is never corrected.

[ ... ]

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

[ ... ]

> +#include "chip.h"
> +#include "devlink.h"
> +#include "smi.h"
> +#include "tc.h"
> +
> +#define to_yt921x_priv(_ds) container_of_const(_ds, struct yt921x_priv, ds)

[Severity: Low]
This isn't a bug, but this macro duplicates dsa_to_yt921x_priv() in
chip.h, which is already included here:

    #define dsa_to_yt921x_priv(_ds) container_of_const(_ds, struct yt921x_priv, ds)

Could yt921x_dsa_devlink_info_get() use the existing helper instead?

[ ... ]

> +static u64 yt921x_devlink_occ_get_fdb_static(void *ctx)
> +{
> +	struct yt921x_priv *priv = ctx;
> +
> +	return READ_ONCE(priv->fdb_static_cnt);
> +}

[Severity: Low]
Is this read meant to be lockless? It is the only occ_get callback in
this file that does not take priv->reg_lock.

The writers in yt921x_fdb_add(), yt921x_fdb_leave() and yt921x_fdb_join()
use plain ++/-- under reg_lock, without WRITE_ONCE(). Also,
yt921x_devlink_occ_get_fdb() reads the same field plainly under reg_lock.

This callback runs under devl_lock only, which does not exclude the FDB
writers, so the READ_ONCE() here is paired with unmarked writes.

Would it be simpler to take reg_lock like the other callbacks do, or else
to use WRITE_ONCE() in every writer?

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

[Severity: Medium]
Can this sum count the same FDB entry twice?

The total is LEARN_UCAST_CNT + LEARN_MCAST_CNT + fdb_static_cnt. That is
only correct if neither hardware counter includes static entries that the
CPU installed or converted.

For multicast: SA learning cannot create multicast entries, and the
driver does not configure hardware IGMP/MLD snooping. So every multicast
entry comes from this path:

  yt921x_dsa_port_mdb_add()->yt921x_fdb_join()->yt921x_fdb_add()

That path writes STATUS_STATIC and does fdb_static_cnt++. If
YT921X_LEARN_MCAST_CNT counts these entries, every MDB entry is counted
twice in "FDBs". If it does not, the "mcast" resource always reads 0.

For unicast: yt921x_fdb_join() rewrites an existing hardware-learned
entry as static through an index-mode ADD, and then does:

    if ((val1 & YT921X_FDB_IO1_STATUS_M) !=
        YT921X_FDB_IO1_STATUS_STATIC)
        priv->fdb_static_cnt++;

Does YT921X_LEARN_UCAST_CNT drop by one when the CPU rewrites the entry,
or only on age-out or flush? If only on age-out or flush, then a
"bridge fdb add ... static" for a MAC that was already learned is counted
under both ucast and static.

The commit message describes these as hardware counters for "learned FDB
entries". Nothing in the code shows how they treat entries the CPU wrote.
The answer depends on hardware behaviour that can't be confirmed from the
code.

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

[ ... ]

> +static const struct yt921x_devlink_resource yt921x_devlink_resources[] = {

[ ... ]

> +	{
> +		"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,
> +		YT921X_FDB_NUM,
> +		YT921X_DEVLINK_RESOURCE_ID_FDB_UCAST,
> +		YT921X_DEVLINK_RESOURCE_ID_FDB,
> +	},

[Severity: Low]
Do the names "ucast" and "mcast" match what these resources report?

yt921x_devlink_occ_get_fdb_ucast() and yt921x_devlink_occ_get_fdb_mcast()
return only the hardware learn counters (YT921X_LEARN_UCAST_CNT and
YT921X_LEARN_MCAST_CNT). Static entries from yt921x_dsa_port_fdb_add()
and yt921x_dsa_port_mdb_add() only increment fdb_static_cnt. So a
"bridge fdb add ... static" or an MDB entry never shows up under "ucast"
or "mcast".

These names become uAPI once they are released. Would names like
"learned-ucast" and "learned-mcast" be clearer?

> +	{
> +		"mcast", yt921x_devlink_occ_get_fdb_mcast,
> +		YT921X_FDB_NUM,
> +		YT921X_DEVLINK_RESOURCE_ID_FDB_MCAST,
> +		YT921X_DEVLINK_RESOURCE_ID_FDB,
> +	},
> +	{
> +		"static", yt921x_devlink_occ_get_fdb_static,
> +		YT921X_FDB_NUM,
> +		YT921X_DEVLINK_RESOURCE_ID_FDB_STATIC,
> +		YT921X_DEVLINK_RESOURCE_ID_FDB,
> +	},

[Severity: Medium]
Does this hierarchy follow devlink's rule for parent and child sizes?

"FDBs" is registered with YT921X_FDB_NUM entries. Each of its three
children, "ucast", "mcast" and "static", is also registered with
YT921X_FDB_NUM. devlink treats children as parts of the parent:

net/devlink/resource.c:devlink_resource_validate_children() {
    ...
	list_for_each_entry(child_resource, &resource->resource_list, list)
		parts_size += child_resource->size_new;

	if (parts_size > resource->size_new)
		size_valid = false;
    ...
}

Registration does not catch this, because devl_resource_register() sets
size_valid = true. However, devlink_nl_resource_set_doit() re-validates
the parent:

	resource->size_new = size;
	devlink_resource_validate_children(resource);
	if (resource->parent)
		devlink_resource_validate_children(resource->parent);

Since size_min == size_max, the only value a set can use is 4096. Even
that no-op, "devlink resource set ... path /FDBs/ucast size 4096", leaves
"FDBs" permanently reported with size_valid false, because
3 * 4096 > 4096.

The children also overlap. ucast and mcast split entries by address type,
while static splits them by entry type, so a static unicast entry
belongs to two siblings.

Would it work better to register these counters as top-level resources?
mv88e6xxx does this: its ATU_bin_* resources are registered with
DEVLINK_RESOURCE_ID_PARENT_TOP, as siblings of "ATU". Another option is to
give the children non-overlapping sizes that add up to the parent.

[ ... ]

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

[ ... ]

> +#include <net/dsa.h>
> +
> +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);

[Severity: Low]
This isn't a bug, but devlink.h uses struct yt921x_priv without including
chip.h or forward-declaring the struct. leds.h, mdio_bus.h and smi.h all
have:

    struct yt921x_priv;

devlink.h only compiles because chip.c and devlink.c include chip.h
first. Could a forward declaration be added so the header is
self-contained?

> +
> +#endif

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