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

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

[Shuangpeng Bai]

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.

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;
-- 
2.43.0

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

[Vadim Fedorenko]

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;

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

[Shuangpeng Bai]

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.

Count the number of active timestamp-capable channels. Start the shared
timestamp worker when the first such channel opens, stop it when the last
such channel closes, and unwind the count if open fails.

Fixes: 45dfa45f52e6 ("can: gs_usb: add RX and TX hardware timestamp support")
Cc: stable@vger.kernel.org
Suggested-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
---
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.

 drivers/net/can/usb/gs_usb.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index ec9a7cbbbc69..9cc197803e0d 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;
@@ -980,10 +981,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)
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
+		if (!parent->active_timestamp_channels)
 			gs_usb_timestamp_init(parent);
+		parent->active_timestamp_channels++;
+	}
 
+	if (!parent->active_channels) {
 		for (i = 0; i < GS_MAX_RX_URBS; i++) {
 			u8 *buf;
 
@@ -1094,13 +1098,15 @@ static int gs_can_open(struct net_device *netdev)
 out_usb_free_urb:
 	usb_free_urb(urb);
 out_usb_kill_anchored_urbs:
-	if (!parent->active_channels) {
-		usb_kill_anchored_urbs(&parent->rx_submitted);
-
-		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
+		parent->active_timestamp_channels--;
+		if (!parent->active_timestamp_channels)
 			gs_usb_timestamp_stop(parent);
 	}
 
+	if (!parent->active_channels)
+		usb_kill_anchored_urbs(&parent->rx_submitted);
+
 	can_rx_offload_disable(&dev->offload);
 	close_candev(netdev);
 
@@ -1152,13 +1158,15 @@ static int gs_can_close(struct net_device *netdev)
 
 	/* Stop polling */
 	parent->active_channels--;
-	if (!parent->active_channels) {
-		usb_kill_anchored_urbs(&parent->rx_submitted);
-
-		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
+		parent->active_timestamp_channels--;
+		if (!parent->active_timestamp_channels)
 			gs_usb_timestamp_stop(parent);
 	}
 
+	if (!parent->active_channels)
+		usb_kill_anchored_urbs(&parent->rx_submitted);
+
 	/* Stop sending URBs */
 	usb_kill_anchored_urbs(&dev->tx_submitted);
 	atomic_set(&dev->active_tx_urbs, 0);
-- 
2.43.0

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

[Vadim Fedorenko]

On 19.07.2026 04:00, 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.
> 
> Count the number of active timestamp-capable channels. Start the shared
> timestamp worker when the first such channel opens, stop it when the last
> such channel closes, and unwind the count if open fails.
> 
> Fixes: 45dfa45f52e6 ("can: gs_usb: add RX and TX hardware timestamp support")
> Cc: stable@vger.kernel.org
> Suggested-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

no need for this tag, I didn't suggest the idea

> Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
> ---
> 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.
> 
>   drivers/net/can/usb/gs_usb.c | 28 ++++++++++++++++++----------
>   1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index ec9a7cbbbc69..9cc197803e0d 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;
> @@ -980,10 +981,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)
> +	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
> +		if (!parent->active_timestamp_channels)
>   			gs_usb_timestamp_init(parent);
> +		parent->active_timestamp_channels++;

I think it would be better to move active_timestamp_channels management into
gs_usb_timestamp_init()/gs_usb_timestamp_stop() - it will simplify code in main
functions.

Please, start new thread for each version of your patch.


> +	}
>   
> +	if (!parent->active_channels) {
>   		for (i = 0; i < GS_MAX_RX_URBS; i++) {
>   			u8 *buf;
>   
> @@ -1094,13 +1098,15 @@ static int gs_can_open(struct net_device *netdev)
>   out_usb_free_urb:
>   	usb_free_urb(urb);
>   out_usb_kill_anchored_urbs:
> -	if (!parent->active_channels) {
> -		usb_kill_anchored_urbs(&parent->rx_submitted);
> -
> -		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
> +	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
> +		parent->active_timestamp_channels--;
> +		if (!parent->active_timestamp_channels)
>   			gs_usb_timestamp_stop(parent);
>   	}
>   
> +	if (!parent->active_channels)
> +		usb_kill_anchored_urbs(&parent->rx_submitted);
> +
>   	can_rx_offload_disable(&dev->offload);
>   	close_candev(netdev);
>   
> @@ -1152,13 +1158,15 @@ static int gs_can_close(struct net_device *netdev)
>   
>   	/* Stop polling */
>   	parent->active_channels--;
> -	if (!parent->active_channels) {
> -		usb_kill_anchored_urbs(&parent->rx_submitted);
> -
> -		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
> +	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
> +		parent->active_timestamp_channels--;
> +		if (!parent->active_timestamp_channels)
>   			gs_usb_timestamp_stop(parent);
>   	}
>   
> +	if (!parent->active_channels)
> +		usb_kill_anchored_urbs(&parent->rx_submitted);
> +
>   	/* Stop sending URBs */
>   	usb_kill_anchored_urbs(&dev->tx_submitted);
>   	atomic_set(&dev->active_tx_urbs, 0);