Re: [PATCH net v3 4/5] can: rockchip_canfd: prevent timestamp work from rearming after bus-off

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] can: rockchip_canfd: torn read and data race on timecounter state
--

commit ba848a69df4d87c5d955ac1607d9318a5bab577e
Author: Cunhao Lu <1579567540@qq.com>

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

This commit adds a timestamp_lock and timestamp_active flag to prevent the
timestamp work from requeuing itself after cancellation. It synchronizes the
worker's active check, timecounter read, and self-requeue.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
> index ad582afbbd682..0fa791849c957 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)
>  {
>  	const struct delayed_work *delayed_work = to_delayed_work(work);
>  	struct rkcanfd_priv *priv;
> +	unsigned long flags;
>  
>  	priv = container_of(delayed_work, struct rkcanfd_priv, timestamp);
> -	timecounter_read(&priv->tc);
> -
> -	schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
> +	spin_lock_irqsave(&priv->timestamp_lock, flags);
> +	if (priv->timestamp_active) {
> +		timecounter_read(&priv->tc);

[Severity: Medium]
Does this leave a data race with datapath readers?

The worker thread updates the timecounter state here while holding
timestamp_lock. However, the RX/TX datapath calls
rkcanfd_skb_set_timestamp() which reads the timecounter locklessly:

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

Since timecounter_read() modifies the timecounter state non-atomically,
can the concurrent lockless read result in torn reads or an inconsistent
observation of the cycle counter and nanoseconds, especially on 32-bit
platforms?

> +
> +		/* Serialize requeueing with timestamp_stop(). */
> +		schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
> +	}
> +	spin_unlock_irqrestore(&priv->timestamp_lock, flags);
>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-rockchip-canfd-restart-close-v3-0-f0eb41090242@qq.com?part=4