Re: [PATCH can] can: gs_usb: fix hardware timestamp state for mixed channels

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 18.07.2026 06:28, Shuangpeng Bai wrote:
> The hardware timestamp state is shared by struct gs_usb, but gs_can_open()
> and gs_can_close() tie its initialization and teardown to active_channels
> and to the feature bits of the channel being opened or closed.
> 
> This is wrong for mixed-channel devices in both directions. If a
> non-timestamp channel opens first, a later timestamp-capable channel does
> not initialize the shared cyclecounter/timecounter because active_channels
> is already non-zero. Timestamp RX then calls timecounter_cyc2time() with
> parent->tc.cc unset.
> 
> Conversely, if a timestamp-capable channel opens first and starts the
> shared delayed work, then closes while a non-timestamp channel remains
> active, disconnect may close the non-timestamp channel last. The old
> teardown check skips gs_usb_timestamp_stop() in that case and frees
> struct gs_usb while the delayed work timer is still queued.
> 
> Track whether the shared timestamp work has been started in struct
> gs_usb. Start it whenever opening a timestamp-capable channel and the
> shared state is not started yet, and stop it when the last channel closes
> regardless of that channel's feature bits. Initialize the delayed work and
> lock once during probe.

With this code, the gs_usb_timestamp_work will run even if there are no more
timestamp-capable channels are open (after multiple open/close calls). I think
it would be better to implement counter of timestamp-capable channels and rework
timestamp code to use this counter as an indicator of when to start/stop worker.

> 
> Fixes: 45dfa45f52e6 ("can: gs_usb: add RX and TX hardware timestamp support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
> ---
>   drivers/net/can/usb/gs_usb.c | 30 ++++++++++++++++++++----------
>   1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index ec9a7cbbbc69..bf956fef0d19 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;
> +	bool timestamp_started;
>   	u8 channel_cnt;
>   
>   	unsigned int pipe_in;
> @@ -480,19 +481,23 @@ static void gs_usb_timestamp_init(struct gs_usb *parent)
>   	cc->shift = 32 - bits_per(NSEC_PER_SEC / GS_USB_TIMESTAMP_TIMER_HZ);
>   	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);
>   	schedule_delayed_work(&parent->timestamp,
>   			      GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ);
> +
> +	parent->timestamp_started = true;
>   }
>   
>   static void gs_usb_timestamp_stop(struct gs_usb *parent)
>   {
> +	if (!parent->timestamp_started)
> +		return;
> +
>   	cancel_delayed_work_sync(&parent->timestamp);
> +	parent->timestamp_started = false;
>   }
>   
>   static void gs_update_state(struct gs_can *dev, struct can_frame *cf)
> @@ -959,6 +964,7 @@ static int gs_can_open(struct net_device *netdev)
>   	struct urb *urb = NULL;
>   	u32 ctrlmode;
>   	u32 flags = 0;
> +	bool timestamp_started = false;
>   	int rc, i;
>   
>   	rc = open_candev(netdev);
> @@ -980,10 +986,13 @@ static int gs_can_open(struct net_device *netdev)
>   
>   	can_rx_offload_enable(&dev->offload);
>   
> -	if (!parent->active_channels) {
> -		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
> -			gs_usb_timestamp_init(parent);
> +	if ((dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) &&
> +	    !parent->timestamp_started) {
> +		gs_usb_timestamp_init(parent);
> +		timestamp_started = true;
> +	}
>   
> +	if (!parent->active_channels) {
>   		for (i = 0; i < GS_MAX_RX_URBS; i++) {
>   			u8 *buf;
>   
> @@ -1094,11 +1103,11 @@ static int gs_can_open(struct net_device *netdev)
>   out_usb_free_urb:
>   	usb_free_urb(urb);
>   out_usb_kill_anchored_urbs:
> +	if (timestamp_started)
> +		gs_usb_timestamp_stop(parent);
> +
>   	if (!parent->active_channels) {
>   		usb_kill_anchored_urbs(&parent->rx_submitted);
> -
> -		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
> -			gs_usb_timestamp_stop(parent);
>   	}
>   
>   	can_rx_offload_disable(&dev->offload);
> @@ -1155,8 +1164,7 @@ static int gs_can_close(struct net_device *netdev)
>   	if (!parent->active_channels) {
>   		usb_kill_anchored_urbs(&parent->rx_submitted);
>   
> -		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
> -			gs_usb_timestamp_stop(parent);
> +		gs_usb_timestamp_stop(parent);
>   	}
>   
>   	/* Stop sending URBs */
> @@ -1577,6 +1585,8 @@ static int gs_usb_probe(struct usb_interface *intf,
>   	parent->channel_cnt = icount;
>   
>   	init_usb_anchor(&parent->rx_submitted);
> +	spin_lock_init(&parent->tc_lock);
> +	INIT_DELAYED_WORK(&parent->timestamp, gs_usb_timestamp_work);
>   
>   	usb_set_intfdata(intf, parent);
>   	parent->udev = udev;