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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 (1) · Low: 1 (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] With this patch, INIT_DELAYED_WORK() runs on every
  yt921x_dsa_setup() call.
- [Medium] After this patch, INIT_DELAYED_WORK(&pp->mib_read,
  yt921x_poll_mib) runs only in yt921x_dsa_setup() (chip.c:4556-4562).
- [Low] The subject line, "move mib start from probe() to dsa_setup()",
  does not describe the change.

Pre-existing issues:
- [Medium] yt921x_dsa_setup() hard-resets the chip (yt921x_chip_reset)
  and yt921x_chip_setup() then clears all hardware MIB counters…
- [Low] yt921x_phylink_mac_link_down() stops MIB polling with non-sync
  cancel_delayed_work(&priv->ports[port].mib_read) (chip.c:3962).

--

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

This patch moves the per-port pp->index assignment and the
INIT_DELAYED_WORK() of the MIB polling work out of yt921x_mdio_probe()
and into the DSA .setup() callback, yt921x_dsa_setup(). The works are
now initialized once per .setup() call rather than once per probe.

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

[Severity: Low]
This isn't a bug, but does the subject line match the change? Neither
probe() nor .setup() starts MIB polling. The patch only moves
INIT_DELAYED_WORK(). Polling starts in yt921x_phylink_mac_link_up(),
which this patch leaves alone:

	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 repeats what the diff does but doesn't say why the change is
needed. It also doesn't mention that yt921x_mdio_remove() still expects
the works to be initialized at probe time until the next patch in the
series.

Could the message give the reason for the change, and either mention
that dependency or squash this with "net: dsa: motorcomm: move mib stop
from remove() to teardown()"?

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 6f2606fd7807..d9d73bb7cb23 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -4553,6 +4553,14 @@ static int yt921x_dsa_setup(struct dsa_switch *ds)
>  	struct device_node *child;
>  	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]
Now that this runs on every yt921x_dsa_setup() call, can it
re-initialize a mib_read work that is still armed or queued?

In a multi-switch tree, removing one member goes through
dsa_switch_remove()->dsa_tree_teardown() for the whole tree. When that
member registers again, ds->ops->setup() runs again on the switches
that stayed.

At this commit, nothing on the teardown side stops mib_read
synchronously. yt921x_dsa_teardown() doesn't touch the work.
yt921x_phylink_mac_link_down() only does a non-sync cancel:

	/* 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);

If the work is still pending when .setup() runs again,
INIT_DELAYED_WORK() resets timer->entry.pprev, work->entry and
work->data while they are still linked into the timer wheel or the pool
worklist. Could that corrupt those lists?

The next patch, "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, but this intermediate commit is still affected.

[Severity: Low]
This is a pre-existing issue, and this patch doesn't introduce it.
Related to the above, can MIB polling keep running on a port whose link
is down?

If yt921x_phylink_mac_link_down() runs while yt921x_poll_mib() is
executing, the pending bit is already clear, so cancel_delayed_work()
does nothing. The handler then re-queues itself with
schedule_delayed_work() after dropping reg_lock. It never checks link
state first.

Polling on that port would then go on until the next teardown or
remove.

> +	}
> +
>  	mutex_lock(&priv->reg_lock);
>  	res = yt921x_chip_reset(priv);
>  	mutex_unlock(&priv->reg_lock);

[Severity: Medium]
This isn't a bug introduced by this patch, but what happens to the
software MIB counters on a second .setup() for the same probe?

yt921x_chip_reset() is followed by yt921x_chip_setup(), which clears
the hardware counters:

	/* Clear MIB */
	ctrl = YT921X_MIB_CTRL_CLEAN | YT921X_MIB_CTRL_ALL_PORT;

pp->mib, pp->rx_frames and pp->tx_frames are only zeroed once, by
devm_kzalloc() in probe. yt921x_read_mib() says "MIB buffer must be
zeroed before", and it extends 32-bit counters like this:

			val = (old_val & ~(u64)U32_MAX) | val0;
			if (val < old_val)
				val += 1ull << 32;

Would the first poll after re-setup read the hardware clear as a wrap
and add 2^32 to each 32-bit counter? The 64-bit counters would go
backwards instead.

Since this loop now runs on every .setup(), could it also reset the
software accumulators?

> @@ -4810,13 +4818,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]
With this loop gone from yt921x_mdio_probe(), can yt921x_mdio_remove()
run on delayed works that were never initialized?

dsa_register_switch() can succeed without ever calling
ds->ops->setup(), because dsa_tree_setup() returns early when the tree
is incomplete:

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

yt921x_mdio_remove() still does this for every port:

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

		disable_delayed_work_sync(&pp->mib_read);
	}

If the device is unbound in that state, the path
disable_delayed_work_sync()->__cancel_work_sync()->__flush_work() would
hit WARN_ON(!work->func) once for each port.

The follow-up patch, "net: dsa: motorcomm: move mib stop from remove()
to teardown()", moves this loop into yt921x_dsa_teardown(). DSA skips
that callback when ds->setup is false, so the final state of the series
doesn't have this problem. Only this intermediate commit breaks
bisection.

Also, in that follow-up patch, yt921x_dsa_teardown() seems to use priv
outside the #if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS) block where priv
is declared. Does it build with CONFIG_NET_DSA_YT921X_LEDS=n?

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

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