[PATCH net-next] net: dsa: motorcomm: Free the MIB block on port teardown

[PATCH net-next] net: dsa: motorcomm: Free the MIB block on port teardown

[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.

The old claim, get_stats64() not being synchronized with
.port_teardown(), is not true, so free the MIB block in
.port_teardown(), which pairs with every successful .port_setup()
including the error unwinds, so the next setup allocates zeroed blocks.
Not carrying a fix tag since there are no known multi-chip yt921x users.

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

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 33e5fc127244..fd737c3e2e18 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -1994,11 +1994,11 @@ static void yt921x_dsa_port_teardown(struct dsa_switch *ds, int port)
 	struct yt921x_port *pp = &priv->ports[port];
 	struct yt921x_mib *pm = pp->mib;
 
-	/* get_stats64() might not be synchronized with port_teardown(), hold
-	 * the MIB block till the device teardown
-	 */
-	if (pm)
+	if (pm) {
 		disable_delayed_work_sync(&pm->work);
+		pp->mib = NULL;
+		kvfree(pm);
+	}
 }
 
 static int yt921x_dsa_port_setup(struct dsa_switch *ds, int port)
@@ -2006,12 +2006,11 @@ static int yt921x_dsa_port_setup(struct dsa_switch *ds, int port)
 	struct yt921x_priv *priv = dsa_to_yt921x_priv(ds);
 	struct yt921x_port *pp = &priv->ports[port];
 	const struct yt921x_info *info = priv->info;
-	struct device *dev = yt921x_priv_to_device(priv);
 	struct yt921x_mib *pm = pp->mib;
 	int res;
 
 	if (!pm && (BIT(port) & yt921x_info_ports_mask(info))) {
-		pm = devm_kzalloc(dev, sizeof(*pm), GFP_KERNEL);
+		pm = kvzalloc_obj(*pm);
 		if (!pm)
 			return -ENOMEM;
 		pp->mib = pm;
@@ -2019,10 +2018,6 @@ static int yt921x_dsa_port_setup(struct dsa_switch *ds, int port)
 		pm->port = pp;
 		u64_stats_init(&pm->syncp);
 		INIT_DELAYED_WORK(&pm->work, yt921x_mib_poll);
-		/* Start disabled, so that the enable below always pairs with
-		 * either this disable or the one from a previous teardown
-		 */
-		disable_delayed_work_sync(&pm->work);
 	}
 
 	mutex_lock(&priv->reg_lock);
@@ -2031,9 +2026,6 @@ static int yt921x_dsa_port_setup(struct dsa_switch *ds, int port)
 	if (res)
 		return res;
 
-	if (pm)
-		enable_delayed_work(&pm->work);
-
 	return 0;
 }
 
-- 
2.53.0

Re: [PATCH net-next] net: dsa: motorcomm: Free the MIB block on port teardown

[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-next] net: dsa: motorcomm: Free the MIB block on port teardown

[David Yang]

On Sun, Nov 8, 2026 at 6:22 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.

not a fix - explained in the message

Re: [PATCH net-next] net: dsa: motorcomm: Free the MIB block on port teardown

[Andrew Lunn]

> @@ -2006,12 +2006,11 @@ static int yt921x_dsa_port_setup(struct dsa_switch *ds, int port)
>  	struct yt921x_priv *priv = dsa_to_yt921x_priv(ds);
>  	struct yt921x_port *pp = &priv->ports[port];
>  	const struct yt921x_info *info = priv->info;
> -	struct device *dev = yt921x_priv_to_device(priv);
>  	struct yt921x_mib *pm = pp->mib;
>  	int res;
>  
>  	if (!pm && (BIT(port) & yt921x_info_ports_mask(info))) {
> -		pm = devm_kzalloc(dev, sizeof(*pm), GFP_KERNEL);
> +		pm = kvzalloc_obj(*pm);

Is the if (!pm still needed?

   Andrew

Re: [PATCH net-next] net: dsa: motorcomm: Free the MIB block on port teardown

[netdev-bot+sashiko]

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

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

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

- [Medium] The patch adds a memory leak on the yt921x_dsa_port_setup()
  error path (drivers/net/dsa/motorcomm/chip.c).
- [Low] A comment in yt921x_phylink_mac_link_down()
  (drivers/net/dsa/motorcomm/chip.c) is now false.

--

net: dsa: motorcomm: Free the MIB block on port teardown

The patch moves the per-port MIB block from devm_kzalloc() to
kvzalloc_obj() and frees it in yt921x_dsa_port_teardown(). The next
.setup() then gets zeroed counters after the hardware reset. The
disable/enable pairing of the MIB poll work in port_setup is dropped.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 33e5fc127244d..fd737c3e2e184 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -1994,11 +1994,11 @@ static void yt921x_dsa_port_teardown(struct dsa_switch *ds, int port)
>  	struct yt921x_port *pp = &priv->ports[port];
>  	struct yt921x_mib *pm = pp->mib;
>  
> -	/* get_stats64() might not be synchronized with port_teardown(), hold
> -	 * the MIB block till the device teardown
> -	 */
> -	if (pm)
> +	if (pm) {
>  		disable_delayed_work_sync(&pm->work);
> +		pp->mib = NULL;
> +		kvfree(pm);
> +	}
>  }

[Severity: Low]

This isn't a bug, but the MIB block is now freed here on every tree
teardown, so the comment in yt921x_phylink_mac_link_down() looks stale:

drivers/net/dsa/motorcomm/chip.c:yt921x_phylink_mac_link_down() {
    ...
	/* No need to sync; port control block is hold until device remove */
	pm = priv->ports[port].mib;
	if (pm)
		cancel_delayed_work(&pm->work);
    ...
}

The code still looks safe, but for a different reason. DSA stops and
destroys phylink before .port_teardown():

  user ports:     dsa_user_destroy()->dsa_port_phylink_destroy()
  CPU/DSA ports:  dsa_port_disable_rt()->phylink_stop(), then
                  dsa_shared_port_link_unregister_of()

All of these run before dsa_port_devlink_teardown().

Could this comment be updated to give that reason instead of the old
devm lifetime?

[ ... ]

> @@ -2006,12 +2006,11 @@ static int yt921x_dsa_port_setup(struct dsa_switch *ds, int port)
>  	struct yt921x_priv *priv = dsa_to_yt921x_priv(ds);
>  	struct yt921x_port *pp = &priv->ports[port];
>  	const struct yt921x_info *info = priv->info;
> -	struct device *dev = yt921x_priv_to_device(priv);
>  	struct yt921x_mib *pm = pp->mib;
>  	int res;
>  
>  	if (!pm && (BIT(port) & yt921x_info_ports_mask(info))) {
> -		pm = devm_kzalloc(dev, sizeof(*pm), GFP_KERNEL);
> +		pm = kvzalloc_obj(*pm);
>  		if (!pm)
>  			return -ENOMEM;
>  		pp->mib = pm;

[ ... ]

> @@ -2031,9 +2026,6 @@ static int yt921x_dsa_port_setup(struct dsa_switch *ds, int port)
>  	if (res)
>  		return res;

[Severity: Medium]

Now that the MIB block is no longer devm-managed, does this error path
leak it?

pm is allocated with kvzalloc_obj() and stored in pp->mib before
yt921x_port_setup() runs. yt921x_port_setup() can fail, for example
when a register write fails in yt921x_userport_standalone() or in the
YT921X_PORTn_PRIO_ORD or YT921X_PORTn_ISOLATION writes. In that case
this function returns without freeing pm.

The DSA core doesn't call .port_teardown() after a failed .port_setup():

net/dsa/devlink.c:dsa_port_devlink_setup() {
    ...
		err = ds->ops->port_setup(ds, dp->index);
		if (err)
			return err;
    ...
}

dp->setup stays false, so dsa_tree_teardown_ports()->dsa_port_teardown()
skips this port:

net/dsa/dsa.c:dsa_port_teardown() {
    ...
	if (!dp->setup)
		return;
    ...
}

yt921x_mdio_remove() frees the ACL blocks but not priv->ports[].mib.

With a single switch, dsa_register_switch() fails and so does probe.
The devm-allocated priv, which holds the only pointer to the block, is
then freed.

For user ports, dsa_port_setup_as_unused() retries and reuses the
leftover pm. If that retry also fails, the block leaks the same way.

The commit message says .port_teardown() "pairs with every successful
.port_setup()", which doesn't cover the failed-setup case.

Could this path kvfree(pm) and clear pp->mib when this call did the
allocation? Another option is to allocate only after yt921x_port_setup()
succeeds.

[ ... ]

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