Re: [PATCH net-next v8 4/6] net: dsa: motorcomm: move mib stop from remove() to teardown()

[netdev-bot+sashiko@kernel.org]

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] Commit message inaccuracy plus fragile use of a counting-disable
  API.

--

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

The per-port MIB polling work is now disabled in yt921x_dsa_teardown()
instead of yt921x_mdio_remove(). The loop calling
disable_delayed_work_sync() on priv->ports[i].mib_read is moved verbatim
from the mdio remove path into the DSA teardown op.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 167b208357710..2412fe2f696f8 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -4233,6 +4233,11 @@ static void yt921x_dsa_teardown(struct dsa_switch *ds)
>  #if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
>  	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: Low]
The changelog says this change "ensures symmetry with the MIB start
operation, which is already invoked in the setup()".  Is that accurate?
The only MIB work operation in yt921x_dsa_setup() is the initialization:

	INIT_DELAYED_WORK(&pp->mib_read, yt921x_poll_mib);

The polling is actually started from yt921x_phylink_mac_link_up():

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

and re-armed by yt921x_poll_mib() itself:

	schedule_delayed_work(&pp->mib_read, delay);

So what the new teardown call pairs with looks like the work
initialization rather than a start.  Would it be worth rewording that
sentence?

A second question about the API choice here.  disable_delayed_work_sync()
is a counting disable:

kernel/workqueue.c:disable_delayed_work_sync() {
	return __cancel_work_sync(&dwork->work,
				  WORK_CANCEL_DELAYED | WORK_CANCEL_DISABLE);
}

The disable depth it leaves in work->data is only cleared by
enable_delayed_work(), and there is no enable_delayed_work() call anywhere
under drivers/net/dsa/motorcomm/.  ds->ops->teardown is repeatable —
dsa_switch_teardown() runs it on unregister and the dsa_switch_setup()
error path runs it too — and a later ->setup() can run on the same priv.

Today this still works only because yt921x_dsa_setup() re-runs
INIT_DELAYED_WORK() for every port, which resets work->data and silently
drops the accumulated disable count.  If that one-shot init is ever moved
back to probe(), which is the more usual DSA pattern, would the
schedule_delayed_work() in yt921x_phylink_mac_link_up() become a no-op
after the first teardown/setup cycle, freezing the per-port MIB counters?

Would cancel_delayed_work_sync() in teardown (or an explicit
enable_delayed_work() in setup) make the pairing symmetric without
depending on the re-init side effect?

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