Re: [PATCH net v8 10/10] can: rockchip_canfd: serialize timecounter timestamp conversion

[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] The fix only works because the preceding series patch "can:
  rockchip_canfd: prevent timestamp work from rearming after bus-off"…

--

can: rockchip_canfd: serialize timecounter timestamp conversion

The commit takes timestamp_lock around timecounter_cyc2time() in
rkcanfd_skb_set_timestamp(), so a conversion can no longer run while the
timestamp worker is inside timecounter_read(). It also updates the
comment on timestamp_lock in struct rkcanfd_priv to say that the lock
covers tc.

> Hold timestamp_lock across timecounter_cyc2time(), using the same lock
> as timecounter_read() and timecounter_init(). Keep the SKB timestamp
> assignment outside the critical section.
>
> TX completion nests timestamp_lock inside tx_lock; no timestamp-lock
> holder acquires tx_lock.
>
> Fixes: 4e1a18bab124 ("can: rockchip_canfd: add hardware timestamping support")
> Cc: stable@vger.kernel.org

[Severity: Low]
Should this commit message state its backport prerequisites?

At 4e1a18bab124, struct rkcanfd_priv has no timestamp_lock and no
timestamp_active field. Also, rkcanfd_timestamp_work() calls
timecounter_read(&priv->tc) with no lock held before it calls
schedule_delayed_work().

The statement "using the same lock as timecounter_read() and
timecounter_init()" is only true once the preceding patch in this
series is applied:

  806691a20048 ("can: rockchip_canfd: prevent timestamp work from
  rearming after bus-off")

That patch adds the lock and the active flag. It also adds the locking
in rkcanfd_timestamp_work() and rkcanfd_timestamp_start():

drivers/net/can/rockchip/rockchip_canfd-timestamp.c:rkcanfd_timestamp_work() {
    ...
	spin_lock_irqsave(&priv->timestamp_lock, flags);
	if (priv->timestamp_active) {
		timecounter_read(&priv->tc);
    ...
}

The paragraph about lock nesting also depends on tx_lock. That lock comes
from the earlier patches "can: rockchip_canfd: serialize TX state and
command writes" and "can: rockchip_canfd: serialize TX with bus-off and
restart".

A plain cherry-pick onto a tree without 806691a20048 would fail to build,
and that patch carries the same Fixes: tag and Cc: stable. But suppose
someone adapts this patch by hand and only adds a lock in
rkcanfd_skb_set_timestamp(). The timecounter_read() in the worker would
still run unlocked, so the race would stay open.

Other patches in this series include a note such as "Backports must
include the preceding TX serialization fix...". Could a similar note be
added here, naming 806691a20048 and the tx_lock patches as prerequisites?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108-rockchip-canfd-restart-close-v8-0-3e7a23fd9eae%40qq.com