[PATCH net] net: dsa: motorcomm: Clear the MIB buffer on chip reset

[PATCH net] net: dsa: motorcomm: Clear the MIB buffer on chip reset

[David Yang]

.setup() can run again within one registration: removing another switch
from the tree tears the whole tree down, and re-probing sets every
member switch up again. The hardware reset at the start of .setup()
brings the MIB counters back to zero, but a MIB block kept across that
cycle still holds the old software buffer, so the next yt921x_mib_read()
treats the small value as a 32-bit wrap and adds 2^32 to every counter
it extends.

.port_teardown() keeps the MIB block on purpose and .port_setup() only
zeroes a freshly allocated one, so clear the buffer in .setup() together
with the hardware counters it extends. The published stats need no
handling; every entry is republished from the buffer.

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")
Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/chip.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 33e5fc127244..ff17233848a8 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -2458,6 +2458,16 @@ static int yt921x_dsa_setup(struct dsa_switch *ds)
 		return -ENODEV;
 	}
 
+	for (int port = 0; port < YT921X_PORT_NUM; port++) {
+		struct yt921x_port *pp = &priv->ports[port];
+		struct yt921x_mib *pm = pp->mib;
+
+		if (!pm)
+			continue;
+
+		memset(&pm->data, 0, sizeof(pm->data));
+	}
+
 	mask = priv->info->serdes_mask;
 	for_each_set_bit(port, &mask, YT921X_PORT_NUM) {
 		struct yt921x_port *pp = &priv->ports[port];
-- 
2.53.0

Re: [PATCH net] net: dsa: motorcomm: Clear the MIB buffer on chip reset

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] net: dsa: motorcomm: Clear the MIB buffer on chip reset

[David Yang]

On Mon, Nov 2, 2026 at 6:13 PM <netdev-bot+sinfo@kernel.org> wrote:
>
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.
>
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.
>
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
>
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

Reported by sashiko

Re: [PATCH net] net: dsa: motorcomm: Clear the MIB buffer on chip reset

[Andrew Lunn]

On Mon, Nov 02, 2026 at 06:20:52PM +0800, David Yang wrote:
> On Mon, Nov 2, 2026 at 6:13 PM <netdev-bot+sinfo@kernel.org> wrote:
> >
> > Hi!
> >
> > This is an automated message. This series looks like a fix, but its
> > commit messages seem to be missing some information:
> >
> >  - How the issue was discovered, e.g. hit in production, hit during
> >    development, syzbot report, manual code inspection, LLM or static
> >    analysis tool scan.
> >
> >  - Whether the issue was actually triggered, or is only theoretical
> >    (e.g. found by code inspection). If it was triggered please include
> >    the symptoms, like the stack trace or error messages.
> >
> > Please do not repost the series just to address the above. Instead,
> > reply to this email with the missing information, so that reviewers
> > can take it into account. If the series needs another revision for
> > other reasons, please include the information in the commit messages
> > then.
> >
> > The evaluation is done by an LLM so it may be wrong, if you think
> > that is the case please reply and explain.
> 
> Reported by sashiko

What is also interesting is that i expect you have tested this on real
hardware, unlike most of the patches we are getting at the moment. It
is worth pointing that out, it might help get your patch merged
faster.

	Andrew

Re: [PATCH net] net: dsa: motorcomm: Clear the MIB buffer on chip reset

[Andrew Lunn]

On Mon, Nov 02, 2026 at 06:11:21PM +0800, David Yang wrote:
> .setup() can run again within one registration: removing another switch
> from the tree tears the whole tree down

Is D in DSA supported by the Motorcomm devices? Or is this tree always
a single node?

> Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")

What i'm trying to understand is, is this theoretical, but never
happens? If so, it should be based on net-next. Or is this an issue
which actually bothers somebody and so is justified having a Fixes:
tag and posted to net?

This is also what that automated email is about. We need information
to decided if the correct tree has been picked. It frequently is wrong
in the LLM age.

   Andrew

Re: [PATCH net] net: dsa: motorcomm: Clear the MIB buffer on chip reset

[David Yang]

On Tue, Nov 3, 2026 at 12:44 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Nov 02, 2026 at 06:11:21PM +0800, David Yang wrote:
> > .setup() can run again within one registration: removing another switch
> > from the tree tears the whole tree down
>
> Is D in DSA supported by the Motorcomm devices? Or is this tree always
> a single node?
>
> > Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")
>
> What i'm trying to understand is, is this theoretical, but never
> happens? If so, it should be based on net-next. Or is this an issue
> which actually bothers somebody and so is justified having a Fixes:
> tag and posted to net?
>
> This is also what that automated email is about. We need information
> to decided if the correct tree has been picked. It frequently is wrong
> in the LLM age.
>
>    Andrew

Motorcomm does support cascading, but not in our driver. So yes, this
is largely theoretical and I just follow what sashiko said.

Re: [PATCH net] net: dsa: motorcomm: Clear the MIB buffer on chip reset

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says "The published stats need no handling;
  every entry is republished from the buffer".
- [Low] net: dsa: motorcomm: port variable shadowing in
  yt921x_dsa_setup()

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it blocks the
  re-setup path the patch is meant to fix.

--

net: dsa: motorcomm: Clear the MIB buffer on chip reset

This patch clears the per-port software MIB buffer (pm->data) in
yt921x_dsa_setup() after the chip reset. Without it, a re-setup of the
DSA tree leaves the buffer holding old values, and the next
yt921x_mib_read() reads the reset counters as a 32-bit wrap.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 33e5fc127244d..ff17233848a80 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -2458,6 +2458,16 @@ static int yt921x_dsa_setup(struct dsa_switch *ds)
>  		return -ENODEV;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but can yt921x_dsa_setup() reach the new
loop on the re-setup path from the commit message? The question applies
when the switch node has an "mdio" child, as in the motorcomm,yt921x.yaml
example.

Earlier in yt921x_dsa_setup(), every .setup() call does this:

	child = of_get_child_by_name(np, "mdio");
	if (child) {
		res = yt921x_mbus_int_init(priv, child);
		...

yt921x_mbus_int_init() registers a devm managed bus with the same id
each time:

	snprintf(mbus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
	...
	mbus->parent = dev;
	...
	res = devm_of_mdiobus_register(dev, mbus, mnp);

devm only releases the bus when the yt921x device unbinds.
yt921x_dsa_teardown() only calls yt921x_leds_remove(). So the first bus
is still registered when the tree is set up again, for example after
another member switch is removed and re-probed.

The second registration would then hit this in __mdiobus_register(),
with a duplicate name under the same parent:

	dev_set_name(&bus->dev, "%s", bus->id);
	...
	err = device_register(&bus->dev);
	if (err) {
		pr_err("mii_bus %s failed to register\n", bus->id);
		return -EINVAL;
	}

In that case yt921x_dsa_setup() returns before the MIB clear and before
yt921x_chip_setup(). dsa_switch_setup() then fails, and the whole tree
is torn down.

Would the tree then stay down until the yt921x driver is unbound, so
that this fix never runs? Should the internal MDIO bus be registered
outside .setup(), or unregistered in .teardown()?

> +	for (int port = 0; port < YT921X_PORT_NUM; port++) {
> +		struct yt921x_port *pp = &priv->ports[port];
> +		struct yt921x_mib *pm = pp->mib;
> +
> +		if (!pm)
> +			continue;
> +
> +		memset(&pm->data, 0, sizeof(pm->data));
> +	}

[Severity: Low]
This loop declares its own int port, but yt921x_dsa_setup() already
has an int port at function scope. The serdes_mask for_each_set_bit()
loop just below uses that outer variable.

The inner declaration shadows the outer one. A W=2 build, which
enables -Wshadow, warns about this. With W=e or CONFIG_WERROR=y the
warning becomes a build failure in chip.c.

There is no runtime problem, because for_each_set_bit() assigns the
outer port before reading it. Could the new loop reuse the existing
variable instead of declaring a new one, as in:

	for (port = 0; port < YT921X_PORT_NUM; port++) {

That would also match how the rest of yt921x_dsa_setup() walks the
ports.

[Severity: Medium]
This loop only clears pm->data. What happens to the published copies in
the same MIB block (pm->stats, pm->rx_frames and pm->tx_frames)?

The commit message says:

  The published stats need no handling; every entry is republished from
  the buffer.

That seems true only after yt921x_mib_read() runs again. On re-setup,
yt921x_dsa_port_setup() keeps the existing pm and only does this, which
does not queue the poll:

	if (pm)
		enable_delayed_work(&pm->work);

The only place that queues the poll is yt921x_phylink_mac_link_up():

	pm = priv->ports[port].mib;
	if (pm)
		schedule_delayed_work(&pm->work, 0);

Meanwhile, yt921x_dsa_get_stats64() in mib.c reads the published copies
under syncp and never calls yt921x_mib_read():

		stats->rx_packets = u64_stats_read(&pm->rx_frames);
		stats->tx_packets = u64_stats_read(&pm->tx_frames);
		stats->rx_bytes = u64_stats_read(&mib->rx_good_bytes) -
				  ETH_FCS_LEN * stats->rx_packets;

Take a port whose link stays down after the tree is set up again. Would
its new user netdev keep reporting the old packet, byte and error
counters in ip -s link and /proc/net/dev indefinitely?

The next mib_read could come from the link-up poll or from an ethtool
-S, eth-mac, eth-ctrl, rmon or pause query. At that point every counter
would drop to the small post-reset value, so the counters would go
backwards.

Could this loop also clear or republish the syncp-protected copies,
inside u64_stats_update_begin_irqsave() and
u64_stats_update_end_irqrestore()?

>  	mask = priv->info->serdes_mask;
>  	for_each_set_bit(port, &mask, YT921X_PORT_NUM) {
>  		struct yt921x_port *pp = &priv->ports[port];

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

Re: [PATCH net] net: dsa: motorcomm: Clear the MIB buffer on chip reset

[Andrew Lunn]

> Motorcomm does support cascading, but not in our driver. So yes, this
> is largely theoretical and I just follow what sashiko said.

Then i would target net-next.


    Andrew

---
pw-bot: cr