Re: [PATCH net v9 09/10] can: rockchip_canfd: prevent timestamp work from rearming after bus-off

[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 "can: rockchip_canfd: prevent timestamp work from rearming
  after bus-off" is tagged Cc: stable but does not say it depends on the…

--

can: rockchip_canfd: prevent timestamp work from rearming after bus-off

After bus-off, the rockchip_canfd timestamp worker could requeue itself
and keep polling. This patch switches the worker to disable_delayed_work()
and enable_delayed_work() to stop that. It also adds timestamp_lock and
timestamp_active so counter reads are serialized with start and stop, and
rkcanfd_timestamp_stop_sync() now calls the non-sync stop before it
cancels the work.

> Keep workqueue operations outside timestamp_lock; the IRQ and restart
> paths already serialize start and stop. Synchronous stop waits for the
> callback before reinitialization or clock shutdown.
>
> Fixes: 4e1a18bab124 ("can: rockchip_canfd: add hardware timestamping support")
> Cc: stable@vger.kernel.org

[Severity: Low]
This doesn't affect the current tree, but should this patch say that
it depends on the previous commit, "can: rockchip_canfd: serialize TX
with bus-off and restart"?

That commit created the one-time probe helper that this patch extends.
It moved INIT_DELAYED_WORK() out of rkcanfd_timestamp_init(), which
runs on every chip start. With this patch, rkcanfd_timestamp_work_init()
in rockchip_canfd-timestamp.c becomes:

	spin_lock_init(&priv->timestamp_lock);
	priv->timestamp_active = false;
	INIT_DELAYED_WORK(&priv->timestamp, rkcanfd_timestamp_work);
	disable_delayed_work(&priv->timestamp);

Three calls need to stay balanced: the disable at probe,
enable_delayed_work() in rkcanfd_timestamp_start(), and
disable_delayed_work() in rkcanfd_timestamp_stop(). That only works if
the work is initialized exactly once.

The sentence "the IRQ and restart paths already serialize start and
stop" also relies on that commit. It added the restart quiescing in
rkcanfd_set_mode() for CAN_MODE_START:

	WRITE_ONCE(priv->irq_suspended, true);
	/* Wait for handlers that already passed the suspended check. */
	synchronize_irq(ndev->irq);
	rkcanfd_timestamp_stop_sync(priv);

Without the prerequisite, this patch won't apply to stable trees.
Suppose a backport fixes the conflict by hand and leaves
INIT_DELAYED_WORK() in rkcanfd_timestamp_init(). Then every chip start
resets the disable count to 0 just before enable_delayed_work() runs.
That would trigger the workqueue disable count underflow warning and
undo the requeue blocking added here.

The prerequisite is in the same series and is also Cc: stable, so
stable tooling would normally apply both in order. Could the dependency
still be made explicit? A prerequisite note on the Cc: stable line, as
stable-kernel-rules.rst describes, or a "Backports must include ..."
line would do it.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-rockchip-canfd-restart-close-v9-0-6643f3825ba3%40qq.com