[PATCH can-next v5 0/6] gs_usb: fixes

[PATCH can-next v5 0/6] gs_usb: fixes

[Marc Kleine-Budde]

Hello,

this series contains fixes for issues found by sashiko. Also picked up the
patches by Ciprian Marian Costea and Shuangpeng Bai and integrated them
into the series.

regards,
Marc

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Changes in v5:
- patch #1: add new patch
- patch #3: gs_usb_disconnect(): first unregister_candev() all CAN
  interfaces, then call gs_destroy_candev()
- patch #4: update Fixes tag (pointed out by sashiko)
- patch #4: use stats pointer directly (pointed out by sashiko)
- patch #5: add new patch
- Link to v4: https://patch.msgid.link/20261026-gs_usb-fixes-v4-0-59e3b6360053@pengutronix.de

Changes in v4:
- patch #2: fix cleanup if register_candev() fails (pointed out by sashiko)
- Link to v3: https://patch.msgid.link/20261026-gs_usb-fixes-v3-0-3a68f1f62813@pengutronix.de

Changes in v3:
- patch #2: fix cleanup if register_candev() fails (pointed out by sashiko)
- Link to v2: https://patch.msgid.link/20261025-gs_usb-fixes-v2-0-535d45097f72@pengutronix.de

Changes in v2:
- patch #2: add missing cleanup if register_candev() fails (pointed out by sashiko)
- patch #3: do not increase rx_errors during OOM (pointed out by sashiko)
- Link to v1: https://patch.msgid.link/20261025-gs_usb-fixes-v1-0-f74ebbd4da36@pengutronix.de

---
Ciprian Marian Costea (1):
      can: gs_usb: gs_make_candev(): check can_rx_offload_add_manual() return value

Marc Kleine-Budde (4):
      can: gs_usb: gs_can_start_xmit(): don't leak uninitialized memory over USB
      can: gs_usb: gs_usb_probe(): init netdev completely before register_candev()
      can: gs_usb: gs_usb_receive_bulk_callback(): check for overflow flag if SKB allocation fails
      can: gs_usb: gs_usb_receive_bulk_callback(): add missing call to can_rx_offload_irq_finish

Shuangpeng Bai (1):
      can: gs_usb: fix hardware timestamp state for mixed channels

 drivers/net/can/usb/gs_usb.c | 143 +++++++++++++++++++++++++++++--------------
 1 file changed, 96 insertions(+), 47 deletions(-)
---
base-commit: fa5196392691051963726787944db872420dfa75
base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git main
change-id: 20261025-gs_usb-fixes-250f38833107

Best regards,
--  
Marc Kleine-Budde <mkl@pengutronix.de>

[PATCH can-next v5 1/6] can: gs_usb: gs_make_candev(): check can_rx_offload_add_manual() return value

[Marc Kleine-Budde]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

gs_make_candev() ignores the return value of can_rx_offload_add_manual().
In an upcoming series can_rx_offload will be improved and
can_rx_offload_add_manual() can fail, leaving offload->skb_irq_queue ==
NULL.

Check the return value and exit via the existing free_candev() on error
path.

Fixes: 24bc41b45583 ("can: gs_usb: convert to NAPI/rx-offload to avoid OoO reception")
Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
Link: https://patch.msgid.link/20260907104942.17089-4-ciprianmarian.costea@oss.nxp.com
[mkl: update patch description]
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 3b9b2f104d86..cac4ce323a1d 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -1487,7 +1487,10 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 		dev->can.fd.data_bittiming_const = &dev->data_bt_const;
 	}
 
-	can_rx_offload_add_manual(netdev, &dev->offload, GS_NAPI_WEIGHT);
+	rc = can_rx_offload_add_manual(netdev, &dev->offload, GS_NAPI_WEIGHT);
+	if (rc)
+		goto out_free_candev;
+
 	SET_NETDEV_DEV(netdev, &intf->dev);
 
 	rc = register_candev(dev->netdev);

-- 
2.53.0

[PATCH can-next v5 2/6] can: gs_usb: gs_can_start_xmit(): don't leak uninitialized memory over USB

[Marc Kleine-Budde]

The driver allocates the transmission buffer using kmalloc(), which means
the memory remains uninitialized. Then various headers are filled and, but
only the actual CAN frame data payload (of length cfd->len or cf->len) is
copied into the buffer, leaving the remaining buffer uninitialized.

Fix this information leak, allocate the buffer with kzalloc().

Closes: https://sashiko.dev/#/bug/linux-0a8c0e8a-c623-426a-8db3-f55111f6cd84
Fixes: 26949ac935e3 ("can: gs_usb: add CAN-FD support")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index cac4ce323a1d..aa90cc58f28b 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -861,7 +861,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb,
 	if (!urb)
 		goto nomem_urb;
 
-	hf = kmalloc(dev->hf_size_tx, GFP_ATOMIC);
+	hf = kzalloc(dev->hf_size_tx, GFP_ATOMIC);
 	if (!hf)
 		goto nomem_hf;
 

-- 
2.53.0

[PATCH can-next v5 3/6] can: gs_usb: gs_usb_probe(): init netdev completely before register_candev()

[Marc Kleine-Budde]

In gs_usb_probe(), gs_make_candev() is called to allocate and set up the
CAN channel, as well as to call register_candev(). However, gs_usb_probe()
defers the assignment of dev->parent and parent->hf_size_rx until after
gs_make_candev() returns. This opens a race window for the user space to
bring up the CAN interface, while it's not completely setup.

To close the race window, initialize all CAN interfaces in a first loop and
then register them in a second loop: Don't call register_candev() from
gs_make_candev() but explicitly from gs_usb_probe(). Adjust
gs_destroy_candev() likewise, move unregister_candev() to
gs_usb_disconnect() and the error paths in gs_usb_probe(), to keep it
inverse to symmetric to gs_make_candev().

Closes: https://sashiko.dev/#/bug/linux-be54de7d-0b75-408c-b3ee-dd90c9756857
Closes: https://sashiko.dev/#/bug/linux-71076d59-0843-4749-a5ab-fdd98a08582f
Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 64 +++++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 24 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index aa90cc58f28b..1527163e8c35 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -1493,18 +1493,8 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 
 	SET_NETDEV_DEV(netdev, &intf->dev);
 
-	rc = register_candev(dev->netdev);
-	if (rc) {
-		dev_err(&intf->dev,
-			"Couldn't register candev for channel %d (%pe)\n",
-			channel, ERR_PTR(rc));
-		goto out_can_rx_offload_del;
-	}
-
 	return dev;
 
-out_can_rx_offload_del:
-	can_rx_offload_del(&dev->offload);
 out_free_candev:
 	free_candev(dev->netdev);
 	return ERR_PTR(rc);
@@ -1512,7 +1502,6 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 
 static void gs_destroy_candev(struct gs_can *dev)
 {
-	unregister_candev(dev->netdev);
 	can_rx_offload_del(&dev->offload);
 	free_candev(dev->netdev);
 }
@@ -1590,21 +1579,16 @@ static int gs_usb_probe(struct usb_interface *intf,
 
 	for (i = 0; i < icount; i++) {
 		unsigned int hf_size_rx = 0;
+		struct gs_can *dev;
 
-		parent->canch[i] = gs_make_candev(i, intf, &dconf);
-		if (IS_ERR_OR_NULL(parent->canch[i])) {
+		dev = gs_make_candev(i, intf, &dconf);
+		if (IS_ERR_OR_NULL(dev)) {
 			/* save error code to return later */
-			rc = PTR_ERR(parent->canch[i]);
+			rc = PTR_ERR(dev);
 
-			/* on failure destroy previously created candevs */
-			icount = i;
-			for (i = 0; i < icount; i++)
-				gs_destroy_candev(parent->canch[i]);
-
-			usb_kill_anchored_urbs(&parent->rx_submitted);
-			kfree(parent);
-			return rc;
+			goto out_gs_destroy_candev;
 		}
+		parent->canch[i] = dev;
 		parent->canch[i]->parent = parent;
 
 		/* set RX packet size based on FD and if hardware
@@ -1624,7 +1608,37 @@ static int gs_usb_probe(struct usb_interface *intf,
 		parent->hf_size_rx = max(parent->hf_size_rx, hf_size_rx);
 	}
 
+	for (i = 0; i < icount; i++) {
+		struct gs_can *dev = parent->canch[i];
+
+		rc = register_candev(dev->netdev);
+		if (rc) {
+			const unsigned int cur = i;
+
+			dev_err(&intf->dev,
+				"Couldn't register candev for channel %d (%pe)\n",
+				i, ERR_PTR(rc));
+
+			for (i = 0; i < cur; i++)
+				unregister_candev(parent->canch[i]->netdev);
+
+			goto out_gs_destroy_candev;
+		}
+	}
+
 	return 0;
+
+out_gs_destroy_candev:
+	for (i = 0; i < parent->channel_cnt; i++) {
+		if (!parent->canch[i])
+			continue;
+
+		gs_destroy_candev(parent->canch[i]);
+	}
+
+	kfree(parent);
+
+	return rc;
 }
 
 static void gs_usb_disconnect(struct usb_interface *intf)
@@ -1640,8 +1654,10 @@ static void gs_usb_disconnect(struct usb_interface *intf)
 	}
 
 	for (i = 0; i < parent->channel_cnt; i++)
-		if (parent->canch[i])
-			gs_destroy_candev(parent->canch[i]);
+		unregister_candev(parent->canch[i]->netdev);
+
+	for (i = 0; i < parent->channel_cnt; i++)
+		gs_destroy_candev(parent->canch[i]);
 
 	kfree(parent);
 }

-- 
2.53.0

[PATCH can-next v5 4/6] can: gs_usb: gs_usb_receive_bulk_callback(): check for overflow flag if SKB allocation fails

[Marc Kleine-Budde]

If the allocation of an SKB fails in gs_usb_receive_bulk_callback(), the
function neither updates the statistics nor checks whether an overflow flag
is set in the host frame.

Instead of directly resubmitting the URB, first update the dropped RX
statistics then check if an overflow flag is set.

Closes: https://sashiko.dev/#/bug/linux-7b3fe6a0-4628-4d0e-aec0-69f4bf8ad5d4
Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
Fixes: 26949ac935e3 ("can: gs_usb: add CAN-FD support")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 1527163e8c35..a42dba9d158e 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -673,8 +673,11 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 	if (hf->echo_id == GS_HOST_FRAME_ECHO_ID_RX) { /* normal rx */
 		if (hf->flags & GS_CAN_FLAG_FD) {
 			skb = alloc_canfd_skb(netdev, &cfd);
-			if (!skb)
-				goto resubmit_urb;
+			if (!skb) {
+				stats->rx_dropped++;
+
+				goto check_overflow;
+			}
 
 			cfd->can_id = le32_to_cpu(hf->can_id);
 			cfd->len = data_length;
@@ -686,8 +689,11 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 			memcpy(cfd->data, hf->canfd->data, data_length);
 		} else {
 			skb = alloc_can_skb(netdev, &cf);
-			if (!skb)
-				goto resubmit_urb;
+			if (!skb) {
+				stats->rx_dropped++;
+
+				goto check_overflow;
+			}
 
 			cf->can_id = le32_to_cpu(hf->can_id);
 			can_frame_set_cc_len(cf, hf->can_dlc, dev->can.ctrlmode);
@@ -728,6 +734,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 		netif_wake_queue(netdev);
 	}
 
+check_overflow:
 	if (hf->flags & GS_CAN_FLAG_OVERFLOW) {
 		stats->rx_over_errors++;
 		stats->rx_errors++;

-- 
2.53.0

[PATCH can-next v5 5/6] can: gs_usb: gs_usb_receive_bulk_callback(): add missing call to can_rx_offload_irq_finish

[Marc Kleine-Budde]

If the overflow flag is set for a host frame and the allocation of the
error SKB fails, the URB should not be resubmitted immediately.

Instead, call can_rx_offload_irq_finish(), since an SKB may have been added
to rx-offload.

Closes: https://sashiko.dev/#/bug/linux-441137e2-cd75-411f-9ff5-dc4d473c5ddb
Fixes: 24bc41b45583 ("can: gs_usb: convert to NAPI/rx-offload to avoid OoO reception")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index a42dba9d158e..6327069f63c9 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -741,7 +741,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 
 		skb = alloc_can_err_skb(netdev, &cf);
 		if (!skb)
-			goto resubmit_urb;
+			goto can_rx_offload_irq_finish;
 
 		cf->can_id |= CAN_ERR_CRTL;
 		cf->len = CAN_ERR_DLC;
@@ -750,6 +750,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 		gs_usb_rx_offload(dev, skb, hf);
 	}
 
+can_rx_offload_irq_finish:
 	can_rx_offload_irq_finish(&dev->offload);
 
 resubmit_urb:

-- 
2.53.0

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

[Marc Kleine-Budde]

From: Shuangpeng Bai <shuangpeng.kernel@gmail.com>

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@gmail.com>
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@vger.kernel.org
Cc: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
Cc: Vadim Fedorenko <vadim.fedorenko@linux.dev>
Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
Co-developed-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
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@gmail.com/

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

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@gmail.com>
> 
> 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@gmail.com>
> 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@vger.kernel.org
> Cc: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
> Cc: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
> Co-developed-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
> 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@gmail.com/
> 
> 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?

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2883 bytes --]

On 09.11.2026 12:40:14, Vadim Fedorenko wrote:
> > 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.

disable is needed to stop the worker from self-scheduling.

Sadly workers only support multiple levels of disable, but not of
enable. So we have use active_timestamp_channels, too.

> Just wonder cannot we keep this worker running regardless of channels?

For proper power management (on the Linux and on the µC side) I don't
want to keep it running if the interface is down.

There are also (more or less) gs_usb compatible devices which implement
(more or less) independent USB interfaces. All of those interfaces have
a common clock, so I want to share the HW timestamp infrastructure
between them.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]