Re: [PATCH net v8 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] The patch is tagged Cc: stable but does not say that it depends
  on the preceding commit "can: rockchip_canfd: serialize TX with bus-…
- [Low] The commit message gives only one consequence: the worker can
  "keep polling while the controller remains bus-off".

Pre-existing issues:
- [Medium] Concern: in this commit, rkcanfd_skb_set_timestamp() reads
  priv->tc through timecounter_cyc2time() from hardirq context without…

--

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

This adds timestamp_lock and a timestamp_active flag to rkcanfd_priv.
With them, the timestamp worker reads the timecounter and requeues itself
only while timestamping is active. The start and stop helpers set or clear
the flag under the lock, and the work is cancelled outside it.

> The bus-off IRQ cancels timestamp work asynchronously. If the worker
> is already running, it can requeue itself after cancellation and keep
> polling while the controller remains bus-off.

[Severity: Low]
Can the commit message name a user-visible consequence of this polling?

The only trigger is the bus-off path:

rkcanfd_handle_state_error_int()
  rkcanfd_chip_stop()
    rkcanfd_timestamp_stop()
      cancel_delayed_work()

As far as I can tell, every way out of bus-off already stops the worker
synchronously:

- Restart through rkcanfd_set_mode() calls rkcanfd_timestamp_stop_sync()
  before rkcanfd_chip_start().
- Close goes through rkcanfd_chip_stop_sync().

In both cases cancel_delayed_work_sync() handles a work item that requeues
itself.

Runtime PM is held until after rkcanfd_chip_stop_sync() in rkcanfd_stop().
That means the stray worker never reads a clock-gated controller.

Anything it adds to priv->tc is thrown away when rkcanfd_timestamp_start()
runs timecounter_init() again.

Given the Cc: stable tag, could the message either describe a concrete
consequence or say that this is a hardening change?

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
> index ad582afbbd682..1d2a122770dc8 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
> @@ -30,18 +30,27 @@ static void rkcanfd_timestamp_work(struct work_struct *work)

[ ... ]

>  void rkcanfd_timestamp_work_init(struct rkcanfd_priv *priv)
>  {
> +	spin_lock_init(&priv->timestamp_lock);
> +	priv->timestamp_active = false;
>  	INIT_DELAYED_WORK(&priv->timestamp, rkcanfd_timestamp_work);
>  }
>  
> +/* The caller must quiesce IRQs and timestamp work before reconfiguration. */
>  void rkcanfd_timestamp_init(struct rkcanfd_priv *priv)

[Severity: Low]
Should the commit message say that this depends on the preceding commit,
"can: rockchip_canfd: serialize TX with bus-off and restart"?

That commit added rkcanfd_timestamp_work_init() and its one-time call from
rkcanfd_probe(). It also moved INIT_DELAYED_WORK() out of
rkcanfd_timestamp_init().

Before it, the work is initialized on every rkcanfd_chip_start(), which
means on every open and every restart:

rkcanfd_timestamp_init() {
    ...
	priv->work_delay_jiffies = div_u64(work_delay_ns, 3u * NSEC_PER_SEC / HZ);
	INIT_DELAYED_WORK(&priv->timestamp, rkcanfd_timestamp_work);
    ...
}

The new comment on rkcanfd_timestamp_init() also relies on code from that
commit: the synchronize_irq() and rkcanfd_timestamp_stop_sync() calls in
rkcanfd_set_mode().

On its own, this patch won't apply to stable trees. An obvious manual
backport would put spin_lock_init() next to the old INIT_DELAYED_WORK().
That would re-initialize timestamp_lock on every restart, while an old
worker may still hold it.

Other patches in this series have a "Backports must include ..."
paragraph. Would a similar note help here?

[ ... ]

> diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
> index 3664124a8da25..078876769a05d 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd.h
> +++ b/drivers/net/can/rockchip/rockchip_canfd.h
> @@ -477,6 +477,8 @@ struct rkcanfd_priv {
>  
>  	/* Serialize TX submission/completion with bus-off and restart. */
>  	spinlock_t tx_lock;
> +	/* Protects timestamp_active and worker reads/requeueing. */
> +	spinlock_t timestamp_lock;

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch.
priv->tc still has a reader that does not take the lock.
rkcanfd_skb_set_timestamp() calls timecounter_cyc2time() from hardirq
context without timestamp_lock:

rkcanfd_skb_set_timestamp() {
    ...
	ns = timecounter_cyc2time(&priv->tc, timestamp);
    ...
}

Meanwhile, timecounter_read() in rkcanfd_timestamp_work() and
timecounter_init() in rkcanfd_timestamp_start() now change tc under
timestamp_lock.

If a conversion on another CPU races with the worker, can it combine a
new cycle_last with an old nsec? That would give wrong or non-monotonic
hardware timestamps on RX, TX-done and error frames.

The last patch in the series fixes this: "can: rockchip_canfd: serialize
timecounter timestamp conversion". It takes timestamp_lock around
timecounter_cyc2time() in rkcanfd_skb_set_timestamp(). It also updates
this comment to "Protects tc, timestamp_active and worker requeueing."

[ ... ]

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