[PATCH can-next 00/10] can: gs_usb: cleanups and fixes

[PATCH can-next 00/10] can: gs_usb: cleanups and fixes

[Marc Kleine-Budde]

This series contains all the cleanups and fixes of the "can: gs_usb:
implement new features recently added to candleLight firmware" series [1],
as well as some additonal patches.

[1] https://lore.kernel.org/all/20260720-gs_usb-new-features-v1-0-427a8013c380@pengutronix.de/

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Marc Kleine-Budde (10):
      can: gs_usb: remove unused define GS_CAN_MODE_NORMAL
      can: gs_usb: replace all GS_CAN_MODE_* by GS_CAN_FEATURE_*
      can: gs_usb: gs_make_candev(): reduce scope of variable bt_const_extended
      can: gs_usb: gs_make_candev(): sort evaluation of device features
      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
      can: gs_usb: gs_usb_receive_bulk_callback(): report channel number out of range error
      can: gs_usb: gs_usb_receive_bulk_callback(): reduce scope of several variables
      can: gs_usb: gs_usb_receive_bulk_callback(): no need to assign CAN_ERR_DLC
      can: gs_usb: gs_usb_get_minimum_rx_length(): enforce data_length of 8 bytes for CAN error frames

 drivers/net/can/usb/gs_usb.c | 127 ++++++++++++++++++++++---------------------
 1 file changed, 66 insertions(+), 61 deletions(-)
---
base-commit: 4fc3e28a2929f60f2a9f05c612f96384f8c8f5e9
change-id: 20261003-gs_usb-cleanups-and-fixes-bc71af91c666

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

[PATCH can-next 01/10] can: gs_usb: remove unused define GS_CAN_MODE_NORMAL

[Marc Kleine-Budde]

The value GS_CAN_MODE_NORMAL was part of the initial commit
d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices") of
the gs_usb driver, but never used. Remove it.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 3b9b2f104d86..bfb3780e1163 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -127,7 +127,6 @@ struct gs_device_config {
 	__le32 hw_version;
 } __packed;
 
-#define GS_CAN_MODE_NORMAL 0
 #define GS_CAN_MODE_LISTEN_ONLY BIT(0)
 #define GS_CAN_MODE_LOOP_BACK BIT(1)
 #define GS_CAN_MODE_TRIPLE_SAMPLE BIT(2)

-- 
2.53.0

[PATCH can-next 02/10] can: gs_usb: replace all GS_CAN_MODE_* by GS_CAN_FEATURE_*

[Marc Kleine-Budde]

The values for the defines GS_CAN_MODE_* and GS_CAN_FEATURE_* are
intentionally identical.

The device signals its capabilities with GS_CAN_FEATURE_* in struct
gs_device_bt_const::feature and the driver activates them with
GS_CAN_MODE_* in struct gs_device_mode::flags.

Standardize on GS_CAN_FEATURE_* to eliminate redundant macro definitions
and align the driver with the candlelight firmware implementation (commit
cdadf34281c7 ("gs_usb: replace all GS_CAN_MODE_xxx by GS_CAN_FEATURE_xxx").

Link: https://github.com/candle-usb/candleLight_fw/commit/cdadf34281c777740229b152ce928ca9a819d727
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index bfb3780e1163..5e96c1a7e5cc 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -127,21 +127,6 @@ struct gs_device_config {
 	__le32 hw_version;
 } __packed;
 
-#define GS_CAN_MODE_LISTEN_ONLY BIT(0)
-#define GS_CAN_MODE_LOOP_BACK BIT(1)
-#define GS_CAN_MODE_TRIPLE_SAMPLE BIT(2)
-#define GS_CAN_MODE_ONE_SHOT BIT(3)
-#define GS_CAN_MODE_HW_TIMESTAMP BIT(4)
-/* GS_CAN_FEATURE_IDENTIFY BIT(5) */
-/* GS_CAN_FEATURE_USER_ID BIT(6) */
-#define GS_CAN_MODE_PAD_PKTS_TO_MAX_PKT_SIZE BIT(7)
-#define GS_CAN_MODE_FD BIT(8)
-/* GS_CAN_FEATURE_REQ_USB_QUIRK_LPC546XX BIT(9) */
-/* GS_CAN_FEATURE_BT_CONST_EXT BIT(10) */
-/* GS_CAN_FEATURE_TERMINATION BIT(11) */
-#define GS_CAN_MODE_BERR_REPORTING BIT(12)
-/* GS_CAN_FEATURE_GET_STATE BIT(13) */
-
 struct gs_device_mode {
 	__le32 mode;
 	__le32 flags;
@@ -1033,26 +1018,26 @@ static int gs_can_open(struct net_device *netdev)
 
 	/* flags */
 	if (ctrlmode & CAN_CTRLMODE_LOOPBACK)
-		flags |= GS_CAN_MODE_LOOP_BACK;
+		flags |= GS_CAN_FEATURE_LOOP_BACK;
 
 	if (ctrlmode & CAN_CTRLMODE_LISTENONLY)
-		flags |= GS_CAN_MODE_LISTEN_ONLY;
+		flags |= GS_CAN_FEATURE_LISTEN_ONLY;
 
 	if (ctrlmode & CAN_CTRLMODE_3_SAMPLES)
-		flags |= GS_CAN_MODE_TRIPLE_SAMPLE;
+		flags |= GS_CAN_FEATURE_TRIPLE_SAMPLE;
 
 	if (ctrlmode & CAN_CTRLMODE_ONE_SHOT)
-		flags |= GS_CAN_MODE_ONE_SHOT;
+		flags |= GS_CAN_FEATURE_ONE_SHOT;
 
 	if (ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
-		flags |= GS_CAN_MODE_BERR_REPORTING;
+		flags |= GS_CAN_FEATURE_BERR_REPORTING;
 
 	if (ctrlmode & CAN_CTRLMODE_FD)
-		flags |= GS_CAN_MODE_FD;
+		flags |= GS_CAN_FEATURE_FD;
 
 	/* if hardware supports timestamps, enable it */
 	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
-		flags |= GS_CAN_MODE_HW_TIMESTAMP;
+		flags |= GS_CAN_FEATURE_HW_TIMESTAMP;
 
 	rc = gs_usb_set_bittiming(dev);
 	if (rc) {

-- 
2.53.0

[PATCH can-next 03/10] can: gs_usb: gs_make_candev(): reduce scope of variable bt_const_extended

[Marc Kleine-Budde]

To improve readability of the code, reduce the scope of the variable
bt_const_extended.

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 5e96c1a7e5cc..b0e7202f0fc0 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -1305,7 +1305,6 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 	struct gs_can *dev;
 	struct net_device *netdev;
 	int rc;
-	struct gs_device_bt_const_extended bt_const_extended;
 	struct gs_device_bt_const bt_const;
 	u32 feature;
 
@@ -1445,6 +1444,8 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 	 */
 	if (feature & GS_CAN_FEATURE_FD &&
 	    feature & GS_CAN_FEATURE_BT_CONST_EXT) {
+		struct gs_device_bt_const_extended bt_const_extended;
+
 		rc = usb_control_msg_recv(interface_to_usbdev(intf), 0,
 					  GS_USB_BREQ_BT_CONST_EXT,
 					  USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,

-- 
2.53.0

[PATCH can-next 04/10] can: gs_usb: gs_make_candev(): sort evaluation of device features

[Marc Kleine-Budde]

To simplify maintenance and improve readability, sort the evaluation of the
device features by the value of each feature.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 54 +++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index b0e7202f0fc0..406c11efba79 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -1370,6 +1370,8 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 
 	feature = le32_to_cpu(bt_const.feature);
 	dev->feature = FIELD_GET(GS_CAN_FEATURE_MASK, feature);
+
+	/* keep sorted by GS_CAN_FEATURE */
 	if (feature & GS_CAN_FEATURE_LISTEN_ONLY)
 		dev->can.ctrlmode_supported |= CAN_CTRLMODE_LISTENONLY;
 
@@ -1382,6 +1384,11 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 	if (feature & GS_CAN_FEATURE_ONE_SHOT)
 		dev->can.ctrlmode_supported |= CAN_CTRLMODE_ONE_SHOT;
 
+	/* GS_CAN_FEATURE_IDENTIFY is only supported for sw_version > 1 */
+	if (!(le32_to_cpu(dconf->sw_version) > 1 &&
+	      feature & GS_CAN_FEATURE_IDENTIFY))
+		dev->feature &= ~GS_CAN_FEATURE_IDENTIFY;
+
 	if (feature & GS_CAN_FEATURE_FD) {
 		dev->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
 		/* The data bit timing will be overwritten, if
@@ -1390,27 +1397,6 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 		dev->can.fd.data_bittiming_const = &dev->bt_const;
 	}
 
-	if (feature & GS_CAN_FEATURE_TERMINATION) {
-		rc = gs_usb_get_termination(netdev, &dev->can.termination);
-		if (rc) {
-			dev->feature &= ~GS_CAN_FEATURE_TERMINATION;
-
-			dev_info(&intf->dev,
-				 "Disabling termination support for channel %d (%pe)\n",
-				 channel, ERR_PTR(rc));
-		} else {
-			dev->can.termination_const = gs_usb_termination_const;
-			dev->can.termination_const_cnt = ARRAY_SIZE(gs_usb_termination_const);
-			dev->can.do_set_termination = gs_usb_set_termination;
-		}
-	}
-
-	if (feature & GS_CAN_FEATURE_BERR_REPORTING)
-		dev->can.ctrlmode_supported |= CAN_CTRLMODE_BERR_REPORTING;
-
-	if (feature & GS_CAN_FEATURE_GET_STATE)
-		dev->can.do_get_berr_counter = gs_usb_can_get_berr_counter;
-
 	/* The CANtact Pro from LinkLayer Labs is based on the
 	 * LPC54616 µC, which is affected by the NXP LPC USB transfer
 	 * erratum. However, the current firmware (version 2) doesn't
@@ -1434,11 +1420,6 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 		dev->feature |= GS_CAN_FEATURE_REQ_USB_QUIRK_LPC546XX |
 			GS_CAN_FEATURE_QUIRK_BREQ_CANTACT_PRO;
 
-	/* GS_CAN_FEATURE_IDENTIFY is only supported for sw_version > 1 */
-	if (!(le32_to_cpu(dconf->sw_version) > 1 &&
-	      feature & GS_CAN_FEATURE_IDENTIFY))
-		dev->feature &= ~GS_CAN_FEATURE_IDENTIFY;
-
 	/* fetch extended bit timing constants if device has feature
 	 * GS_CAN_FEATURE_FD and GS_CAN_FEATURE_BT_CONST_EXT
 	 */
@@ -1472,6 +1453,27 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 		dev->can.fd.data_bittiming_const = &dev->data_bt_const;
 	}
 
+	if (feature & GS_CAN_FEATURE_TERMINATION) {
+		rc = gs_usb_get_termination(netdev, &dev->can.termination);
+		if (rc) {
+			dev->feature &= ~GS_CAN_FEATURE_TERMINATION;
+
+			dev_info(&intf->dev,
+				 "Disabling termination support for channel %d (%pe)\n",
+				 channel, ERR_PTR(rc));
+		} else {
+			dev->can.termination_const = gs_usb_termination_const;
+			dev->can.termination_const_cnt = ARRAY_SIZE(gs_usb_termination_const);
+			dev->can.do_set_termination = gs_usb_set_termination;
+		}
+	}
+
+	if (feature & GS_CAN_FEATURE_BERR_REPORTING)
+		dev->can.ctrlmode_supported |= CAN_CTRLMODE_BERR_REPORTING;
+
+	if (feature & GS_CAN_FEATURE_GET_STATE)
+		dev->can.do_get_berr_counter = gs_usb_can_get_berr_counter;
+
 	can_rx_offload_add_manual(netdev, &dev->offload, GS_NAPI_WEIGHT);
 	SET_NETDEV_DEV(netdev, &intf->dev);
 

-- 
2.53.0

[PATCH can-next 05/10] 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 does not check whether an overflow flag on the host frame is set.

Instead of directly resubmitting the URB, first check if an overflow flag
is set.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 406c11efba79..01b4b6c980c9 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -658,7 +658,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 		if (hf->flags & GS_CAN_FLAG_FD) {
 			skb = alloc_canfd_skb(netdev, &cfd);
 			if (!skb)
-				goto resubmit_urb;
+				goto check_overflow;
 
 			cfd->can_id = le32_to_cpu(hf->can_id);
 			cfd->len = data_length;
@@ -671,7 +671,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 		} else {
 			skb = alloc_can_skb(netdev, &cf);
 			if (!skb)
-				goto resubmit_urb;
+				goto check_overflow;
 
 			cf->can_id = le32_to_cpu(hf->can_id);
 			can_frame_set_cc_len(cf, hf->can_dlc, dev->can.ctrlmode);
@@ -712,6 +712,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

Re: [PATCH can-next 05/10] can: gs_usb: gs_usb_receive_bulk_callback(): check for overflow flag if SKB allocation fails

[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] The commit "can: gs_usb: gs_usb_receive_bulk_callback(): check
  for overflow flag if SKB allocation fails" fixes a real bug but has no…

--

can: gs_usb: gs_usb_receive_bulk_callback(): check for overflow flag if SKB allocation fails

When SKB allocation fails in gs_usb_receive_bulk_callback(), this patch
jumps to a new check_overflow label instead of jumping straight to
resubmit_urb. As a result, the GS_CAN_FLAG_OVERFLOW handling still runs
when alloc_canfd_skb() or alloc_can_skb() fails.

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 0a9c490c0c445..f40f825b7eb29 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c

[ ... ]

> @@ -712,6 +712,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++;

[Severity: Low]
Should this commit have Fixes: tags and a Cc: stable?

Before this change, gs_usb_receive_bulk_callback() skipped the
GS_CAN_FLAG_OVERFLOW block whenever alloc_canfd_skb() or alloc_can_skb()
failed. In that case:

- rx_over_errors and rx_errors were not incremented
- no CAN_ERR_CRTL_RX_OVERFLOW error frame was attempted

For classic CAN frames, the missed check goes back to the original driver.
For the FD branch, it goes back to the CAN-FD support:

  d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
  26949ac935e3 ("can: gs_usb: add CAN-FD support")

An earlier fix to these same lines has both of those Fixes: tags and
Cc: stable@kernel.org:

  68c5724ecd15 ("can: gs_usb: gs_usb_receive_bulk_callback(): resubmit URB
  on skb allocation failure")

Without matching tags here, stable trees that took 68c5724ecd15 would not
automatically get this follow-up fix for the overflow accounting.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004-gs_usb-cleanups-and-fixes-v1-0-7b4b06244a77%40pengutronix.de

[PATCH can-next 06/10] 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,
can_rx_offload_irq_finish() should be called, since an SKB may have been
added to rx-offload.

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 01b4b6c980c9..848a84a05fd6 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -719,7 +719,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;
@@ -728,6 +728,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

Re: [PATCH can-next 06/10] can: gs_usb: gs_usb_receive_bulk_callback(): add missing call to can_rx_offload_irq_finish

[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] The commit fixes a real bug introduced by commit 24bc41b45583
  ("can: gs_usb: convert to NAPI/rx-offload to avoid OoO reception"),…

--

can: gs_usb: gs_usb_receive_bulk_callback(): add missing call to can_rx_offload_irq_finish

The patch fixes one error path in gs_usb_receive_bulk_callback(). That path
runs when the host frame has GS_CAN_FLAG_OVERFLOW set and alloc_can_err_skb()
fails. It now jumps to a new can_rx_offload_irq_finish label instead of going
straight to resubmit_urb.

> 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,
> can_rx_offload_irq_finish() should be called, since an SKB may have been
> added to rx-offload.
>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

[Severity: Low]
The code change looks right. Should the commit message also have a Fixes
tag, so the fix gets backported to stable?

The regression appears to come from:

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

That commit changed gs_usb_rx_offload() and gs_usb_get_echo_skb() to queue
SKBs on offload->skb_irq_queue. Only can_rx_offload_irq_finish() moves those
SKBs to skb_queue and calls napi_schedule(). The same commit added the
can_rx_offload_irq_finish() call just above resubmit_urb.

The goto resubmit_urb on the overflow path is older (from d08e973a77d1).
After 24bc41b45583, that goto started skipping the new
can_rx_offload_irq_finish() call.

The commit message could also say what the user sees. Here is the sequence:

gs_usb_receive_bulk_callback()
    gs_usb_rx_offload() or gs_usb_get_echo_skb()
        __skb_queue_tail(&offload->skb_irq_queue, skb)
    hf->flags & GS_CAN_FLAG_OVERFLOW
        alloc_can_err_skb() returns NULL
        goto resubmit_urb  /* skips can_rx_offload_irq_finish() */

The RX frame or TX echo frame queued earlier stays on skb_irq_queue, and
NAPI is not scheduled. It is only delivered when a later URB completion calls
can_rx_offload_irq_finish(). On a quiet bus, that could take a long time.

Would it help to mention this delayed delivery in the commit message, along
with the Fixes tag?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004-gs_usb-cleanups-and-fixes-v1-0-7b4b06244a77%40pengutronix.de

[PATCH can-next 07/10] can: gs_usb: gs_usb_receive_bulk_callback(): report channel number out of range error

[Marc Kleine-Budde]

If the USB device sends an invalid channel, the device should not be
silently detached; instead, the display an error message before detaching.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 848a84a05fd6..86f7cac093a1 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -627,8 +627,13 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 	}
 
 	/* device reports out of range channel id */
-	if (hf->channel >= parent->channel_cnt)
+	if (hf->channel >= parent->channel_cnt) {
+		dev_err_ratelimited(&parent->udev->dev,
+				    "channel number out of range (channel=%u, channel_cnt=%u)\n",
+				    hf->channel, parent->channel_cnt);
+
 		goto device_detach;
+	}
 
 	dev = parent->canch[hf->channel];
 

-- 
2.53.0

Re: [PATCH can-next 07/10] can: gs_usb: gs_usb_receive_bulk_callback(): report channel number out of range error

[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] The commit message body of "can: gs_usb:
  gs_usb_receive_bulk_callback(): report channel number out of range…

Pre-existing issues:
- [High] In gs_usb_receive_bulk_callback()
  (drivers/net/can/usb/gs_usb.c), the channel check only tests the upper…

--

can: gs_usb: gs_usb_receive_bulk_callback(): report channel number out of range error

If the USB device sends a frame with an out of range channel number,
gs_usb_receive_bulk_callback() now logs a rate limited error before it
jumps to the device_detach label. Before this patch it detached without
logging anything. Nothing else changes.

> If the USB device sends an invalid channel, the device should not be
> silently detached; instead, the display an error message before detaching.

[Severity: Low]
This isn't a bug, but "instead, the display an error message" reads
wrong. Maybe "instead, display an error message before detaching"?

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 9a0635b9c2d44..85a10c43b6f73 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -627,8 +627,13 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  	}
>  
>  	/* device reports out of range channel id */
> -	if (hf->channel >= parent->channel_cnt)
> +	if (hf->channel >= parent->channel_cnt) {
> +		dev_err_ratelimited(&parent->udev->dev,
> +				    "channel number out of range (channel=%u, channel_cnt=%u)\n",
> +				    hf->channel, parent->channel_cnt);
> +
>  		goto device_detach;
> +	}
>  
>  	dev = parent->canch[hf->channel];

[Severity: High]
This isn't a bug introduced by this patch, but this check only tests the
upper bound. Can parent->canch[hf->channel] still be NULL when it is read
here?

The code right after this does:

	dev = parent->canch[hf->channel];

	netdev = dev->netdev;

gs_usb_probe() sets channel_cnt to cover all channels before any slot is
filled:

	parent->channel_cnt = icount;
	...
	for (i = 0; i < icount; i++) {
		...
		parent->canch[i] = gs_make_candev(i, intf, &dconf);

gs_make_candev() calls register_candev() before it returns. That means
can0 is visible to userspace while probe is still running the control
transfers for channel 1. Each transfer can take up to 1000 ms, and the
device decides how long it takes to answer.

If userspace brings can0 up in that window, gs_can_open() submits the RX
URBs:

	if (!parent->active_channels) {
		...

A device that reports icount >= 2 could then send a bulk-IN frame with
channel=1. The check above passes because 1 < channel_cnt. dev is then
NULL, and dev->netdev is dereferenced in URB completion context.

There is a related case when gs_make_candev() fails for a later channel.
The ERR_PTR is stored in the slot first:

	parent->canch[i] = gs_make_candev(i, intf, &dconf);
	if (IS_ERR_OR_NULL(parent->canch[i])) {

The rollback kills the RX URBs only when it destroys the earlier
channels. Until then, a frame for that channel would dereference the
ERR_PTR.

The device_detach loop has the same gap. Its test lets an ERR_PTR
through:

	if (parent->canch[rc])
		netif_device_detach(parent->canch[rc]->netdev);

Should this path also reject an empty slot, for example with
!parent->canch[hf->channel]? Another option is to stop storing ERR_PTR
values in canch[]. A third is to create all channels before any of them
is registered.

This check is still unchanged at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004-gs_usb-cleanups-and-fixes-v1-0-7b4b06244a77%40pengutronix.de

[PATCH can-next 08/10] can: gs_usb: gs_usb_receive_bulk_callback(): reduce scope of several variables

[Marc Kleine-Budde]

To improve readability of the code, reduce the scope of the variables txc,
cf and cfd and skb.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 86f7cac093a1..190cd3701202 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -599,10 +599,6 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 	struct net_device_stats *stats;
 	struct gs_host_frame *hf = urb->transfer_buffer;
 	unsigned int minimum_length, data_length;
-	struct gs_tx_context *txc;
-	struct can_frame *cf;
-	struct canfd_frame *cfd;
-	struct sk_buff *skb;
 
 	BUG_ON(!parent);
 
@@ -660,7 +656,11 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 	}
 
 	if (hf->echo_id == GS_HOST_FRAME_ECHO_ID_RX) { /* normal rx */
+		struct sk_buff *skb;
+
 		if (hf->flags & GS_CAN_FLAG_FD) {
+			struct canfd_frame *cfd;
+
 			skb = alloc_canfd_skb(netdev, &cfd);
 			if (!skb)
 				goto check_overflow;
@@ -674,6 +674,8 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 
 			memcpy(cfd->data, hf->canfd->data, data_length);
 		} else {
+			struct can_frame *cf;
+
 			skb = alloc_can_skb(netdev, &cf);
 			if (!skb)
 				goto check_overflow;
@@ -690,6 +692,9 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 
 		gs_usb_rx_offload(dev, skb, hf);
 	} else { /* echo_id == hf->echo_id */
+		struct gs_tx_context *txc;
+		struct sk_buff *skb;
+
 		if (hf->echo_id >= GS_MAX_TX_URBS) {
 			netdev_err(netdev,
 				   "Unexpected out of range echo id %u\n",
@@ -719,6 +724,9 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 
 check_overflow:
 	if (hf->flags & GS_CAN_FLAG_OVERFLOW) {
+		struct can_frame *cf;
+		struct sk_buff *skb;
+
 		stats->rx_over_errors++;
 		stats->rx_errors++;
 

-- 
2.53.0

[PATCH can-next 09/10] can: gs_usb: gs_usb_receive_bulk_callback(): no need to assign CAN_ERR_DLC

[Marc Kleine-Budde]

There's no need to assign CAN_ERR_DLC to the length of CAN error frames
allocated with alloc_can_err_skb(), this function already sets the length.

Remove unneeded assignment of the CAN error frame's len.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 190cd3701202..3c0464edb5d3 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -735,7 +735,6 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 			goto can_rx_offload_irq_finish;
 
 		cf->can_id |= CAN_ERR_CRTL;
-		cf->len = CAN_ERR_DLC;
 		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
 
 		gs_usb_rx_offload(dev, skb, hf);

-- 
2.53.0

[PATCH can-next 10/10] can: gs_usb: gs_usb_get_minimum_rx_length(): enforce data_length of 8 bytes for CAN error frames

[Marc Kleine-Budde]

By definition, CAN error frames have a length of CAN_ERR_DLC (= 8) bytes.
The gs_update_state() function accesses the data from CAN error frames;
therefore, for error frames increase the value of data_length in
gs_usb_get_minimum_rx_length() to CAN_ERR_DLC.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 3c0464edb5d3..9ec6fed45b81 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -576,8 +576,12 @@ gs_usb_get_minimum_rx_length(const struct gs_can *dev, const struct gs_host_fram
 			minimum_length = sizeof(hf->header) + data_length;
 	} else {
 		if (hf->echo_id == GS_HOST_FRAME_ECHO_ID_RX &&
-		    !(hf->can_id & cpu_to_le32(CAN_RTR_FLAG)))
-			data_length = can_cc_dlc2len(hf->can_dlc);
+		    !(hf->can_id & cpu_to_le32(CAN_RTR_FLAG))) {
+			if (hf->can_id & cpu_to_le32(CAN_ERR_FLAG))
+				data_length = CAN_ERR_DLC;
+			else
+				data_length = can_cc_dlc2len(hf->can_dlc);
+		}
 
 		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
 			/* timestamp follows data field of max size */

-- 
2.53.0

Re: [PATCH can-next 10/10] can: gs_usb: gs_usb_get_minimum_rx_length(): enforce data_length of 8 bytes for CAN error frames

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 2 (1)

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).

- [High] The patch fixes the error-frame check for one flag combination
  but misses another.
- [Low] For classic error frames, gs_usb_get_minimum_rx_length() now
  returns data_length = CAN_ERR_DLC (8), and…
- [Low] The commit fixes a regression but has no Fixes: tag.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch.

--

can: gs_usb: gs_usb_get_minimum_rx_length(): enforce data_length of 8 bytes for CAN error frames

This patch changes gs_usb_get_minimum_rx_length() so that classic CAN
error frames received from the device use a data_length of CAN_ERR_DLC (8)
instead of the length derived from the DLC. With that change, both the
minimum URB length check and the copy into cf->data cover the error frame
payload that gs_update_state() reads.

> therefore, for error frames increase the value of data_length in
> gs_usb_get_minimum_rx_length() to CAN_ERR_DLC.
>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

[Severity: Low]
Should this have a Fixes: tag?

Commit 395d988f9386 ("can: gs_usb: gs_usb_receive_bulk_callback(): check
actual_length before accessing data") changed the classic copy from this:

    memcpy(cf->data, hf->classic_can->data, 8);

to a copy based on data_length. After that change, gs_update_state() reads
a zeroed cf->data[1] for error frames with can_dlc < 2.

395d988f9386 has its own Fixes: tag and is a stable candidate. Would adding
the following help stable trees pick up this correction too?

Fixes: 395d988f9386 ("can: gs_usb: gs_usb_receive_bulk_callback(): check actual_length before accessing data")

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 3d49a73446927..dd01fd22510e2 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -576,8 +576,12 @@ gs_usb_get_minimum_rx_length(const struct gs_can *dev, const struct gs_host_fram
>  			minimum_length = sizeof(hf->header) + data_length;
>  	} else {
>  		if (hf->echo_id == GS_HOST_FRAME_ECHO_ID_RX &&
> -		    !(hf->can_id & cpu_to_le32(CAN_RTR_FLAG)))
> -			data_length = can_cc_dlc2len(hf->can_dlc);
> +		    !(hf->can_id & cpu_to_le32(CAN_RTR_FLAG))) {
> +			if (hf->can_id & cpu_to_le32(CAN_ERR_FLAG))

[Severity: High]
Does a frame with both CAN_ERR_FLAG and CAN_RTR_FLAG set still end up with
data_length == 0 here?

The new CAN_ERR_FLAG test sits inside the !CAN_RTR_FLAG condition. A
classic RX frame from the device with both flags set in can_id therefore
gets data_length = 0. Without GS_CAN_FEATURE_HW_TIMESTAMP, minimum_length
is then just sizeof(hf->header).

gs_usb_receive_bulk_callback() then copies nothing. It still calls
gs_update_state(), because that call only tests CAN_ERR_FLAG:

    memcpy(cf->data, hf->classic_can->data, data_length);

    /* ERROR frames tell us information about the controller */
    if (le32_to_cpu(hf->can_id) & CAN_ERR_FLAG)
        gs_update_state(dev, cf);

If CAN_ERR_CRTL is also set, gs_update_state() reads cf->data[1]. That
byte is the zero fill from alloc_can_skb(), not data from the device, so
the code takes this branch:

    } else {
        dev->can.state = CAN_STATE_ERROR_ACTIVE;
    }

The driver then reports error active instead of the warning or passive
state the device sent. It also skips the error_warning and error_passive
counters.

With GS_CAN_FEATURE_HW_TIMESTAMP enabled, the URB must already contain the
full data field, but those bytes are still not copied.

Before 395d988f9386 the classic path always copied 8 bytes, so this flag
combination is still a regression after this patch. Could CAN_ERR_FLAG be
tested before the RTR exclusion, or could ERR|RTR frames be rejected?

> +				data_length = CAN_ERR_DLC;

[Severity: Low]
For classic error frames the copy length is now CAN_ERR_DLC. However,
gs_usb_receive_bulk_callback() still sets cf->len from the device's
hf->can_dlc:

    can_frame_set_cc_len(cf, hf->can_dlc, dev->can.ctrlmode);

    memcpy(cf->data, hf->classic_can->data, data_length);

can_frame_set_cc_len() only clamps the value to CAN_MAX_DLEN. Suppose a
device sends an error frame with can_dlc < 8. Would userspace then get a
frame with cf->len < CAN_ERR_DLC, while the driver has copied and acted on
bytes past cf->len? Since 395d988f9386 the copy length and cf->len always
matched.

The commit message says "By definition, CAN error frames have a length of
CAN_ERR_DLC (= 8) bytes". Should cf->len also be set to CAN_ERR_DLC for
error frames?

> +			else
> +				data_length = can_cc_dlc2len(hf->can_dlc);
> +		}

[ ... ]

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch.
gs_usb_receive_bulk_callback() passes the raw hf->can_dlc from the device
to can_frame_set_cc_len() without checking its range:

    can_frame_set_cc_len(cf, hf->can_dlc, dev->can.ctrlmode);

The helper in include/linux/can/length.h expects the caller to have
checked the value already:

    /* the caller already ensured that dlc is a value from 0 .. 15 */
    if (ctrlmode & CAN_CTRLMODE_CC_LEN8_DLC && dlc > CAN_MAX_DLEN)
        cf->len8_dlc = dlc;

gs_make_candev() advertises CAN_CTRLMODE_CC_LEN8_DLC. With cc-len8-dlc
enabled, can a device that sends can_dlc > 15 pass a len8_dlc value of up
to 255 to raw socket readers?

In-kernel users read the value through can_get_cc_dlc(), which rejects
values above CAN_MAX_RAW_DLC.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004-gs_usb-cleanups-and-fixes-v1-0-7b4b06244a77%40pengutronix.de