Re: [PATCH can-next v5 6/6] can: gs_usb: fix hardware timestamp state for mixed channels

[Vadim Fedorenko]

On 08/11/2026 20:47, Marc Kleine-Budde wrote:
> From: Shuangpeng Bai <shuangpeng.kernel@xxxxxxxxx>
>
> For hardware timestamping, the driver manages the
> infrastructure (cyclecounter/timecounter) for the entire USB interface,
> assuming that all channels share a common time base, even though each
> channel individually indicates support for hardware timestamping via the
> GS_CAN_FEATURE_HW_TIMESTAMP flag.
>
> Currently both initialization and shutdown of the HW timestamping
> infrastructure is tied to the flags of the first channel opened
> respectively closed. This leads to problems if the first channel opened
> doesn't have the timestamping feature, but the second channel has.
>
> Keep the initialization of the HW timestamping infrastructure in
> gs_usb_timestamp_init(), and move initial time init and the initial
> scheduling of the delayed worker into gs_usb_timestamp_start().
>
> Initialize the HW timestamping infrastructure in gs_usb_probe() using
> gs_usb_timestamp_init() if at least one channel has the
> GS_CAN_FEATURE_HW_TIMESTAMP flag set.
>
> Extend the functions gs_usb_timestamp_start()/gs_usb_timestamp_stop()
> to track the number of active timestamp channels. Enabling or disabling the
> delayed worker prevents concurrent rescheduling while
> gs_usb_timestamp_stop() is running.
>
> Call gs_usb_timestamp_start()/gs_usb_timestamp_stop() based on the
> channel's GS_CAN_FEATURE_HW_TIMESTAMP flag, rather than relying on the
> number of active channels.
>
> Reported-by: Shuangpeng Bai <shuangpeng.kernel@xxxxxxxxx>
> Closes: https://sashiko.dev/#/bug/linux-b39360f9-6cd8-4f47-83bb-f0f78331f308
> Closes: https://sashiko.dev/#/bug/linux-5f8ecd4e-344e-4ceb-9b46-886f4942a5f6
> Fixes: 5886e4d5ecec ("can: gs_usb: fix time stamp counter initialization")
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Shuangpeng Bai <shuangpeng.kernel@xxxxxxxxx>
> Cc: Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx>
> Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@xxxxxxxxx>
> Co-developed-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> ---
> Changes in v4:
> - switch to enable_delayed_work()/disable_delayed_work_sync()
> - move starting from gs_usb_timestamp_init() into gs_usb_timestamp_start()
> - polish ref-counting/locking
> - Link to v3: https://patch.msgid.link/20260720184042.1712217-1-shuangpeng.kernel@xxxxxxxxx/
>
> Changes in v3:
> - Drop Suggested-by tag.
> - Move active_timestamp_channels management into the timestamp init/stop
>    helpers.
> - Protect active_timestamp_channels with tc_lock, and keep it zero until
>    timecounter_init() has completed.
> - Make RX timestamp conversion and the delayed work skip the timecounter
>    while no timestamp-capable channel is active.
> - Initialize the shared timestamp lock and delayed work once during probe.
>
> Changes in v2:
> - Count active timestamp-capable channels instead of tracking only whether
>    the shared timestamp worker has been started.
> - Stop the shared timestamp worker when the last timestamp-capable channel
>    closes, even if non-timestamp channels remain open.
> - Unwind the timestamp-capable channel count on gs_can_open() failures.
>
> ---
>
> Picking up Shuangpeng Bai's work, I've ported it on top of other fixes for
> problems identified by sashiko.
>
> Marc
> ---
>   drivers/net/can/usb/gs_usb.c | 54 +++++++++++++++++++++++++++++++-------------
>   1 file changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 6327069f63c9..0a1cd14c940c 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -337,6 +337,7 @@ struct gs_usb {
>   
>   	unsigned int hf_size_rx;
>   	u8 active_channels;
> +	u8 active_timestamp_channels;
>   	u8 channel_cnt;
>   
>   	unsigned int pipe_in;
> @@ -481,18 +482,35 @@ static void gs_usb_timestamp_init(struct gs_usb *parent)
>   	cc->mult = clocksource_hz2mult(GS_USB_TIMESTAMP_TIMER_HZ, cc->shift);
>   
>   	spin_lock_init(&parent->tc_lock);
> -	spin_lock_bh(&parent->tc_lock);
> -	timecounter_init(&parent->tc, &parent->cc, ktime_get_real_ns());
> -	spin_unlock_bh(&parent->tc_lock);
>   
>   	INIT_DELAYED_WORK(&parent->timestamp, gs_usb_timestamp_work);
> +	disable_delayed_work(&parent->timestamp);
> +}
> +
> +static void gs_usb_timestamp_start(struct gs_usb *parent)
> +{
> +	scoped_guard(spinlock_bh, &parent->tc_lock) {
> +		parent->active_timestamp_channels++;
> +		if (parent->active_timestamp_channels > 1)
> +			return;
> +
> +		timecounter_init(&parent->tc, &parent->cc, ktime_get_real_ns());
> +	}
> +
> +	enable_delayed_work(&parent->timestamp);
>   	schedule_delayed_work(&parent->timestamp,
>   			      GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ);
>   }
>   
>   static void gs_usb_timestamp_stop(struct gs_usb *parent)
>   {
> -	cancel_delayed_work_sync(&parent->timestamp);
> +	scoped_guard(spinlock_bh, &parent->tc_lock) {
> +		parent->active_timestamp_channels--;
> +		if (parent->active_timestamp_channels > 0)
> +			return;
> +	}
> +
> +	disable_delayed_work_sync(&parent->timestamp);
>   }

That's quite defensive code. You have both disable/enable delayed_work
and active_timestamp_channels counter just to be sure to schedule
timestamp worker only once. Just wonder cannot we keep this worker
running regardless of channels?