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

[Marc Kleine-Budde]

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);
 }
 
 static void gs_update_state(struct gs_can *dev, struct can_frame *cf)
@@ -988,10 +1006,10 @@ 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)
+		gs_usb_timestamp_start(parent);
 
+	if (!parent->active_channels) {
 		for (i = 0; i < GS_MAX_RX_URBS; i++) {
 			u8 *buf;
 
@@ -1102,12 +1120,10 @@ 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) {
+	if (!parent->active_channels)
 		usb_kill_anchored_urbs(&parent->rx_submitted);
-
-		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
-			gs_usb_timestamp_stop(parent);
-	}
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+		gs_usb_timestamp_stop(parent);
 
 	can_rx_offload_disable(&dev->offload);
 	close_candev(netdev);
@@ -1160,12 +1176,11 @@ static int gs_can_close(struct net_device *netdev)
 
 	/* Stop polling */
 	parent->active_channels--;
-	if (!parent->active_channels) {
+	if (!parent->active_channels)
 		usb_kill_anchored_urbs(&parent->rx_submitted);
 
-		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
-			gs_usb_timestamp_stop(parent);
-	}
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+		gs_usb_timestamp_stop(parent);
 
 	/* Stop sending URBs */
 	usb_kill_anchored_urbs(&dev->tx_submitted);
@@ -1526,6 +1541,7 @@ static int gs_usb_probe(struct usb_interface *intf,
 	};
 	struct gs_device_config dconf;
 	unsigned int icount, i;
+	bool ts_supported;
 	int rc;
 
 	rc = usb_find_common_endpoints(intf->cur_altsetting,
@@ -1599,6 +1615,9 @@ static int gs_usb_probe(struct usb_interface *intf,
 		parent->canch[i] = dev;
 		parent->canch[i]->parent = parent;
 
+		if (parent->canch[i]->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+			ts_supported = true;
+
 		/* set RX packet size based on FD and if hardware
 		 * timestamps are supported.
 		 */
@@ -1616,6 +1635,9 @@ static int gs_usb_probe(struct usb_interface *intf,
 		parent->hf_size_rx = max(parent->hf_size_rx, hf_size_rx);
 	}
 
+	if (ts_supported)
+		gs_usb_timestamp_init(parent);
+
 	for (i = 0; i < icount; i++) {
 		struct gs_can *dev = parent->canch[i];
 

-- 
2.53.0