[PATCH net-next v11 0/7] net: dsa: motorcomm: add support yt922x driver

[PATCH net-next v11 0/7] net: dsa: motorcomm: add support yt922x driver

[Kyle Switch]

This patchset mainly adds support for the yt922x series to the dsa driver.
Currently, only the yt9224 in the yt922x series is supported, and the
corresponding tag handling for yt922x is added.

At the same time, it optimizes the max ports boundary check in the
existing code, and initializes the dsa switch based on the chipid.

changes in v11:
1) Initialize ctrl variable inside yt922x_port_up().
2) Update maitainer for new file.

changes in v10:
1) Rebase the code.

changes in v9:
patch 1: 1) Add a dev_err() when chipid mismatch.
patch 3: 1) Using GENMASK_U* to replace GENMASK

changes in v8:
patch 1: 1) Add a lock operation to acquire the chip ID during the probe()
         stage.
         2) Remove meaningless changes.
patch 3: Modify the changelog description.
patch 6: 1) Remove sds_init() in this patch.
         2) Code refactoring, including yt922x_sds_phyaddr_get() and
            yt921x_needs_extmode_check()
         3) In yt921x_intif_ext_write() and yt921x_intif_ext_read(),
            add a boundary check for phyaddr. The reason is that the
            phyaddr of the top ext register is 9, so max_ports should
            be used for the check to include it.
         4) In the yt922x driver, added initialization of yt921x_port
     for exporting priv.

changes in v7:
patch 1: Fix the series_lookup() redundancy issue.
         Fix the dsa->priv initialization and the code style issue.
patch 6: Move psc definition to yt921x_port
         Fix psc_get_state() and an_restart().
         Use the existing yt921x driver interface for the duplicated
         logic for chip_reset().
         Move the yt922x_cpu_port_set() to chip_setup_dsa() to align
         with yt921x function.

Kyle Switch (7):
  net: dsa: motorcomm: initialize dsa_switch based on chipid
  net: dsa: motorcomm: use max_ports in series for port bounds checking
  net: dsa: motorcomm: use fixed-width GENMASK_U*() helpers
  net: dsa: motorcomm: move mib start from probe() to dsa_setup()
  net: dsa: motorcomm: move mib stop from remove() to teardown()
  net: dsa: tag_yt922x: add support for Motorcomm YT922x tags
  net: dsa: motorcomm: Add support for Motorcomm YT922x

 MAINTAINERS                          |   2 +
 drivers/net/dsa/motorcomm/Kconfig    |   7 +-
 drivers/net/dsa/motorcomm/Makefile   |   1 +
 drivers/net/dsa/motorcomm/chip.c     | 565 +++++++++++++++++++++++++--
 drivers/net/dsa/motorcomm/chip.h     | 110 ++++++
 drivers/net/dsa/motorcomm/mdio_bus.c |  47 ++-
 drivers/net/dsa/motorcomm/mdio_bus.h |   2 +
 drivers/net/dsa/motorcomm/pcs-922x.c | 173 ++++++++
 drivers/net/dsa/motorcomm/pcs.h      |   1 +
 include/net/dsa.h                    |   2 +
 net/dsa/Kconfig                      |   6 +
 net/dsa/Makefile                     |   1 +
 net/dsa/tag_yt922x.c                 | 109 ++++++
 13 files changed, 991 insertions(+), 35 deletions(-)
 create mode 100644 drivers/net/dsa/motorcomm/pcs-922x.c
 create mode 100644 net/dsa/tag_yt922x.c

-- 
2.25.1

[PATCH net-next v11 1/7] net: dsa: motorcomm: initialize dsa_switch based on chipid

[Kyle Switch]

Replace the hardcoded chip handling in yt92xx_register_switch() with a
data-driven loopup based on the chip ID.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Kyle Switch <kyle.switch@motor-comm.com>
---
 drivers/net/dsa/motorcomm/chip.c | 73 ++++++++++++++++++++++++++++----
 drivers/net/dsa/motorcomm/chip.h | 19 +++++++++
 2 files changed, 84 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 3ff2de383157..79422def16ff 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -4397,6 +4397,67 @@ static const struct dsa_switch_ops yt921x_dsa_switch_ops = {
 	.setup			= yt921x_dsa_setup,
 };
 
+static const struct yt92xx_series yt92xx_series_table[] = {
+	[YT92XX_MODE_YT921X] = {
+		.name = "YT921x",
+		.max_ports = YT921X_PORT_NUM,
+		.num_lag_ids = YT921X_LAG_NUM,
+		.ageing_time_min = 1 * 5000,
+		.ageing_time_max = U16_MAX * 5000,
+		.dscp_prio_mapping_is_global = true,
+		.assisted_learning_on_cpu_port = true,
+		.switch_ops = &yt921x_dsa_switch_ops,
+		.mac_ops = &yt921x_phylink_mac_ops
+	},
+};
+
+static const struct yt92xx_series *yt92xx_series_lookup(u32 major)
+{
+	if (major == YT9215_MAJOR || major == YT9218_MAJOR)
+		return &yt92xx_series_table[YT92XX_MODE_YT921X];
+	else
+		return NULL;
+}
+
+static int yt92xx_register_switch(struct dsa_switch *ds)
+{
+	struct yt921x_priv *priv = to_yt921x_priv(ds);
+	struct device *dev = to_device(priv);
+	const struct yt92xx_series *series;
+	u32 chipid;
+	u32 major;
+	int res;
+
+	mutex_lock(&priv->reg_lock);
+	res = yt921x_reg_read(priv, YT921X_CHIP_ID, &chipid);
+	mutex_unlock(&priv->reg_lock);
+	if (res) {
+		dev_err(dev, "Unexpected chipid 0x%x\n", chipid);
+		return res;
+	}
+
+	major = FIELD_GET(YT921X_CHIP_ID_MAJOR, chipid);
+	series = yt92xx_series_lookup(major);
+	if (!series) {
+		dev_err(dev, "Undefined series info for the corresponding major\n");
+		return -ENODEV;
+	}
+	priv->series = series;
+
+	ds->assisted_learning_on_cpu_port =
+		priv->series->assisted_learning_on_cpu_port;
+	ds->dscp_prio_mapping_is_global =
+		priv->series->dscp_prio_mapping_is_global;
+	ds->ageing_time_min = priv->series->ageing_time_min;
+	ds->ageing_time_max = priv->series->ageing_time_max;
+	ds->num_lag_ids = priv->series->num_lag_ids;
+	ds->num_ports = priv->series->max_ports;
+	ds->ops = priv->series->switch_ops;
+	ds->phylink_mac_ops = priv->series->mac_ops;
+
+	return 0;
+}
+
 static void yt921x_mdio_shutdown(struct mdio_device *mdiodev)
 {
 	struct yt921x_priv *priv = mdiodev_get_drvdata(mdiodev);
@@ -4445,6 +4506,7 @@ static int yt921x_mdio_probe(struct mdio_device *mdiodev)
 	struct yt921x_reg_mdio *mdio;
 	struct yt921x_priv *priv;
 	struct dsa_switch *ds;
+	int res;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -4472,15 +4534,10 @@ static int yt921x_mdio_probe(struct mdio_device *mdiodev)
 
 	ds = &priv->ds;
 	ds->dev = dev;
-	ds->assisted_learning_on_cpu_port = true;
-	ds->dscp_prio_mapping_is_global = true;
 	ds->priv = priv;
-	ds->ops = &yt921x_dsa_switch_ops;
-	ds->ageing_time_min = 1 * 5000;
-	ds->ageing_time_max = U16_MAX * 5000;
-	ds->phylink_mac_ops = &yt921x_phylink_mac_ops;
-	ds->num_lag_ids = YT921X_LAG_NUM;
-	ds->num_ports = YT921X_PORT_NUM;
+	res = yt92xx_register_switch(ds);
+	if (res)
+		return res;
 
 	mdiodev_set_drvdata(mdiodev, priv);
 
diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h
index 28e4f26f9cc4..69176995090a 100644
--- a/drivers/net/dsa/motorcomm/chip.h
+++ b/drivers/net/dsa/motorcomm/chip.h
@@ -955,9 +955,28 @@ struct yt921x_info {
 	u16 serdes_mask;
 };
 
+enum yt92xx_mode {
+	YT92XX_MODE_YT921X,
+	YT92XX_MODE_YT922X,
+	YT92XX_MODE_MAX,
+};
+
+struct yt92xx_series {
+	const char *name;
+	unsigned int max_ports;
+	unsigned int num_lag_ids;
+	unsigned int ageing_time_min;
+	unsigned int ageing_time_max;
+	u32 dscp_prio_mapping_is_global;
+	u32 assisted_learning_on_cpu_port;
+	const struct dsa_switch_ops *switch_ops;
+	const struct phylink_mac_ops *mac_ops;
+};
+
 struct yt921x_priv {
 	struct dsa_switch ds;
 
+	const struct yt92xx_series *series;
 	const struct yt921x_info *info;
 	unsigned int meter_slot_ns;
 	unsigned int port_shape_slot_ns;
-- 
2.25.1

Re: [PATCH net-next v11 1/7] net: dsa: motorcomm: initialize dsa_switch based on chipid

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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] In yt92xx_register_switch()
  (drivers/net/dsa/motorcomm/chip.c), `u32 chipid;` has no initializer.

--

net: dsa: motorcomm: initialize dsa_switch based on chipid

This patch moves the hardcoded dsa_switch setup out of
yt921x_mdio_probe() and into a new helper, yt92xx_register_switch().
The helper reads the chip ID, looks up a yt92xx_series table entry by
major number, and fills in the dsa_switch fields from that entry.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 3ff2de383157a..79422def16ffb 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> +static int yt92xx_register_switch(struct dsa_switch *ds)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	struct device *dev = to_device(priv);
> +	const struct yt92xx_series *series;
> +	u32 chipid;
> +	u32 major;
> +	int res;
> +
> +	mutex_lock(&priv->reg_lock);
> +	res = yt921x_reg_read(priv, YT921X_CHIP_ID, &chipid);
> +	mutex_unlock(&priv->reg_lock);
> +	if (res) {
> +		dev_err(dev, "Unexpected chipid 0x%x\n", chipid);
> +		return res;
> +	}

[Severity: Medium]
Can this dev_err() print an uninitialized chipid?

chipid has no initializer. This branch runs only when yt921x_reg_read()
fails. The read goes through yt921x_reg_mdio_read(), and every MDIO
failure there jumps past the store to the caller's variable:

drivers/net/dsa/motorcomm/chip.c:yt921x_reg_mdio_read() {
    ...
	res = __mdiobus_read(bus, addr, reg_data);
	if (res < 0)
		goto end;
	val = (val << 16) | (u16)res;

	*valp = val;
	res = 0;

end:
	mutex_unlock(&bus->mdio_lock);
	return res;
}

So an MDIO bus error during yt921x_mdio_probe()->yt92xx_register_switch()
would log leftover stack contents as the chip ID. KMSAN would also flag
this as uninit-value.

The message is also misleading in this case, because no chip ID was
read. Could this branch print the error code instead, for example
"Failed to read chip ID: %d"?

> +
> +	major = FIELD_GET(YT921X_CHIP_ID_MAJOR, chipid);
> +	series = yt92xx_series_lookup(major);
> +	if (!series) {
> +		dev_err(dev, "Undefined series info for the corresponding major\n");
> +		return -ENODEV;
> +	}

This branch is where the chip ID really is unexpected, but the message
leaves out chipid.

The existing yt921x_chip_detect() handles both cases the other way
round. It returns res without a message when the read fails, and prints
"Unexpected chipid 0x%x" only after a successful read with an unknown
major:

drivers/net/dsa/motorcomm/chip.c:yt921x_chip_detect() {
    ...
	res = yt921x_reg_read(priv, YT921X_CHIP_ID, &chipid);
	if (res)
		return res;
    ...
	if (!info->name) {
		dev_err(dev, "Unexpected chipid 0x%x\n", chipid);
		return -ENODEV;
	}
    ...
}

Should the chipid value be printed here, in the unknown-major branch,
instead of in the read-failure branch above?

> +	priv->series = series;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002173706.1225502-1-kyle.switch%40motor-comm.com

[PATCH net-next v11 2/7] net: dsa: motorcomm: use max_ports in series for port bounds checking

[Kyle Switch]

Replace the hardcoded YT921X_PORT_NUM macro with the per-series
max_ports field in port validation. This removes family-specific
constants from the common code path and simplifies adding new
switch families with different port counts.

No functional change for existing YT921X devices.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Kyle Switch <kyle.switch@motor-comm.com>
---
 drivers/net/dsa/motorcomm/mdio_bus.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/mdio_bus.c b/drivers/net/dsa/motorcomm/mdio_bus.c
index 1a3f3cc68275..c5a0f1bc1e36 100644
--- a/drivers/net/dsa/motorcomm/mdio_bus.c
+++ b/drivers/net/dsa/motorcomm/mdio_bus.c
@@ -115,7 +115,7 @@ static int yt921x_mbus_int_read(struct mii_bus *mbus, int port, int reg)
 	u16 val;
 	int res;
 
-	if (port >= YT921X_PORT_NUM)
+	if (port >= priv->series->max_ports)
 		return U16_MAX;
 
 	mutex_lock(&priv->reg_lock);
@@ -133,7 +133,7 @@ yt921x_mbus_int_write(struct mii_bus *mbus, int port, int reg, u16 data)
 	struct yt921x_priv *priv = mbus->priv;
 	int res;
 
-	if (port >= YT921X_PORT_NUM)
+	if (port >= priv->series->max_ports)
 		return -ENODEV;
 
 	mutex_lock(&priv->reg_lock);
@@ -147,6 +147,7 @@ int yt921x_mbus_int_init(struct yt921x_priv *priv, struct device_node *mnp)
 {
 	struct device *dev = to_device(priv);
 	struct mii_bus *mbus;
+	u32 max_ports;
 	int res;
 
 	mbus = devm_mdiobus_alloc(dev);
@@ -159,7 +160,8 @@ int yt921x_mbus_int_init(struct yt921x_priv *priv, struct device_node *mnp)
 	mbus->read = yt921x_mbus_int_read;
 	mbus->write = yt921x_mbus_int_write;
 	mbus->parent = dev;
-	mbus->phy_mask = (u32)~GENMASK(YT921X_PORT_NUM - 1, 0);
+	max_ports = priv->series->max_ports;
+	mbus->phy_mask = (u32)~GENMASK(max_ports - 1, 0);
 
 	res = devm_of_mdiobus_register(dev, mbus, mnp);
 	if (res)
-- 
2.25.1

[PATCH net-next v11 3/7] net: dsa: motorcomm: use fixed-width GENMASK_U*() helpers

[Kyle Switch]

Replace the plain GENMASK() uses with the fixed-width GENMASK_U16()
and GENMASK_U32() variants.

Signed-off-by: Kyle Switch <kyle.switch@motor-comm.com>
---
 drivers/net/dsa/motorcomm/chip.c     | 12 ++++++------
 drivers/net/dsa/motorcomm/mdio_bus.c |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 79422def16ff..13fc286d4194 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -101,19 +101,19 @@ static const struct yt921x_mib_desc yt921x_mib_descs[] = {
 static const struct yt921x_info yt921x_infos[] = {
 	{
 		"YT9215SC", YT9215_MAJOR, 1, 0,
-		GENMASK(4, 0),
+		GENMASK_U16(4, 0),
 		BIT(9),
 		BIT(8) | BIT(9),
 	},
 	{
 		"YT9215S", YT9215_MAJOR, 2, 0,
-		GENMASK(4, 0),
+		GENMASK_U16(4, 0),
 		BIT(9),
 		BIT(8),
 	},
 	{
 		"YT9215RB", YT9215_MAJOR, 3, 0,
-		GENMASK(4, 0),
+		GENMASK_U16(4, 0),
 		BIT(8) | BIT(9),
 		0,
 	},
@@ -131,13 +131,13 @@ static const struct yt921x_info yt921x_infos[] = {
 	},
 	{
 		"YT9218N", YT9218_MAJOR, 0, 0,
-		GENMASK(7, 0),
+		GENMASK_U16(7, 0),
 		0,
 		0,
 	},
 	{
 		"YT9218MB", YT9218_MAJOR, 1, 0,
-		GENMASK(7, 0),
+		GENMASK_U16(7, 0),
 		BIT(8) | BIT(9),
 		BIT(8) | BIT(9),
 	},
@@ -4061,7 +4061,7 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
 	if (res)
 		return res;
 
-	ctrl = GENMASK(10, 0);
+	ctrl = GENMASK_U32(10, 0);
 	res = yt921x_reg_write(priv, YT921X_FILTER_UNK_UCAST, ctrl);
 	if (res)
 		return res;
diff --git a/drivers/net/dsa/motorcomm/mdio_bus.c b/drivers/net/dsa/motorcomm/mdio_bus.c
index c5a0f1bc1e36..1ac6a94f4f9d 100644
--- a/drivers/net/dsa/motorcomm/mdio_bus.c
+++ b/drivers/net/dsa/motorcomm/mdio_bus.c
@@ -161,7 +161,7 @@ int yt921x_mbus_int_init(struct yt921x_priv *priv, struct device_node *mnp)
 	mbus->write = yt921x_mbus_int_write;
 	mbus->parent = dev;
 	max_ports = priv->series->max_ports;
-	mbus->phy_mask = (u32)~GENMASK(max_ports - 1, 0);
+	mbus->phy_mask = ~GENMASK_U32(max_ports - 1, 0);
 
 	res = devm_of_mdiobus_register(dev, mbus, mnp);
 	if (res)
-- 
2.25.1

Re: [PATCH net-next v11 3/7] net: dsa: motorcomm: use fixed-width GENMASK_U*() helpers

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says what the patch does but not why, and its
  wording claims more than the patch does.

--

net: dsa: motorcomm: use fixed-width GENMASK_U*() helpers

This commit switches the GENMASK() call sites in chip.c and mdio_bus.c to
the fixed-width GENMASK_U16() and GENMASK_U32() helpers. The converted
values are unchanged, so the patch has no runtime effect.

> Replace the plain GENMASK() uses with the fixed-width GENMASK_U16()
> and GENMASK_U32() variants.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 79422def16ffb..13fc286d4194b 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -101,19 +101,19 @@ static const struct yt921x_mib_desc yt921x_mib_descs[] = {
>  static const struct yt921x_info yt921x_infos[] = {
>  	{
>  		"YT9215SC", YT9215_MAJOR, 1, 0,
> -		GENMASK(4, 0),
> +		GENMASK_U16(4, 0),
>  		BIT(9),
>  		BIT(8) | BIT(9),
>  	},

[ ... ]

[Severity: Low]
This isn't a bug, but does the commit message match what the patch does?

"Replace the plain GENMASK() uses" reads as if every plain GENMASK() in
the driver is converted. Only six call sites in chip.c and mdio_bus.c
change.

After this patch, drivers/net/dsa/motorcomm/ still has 153 plain GENMASK()
uses: 133 in chip.h, 16 in leds.h and 4 in mdio_bus.h. The mdio_bus.h ones
are the field masks for the mdio_bus.c file this patch edits:

drivers/net/dsa/motorcomm/mdio_bus.h:
#define  YT921X_MBUS_CTRL_PORT_M		GENMASK(25, 21)
#define  YT921X_MBUS_CTRL_REG_M			GENMASK(20, 16)
...

Even inside yt921x_infos[], only the GENMASK() values for the u16
internal_mask field are converted. The BIT() values for xmii_mask and
serdes_mask, which are also u16, stay as unsigned long BIT() constants.
So does internal_mask for YT9214NB and YT9213NB:

	{
		"YT9214NB", YT9215_MAJOR, 3, 2,
		BIT(1) | BIT(3),
		BIT(9),
		BIT(8),
	},

include/linux/bits.h already has BIT_U16() and BIT_U32() for these.

The later YT922x patch in this series ("net: dsa: motorcomm: Add support
for Motorcomm YT922x") also adds new plain GENMASK() defines to chip.h,
such as YT922X_PORT_SPEED_M and YT922X_SERDES_MODE_M. That means the
series never finishes the conversion.

Could the commit message explain why this conversion is wanted and why it
stops at these call sites? Could it also say that there is no functional
change?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002173706.1225502-1-kyle.switch%40motor-comm.com

[PATCH net-next v11 4/7] net: dsa: motorcomm: move mib start from probe() to dsa_setup()

[Kyle Switch]

Move the delayed work initialization of the per-port MIB polling
from yt921x_mdio_probe() into the DSA .setup() callback, changing
its lifetime from once per probe() to once per DSA .setup().

Signed-off-by: Kyle Switch <kyle.switch@motor-comm.com>
---
 drivers/net/dsa/motorcomm/chip.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 13fc286d4194..c7cfaf244274 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -4251,6 +4251,14 @@ static int yt921x_dsa_setup(struct dsa_switch *ds)
 	int port;
 	int res;
 
+	/* mib polling init */
+	for (size_t i = 0; i < ARRAY_SIZE(priv->ports); i++) {
+		struct yt921x_port *pp = &priv->ports[i];
+
+		pp->index = i;
+		INIT_DELAYED_WORK(&pp->mib_read, yt921x_poll_mib);
+	}
+
 	mutex_lock(&priv->reg_lock);
 	res = yt921x_chip_reset(priv);
 	mutex_unlock(&priv->reg_lock);
@@ -4525,13 +4533,6 @@ static int yt921x_mdio_probe(struct mdio_device *mdiodev)
 	priv->reg_ops = &yt921x_reg_ops_mdio;
 	priv->reg_ctx = mdio;
 
-	for (size_t i = 0; i < ARRAY_SIZE(priv->ports); i++) {
-		struct yt921x_port *pp = &priv->ports[i];
-
-		pp->index = i;
-		INIT_DELAYED_WORK(&pp->mib_read, yt921x_poll_mib);
-	}
-
 	ds = &priv->ds;
 	ds->dev = dev;
 	ds->priv = priv;
-- 
2.25.1

Re: [PATCH net-next v11 4/7] net: dsa: motorcomm: move mib start from probe() to dsa_setup()

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · 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).

- [High] In this commit, mib_read is re-initialized on every
  yt921x_dsa_setup(), but yt921x_dsa_teardown() only removes LEDs and…
- [Medium] In this commit, the only call to
  INIT_DELAYED_WORK(&pp->mib_read, yt921x_poll_mib) is inside…
- [Medium] In this commit, yt921x_mdio_remove() runs its
  disable_delayed_work_sync() loop before dsa_unregister_switch() and…
- [Low] The subject 'move mib start from probe() to dsa_setup()' does not
  match the code.

--

net: dsa: motorcomm: move mib start from probe() to dsa_setup()

This commit moves the per-port pp->index assignment and the
INIT_DELAYED_WORK() of the MIB polling work from yt921x_mdio_probe()
into yt921x_dsa_setup(). The works are now initialized once per DSA
.setup() instead of once per probe().

> net: dsa: motorcomm: move mib start from probe() to dsa_setup()

[Severity: Low]
This isn't a bug, but the subject says "mib start". Only the
INIT_DELAYED_WORK() moves. Polling actually starts in
yt921x_phylink_mac_link_up(), and this patch does not change that code:

	schedule_delayed_work(&priv->ports[port].mib_read, 0);

> Move the delayed work initialization of the per-port MIB polling
> from yt921x_mdio_probe() into the DSA .setup() callback, changing
> its lifetime from once per probe() to once per DSA .setup().

The body does not say why the move is needed. It also leaves out that
pp->index moves as well.

pp->index is not specific to MIB polling. yt921x_port_to_priv() depends
on it, and so do the PCS and LED code. Should the commit message mention
this? Should the assignment sit under a comment other than
"mib polling init"?

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 13fc286d4194b..c7cfaf2442749 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -4251,6 +4251,14 @@ static int yt921x_dsa_setup(struct dsa_switch *ds)
>  	int port;
>  	int res;
>  
> +	/* mib polling init */
> +	for (size_t i = 0; i < ARRAY_SIZE(priv->ports); i++) {
> +		struct yt921x_port *pp = &priv->ports[i];
> +
> +		pp->index = i;
> +		INIT_DELAYED_WORK(&pp->mib_read, yt921x_poll_mib);

[Severity: High]
Can this re-initialize a delayed work that is still armed?

INIT_DELAYED_WORK() now runs on every tree setup. In this commit,
yt921x_dsa_teardown() only removes the LEDs:

static void yt921x_dsa_teardown(struct dsa_switch *ds)
{
#if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
	struct yt921x_priv *priv = to_yt921x_priv(ds);

	yt921x_leds_remove(priv);
#endif
}

In a multi-switch tree, unbinding a peer switch tears down the whole
tree. That includes this switch, which stays bound:

dsa_unregister_switch() (peer)
  dsa_switch_remove()
    dsa_tree_teardown(dst)

When the peer rebinds, yt921x_dsa_setup() runs again on the same priv.

On the teardown path, the only thing that stops the work is the non-sync
cancel in yt921x_phylink_mac_link_down():

	/* No need to sync; port control block is hold until device remove */
	cancel_delayed_work(&priv->ports[port].mib_read);

yt921x_poll_mib() also always re-arms itself:

	schedule_delayed_work(&pp->mib_read, delay);

A poll can be running when that cancel happens, for example while it
waits on reg_lock, which link_down takes right after the cancel. The poll
then re-arms and the work is still pending after teardown.

The next INIT_DELAYED_WORK() would then reset a timer that is still
enqueued, and possibly a work that is still queued. Could that corrupt
the timer wheel or worklist linkage? Debugobjects would also report
"init active object".

Before this patch the init ran only once in probe, so a later setup
never touched a live work.

The next patch in the series, "net: dsa: motorcomm: move mib stop from
remove() to teardown()", adds disable_delayed_work_sync() for every port
to yt921x_dsa_teardown(). That fixes this in the final state of the
series. Could the two changes be merged so every commit bisects cleanly?

Separately, in that later patch priv looks like it is declared only under
CONFIG_NET_DSA_YT921X_LEDS in yt921x_dsa_teardown(). Does
yt921x_dsa_teardown() still build when the LED option is disabled?

[Severity: Medium]
Can this INIT_DELAYED_WORK() undo the disable that yt921x_mdio_remove()
just did?

In this commit, yt921x_mdio_remove() disables the works before calling
dsa_unregister_switch(), and it does so without holding dsa2_mutex:

	for (size_t i = ARRAY_SIZE(priv->ports); i-- > 0; ) {
		struct yt921x_port *pp = &priv->ports[i];

		disable_delayed_work_sync(&pp->mib_read);
	}

	dsa_unregister_switch(&priv->ds);

Another member of the same tree may re-register at the same time, after
an earlier unbind tore the tree down. If that completes the tree,
dsa_tree_setup() calls yt921x_dsa_setup() for this switch.

INIT_DELAYED_WORK() resets work->data, which clears the disable count
that remove() just set. yt921x_phylink_mac_link_up() can then arm the
works again.

The non-sync cancel in yt921x_phylink_mac_link_down() can also race with
a running yt921x_poll_mib(). Together, could a re-armed work survive
teardown (which has no cancel in this commit) and run after devm has
freed priv?

This needs two timing windows and root. Before this patch, though,
.setup() never touched the work state, so nothing could undo the disable
in remove().

The later patch "net: dsa: motorcomm: move mib stop from remove() to
teardown()" moves the disable into yt921x_dsa_teardown(). That runs
under dsa2_mutex, as dsa_tree_setup() does, so the race goes away by the
end of the series.

> +	}
> +
>  	mutex_lock(&priv->reg_lock);
>  	res = yt921x_chip_reset(priv);
>  	mutex_unlock(&priv->reg_lock);
> @@ -4525,13 +4533,6 @@ static int yt921x_mdio_probe(struct mdio_device *mdiodev)
>  	priv->reg_ops = &yt921x_reg_ops_mdio;
>  	priv->reg_ctx = mdio;
>  
> -	for (size_t i = 0; i < ARRAY_SIZE(priv->ports); i++) {
> -		struct yt921x_port *pp = &priv->ports[i];
> -
> -		pp->index = i;
> -		INIT_DELAYED_WORK(&pp->mib_read, yt921x_poll_mib);
> -	}
> -

[Severity: Medium]
After this removal, does yt921x_mdio_remove() call
disable_delayed_work_sync() on works that were never initialized?

The only INIT_DELAYED_WORK() for mib_read is now in yt921x_dsa_setup().
yt921x_mdio_probe() returns the result of dsa_register_switch(ds).

For an incomplete tree, dsa_tree_setup() returns success without calling
.setup(). An example is a multi-switch tree whose peer has not registered
yet:

net/dsa/dsa.c:dsa_tree_setup() {
    ...
	complete = dsa_tree_setup_routing_table(dst);
	if (!complete)
		return 0;
    ...
}

If the device is unbound in that state, yt921x_mdio_remove() still loops
over every port and calls this on a delayed_work that devm_kzalloc() left
zeroed:

		disable_delayed_work_sync(&pp->mib_read);

The call goes through __cancel_work_sync()->__flush_work(), which hits
WARN_ON(!work->func) once per port. Debugobjects may also report the
timer_delete() on the uninitialized timer.

The next patch, "net: dsa: motorcomm: move mib stop from remove() to
teardown()", takes this loop out of remove() and puts it in
yt921x_dsa_teardown(). dsa_switch_teardown() only calls that when
ds->setup is set, so the problem is limited to the window between the
two patches. Could the init stay in probe until the stop side moves too?

>  	ds = &priv->ds;
>  	ds->dev = dev;
>  	ds->priv = priv;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002173706.1225502-1-kyle.switch%40motor-comm.com

[PATCH net-next v11 5/7] net: dsa: motorcomm: move mib stop from remove() to teardown()

[Kyle Switch]

This change ensures symmetry with the MIB start operation,
which is already invoked in the setup(). Additionally, it
consolidates per-switch operation into the appropriate DSA ops,
improving code organization and maintainability.

Signed-off-by: Kyle Switch <kyle.switch@motor-comm.com>
---
 drivers/net/dsa/motorcomm/chip.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index c7cfaf244274..9380d74d79ba 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -3924,6 +3924,12 @@ static void yt921x_dsa_teardown(struct dsa_switch *ds)
 
 	yt921x_leds_remove(priv);
 #endif
+
+	for (size_t i = ARRAY_SIZE(priv->ports); i-- > 0; ) {
+		struct yt921x_port *pp = &priv->ports[i];
+
+		disable_delayed_work_sync(&pp->mib_read);
+	}
 }
 
 static int yt921x_chip_detect(struct yt921x_priv *priv)
@@ -4483,12 +4489,6 @@ static void yt921x_mdio_remove(struct mdio_device *mdiodev)
 	if (!priv)
 		return;
 
-	for (size_t i = ARRAY_SIZE(priv->ports); i-- > 0; ) {
-		struct yt921x_port *pp = &priv->ports[i];
-
-		disable_delayed_work_sync(&pp->mib_read);
-	}
-
 	dsa_unregister_switch(&priv->ds);
 
 	for (unsigned int i = 0; i < ARRAY_SIZE(priv->acl_blks); i++) {
-- 
2.25.1

Re: [PATCH net-next v11 5/7] net: dsa: motorcomm: move mib stop from remove() to teardown()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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] Build failure when CONFIG_NET_DSA_YT921X_LEDS=n.

--

net: dsa: motorcomm: move mib stop from remove() to teardown()

This commit moves the loop that calls disable_delayed_work_sync() on each
port's mib_read work out of yt921x_mdio_remove() and into
yt921x_dsa_teardown(). The goal is to match the MIB start, which already
happens in setup().

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index c7cfaf2442749..9380d74d79ba2 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -3924,6 +3924,12 @@ static void yt921x_dsa_teardown(struct dsa_switch *ds)
>  
>  	yt921x_leds_remove(priv);
>  #endif
> +
> +	for (size_t i = ARRAY_SIZE(priv->ports); i-- > 0; ) {
> +		struct yt921x_port *pp = &priv->ports[i];
> +
> +		disable_delayed_work_sync(&pp->mib_read);
> +	}
>  }

[Severity: Medium]
Will this build with CONFIG_NET_DSA_YT921X_LEDS=n? In yt921x_dsa_teardown(),
priv is only declared inside the #if block:

static void yt921x_dsa_teardown(struct dsa_switch *ds)
{
#if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
	struct yt921x_priv *priv = to_yt921x_priv(ds);

	yt921x_leds_remove(priv);
#endif

	for (size_t i = ARRAY_SIZE(priv->ports); i-- > 0; ) {
	...

The new loop sits after the #endif and uses priv->ports. With LED support
disabled, priv is undeclared and chip.c fails to compile.

That configuration looks reachable. NET_DSA_YT921X_LEDS is a user-visible
bool that depends on "LEDS_CLASS=y || LEDS_CLASS=NET_DSA_YT921X". So it is
off when the user turns it off, when LEDS_CLASS=n, or when LEDS_CLASS=m with
NET_DSA_YT921X=y.

The old loop in yt921x_mdio_remove() used a priv that is declared
unconditionally, so the failure starts with this commit. The code is still
the same in the last patch of the series ("net: dsa: motorcomm: Add support
for Motorcomm YT922x"), so the series as a whole does not build in this
configuration either.

Could priv be declared unconditionally, with only the yt921x_leds_remove()
call left under the #if? The #if could also be dropped entirely, because
leds.h already provides a no-op inline stub for yt921x_leds_remove() when
LED support is disabled.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002173706.1225502-1-kyle.switch%40motor-comm.com

[PATCH net-next v11 6/7] net: dsa: tag_yt922x: add support for Motorcomm YT922x tags

[Kyle Switch]

Add support for Motorcomm YT922x tags with 8bytes. which includes
ethertype field (default to 0x9988).

Signed-off-by: Kyle Switch <kyle.switch@motor-comm.com>
---
 MAINTAINERS          |   2 +
 include/net/dsa.h    |   2 +
 net/dsa/Kconfig      |   6 +++
 net/dsa/Makefile     |   1 +
 net/dsa/tag_yt922x.c | 109 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 120 insertions(+)
 create mode 100644 net/dsa/tag_yt922x.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3011f995437f..1a48e4570c69 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18433,11 +18433,13 @@ F:	drivers/net/phy/motorcomm.c
 
 MOTORCOMM YT921X ETHERNET SWITCH DRIVER
 M:	David Yang <mmyangfl@gmail.com>
+M:	Kyle Switch <kyle.switch@motor-comm.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/net/dsa/motorcomm,yt921x.yaml
 F:	drivers/net/dsa/motorcomm/
 F:	net/dsa/tag_yt921x.c
+F:	net/dsa/tag_yt922x.c
 
 MOXA SMARTIO/INDUSTIO/INTELLIO SERIAL CARD
 M:	Jiri Slaby <jirislaby@kernel.org>
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 5d12191b6f6f..5a173fe2bb87 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -62,6 +62,7 @@ struct tc_action;
 #define DSA_TAG_PROTO_KSZ8463_VALUE		34
 #define DSA_TAG_PROTO_MT7628_VALUE		35
 #define DSA_TAG_PROTO_KS8995_VALUE		36
+#define DSA_TAG_PROTO_YT922X_VALUE		37
 
 enum dsa_tag_protocol {
 	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
@@ -101,6 +102,7 @@ enum dsa_tag_protocol {
 	DSA_TAG_PROTO_KSZ8463		= DSA_TAG_PROTO_KSZ8463_VALUE,
 	DSA_TAG_PROTO_MT7628		= DSA_TAG_PROTO_MT7628_VALUE,
 	DSA_TAG_PROTO_KS8995		= DSA_TAG_PROTO_KS8995_VALUE,
+	DSA_TAG_PROTO_YT922X		= DSA_TAG_PROTO_YT922X_VALUE,
 };
 
 struct dsa_switch;
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 4f44bf3ede23..86802058678c 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -233,4 +233,10 @@ config NET_DSA_TAG_YT921X
 	  Say Y or M if you want to enable support for tagging frames for
 	  Motorcomm YT921x switches.
 
+config NET_DSA_TAG_YT922X
+  tristate "Tag driver for Motorcomm YT922x switches"
+  help
+    Say Y or M if you want to enable support for tagging frames for
+    Motorcomm YT922x switches.
+
 endif
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 1f9cc30e9988..0ef6dfce3b92 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
 obj-$(CONFIG_NET_DSA_TAG_VSC73XX_8021Q) += tag_vsc73xx_8021q.o
 obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o
 obj-$(CONFIG_NET_DSA_TAG_YT921X) += tag_yt921x.o
+obj-$(CONFIG_NET_DSA_TAG_YT922X) += tag_yt922x.o
 
 # for tracing framework to find trace.h
 CFLAGS_trace.o := -I$(src)
diff --git a/net/dsa/tag_yt922x.c b/net/dsa/tag_yt922x.c
new file mode 100644
index 000000000000..3c9fef651cf5
--- /dev/null
+++ b/net/dsa/tag_yt922x.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Motorcomm YT922x Switch Extended CPU Port Tagging
+ *
+ * Copyright (c) 2026 Kyle switch <kyle.switch@motor-comm.com>
+ *
+ */
+
+#include <linux/etherdevice.h>
+
+#include "tag.h"
+
+#define YT922X_TAG_LEN 8
+
+/*
+ * To define the from cpu tag format 8 bytes.
+ */
+#define YT922X_TAG_NAME		"yt922x"
+#define YT922X_TAG_PORTMASK_0	BIT(15)
+#define YT922X_TAG_PORTMASK_M	GENMASK(8, 0)
+#define  YT922X_TAG_PORTS(x)		FIELD_PREP(YT922X_TAG_PORTMASK_M, (x))
+#define YT922X_TAG_FORCE_DST	BIT(9)
+#define YT922X_TAG_PRIO_M	GENMASK(12, 10)
+#define YT922X_TAG_PRIO_EN	BIT(13)
+#define  YT922X_TAG_PRIO(x)	(FIELD_PREP(YT922X_TAG_PRIO_M, (x)) | YT922X_TAG_PRIO_EN)
+#define YT922X_TAG_RX_PORT_M	GENMASK(5, 2)
+
+static struct sk_buff *
+yt922x_tag_xmit(struct sk_buff *skb, struct net_device *netdev)
+{
+	unsigned long ports;
+	__be16 *tag;
+	u16 ctrl;
+
+	skb_push(skb, YT922X_TAG_LEN);
+	dsa_alloc_etype_header(skb, YT922X_TAG_LEN);
+	tag = dsa_etype_header_pos_tx(skb);
+
+	tag[0] = htons(ETH_P_YT921X);
+	ports = dsa_xmit_port_mask(skb, netdev);
+	/*To fill in the case where the port index is not 0 */
+	ctrl = YT922X_TAG_PRIO(skb->priority) | YT922X_TAG_FORCE_DST |
+	       YT922X_TAG_PORTS(ports >> 1);
+	tag[1] = htons(ctrl);
+	if (ports & BIT(0)) {
+		/* To fill in the case where the port index is 0 */
+		ctrl = YT922X_TAG_PORTMASK_0;
+		tag[2] = htons(ctrl);
+	} else {
+		tag[2] = 0;
+	}
+	tag[3] = 0;
+
+	return skb;
+}
+
+static struct sk_buff *
+yt922x_tag_rcv(struct sk_buff *skb, struct net_device *netdev)
+{
+	unsigned int port;
+	__be16 *tag;
+	u16 rx;
+
+	if (unlikely(!pskb_may_pull(skb, YT922X_TAG_LEN))) {
+		kfree_skb(skb);
+		return NULL;
+	}
+
+	tag = dsa_etype_header_pos_rx(skb);
+
+	if (unlikely(tag[0] != htons(ETH_P_YT921X))) {
+		dev_warn_ratelimited(&netdev->dev,
+				     "Unexpected EtherType 0x%04x\n",
+				     ntohs(tag[0]));
+		kfree_skb(skb);
+		return NULL;
+	}
+
+	/* Locate which port this is coming from */
+	rx = ntohs(tag[2]);
+	port = FIELD_GET(YT922X_TAG_RX_PORT_M, rx);
+	skb->dev = dsa_conduit_find_user(netdev, 0, port);
+	if (unlikely(!skb->dev)) {
+		dev_warn_ratelimited(&netdev->dev,
+				     "Couldn't decode source port %u\n", port);
+		kfree_skb(skb);
+		return NULL;
+	}
+
+	/* Remove tag and update checksum */
+	skb_pull_rcsum(skb, YT922X_TAG_LEN);
+	dsa_strip_etype_header(skb, YT922X_TAG_LEN);
+
+	return skb;
+}
+
+static const struct dsa_device_ops yt922x_netdev_ops = {
+	.name = YT922X_TAG_NAME,
+	.proto = DSA_TAG_PROTO_YT922X,
+	.xmit = yt922x_tag_xmit,
+	.rcv = yt922x_tag_rcv,
+	.needed_headroom = YT922X_TAG_LEN,
+};
+
+MODULE_DESCRIPTION("DSA tag driver for Motorcomm YT922x switches");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_YT922X, YT922X_TAG_NAME);
+
+module_dsa_tag_driver(yt922x_netdev_ops);
-- 
2.25.1

Re: [PATCH net-next v11 6/7] net: dsa: tag_yt922x: add support for Motorcomm YT922x tags

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message for "net: dsa: tag_yt922x: add support for
  Motorcomm YT922x tags" is one line ('Add support for Motorcomm YT922x…
- [Low] Concern: yt922x_tag_rcv() never sets skb->offload_fwd_mark, so
  the software bridge would duplicate frames the hardware already…
- [Low] The new NET_DSA_TAG_YT922X entry in net/dsa/Kconfig is indented
  with spaces: 2 spaces before 'tristate' and 'help', and 4 spaces…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the new
  yt922x_tag_rcv() calls dsa_conduit_find_user(netdev, 0, port) and so…

--

net: dsa: tag_yt922x: add support for Motorcomm YT922x tags

This commit adds a DSA tag driver for Motorcomm YT922x switches. The tag
is 8 bytes, EtherType based, and uses the same EtherType as YT921x
(0x9988). The commit also adds DSA_TAG_PROTO_YT922X and the Kconfig and
Makefile entries, and updates the existing YT921x MAINTAINERS entry.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3011f995437fd..1a48e4570c698 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18433,11 +18433,13 @@ F:	drivers/net/phy/motorcomm.c
>  
>  MOTORCOMM YT921X ETHERNET SWITCH DRIVER
>  M:	David Yang <mmyangfl@gmail.com>
> +M:	Kyle Switch <kyle.switch@motor-comm.com>

[Severity: Low]
This isn't a bug, but the commit message is one sentence:

  Add support for Motorcomm YT922x tags with 8bytes. which includes
  ethertype field (default to 0x9988).

It doesn't mention this new M: line. The existing entry already has a
maintainer and covers drivers/net/dsa/motorcomm/, the DT binding and
tag_yt921x.c. Should the co-maintainer addition be a separate patch with
an ack from David Yang?

The commit message also doesn't say why YT922x needs a new tag protocol.
It uses the same ETH_P_YT921X EtherType and the same 8-byte length as
tag_yt921x. Could that be explained?

In net/dsa/tag_yt922x.c, the only description of the wire format is:

  /*
   * To define the from cpu tag format 8 bytes.
   */

The to-CPU layout isn't described at all. tag_yt921x.c, by contrast, has
a full bit layout diagram. Could both directions be documented here?

Some examples are that bits 5:2 of tag[2] carry the RX source port, and
that bit 15 of tag[2] selects TX port 0. It would also help to say why
the RX path doesn't check a valid bit, restore the priority or decode a
forward code.

>  L:	netdev@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/net/dsa/motorcomm,yt921x.yaml
>  F:	drivers/net/dsa/motorcomm/
>  F:	net/dsa/tag_yt921x.c
> +F:	net/dsa/tag_yt922x.c

[ ... ]

> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index 4f44bf3ede238..86802058678c0 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -233,4 +233,10 @@ config NET_DSA_TAG_YT921X
>  	  Say Y or M if you want to enable support for tagging frames for
>  	  Motorcomm YT921x switches.
>  
> +config NET_DSA_TAG_YT922X
> +  tristate "Tag driver for Motorcomm YT922x switches"
> +  help
> +    Say Y or M if you want to enable support for tagging frames for
> +    Motorcomm YT922x switches.

[Severity: Low]
This isn't a bug, but this entry is indented with spaces. The
NET_DSA_TAG_YT921X entry just above it, and the rest of net/dsa/Kconfig,
indent attributes with one tab and help text with a tab plus two spaces.

[ ... ]

> diff --git a/net/dsa/tag_yt922x.c b/net/dsa/tag_yt922x.c
> new file mode 100644
> index 0000000000000..3c9fef651cf51
> --- /dev/null
> +++ b/net/dsa/tag_yt922x.c

[ ... ]

> +static struct sk_buff *
> +yt922x_tag_rcv(struct sk_buff *skb, struct net_device *netdev)
> +{

[ ... ]

> +	/* Locate which port this is coming from */
> +	rx = ntohs(tag[2]);
> +	port = FIELD_GET(YT922X_TAG_RX_PORT_M, rx);
> +	skb->dev = dsa_conduit_find_user(netdev, 0, port);

[Severity: High]
This isn't a bug introduced by this patch, but the new tagger inherits an
existing race in the DSA receive path. Can dsa_conduit_find_user() see a
NULL dev->dsa_ptr here?

dsa_switch_rcv() checks the pointer once, before it calls the tagger:

net/dsa/tag.c:dsa_switch_rcv() {
    struct dsa_port *cpu_dp = dev->dsa_ptr;
    ...
    if (unlikely(!cpu_dp)) {
        kfree_skb(skb);
        return 0;
    }
    ...
}

dsa_conduit_find_user() then reloads it and dereferences it without a
check:

net/dsa/tag.h:dsa_conduit_find_user() {
    struct dsa_port *cpu_dp = dev->dsa_ptr;
    struct dsa_switch_tree *dst = cpu_dp->dst;
    ...
}

Meanwhile, teardown clears the pointer under RTNL while the conduit can
still be receiving frames:

dsa_unregister_switch()
  dsa_tree_teardown()
    dsa_tree_teardown_conduit()
      dsa_conduit_teardown()
        dev->dsa_ptr = NULL;
        wmb();

The wmb() only orders the store. Nothing like synchronize_net() waits for
receive callbacks that are already running.

Suppose the store lands after the check in dsa_switch_rcv() but before
the reload in dsa_conduit_find_user(). Would the NET_RX softirq then
dereference NULL->dst? Every tagger that uses dsa_conduit_find_user() has
the same pattern.

> +	if (unlikely(!skb->dev)) {
> +		dev_warn_ratelimited(&netdev->dev,
> +				     "Couldn't decode source port %u\n", port);
> +		kfree_skb(skb);
> +		return NULL;
> +	}
> +
> +	/* Remove tag and update checksum */
> +	skb_pull_rcsum(skb, YT922X_TAG_LEN);
> +	dsa_strip_etype_header(skb, YT922X_TAG_LEN);
> +
> +	return skb;
> +}

[Severity: Low]
This isn't a bug with the series as it stands, but yt922x_tag_rcv() never
sets skb->offload_fwd_mark.

That is fine today. yt922x_dsa_switch_ops in
drivers/net/dsa/motorcomm/chip.c has no .port_bridge_join, so
dsa_user_changeupper() falls back to a software bridge.

yt922x_port_setup() also isolates each user port so that it can only
reach the CPU port:

	ctrl = ~priv->cpu_ports_mask;
	res = yt921x_reg_write(priv, YT922X_PORTn_ISOLATION(port), ctrl);

If bridge offload is added later, would this tagger have to decode the
hardware forward or reason code and set offload_fwd_mark, as tag_yt921x
does? Otherwise the bridge could forward duplicate frames.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002173706.1225502-1-kyle.switch%40motor-comm.com

[PATCH net-next v11 7/7] net: dsa: motorcomm: Add support for Motorcomm YT922x

[Kyle Switch]

Add support for Motorcomm YT922X, which is series of
ethernet switches developed by Motorcomm Electronic
Technology, includes YT9224 and YT9228.

This patch only adds support for the YT9224 variant.
YT9228 is not supported yet.

This patch adds basic support for a working DSA switch,
includes .port_setup, .setup, .phylink_get_caps,
.get_tag_protocol.

Signed-off-by: Kyle Switch <kyle.switch@motor-comm.com>
---
 drivers/net/dsa/motorcomm/Kconfig    |   7 +-
 drivers/net/dsa/motorcomm/Makefile   |   1 +
 drivers/net/dsa/motorcomm/chip.c     | 453 ++++++++++++++++++++++++++-
 drivers/net/dsa/motorcomm/chip.h     |  91 ++++++
 drivers/net/dsa/motorcomm/mdio_bus.c |  39 +++
 drivers/net/dsa/motorcomm/mdio_bus.h |   2 +
 drivers/net/dsa/motorcomm/pcs-922x.c | 173 ++++++++++
 drivers/net/dsa/motorcomm/pcs.h      |   1 +
 8 files changed, 762 insertions(+), 5 deletions(-)
 create mode 100644 drivers/net/dsa/motorcomm/pcs-922x.c

diff --git a/drivers/net/dsa/motorcomm/Kconfig b/drivers/net/dsa/motorcomm/Kconfig
index 79cdd79a1fd2..648c49d6c527 100644
--- a/drivers/net/dsa/motorcomm/Kconfig
+++ b/drivers/net/dsa/motorcomm/Kconfig
@@ -1,11 +1,12 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config NET_DSA_YT921X
-	tristate "Motorcomm YT9215 ethernet switch chip support"
+	tristate "Motorcomm YT9215 and YT9224 ethernet switch chip support"
 	select NET_DSA_TAG_YT921X
+	select NET_DSA_TAG_YT922X
 	select NET_IEEE8021Q_HELPERS if DCB
 	help
-	  This enables support for the Motorcomm YT9215 ethernet switch
-	  chip.
+	  This enables support for the Motorcomm YT9215 and YT9224
+	  ethernet switch chip.
 
 config NET_DSA_YT921X_LEDS
 	bool "LED support for Motorcomm YT9215"
diff --git a/drivers/net/dsa/motorcomm/Makefile b/drivers/net/dsa/motorcomm/Makefile
index 1d2c1b3064c4..d1cd7c8a3852 100644
--- a/drivers/net/dsa/motorcomm/Makefile
+++ b/drivers/net/dsa/motorcomm/Makefile
@@ -3,5 +3,6 @@ obj-$(CONFIG_NET_DSA_YT921X) += yt921x.o
 yt921x-objs := chip.o
 yt921x-$(CONFIG_NET_DSA_YT921X_LEDS) += leds.o
 yt921x-objs += mdio_bus.o
+yt921x-objs += pcs-922x.o
 yt921x-objs += pcs-921x.o
 yt921x-objs += smi.o
diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 9380d74d79ba..175855cbc237 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
- * Driver for Motorcomm YT921x Switch
+ * Driver for Motorcomm YT921x and YT922x Switch
  *
  * Should work on YT9213/YT9214/YT9215/YT9218, but only tested on YT9215+SGMII,
  * be sure to do your own checks before porting to another chip.
@@ -141,6 +141,18 @@ static const struct yt921x_info yt921x_infos[] = {
 		BIT(8) | BIT(9),
 		BIT(8) | BIT(9),
 	},
+	{
+		"YT9224", YT9224_MAJOR, 0, 0,
+		GENMASK_U16(7, 4),
+		0,
+		BIT(8) | BIT(9),
+	},
+	{
+		"YT9228", YT9224_MAJOR, 0, 0,
+		GENMASK_U16(7, 4),
+		0,
+		BIT(8) | GENMASK_U16(3, 0),
+	},
 	{}
 };
 
@@ -3932,6 +3944,11 @@ static void yt921x_dsa_teardown(struct dsa_switch *ds)
 	}
 }
 
+static bool yt921x_needs_extmode_check(u32 major)
+{
+	return (major == YT9224_MAJOR) ? false : true;
+}
+
 static int yt921x_chip_detect(struct yt921x_priv *priv)
 {
 	struct device *dev = to_device(priv);
@@ -3957,6 +3974,9 @@ static int yt921x_chip_detect(struct yt921x_priv *priv)
 		return -ENODEV;
 	}
 
+	if (!yt921x_needs_extmode_check(major))
+		goto skip_extmode_check;
+
 	res = yt921x_reg_read(priv, YT921X_CHIP_MODE, &mode);
 	if (res)
 		return res;
@@ -3996,6 +4016,14 @@ static int yt921x_chip_detect(struct yt921x_priv *priv)
 
 	priv->info = info;
 
+	return 0;
+
+skip_extmode_check:
+	dev_info(dev,
+		 "Motorcomm %s ethernet switch, chipid: 0x%x\n",
+		 info->name, chipid);
+	priv->info = info;
+
 	return 0;
 }
 
@@ -4411,6 +4439,412 @@ static const struct dsa_switch_ops yt921x_dsa_switch_ops = {
 	.setup			= yt921x_dsa_setup,
 };
 
+static int yt922x_port_down(struct yt921x_priv *priv, int port)
+{
+	u32 mask;
+	int res;
+
+	/* mac force down */
+	mask = YT922X_PORT_LINK | YT922X_PORT_RX_MAC_EN |
+	YT922X_PORT_TX_MAC_EN | YT922X_PORT_LINK_AN;
+	res = yt921x_reg_clear_bits(priv, YT922X_PORTn_CTRL(port), mask);
+	if (res)
+		return res;
+	/* Need force op to make soft configuration effective */
+	mask = YT922X_PORT_FORCE_OP;
+	res = yt921x_reg_set_bits(priv, YT922X_PORTn_CTRL(port), mask);
+	if (res)
+		return res;
+
+	/* disable en_phy */
+	res = yt921x_reg_clear_bits(priv, YT922X_EN_PHY_VALUE, BIT(port));
+	if (res)
+		return res;
+	res = yt921x_reg_set_bits(priv, YT922X_EN_PHY_OVERWRITE, BIT(port));
+	if (res)
+		return res;
+
+	return 0;
+}
+
+static void
+yt922x_phylink_mac_link_down(struct phylink_config *config, unsigned int mode,
+			     phy_interface_t interface)
+{
+	struct dsa_port *dp = dsa_phylink_to_port(config);
+	struct yt921x_priv *priv = to_yt921x_priv(dp->ds);
+	int port = dp->index;
+	int res;
+
+	mutex_lock(&priv->reg_lock);
+	res = yt922x_port_down(priv, port);
+	mutex_unlock(&priv->reg_lock);
+
+	if (res)
+		dev_err(dp->ds->dev, "Failed to %s port %d: %i\n", "bring down",
+			port, res);
+}
+
+static int
+yt922x_port_up(struct yt921x_priv *priv, int port, unsigned int mode,
+	       phy_interface_t interface, int speed, int duplex,
+	       bool tx_pause, bool rx_pause)
+{
+	int ps = ethtool_speed_to_yt921x(speed);
+	u32 ctrl = 0;
+	u32 mask;
+	int res;
+
+	if (ps == YT921X_SPEED_INVALID)
+		return -EINVAL;
+	ctrl |= ps;
+
+	if (duplex == DUPLEX_FULL)
+		ctrl |= YT922X_PORT_DUPLEX_FULL;
+	if (tx_pause)
+		ctrl |= YT922X_PORT_TX_PAUSE;
+	if (rx_pause)
+		ctrl |= YT922X_PORT_RX_PAUSE;
+	ctrl |= YT922X_PORT_RX_MAC_EN | YT922X_PORT_TX_MAC_EN |
+			YT922X_PORT_CFG_TX_EN | YT922X_PORT_LINK |
+			YT922X_PORT_CFG_RX_EN;
+	ctrl &= ~(YT922X_PORT_FC_AN | YT922X_PORT_LINK_AN);
+	res = yt921x_reg_write(priv, YT922X_PORTn_CTRL(port), ctrl);
+	if (res)
+		return res;
+
+	/* force op */
+	mask = YT922X_PORT_FORCE_OP;
+	res = yt921x_reg_set_bits(priv, YT922X_PORTn_CTRL(port), mask);
+	if (res)
+		return res;
+
+	/* enable en_phy */
+	res = yt921x_reg_set_bits(priv, YT922X_EN_PHY_VALUE, BIT(port));
+	if (res)
+		return res;
+	res = yt921x_reg_set_bits(priv, YT922X_EN_PHY_OVERWRITE, BIT(port));
+	if (res)
+		return res;
+
+	return 0;
+}
+
+static void
+yt922x_phylink_mac_link_up(struct phylink_config *config,
+			   struct phy_device *phydev, unsigned int mode,
+			   phy_interface_t interface, int speed, int duplex,
+			   bool tx_pause, bool rx_pause)
+{
+	struct dsa_port *dp = dsa_phylink_to_port(config);
+	struct yt921x_priv *priv = to_yt921x_priv(dp->ds);
+	int port = dp->index;
+	int res;
+
+	mutex_lock(&priv->reg_lock);
+	res = yt922x_port_up(priv, port, mode, interface, speed, duplex,
+			     tx_pause, rx_pause);
+	mutex_unlock(&priv->reg_lock);
+
+	if (res)
+		dev_err(dp->ds->dev, "Failed to %s port %d: %i\n", "bring up",
+			port, res);
+}
+
+static void
+yt922x_phylink_mac_config(struct phylink_config *config, unsigned int mode,
+			  const struct phylink_link_state *state)
+{
+}
+
+static struct phylink_pcs *
+yt922x_phylink_mac_select_pcs(struct phylink_config *config,
+			      phy_interface_t interface)
+{
+	struct dsa_port *dp = dsa_phylink_to_port(config);
+	struct yt921x_priv *priv = to_yt921x_priv(dp->ds);
+
+	switch (interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_2500BASEX:
+	case PHY_INTERFACE_MODE_USXGMII:
+		return &priv->ports[dp->index].pcs;
+
+	default:
+		return NULL;
+	}
+}
+
+static const struct phylink_mac_ops yt922x_phylink_mac_ops = {
+	.mac_select_pcs = yt922x_phylink_mac_select_pcs,
+	.mac_link_down = yt922x_phylink_mac_link_down,
+	.mac_link_up = yt922x_phylink_mac_link_up,
+	.mac_config = yt922x_phylink_mac_config,
+};
+
+static enum dsa_tag_protocol
+yt922x_dsa_get_tag_protocol(struct dsa_switch *ds, int port,
+			    enum dsa_tag_protocol m)
+{
+	return DSA_TAG_PROTO_YT922X;
+}
+
+static void
+yt922x_dsa_phylink_get_caps(struct dsa_switch *ds, int port,
+			    struct phylink_config *config)
+{
+	struct yt921x_priv *priv = to_yt921x_priv(ds);
+	const struct yt921x_info *info = priv->info;
+
+	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
+				   MAC_10 | MAC_100 | MAC_1000;
+
+	if (info->internal_mask & BIT(port)) {
+		/* port 4 to port 7, internal utp */
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
+		config->mac_capabilities |= MAC_2500FD;
+	}
+	if (info->serdes_mask & BIT(port)) {
+		/* serdes */
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
+			  config->supported_interfaces);
+		config->mac_capabilities |= MAC_2500FD;
+		__set_bit(PHY_INTERFACE_MODE_USXGMII,
+			  config->supported_interfaces);
+		config->mac_capabilities |= MAC_5000FD;
+		config->mac_capabilities |= MAC_10000FD;
+	}
+}
+
+static int yt922x_port_setup(struct yt921x_priv *priv, int port)
+{
+	struct dsa_switch *ds = &priv->ds;
+	u32 mask;
+	u32 ctrl;
+	int res;
+
+	/* enable user port isolation and disable fdb learning */
+	ctrl = ~priv->cpu_ports_mask;
+	res = yt921x_reg_write(priv, YT922X_PORTn_ISOLATION(port), ctrl);
+	if (res)
+		return res;
+
+	mask = YT922X_PORT_LEARN_DIS;
+	res = yt921x_reg_set_bits(priv, YT922X_PORTn_LEARN(port), mask);
+	if (res)
+		return res;
+
+	if (dsa_is_cpu_port(ds, port)) {
+		ctrl = ~(u32)0;
+		res = yt921x_reg_write(priv, YT922X_PORTn_ISOLATION(port),
+				       ctrl);
+		if (res)
+			return res;
+	}
+
+	return 0;
+}
+
+static int yt922x_dsa_port_setup(struct dsa_switch *ds, int port)
+{
+	struct yt921x_priv *priv = to_yt921x_priv(ds);
+	int res;
+
+	mutex_lock(&priv->reg_lock);
+	res = yt922x_port_setup(priv, port);
+	mutex_unlock(&priv->reg_lock);
+
+	return res;
+}
+
+static int yt922x_cpu_tag_mode_set_8b(struct yt921x_priv *priv)
+{
+	u32 val;
+	u32 val1;
+	int res;
+
+	/* cpu tag mode set to 8b */
+	res = yt921x_reg_read(priv, YT922X_CPU_TAG_RX_CTRL, &val);
+	if (res)
+		return res;
+	res = yt921x_reg_read(priv, YT922X_CPU_TAG_TX_CTRL, &val1);
+	if (res)
+		return res;
+	val &= ~YT922X_CPU_TAG_RX_MODE;
+	val1 &= ~YT922X_CPU_TAG_TX_MODE;
+	val1 &= ~YT922X_CPU_TAG_TX_TYPE;
+	res = yt921x_reg_write(priv, YT922X_CPU_TAG_RX_CTRL, val);
+	if (res)
+		return res;
+	res = yt921x_reg_write(priv, YT922X_CPU_TAG_TX_CTRL, val1);
+	if (res)
+		return res;
+
+	return 0;
+}
+
+static int yt922x_cpu_port_set(struct yt921x_priv *priv)
+{
+	struct dsa_switch *ds = &priv->ds;
+	u32 ctrl;
+	int res;
+
+	/* cpu tag mode */
+	res = yt922x_cpu_tag_mode_set_8b(priv);
+	if (res)
+		return res;
+
+	/* Enable DSA */
+	priv->cpu_ports_mask = dsa_cpu_ports(ds);
+	ctrl = YT921X_EXT_CPU_PORT_TAG_EN | YT921X_EXT_CPU_PORT_PORT_EN |
+	       YT921X_EXT_CPU_PORT_PORT(__ffs(priv->cpu_ports_mask));
+	res = yt921x_reg_write(priv, YT921X_EXT_CPU_PORT, ctrl);
+	if (res)
+		return res;
+
+	/* Setup software switch */
+	ctrl = YT922X_CPU_COPY_TO_EXT_CPU;
+	res = yt921x_reg_write(priv, YT922X_CPU_COPY, ctrl);
+	if (res)
+		return res;
+
+	return res;
+}
+
+static int yt922x_chip_setup_dsa(struct yt921x_priv *priv)
+{
+	unsigned long cpu_ports_mask;
+	u32 ctrl;
+	int port;
+	int res;
+
+	/* cpu port set */
+	res = yt922x_cpu_port_set(priv);
+	if (res)
+		return res;
+
+	ctrl = GENMASK_U32(8, 0);
+	res = yt921x_reg_write(priv, YT922X_FILTER_UNK_UCAST, ctrl);
+	if (res)
+		return res;
+
+	ctrl = 0;
+	for (int i = 0; i < priv->series->max_ports; i++)
+		ctrl |= YT922X_ACT_UNK_ACTn_TRAP(i);
+	cpu_ports_mask = priv->cpu_ports_mask;
+	for_each_set_bit(port, &cpu_ports_mask, priv->series->max_ports) {
+		ctrl &= ~YT922X_ACT_UNK_ACTn_M(port);
+		ctrl |= YT922X_ACT_UNK_ACTn_DROP(port);
+	}
+	res = yt921x_reg_write(priv, YT922X_ACT_UNK_UCAST, ctrl);
+	if (res)
+		return res;
+	res = yt921x_reg_write(priv, YT922X_ACT_UNK_MCAST, ctrl);
+	if (res)
+		return res;
+
+	return 0;
+}
+
+static int yt922x_chip_setup(struct yt921x_priv *priv)
+{
+	u32 ctrl;
+	int res;
+
+	ctrl = YT922X_FUNC_MIB | YT922X_FUNC_ACL;
+	res = yt921x_reg_set_bits(priv, YT921X_FUNC, ctrl);
+	if (res)
+		return res;
+
+	res = yt922x_chip_setup_dsa(priv);
+	if (res)
+		return res;
+
+	return 0;
+}
+
+static void yt922x_pcs_setup(struct dsa_switch *ds)
+{
+	struct yt921x_priv *priv = to_yt921x_priv(ds);
+	const struct yt921x_info *info = priv->info;
+	unsigned long mask;
+	int port;
+
+	mask = info->serdes_mask;
+	for_each_set_bit(port, &mask, priv->series->max_ports) {
+		struct yt921x_port *pp = &priv->ports[port];
+
+		pp->pcs.ops = &yt922x_phylink_pcs_ops;
+		pp->pcs.poll = true;
+
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  pp->pcs.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
+			  pp->pcs.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
+			  pp->pcs.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_USXGMII,
+			  pp->pcs.supported_interfaces);
+	}
+}
+
+static int yt922x_dsa_setup(struct dsa_switch *ds)
+{
+	struct yt921x_priv *priv = to_yt921x_priv(ds);
+	struct device *dev = to_device(priv);
+	struct device_node *np = dev->of_node;
+	struct device_node *child;
+	int res;
+
+	/*  ports index init */
+	for (size_t i = 0; i < ARRAY_SIZE(priv->ports); i++) {
+		struct yt921x_port *pp = &priv->ports[i];
+
+		pp->index = i;
+	}
+
+	mutex_lock(&priv->reg_lock);
+	res = yt921x_chip_reset(priv);
+	mutex_unlock(&priv->reg_lock);
+	if (res)
+		return res;
+
+	/* Register the internal mdio bus. */
+	child = of_get_child_by_name(np, "mdio");
+	if (child) {
+		res = yt921x_mbus_int_init(priv, child);
+		of_node_put(child);
+		if (res)
+			return res;
+	}
+
+	mutex_lock(&priv->reg_lock);
+	res = yt922x_chip_setup(priv);
+	mutex_unlock(&priv->reg_lock);
+	if (res)
+		return res;
+
+	/* switch sds pcs setup */
+	yt922x_pcs_setup(ds);
+
+	return 0;
+}
+
+static const struct dsa_switch_ops yt922x_dsa_switch_ops = {
+	/* port */
+	.get_tag_protocol = yt922x_dsa_get_tag_protocol,
+	.phylink_get_caps = yt922x_dsa_phylink_get_caps,
+	.port_setup  = yt922x_dsa_port_setup,
+	/* chip */
+	.setup   = yt922x_dsa_setup,
+};
+
 static const struct yt92xx_series yt92xx_series_table[] = {
 	[YT92XX_MODE_YT921X] = {
 		.name = "YT921x",
@@ -4423,12 +4857,25 @@ static const struct yt92xx_series yt92xx_series_table[] = {
 		.switch_ops = &yt921x_dsa_switch_ops,
 		.mac_ops = &yt921x_phylink_mac_ops
 	},
+	[YT92XX_MODE_YT922X] = {
+		.name = "YT922x",
+		.max_ports = YT922X_PORT_NUM,
+		.num_lag_ids = YT922X_LAG_NUM,
+		.ageing_time_min = 1 * 6000,
+		.ageing_time_max = U16_MAX * 6000,
+		.dscp_prio_mapping_is_global = true,
+		.assisted_learning_on_cpu_port = true,
+		.switch_ops = &yt922x_dsa_switch_ops,
+		.mac_ops = &yt922x_phylink_mac_ops,
+	},
 };
 
 static const struct yt92xx_series *yt92xx_series_lookup(u32 major)
 {
 	if (major == YT9215_MAJOR || major == YT9218_MAJOR)
 		return &yt92xx_series_table[YT92XX_MODE_YT921X];
+	else if (major == YT9224_MAJOR)
+		return &yt92xx_series_table[YT92XX_MODE_YT922X];
 	else
 		return NULL;
 }
@@ -4547,6 +4994,7 @@ static int yt921x_mdio_probe(struct mdio_device *mdiodev)
 
 static const struct of_device_id yt921x_of_match[] = {
 	{ .compatible = "motorcomm,yt9215" },
+	{ .compatible = "motorcomm,yt9224" },
 	{}
 };
 MODULE_DEVICE_TABLE(of, yt921x_of_match);
@@ -4564,5 +5012,6 @@ static struct mdio_driver yt921x_mdio_driver = {
 mdio_module_driver(yt921x_mdio_driver);
 
 MODULE_AUTHOR("David Yang <mmyangfl@gmail.com>");
-MODULE_DESCRIPTION("Driver for Motorcomm YT921x Switch");
+MODULE_AUTHOR("Kyle Switch <kyle.switch@motor-comm.com>");
+MODULE_DESCRIPTION("Driver for Motorcomm YT921x and YT922x Switch");
 MODULE_LICENSE("GPL");
diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h
index 69176995090a..f11d19ba79b5 100644
--- a/drivers/net/dsa/motorcomm/chip.h
+++ b/drivers/net/dsa/motorcomm/chip.h
@@ -812,6 +812,7 @@ enum yt921x_fdb_entry_status {
 
 #define YT9215_MAJOR	0x9002
 #define YT9218_MAJOR	0x9001
+#define YT9224_MAJOR	0x9004
 
 /* required for a hard reset */
 #define YT921X_RST_DELAY_US	10000
@@ -833,6 +834,96 @@ enum yt921x_fdb_entry_status {
 
 #define YT921X_NAME	"yt921x"
 
+/* yt922x register lists */
+#define YT92XX_PAGE_SELECT	0x1e
+#define YT92XX_PAGE		0x1f
+#define YT922X_PORTn_STATUS(port)	(0x80200 + 4 * (port))
+#define  YT922X_PORT_LINK_STATE			BIT(8)
+#define  YT922X_PORT_LINK_DUPLEX		BIT(7)
+#define  YT922X_PORT_RX_FC_EN			BIT(6)
+#define  YT922X_PORT_TX_FC_EN			BIT(5)
+#define YT922X_PORT_SPEED_10		0
+#define YT922X_PORT_SPEED_100		1
+#define YT922X_PORT_SPEED_1000		2
+#define YT922X_PORT_SPEED_10000		3
+#define YT922X_PORT_SPEED_2500		4
+#define YT922X_PORT_SPEED_5000		5
+#define YT922X_EN_PHY_OVERWRITE		(0x80040)
+#define YT922X_EN_PHY_VALUE		(0x8003c)
+/* CTRL: force op to make soft configuration effective */
+#define YT922X_PORTn_CTRL(port)		(0x80080 + 4 * (port))
+#define  YT922X_PORT_FORCE_OP			BIT(14)
+#define  YT922X_PORT_CFG_TX_EN			BIT(13)
+#define  YT922X_PORT_CFG_RX_EN			BIT(12)
+#define  YT922X_PORT_FC_AN			BIT(11)
+#define  YT922X_PORT_LINK_AN			BIT(10)  /* CTRL: auto negotiation */
+#define  YT922X_PORT_LINK			BIT(9)  /* CTRL: link status */
+#define  YT922X_PORT_HALF_PAUSE			BIT(8)  /* Half-duplex back pressure mode */
+#define  YT922X_PORT_DUPLEX_FULL		BIT(7)
+#define  YT922X_PORT_RX_PAUSE			BIT(6)
+#define  YT922X_PORT_TX_PAUSE			BIT(5)
+#define  YT922X_PORT_RX_MAC_EN			BIT(4)
+#define  YT922X_PORT_TX_MAC_EN			BIT(3)
+#define  YT922X_PORT_SPEED_M			GENMASK(2, 0)
+#define YT922X_PORT_SDSn		0x400
+#define  YT922X_SERDES_MODE_M			GENMASK(6, 4)
+#define   YT922X_SERDES_MODE(x)				FIELD_PREP(YT922X_SERDES_MODE_M, (x))
+#define YT922X_SERDES_MODE_SGMII	YT922X_SERDES_MODE(0)
+#define YT922X_SERDES_MODE_REVSGMII	YT922X_SERDES_MODE(1)
+#define YT922X_SERDES_MODE_1000BASEX	YT922X_SERDES_MODE(2)
+#define YT922X_SERDES_MODE_100BASEX	YT922X_SERDES_MODE(3)
+#define YT922X_SERDES_MODE_2500BASEX	YT922X_SERDES_MODE(4)
+#define YT922X_SERDES_MODE_USXGMII	YT922X_SERDES_MODE(6)
+#define YT922X_PORT_NUM			9
+#define YT922X_PCS_LINK_CTRL		0x11
+#define YT922X_PCS_LINK_STATUS		BIT(10)
+#define YT922X_PCS_AN_COMPLETE		BIT(11)
+
+/* LAG */
+#define YT922X_LAG_NUM			4
+/* ISO */
+#define YT922X_PORTn_ISOLATION(port)	(0x4 * (port) + 0x180d80)
+/* FDB */
+#define YT922X_PORTn_LEARN(port)	(0x180300 + 4 * (port))
+#define  YT922X_PORT_LEARN_DIS			BIT(18)
+/* GLOBAL CTRL */
+#define  YT922X_FUNC_ACL			BIT(5)
+#define  YT922X_FUNC_MIB			BIT(4)
+/* CTRL PKT */
+#define YT922X_FILTER_UNK_UCAST		0x180ec8
+#define YT922X_ACT_UNK_UCAST		0x180ed8
+#define YT922X_ACT_UNK_MCAST		0x180ee0
+#define  YT922X_ACT_UNK_MCAST_BYPASS_DROP_PIM	BIT(22)
+#define  YT922X_ACT_UNK_MCAST_BYPASS_DROP_MLD	BIT(21)
+#define  YT922X_ACT_UNK_MCAST_BYPASS_DROP_IGMP	BIT(20)
+#define  YT922X_ACT_UNK_ACTn_M(port)			GENMASK(2 * (port) + 1, 2 * (port))
+#define  YT922X_ACT_UNK_ACTn(port, x)			((x) << (2 * (port)))
+#define  YT922X_ACT_UNK_ACTn_FORWARD(port)		YT922X_ACT_UNK_ACTn(port, 0)  /* flood */
+#define  YT922X_ACT_UNK_ACTn_DROP(port)			YT922X_ACT_UNK_ACTn(port, 1)  /* discard */
+#define  YT922X_ACT_UNK_ACTn_TRAP(port)			YT922X_ACT_UNK_ACTn(port, 3)  /* steer to CPU */
+
+/* CPU PORT */
+#define YT922X_CPU_COPY			0x181100
+#define  YT922X_CPU_COPY_TO_INT_CPU		BIT(1)
+#define  YT922X_CPU_COPY_TO_EXT_CPU		BIT(0)
+#define YT922X_CPU_TAG_RX_CTRL		0x80504
+#define  YT922X_CPU_TAG_RX_MODE			BIT(0)
+#define YT922X_CPU_TAG_TX_CTRL		0x100710
+#define  YT922X_CPU_TAG_TX_TYPE		BIT(0)
+#define  YT922X_CPU_TAG_TX_MODE		BIT(1)
+#define  YT922X_CPU_TAG_TX_CTAG_OP	BIT(2)
+#define  YT922X_CPU_TAG_TX_STAG_OP	BIT(3)
+/* Define top ext addr for yt922x */
+#define YT922X_COMMON_EXT_PHYADDR 9
+
+enum yt922x_phy_reg_type {
+	YT922X_PHY_REG_TYPE_COMMON_EXT,
+	YT922X_PHY_REG_TYPE_SDS_COMMON_EXT,
+	YT922X_PHY_REG_TYPE_MII,
+	YT922X_PHY_REG_TYPE_EXT,
+	YT922X_PHY_REG_TYPE_MAX
+};
+
 struct yt921x_mib {
 	u64 rx_broadcast;
 	u64 rx_pause;
diff --git a/drivers/net/dsa/motorcomm/mdio_bus.c b/drivers/net/dsa/motorcomm/mdio_bus.c
index 1ac6a94f4f9d..dd465ac338da 100644
--- a/drivers/net/dsa/motorcomm/mdio_bus.c
+++ b/drivers/net/dsa/motorcomm/mdio_bus.c
@@ -301,3 +301,42 @@ int yt921x_mbus_ext_init(struct yt921x_priv *priv, struct device_node *mnp)
 
 	return 0;
 }
+
+/* intif extend register read/write api */
+int
+yt921x_intif_ext_write(struct yt921x_priv *priv, int port, int reg, u16 val)
+{
+	int res;
+
+	if (port > priv->series->max_ports)
+		return -ENODEV;
+
+	res = yt921x_intif_write(priv, port, YT92XX_PAGE_SELECT, reg);
+	if (res)
+		return res;
+
+	res = yt921x_intif_write(priv, port, YT92XX_PAGE, val);
+	if (res)
+		return res;
+
+	return 0;
+}
+
+int
+yt921x_intif_ext_read(struct yt921x_priv *priv, int port, int reg, u16 *valp)
+{
+	int res;
+
+	if (port > priv->series->max_ports)
+		return -ENODEV;
+
+	res = yt921x_intif_write(priv, port, YT92XX_PAGE_SELECT, reg);
+	if (res)
+		return res;
+
+	res = yt921x_intif_read(priv, port, YT92XX_PAGE, valp);
+	if (res)
+		return res;
+
+	return 0;
+}
diff --git a/drivers/net/dsa/motorcomm/mdio_bus.h b/drivers/net/dsa/motorcomm/mdio_bus.h
index e79b725d435b..513a8d2d7618 100644
--- a/drivers/net/dsa/motorcomm/mdio_bus.h
+++ b/drivers/net/dsa/motorcomm/mdio_bus.h
@@ -50,5 +50,7 @@ yt921x_intif_modify(struct yt921x_priv *priv, int port, int reg, u16 mask,
 
 int yt921x_mbus_int_init(struct yt921x_priv *priv, struct device_node *mnp);
 int yt921x_mbus_ext_init(struct yt921x_priv *priv, struct device_node *mnp);
+int yt921x_intif_ext_write(struct yt921x_priv *priv, int port, int reg, u16 val);
+int yt921x_intif_ext_read(struct yt921x_priv *priv, int port, int reg, u16 *valp);
 
 #endif
diff --git a/drivers/net/dsa/motorcomm/pcs-922x.c b/drivers/net/dsa/motorcomm/pcs-922x.c
new file mode 100644
index 000000000000..98d403d01ba3
--- /dev/null
+++ b/drivers/net/dsa/motorcomm/pcs-922x.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2026 Kyle Switch
+ */
+
+#include "chip.h"
+#include "mdio_bus.h"
+#include "pcs.h"
+#include "smi.h"
+
+#define to_device(priv) ((priv)->ds.dev)
+
+static int yt922x_sds_phyaddr_get(int port,
+				  enum yt922x_phy_reg_type reg_type)
+{
+	if (reg_type == YT922X_PHY_REG_TYPE_COMMON_EXT)
+		return YT922X_COMMON_EXT_PHYADDR;
+
+	return port;
+}
+
+static void yt922x_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
+				 struct phylink_link_state *state)
+{
+	struct yt921x_port *pp = pcs_to_yt921x_port(pcs);
+	struct yt921x_priv *priv = yt921x_port_to_priv(pp);
+	int port = pp->index;
+	int res = 0;
+	u16 data;
+	int addr;
+	u16 lp;
+
+	addr = yt922x_sds_phyaddr_get(port, YT922X_PHY_REG_TYPE_MII);
+	if (addr < 0) {
+		state->link = false;
+		return;
+	}
+
+	mutex_lock(&priv->reg_lock);
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_2500BASEX:
+		res = yt921x_intif_read(priv, addr, MII_BMSR, &data);
+		if (res)
+			goto err;
+		res = yt921x_intif_read(priv, addr, MII_LPA, &lp);
+		if (res)
+			goto err;
+		phylink_mii_c22_pcs_decode_state(state, neg_mode, data, lp);
+		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		res = yt921x_intif_read(priv, addr, YT922X_PCS_LINK_CTRL,
+					&data);
+		if (res)
+			goto err;
+		state->link =  FIELD_GET(YT922X_PCS_LINK_STATUS, data);
+		state->an_complete = FIELD_GET(YT922X_PCS_AN_COMPLETE, data);
+		res = yt921x_intif_read(priv, addr, MII_LPA, &lp);
+		if (res)
+			goto err;
+		if (state->link)
+			phylink_decode_usxgmii_word(state, lp);
+		break;
+	default:
+		state->link = false;
+		break;
+	}
+	mutex_unlock(&priv->reg_lock);
+	return;
+
+err:
+	mutex_unlock(&priv->reg_lock);
+	state->link = false;
+}
+
+static void yt922x_pcs_an_restart(struct phylink_pcs *pcs)
+{
+	struct yt921x_port *pp = pcs_to_yt921x_port(pcs);
+	struct yt921x_priv *priv = yt921x_port_to_priv(pp);
+	struct device *dev = to_device(priv);
+	int port = pp->index;
+	u16 data;
+	int addr;
+	int res;
+
+	mutex_lock(&priv->reg_lock);
+	addr = yt922x_sds_phyaddr_get(port, YT922X_PHY_REG_TYPE_MII);
+	if (addr < 0) {
+		res = addr;
+		goto err;
+	}
+	res = yt921x_intif_read(priv, addr, MII_BMCR, &data);
+	if (res)
+		goto err;
+	data |= BMCR_ANRESTART;
+	res = yt921x_intif_write(priv, addr, MII_BMCR, data);
+	if (res)
+		goto err;
+	mutex_unlock(&priv->reg_lock);
+	return;
+
+err:
+	mutex_unlock(&priv->reg_lock);
+	if (res)
+		dev_err(dev, "Failed to %s PCS port %d: %i\n", "an restart",
+			port, res);
+}
+
+static int yt922x_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
+			     phy_interface_t interface,
+			     const unsigned long *advertising,
+			     bool permit_pause_to_mac)
+{
+	struct yt921x_port *pp = pcs_to_yt921x_port(pcs);
+	struct yt921x_priv *priv = yt921x_port_to_priv(pp);
+	int res, port;
+	u16 data;
+	u16 ctrl;
+	int addr;
+
+	port = pp->index;
+	mutex_lock(&priv->reg_lock);
+	addr = yt922x_sds_phyaddr_get
+		(port, YT922X_PHY_REG_TYPE_SDS_COMMON_EXT);
+	if (addr < 0) {
+		res = addr;
+		goto err;
+	}
+	/* write protect */
+	res = yt921x_intif_ext_write(priv, addr, 0x4be, 0xd);
+	if (res)
+		goto err;
+	switch (interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+		ctrl = YT922X_SERDES_MODE_SGMII;
+		break;
+	case PHY_INTERFACE_MODE_1000BASEX:
+		ctrl = YT922X_SERDES_MODE_1000BASEX;
+		break;
+	case PHY_INTERFACE_MODE_2500BASEX:
+		ctrl = YT922X_SERDES_MODE_2500BASEX;
+		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		ctrl = YT922X_SERDES_MODE_USXGMII;
+		break;
+	default:
+		res = -EINVAL;
+		goto err;
+	}
+	res = yt921x_intif_ext_read(priv, addr, YT922X_PORT_SDSn, &data);
+	if (res)
+		goto err;
+	data &= ~YT922X_SERDES_MODE_M;
+	data |= ctrl;
+	res = yt921x_intif_ext_write(priv, addr, YT922X_PORT_SDSn, data);
+	if (res)
+		goto err;
+	mutex_unlock(&priv->reg_lock);
+
+	return res;
+
+err:
+	mutex_unlock(&priv->reg_lock);
+
+	return res;
+}
+
+const struct phylink_pcs_ops yt922x_phylink_pcs_ops = {
+	.pcs_get_state = yt922x_pcs_get_state,
+	.pcs_config = yt922x_pcs_config,
+	.pcs_an_restart = yt922x_pcs_an_restart,
+};
diff --git a/drivers/net/dsa/motorcomm/pcs.h b/drivers/net/dsa/motorcomm/pcs.h
index 42426558086a..c85caa7bf212 100644
--- a/drivers/net/dsa/motorcomm/pcs.h
+++ b/drivers/net/dsa/motorcomm/pcs.h
@@ -9,5 +9,6 @@
 #include <linux/phylink.h>
 
 extern const struct phylink_pcs_ops yt921x_phylink_pcs_ops;
+extern const struct phylink_pcs_ops yt922x_phylink_pcs_ops;
 
 #endif
-- 
2.25.1

Re: [PATCH net-next v11 7/7] net: dsa: motorcomm: Add support for Motorcomm YT922x

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 4 · 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 driver's YT922x port map contradicts itself, so the
  YT9224's second SerDes port (port 9) cannot be used.
- [Medium] The patch adds compatible "motorcomm,yt9224" to
  yt921x_of_match[], but the DT binding is not updated.
- [Medium] The 5 Gb/s capability that YT922x SerDes ports advertise does
  not work: the MAC is never brought up at 5G.
- [Medium] yt922x_pcs_config() (pcs-922x.c:110-161) does not meet the
  phylink pcs_config contract.
- [Low] yt92xx_register_switch() declares `u32 chipid;` with no
  initializer.
- [Low] yt921x_dsa_teardown() does not compile with
  CONFIG_NET_DSA_YT921X_LEDS=n.

--

net: dsa: motorcomm: Add support for Motorcomm YT922x

This patch adds basic DSA support for the Motorcomm YT9224 switch to the
existing yt921x driver. It adds a YT922x series descriptor with its own
dsa_switch_ops and phylink_mac_ops, a new SerDes PCS implementation in
pcs-922x.c, and a "motorcomm,yt9224" compatible.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 9380d74d79ba2..175855cbc2375 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -141,6 +141,18 @@ static const struct yt921x_info yt921x_infos[] = {
>  		BIT(8) | BIT(9),
>  		BIT(8) | BIT(9),
>  	},
> +	{
> +		"YT9224", YT9224_MAJOR, 0, 0,
> +		GENMASK_U16(7, 4),
> +		0,
> +		BIT(8) | BIT(9),
> +	},

[Severity: Medium]
This entry marks port 9 as a SerDes port on YT9224. The YT922x series,
however, uses max_ports = YT922X_PORT_NUM, which is 9, and
yt92xx_register_switch() copies that into ds->num_ports. Can port 9
actually be used?

With ds->num_ports == 9, dsa_switch_parse_ports_of() rejects a DT node
for ethernet-port@9:

net/dsa/dsa.c:dsa_switch_parse_ports_of() {
    ...
	if (reg >= ds->num_ports) {
    ...
}

A board that wires the second SerDes (for example as the CPU port)
would then fail to probe with -EINVAL.

The rest of the new code also seems to assume 9 ports:

  yt922x_pcs_setup() loops with for_each_set_bit(port, &mask,
  priv->series->max_ports), so the PCS for port 9 is never set up.

  yt922x_chip_setup_dsa() writes GENMASK_U32(8, 0) to
  YT922X_FILTER_UNK_UCAST. It also builds the unknown ucast/mcast
  action word only for ports 0..8.

  YT922X_COMMON_EXT_PHYADDR is 9, and yt922x_sds_phyaddr_get() uses the
  port index as the SerDes MII address. A port 9 PCS would therefore
  use the same address as the common ext block.

The new tag_yt922x.c tagger, on the other hand, encodes destination
ports 1..9 in tag[1] bits 8:0 and port 0 in tag[2] BIT(15). That
describes a 10 port device.

Is either serdes_mask here or YT922X_PORT_NUM wrong?

[ ... ]

> @@ -3932,6 +3944,11 @@ static void yt921x_dsa_teardown(struct dsa_switch *ds)
>  	}
>  }
>  
> +static bool yt921x_needs_extmode_check(u32 major)
> +{
> +	return (major == YT9224_MAJOR) ? false : true;
> +}
> +

[Severity: Low]
This comes from an earlier patch in the series rather than this one.
Does yt921x_dsa_teardown(), just above, still build with
CONFIG_NET_DSA_YT921X_LEDS=n?

drivers/net/dsa/motorcomm/chip.c:yt921x_dsa_teardown() {
#if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
	struct yt921x_priv *priv = to_yt921x_priv(ds);

	yt921x_leds_remove(priv);
#endif

	for (size_t i = ARRAY_SIZE(priv->ports); i-- > 0; ) {
    ...
}

priv is declared only inside the #if block, but the loop after the
#endif uses it. NET_DSA_YT921X_LEDS is a user visible bool that depends
on LEDS_CLASS=y || LEDS_CLASS=NET_DSA_YT921X. With LEDS_CLASS=n, or
with LEDS_CLASS=m and NET_DSA_YT921X=y, the build would fail because
priv is undeclared.

The breakage was introduced by "net: dsa: motorcomm: move mib stop from
remove() to teardown()" earlier in the series. It is still there after
this patch, which also edits the driver's Kconfig entry.

[ ... ]

> @@ -4411,6 +4439,412 @@ static const struct dsa_switch_ops yt921x_dsa_switch_ops = {

[ ... ]

> +static int
> +yt922x_port_up(struct yt921x_priv *priv, int port, unsigned int mode,
> +	       phy_interface_t interface, int speed, int duplex,
> +	       bool tx_pause, bool rx_pause)
> +{
> +	int ps = ethtool_speed_to_yt921x(speed);
> +	u32 ctrl = 0;
> +	u32 mask;
> +	int res;
> +
> +	if (ps == YT921X_SPEED_INVALID)
> +		return -EINVAL;

[Severity: Medium]
What happens here when phylink resolves SPEED_5000?

yt922x_dsa_phylink_get_caps() advertises USXGMII and MAC_5000FD for
every SerDes port. However, ethtool_speed_to_yt921x() has no
SPEED_5000 case:

drivers/net/dsa/motorcomm/chip.h:ethtool_speed_to_yt921x() {
	...
	case SPEED_2500:
		return YT921X_SPEED_2500;
	case SPEED_10000:
		return YT921X_SPEED_10000;
	default:
		return YT921X_SPEED_INVALID;
	}
}

YT921X_SPEED_INVALID is 5. That is the same value as the new
YT922X_PORT_SPEED_5000, which is defined but never used.

So with a 5G USXGMII link (for example yt922x_pcs_get_state() ->
phylink_decode_usxgmii_word() with MDIO_USXGMII_5000), yt922x_port_up()
returns -EINVAL before it writes YT922X_PORTn_CTRL, and en_phy is never
re-enabled.

yt922x_phylink_mac_link_up() only logs the error, and phylink_link_up()
still calls netif_carrier_on() afterwards. Would the port then report
carrier up while passing no traffic?

[ ... ]

> @@ -4423,12 +4857,25 @@ static const struct yt92xx_series yt92xx_series_table[] = {

[ ... ]

>  static const struct yt92xx_series *yt92xx_series_lookup(u32 major)
>  {
>  	if (major == YT9215_MAJOR || major == YT9218_MAJOR)
>  		return &yt92xx_series_table[YT92XX_MODE_YT921X];
> +	else if (major == YT9224_MAJOR)
> +		return &yt92xx_series_table[YT92XX_MODE_YT922X];
>  	else
>  		return NULL;
>  }

[Severity: Low]
This patch relies on yt92xx_register_switch(), the caller of this
lookup, to pick the YT922x series. Can its error path print an
uninitialized chipid?

drivers/net/dsa/motorcomm/chip.c:yt92xx_register_switch() {
    ...
	u32 chipid;
    ...
	res = yt921x_reg_read(priv, YT921X_CHIP_ID, &chipid);
	mutex_unlock(&priv->reg_lock);
	if (res) {
		dev_err(dev, "Unexpected chipid 0x%x\n", chipid);
		return res;
	}
    ...
}

The read can fail before *valp is written. For example,
yt921x_reg_mdio_read() bails out after an MDIO write error. In that case
chipid is still uninitialized stack memory when it is printed. The
message also says "Unexpected chipid" when the read itself failed.

This was introduced earlier in the series by "net: dsa: motorcomm:
initialize dsa_switch based on chipid".

[ ... ]

> @@ -4547,6 +4994,7 @@ static int yt921x_mdio_probe(struct mdio_device *mdiodev)
>  
>  static const struct of_device_id yt921x_of_match[] = {
>  	{ .compatible = "motorcomm,yt9215" },
> +	{ .compatible = "motorcomm,yt9224" },
>  	{}
>  };

[Severity: Medium]
Should the DT binding be updated along with this new compatible?

Documentation/devicetree/bindings/net/dsa/motorcomm,yt921x.yaml still
has:

  compatible:
    const: motorcomm,yt9215

together with unevaluatedProperties: false. No binding under
Documentation/devicetree/bindings mentions yt9224.

The binding's description, port list and mdio text also cover only the
YT921x parts. The YT9224 layout is not documented: internal PHYs on
ports 4-7, SerDes on ports 8 and 9, and no xMII.

A DTS that uses "motorcomm,yt9224" would fail dt_binding_check, and
checkpatch would flag the compatible as undocumented.

[ ... ]

> diff --git a/drivers/net/dsa/motorcomm/pcs-922x.c b/drivers/net/dsa/motorcomm/pcs-922x.c
> new file mode 100644
> index 0000000000000..98d403d01ba3c
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/pcs-922x.c
> @@ -0,0 +1,173 @@

[ ... ]

> +static int yt922x_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> +			     phy_interface_t interface,
> +			     const unsigned long *advertising,
> +			     bool permit_pause_to_mac)
> +{

[ ... ]

> +	/* write protect */
> +	res = yt921x_intif_ext_write(priv, addr, 0x4be, 0xd);
> +	if (res)
> +		goto err;
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +		ctrl = YT922X_SERDES_MODE_SGMII;
> +		break;

[ ... ]

> +	default:
> +		res = -EINVAL;
> +		goto err;
> +	}
> +	res = yt921x_intif_ext_read(priv, addr, YT922X_PORT_SDSn, &data);
> +	if (res)
> +		goto err;
> +	data &= ~YT922X_SERDES_MODE_M;
> +	data |= ctrl;
> +	res = yt921x_intif_ext_write(priv, addr, YT922X_PORT_SDSn, data);
> +	if (res)
> +		goto err;
> +	mutex_unlock(&priv->reg_lock);
> +
> +	return res;

[ ... ]

> +const struct phylink_pcs_ops yt922x_phylink_pcs_ops = {
> +	.pcs_get_state = yt922x_pcs_get_state,
> +	.pcs_config = yt922x_pcs_config,
> +	.pcs_an_restart = yt922x_pcs_an_restart,
> +};

[Severity: Medium]
Does yt922x_pcs_config() need to handle neg_mode and advertising?

It only rewrites the YT922X_SERDES_MODE field. neg_mode, advertising
and permit_pause_to_mac are unused. BMCR_ANENABLE is never set or
cleared, and MII_ADVERTISE is never written for 1000BASE-X or
2500BASE-X.

The pcs_config kernel-doc in include/linux/phylink.h says:

 * When operating in %MLO_AN_INBAND, inband should always be enabled,
 * otherwise inband should be disabled.
 ...
 * For 1000BASE-X, the advertisement should be programmed into the PCS.

The sibling yt921x_serdes_config() in pcs-921x.c does this explicitly:

	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
		bmcr |= BMCR_ANENABLE;
	else
		bmcr &= ~BMCR_ANENABLE;

Without that, the PCS keeps whatever in-band AN state it had after
reset. In-band links may then fail to come up, and out-of-band or fixed
links may run with AN enabled.

yt922x_pcs_config() also returns 0 without programming the
advertisement. phylink_change_inband_advert() then never restarts AN,
so advertisement changes made through ethtool are silently dropped.

yt922x_phylink_pcs_ops has no pcs_inband_caps, pcs_link_up, pcs_enable
or pcs_disable. yt922x_port_up() only writes the MAC side
YT922X_PORTn_CTRL. Is anything forcing the SerDes speed for
out-of-band SGMII at 10/100M?

The 0x4be = 0xd write also happens before the interface is checked. For
an unsupported interface the function returns -EINVAL with that write
left in place, and the write is never undone. Should it be moved after
the switch on interface?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002173706.1225502-1-kyle.switch%40motor-comm.com