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

[PATCH can-next v6 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 v6:
- patch #5: add missing initialization hw_ts_supported in gs_usb_probe()
  (sashiko)
- patch #5: work around race window in gs_usb_timestamp_start()
  (sashiko)
- patch #5: start time stamping worker if the device supports HW time
  stamping, if least one interface is brought up
- Link to v5: https://patch.msgid.link/20261108-gs_usb-fixes-v5-0-d52dd4940d46@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 | 187 +++++++++++++++++++++++++++++++------------
 1 file changed, 136 insertions(+), 51 deletions(-)
---
base-commit: 573af801552999954caa8347e4490b04cf4763f0
base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git master
change-id: 20261025-gs_usb-fixes-250f38833107

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

[PATCH can-next v6 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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] On the current tree the new error branch in gs_make_candev()
  cannot be reached.

--

can: gs_usb: gs_make_candev(): check can_rx_offload_add_manual() return value

This patch makes gs_make_candev() check the return value of
can_rx_offload_add_manual(). On failure it now exits through the existing
out_free_candev path. The commit message says the change prepares for an
upcoming can_rx_offload series in which can_rx_offload_add_manual() can
fail.

Link: https://patch.msgid.link/20260907104942.17089-4-ciprianmarian.costea@oss.nxp.com

> Fixes: 24bc41b45583 ("can: gs_usb: convert to NAPI/rx-offload to avoid OoO reception")

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 3b9b2f104d868..cac4ce323a1de 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;
> +

[Severity: Low]
This isn't a bug, but can the new error branch be reached on the current
tree?

can_rx_offload_add_manual() in drivers/net/can/dev/rx-offload.c only
fails when mailbox_read is set:

    if (offload->mailbox_read)
        return -EINVAL;

    return can_rx_offload_init_queue(dev, offload, weight);

gs_usb never sets mailbox_read. The private area returned by
alloc_candev() is zeroed, so this check always passes.

can_rx_offload_init_queue() always returns 0. It also sets up
skb_irq_queue as an embedded struct sk_buff_head, so the
"offload->skb_irq_queue == NULL" case only appears after the upcoming
rx-offload rework.

The commit message does say this failure comes "In an upcoming series".
Given that, would it be clearer to present this as preparation for that
series rather than tag it with Fixes: 24bc41b45583? Today the check
cannot trigger, so backporters may read the Fixes: tag as pointing to a
bug users can hit.

>  	SET_NETDEV_DEV(netdev, &intf->dev);
>  
>  	rc = register_candev(dev->netdev);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113-gs_usb-fixes-v6-0-0c039e1bfd74%40pengutronix.de

[PATCH can-next v6 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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] can: gs_usb: inaccurate Fixes: trailer in commit message

--

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> --- 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;
>  
>

[Severity: Low]
The Fixes: tag points at 26949ac935e3 ("can: gs_usb: add CAN-FD
support"). As far as I can tell, the leak in gs_can_start_xmit() is
older than the CAN-FD support.

The original driver, d08e973a77d1 ("can: gs_usb: Added support for the
GS_USB CAN devices"), already allocated hf with usb_alloc_coherent()
without __GFP_ZERO. It copied only cf->can_dlc bytes into the 8 byte
hf->data array and then submitted the full sizeof(*hf) buffer. So
whenever the DLC was below 8, the tail of hf->data went out over USB
uninitialized.

The kmalloc() call changed here was added later by 62f102c0d156
("can: gs_usb: remove dma allocations"). That commit kept the same
behaviour.

This patch is tagged for stable. With the tag pointing at the CAN-FD
commit, stable trees that have the classic CAN variant of the leak but
not 26949ac935e3 may never pick up a fix.

Should this be Fixes: d08e973a77d1 instead? The one-liner will not
apply before 62f102c0d156, so older trees would need a separate
backport against usb_alloc_coherent(). Would you rather list both
commits, or add a note for the stable maintainers?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113-gs_usb-fixes-v6-0-0c039e1bfd74%40pengutronix.de

[PATCH can-next v6 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 v6 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 v6 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 v6 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 the 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. Initialize the timestamping
infrastructure when opening the first channel. Avoid the race window during
timecounter_init() (because gs_usb_timestamp_read() drops the struct
gs_usb::tc_lock) by reading the initial time stamp prior taking the lock.
Enabling and disabling the delayed worker prevents concurrent rescheduling
while gs_usb_timestamp_stop() is running.

Call gs_usb_timestamp_start()/gs_usb_timestamp_stop() if at least one of
the channels support HW timestamping, 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>
---
 drivers/net/can/usb/gs_usb.c | 98 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 78 insertions(+), 20 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 6327069f63c9..9eafdf63a4ef 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -324,6 +324,10 @@ struct gs_can {
 	atomic_t active_tx_urbs;
 };
 
+enum gs_usb_parent_flags {
+	GS_USB_PARENT_FLAGS_HW_TIMESTAMP = BIT(0),
+};
+
 /* usb interface struct */
 struct gs_usb {
 	struct usb_anchor rx_submitted;
@@ -332,11 +336,14 @@ struct gs_usb {
 	/* time counter for hardware timestamps */
 	struct cyclecounter cc;
 	struct timecounter tc;
-	spinlock_t tc_lock; /* spinlock to guard access tc->cycle_last */
+	spinlock_t tc_lock; /* spinlock to guard access tc and active_timestamp_channels */
 	struct delayed_work timestamp;
+	u32 initial_timestamp;
 
 	unsigned int hf_size_rx;
 	u8 active_channels;
+	u8 active_timestamp_channels;
+	u8 flags;
 	u8 channel_cnt;
 
 	unsigned int pipe_in;
@@ -423,6 +430,13 @@ static inline int gs_usb_get_timestamp(const struct gs_usb *parent,
 	return 0;
 }
 
+static u64 gs_usb_timestamp_read_initial(struct cyclecounter *cc)
+{
+	const struct gs_usb *parent = container_of(cc, struct gs_usb, cc);
+
+	return parent->initial_timestamp;
+}
+
 static u64 gs_usb_timestamp_read(struct cyclecounter *cc) __must_hold(&dev->tc_lock)
 {
 	struct gs_usb *parent = container_of(cc, struct gs_usb, cc);
@@ -475,24 +489,61 @@ static void gs_usb_timestamp_init(struct gs_usb *parent)
 {
 	struct cyclecounter *cc = &parent->cc;
 
-	cc->read = gs_usb_timestamp_read;
 	cc->mask = CYCLECOUNTER_MASK(32);
 	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);
+	disable_delayed_work(&parent->timestamp);
+
+	parent->flags |= GS_USB_PARENT_FLAGS_HW_TIMESTAMP;
+}
+
+static int gs_usb_timestamp_start(struct gs_usb *parent)
+{
+	u32 timestamp = 0;
+	int err;
+
+	/* read current timestamp, might be used later for initial time stamp */
+	err = gs_usb_get_timestamp(parent, &timestamp);
+	if (err)
+		return err;
+
+	scoped_guard(spinlock_bh, &parent->tc_lock) {
+		struct cyclecounter *cc = &parent->cc;
+
+		parent->active_timestamp_channels++;
+		if (parent->active_timestamp_channels > 1)
+			return 0;
+
+		/* gs_usb_timestamp_read() will drop the struct gs_usb::tc_lock. Avoid the race
+		 * window by using gs_usb_timestamp_read_initial() to read the initial timestamp in
+		 * timecounter_init(), switch to gs_usb_timestamp_read() afterwards.
+		 */
+		parent->initial_timestamp = timestamp;
+		cc->read = gs_usb_timestamp_read_initial;
+		timecounter_init(&parent->tc, &parent->cc, ktime_get_real_ns());
+		cc->read = gs_usb_timestamp_read;
+	}
+
+	enable_delayed_work(&parent->timestamp);
 	schedule_delayed_work(&parent->timestamp,
 			      GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ);
+
+	return 0;
 }
 
 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)
@@ -654,7 +705,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 	if (!netif_device_present(netdev))
 		return;
 
-	if (!netif_running(netdev))
+	if (!netif_running(netdev) || !(netdev->flags & IFF_UP))
 		goto resubmit_urb;
 
 	minimum_length = gs_usb_get_minimum_rx_length(dev, hf, &data_length);
@@ -988,10 +1039,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 (parent->flags & GS_USB_PARENT_FLAGS_HW_TIMESTAMP) {
+		rc = gs_usb_timestamp_start(parent);
+		if (rc)
+			goto out_can_rx_offload_disable;
+	}
 
+	if (!parent->active_channels) {
 		for (i = 0; i < GS_MAX_RX_URBS; i++) {
 			u8 *buf;
 
@@ -1102,13 +1156,11 @@ 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 (parent->flags & GS_USB_PARENT_FLAGS_HW_TIMESTAMP)
+		gs_usb_timestamp_stop(parent);
+out_can_rx_offload_disable:
 	can_rx_offload_disable(&dev->offload);
 	close_candev(netdev);
 
@@ -1160,12 +1212,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 (parent->flags & GS_USB_PARENT_FLAGS_HW_TIMESTAMP)
+		gs_usb_timestamp_stop(parent);
 
 	/* Stop sending URBs */
 	usb_kill_anchored_urbs(&dev->tx_submitted);
@@ -1525,6 +1576,7 @@ static int gs_usb_probe(struct usb_interface *intf,
 		.byte_order = cpu_to_le32(0x0000beef),
 	};
 	struct gs_device_config dconf;
+	bool hw_ts_supported = false;
 	unsigned int icount, i;
 	int rc;
 
@@ -1599,6 +1651,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)
+			hw_ts_supported = true;
+
 		/* set RX packet size based on FD and if hardware
 		 * timestamps are supported.
 		 */
@@ -1616,6 +1671,9 @@ static int gs_usb_probe(struct usb_interface *intf,
 		parent->hf_size_rx = max(parent->hf_size_rx, hf_size_rx);
 	}
 
+	if (hw_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 v6 6/6] can: gs_usb: fix hardware timestamp state for mixed channels

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch adds an RX-path change that the commit message never
  mentions.
- [Medium] gs_usb_timestamp_start()
  (drivers/net/can/usb/gs_usb.c:504-536) always sends the…
- [Low] The commit message describes the old behaviour inaccurately and
  understates the impact.
- [Low] The fix relies on disable_delayed_work(), enable_delayed_work()
  and disable_delayed_work_sync().
- [Low] The new counter struct gs_usb::active_timestamp_channels
  (drivers/net/can/usb/gs_usb.c:345), and the rewritten tc_lock comment…

--

can: gs_usb: fix hardware timestamp state for mixed channels

This patch initializes the HW timestamping infrastructure in
gs_usb_probe() if any channel has GS_CAN_FEATURE_HW_TIMESTAMP set. It
reworks gs_usb_timestamp_start()/gs_usb_timestamp_stop() to count opens
and to init the timecounter on the first one, reading the initial
timestamp before taking tc_lock. The delayed worker is disabled and
enabled to prevent rescheduling while stop runs.

Closes: https://sashiko.dev/#/bug/linux-b39360f9-6cd8-4f47-83bb-f0f78331f308
Closes: https://sashiko.dev/#/bug/linux-5f8ecd4e-344e-4ceb-9b46-886f4942a5f6

> Currently both the 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.

[Severity: Low]

Is "first channel ... closed" accurate? Before this patch, gs_can_close()
only called gs_usb_timestamp_stop() once active_channels reached zero. So
shutdown depended on the flags of the last channel closed:

	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);
	}

Could the message also say what "leads to problems" means? The old code
seems to have two crash cases.

If a non-TS channel opens first and a TS channel opens second,
gs_usb_timestamp_init() never runs. The TS channel's RX frames then reach
gs_usb_skb_set_timestamp()->timecounter_cyc2time() while parent->tc.cc is
still NULL.

If a TS channel opens first and a non-TS channel closes last,
cancel_delayed_work_sync() is never called. The self-rearming
gs_usb_timestamp_work() can then run after gs_usb_disconnect() frees
parent.

The Cc: stable tag rests on these, so would it help to name them?

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 6327069f63c95..9eafdf63a4ef0 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c

[ ... ]

> @@ -332,11 +336,14 @@ struct gs_usb {
>  	/* time counter for hardware timestamps */
>  	struct cyclecounter cc;
>  	struct timecounter tc;
> -	spinlock_t tc_lock; /* spinlock to guard access tc->cycle_last */
> +	spinlock_t tc_lock; /* spinlock to guard access tc and active_timestamp_channels */
>  	struct delayed_work timestamp;
> +	u32 initial_timestamp;
>  
>  	unsigned int hf_size_rx;
>  	u8 active_channels;
> +	u8 active_timestamp_channels;

[Severity: Low]

This isn't a bug, but does active_timestamp_channels really count
timestamping channels? Three places call gs_usb_timestamp_start() or
gs_usb_timestamp_stop(): gs_can_open(), its error path, and
gs_can_close(). All three check only the parent-wide
GS_USB_PARENT_FLAGS_HW_TIMESTAMP flag, never dev->feature:

	if (parent->flags & GS_USB_PARENT_FLAGS_HW_TIMESTAMP) {
		rc = gs_usb_timestamp_start(parent);

So on a TS-capable interface the counter just mirrors active_channels.
With only non-TS channels open, gs_usb_timestamp_work() still sends
GS_USB_BREQ_TIMESTAMP every 30 minutes. Opening a non-TS channel also
does a synchronous timestamp read.

The commit message says the functions "track the number of active
timestamp channels", which doesn't match this. Would a different name or
comment be clearer?

[ ... ]

> @@ -475,24 +489,61 @@ static void gs_usb_timestamp_init(struct gs_usb *parent)

[ ... ]

>  	INIT_DELAYED_WORK(&parent->timestamp, gs_usb_timestamp_work);
> +	disable_delayed_work(&parent->timestamp);

[Severity: Low]

Will this backport to the stable trees covered by the Fixes: tag?
disable_delayed_work(), enable_delayed_work() and
disable_delayed_work_sync() arrived in v6.10 with commit 86898fa6b8cd
("workqueue: Implement disable/enable for (delayed) work items").
5886e4d5ecec dates from July 2023 and is in older stable trees such as
6.6.y and 6.1.y.

A mechanical backport would fail to build there. Swapping in
cancel_delayed_work_sync() by hand would silently drop the protection
against gs_usb_timestamp_work() re-arming itself. The commit message
relies on that protection ("Enabling and disabling the delayed worker
prevents concurrent rescheduling while gs_usb_timestamp_stop() is
running").

Could a stable version annotation or a note for backporters be added?

> +
> +	parent->flags |= GS_USB_PARENT_FLAGS_HW_TIMESTAMP;
> +}
> +
> +static int gs_usb_timestamp_start(struct gs_usb *parent)
> +{
> +	u32 timestamp = 0;
> +	int err;
> +
> +	/* read current timestamp, might be used later for initial time stamp */
> +	err = gs_usb_get_timestamp(parent, &timestamp);
> +	if (err)
> +		return err;
> +
> +	scoped_guard(spinlock_bh, &parent->tc_lock) {
> +		struct cyclecounter *cc = &parent->cc;
> +
> +		parent->active_timestamp_channels++;
> +		if (parent->active_timestamp_channels > 1)
> +			return 0;

[Severity: Medium]

Should a failed GS_USB_BREQ_TIMESTAMP read make every channel's open
fail? gs_usb_get_timestamp() runs before the active_timestamp_channels
check. For every open after the first, the value is thrown away. An error
such as a timeout, -EPIPE or -EPROTO still reaches gs_can_open():

	if (parent->flags & GS_USB_PARENT_FLAGS_HW_TIMESTAMP) {
		rc = gs_usb_timestamp_start(parent);
		if (rc)
			goto out_can_rx_offload_disable;
	}

On a mixed device, opening a channel without
GS_CAN_FEATURE_HW_TIMESTAMP now also depends on this request succeeding.

Before the patch, only the first open of a TS-capable channel read the
timestamp, through gs_usb_timestamp_read() inside timecounter_init(). A
failure there was only logged with dev_err() and did not fail the open.
Later opens sent no request at all.

Open and close are serialized by RTNL. Could the count be checked first,
with the read done only on the 0->1 transition? Or could the old
non-fatal behaviour be kept?

[ ... ]

> @@ -654,7 +705,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  	if (!netif_device_present(netdev))
>  		return;
>  
> -	if (!netif_running(netdev))
> +	if (!netif_running(netdev) || !(netdev->flags & IFF_UP))
>  		goto resubmit_urb;

[Severity: Medium]

Is this hunk part of the timestamp fix? The commit message doesn't
mention it.

__dev_open() sets __LINK_STATE_START before ndo_open(), and sets IFF_UP
only after ndo_open() succeeds. On close, __dev_close_many() clears
__LINK_STATE_START before ndo_stop(). So the new check only has an effect
while this channel's gs_can_open() is running.

On a multi-channel interface with another channel already up, the RX URBs
are active. Any frame for this channel that completes in that window is
now dropped and the URB resubmitted. That includes frames received after
the GS_USB_BREQ_MODE start request at the end of gs_can_open() but before
the core sets IFF_UP.

With this patch, every open on a TS-capable interface calls
gs_usb_timestamp_start() before the RX URBs are submitted, so the
timecounter should already be valid. If this hunk fixes a different
problem, perhaps one of the Closes: reports, could it be described in the
commit message or split into its own patch?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113-gs_usb-fixes-v6-0-0c039e1bfd74%40pengutronix.de