[PATCH can-next 00/13] can: gs_usb: implement new features recently added to candleLight firmware

[PATCH can-next 00/13] can: gs_usb: implement new features recently added to candleLight firmware

[Marc Kleine-Budde]

The candleLight firmware was updated to include support for Transceiver
Delay Compensation (TDC) configuration and CAN bus off recovery. This
series adds supports for this.

Link: https://github.com/candle-usb/candleLight_fw/pull/317
Link: https://github.com/candle-usb/candleLight_fw/pull/318
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Marc Kleine-Budde (13):
      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: update USB protocol definitions contributed by Elmue firmware
      can: gs_usb: document GS_CAN_FEATURE_FILTER
      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(): reduce scope of several variables
      can: gs_usb: gs_update_state(): convert CAN state handling to can_change_state()
      can: gs_usb: gs_usb_state_get_by_hf(): optionally print recived CAN error frames
      can: gs_usb: gs_update_state(): set CAN_ERR_CNT if device sends CAN error counters
      can: gs_usb: implement CAN_CTRLMODE_BERR_REPORTING for devices without native support
      can: gs_usb: implement Transceiver Delay Compensation
      can: gs_usb: implement CAN bus off recovery

 drivers/net/can/usb/gs_usb.c | 441 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 365 insertions(+), 76 deletions(-)
---
base-commit: 298bb2b8903323f6ef2eab4819a2e477765f0ff1
change-id: 20260718-gs_usb-new-features-aec1d3b77914

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

[PATCH can-next 01/13] 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 ec9a7cbbbc69..c8bb6eeff667 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/13] 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 c8bb6eeff667..2f55030a21a5 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/13] can: gs_usb: update USB protocol definitions contributed by Elmue firmware

[Marc Kleine-Budde]

The gs_usb compatible firmware by Elmue extended the USB protocol.

Document the newly added GS_CAN_FEATURE_ELM_* bits and the new
GS_USB_BREQ_ELM_GET_* USB requests along with some placeholders for future
extensions.

Link: https://github.com/Elmue/CANable-2.5-firmware-Slcan-and-Candlelight
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 2f55030a21a5..57062983b148 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -72,6 +72,23 @@ enum gs_usb_breq {
 	GS_USB_BREQ_SET_TERMINATION,
 	GS_USB_BREQ_GET_TERMINATION,
 	GS_USB_BREQ_GET_STATE,
+	__GS_USB_BREQ_PLACEHOLDER_15,
+	__GS_USB_BREQ_PLACEHOLDER_16,
+	__GS_USB_BREQ_PLACEHOLDER_17,
+	__GS_USB_BREQ_PLACEHOLDER_18,
+	__GS_USB_BREQ_PLACEHOLDER_19,
+	GS_USB_BREQ_ELM_GET_BOARDINFO = 20,
+	GS_USB_BREQ_ELM_SET_FILTER,
+	GS_USB_BREQ_ELM_GET_LASTERROR,
+	GS_USB_BREQ_ELM_SET_BUSLOADREPORT,
+	GS_USB_BREQ_ELM_SET_PINSTATUS,
+	GS_USB_BREQ_ELM_GET_PINSTATUS,
+	GS_USB_BREQ_ELM_READ_FLASH,
+	GS_USB_BREQ_ELM_WRITE_FLASH,
+	__GS_USB_BREQ_ELM_PLACEHOLDER_28,
+	__GS_USB_BREQ_ELM_PLACEHOLDER_29,
+	__GS_USB_BREQ_ELM_PLACEHOLDER_30,
+	__GS_USB_BREQ_ELM_PLACEHOLDER_31,
 };
 
 enum gs_can_mode {
@@ -168,7 +185,14 @@ struct gs_device_termination_state {
 #define GS_CAN_FEATURE_TERMINATION BIT(11)
 #define GS_CAN_FEATURE_BERR_REPORTING BIT(12)
 #define GS_CAN_FEATURE_GET_STATE BIT(13)
-#define GS_CAN_FEATURE_MASK GENMASK(13, 0)
+#define GS_CAN_FEATURE_ELM_PROTOCOL BIT(14)
+/* supported by Elmue firmware until 0x260528 (including) */
+#define GS_CAN_FEATURE_ELM_DISABLE_TX_ECHO BIT(15)
+/* supported by Elmue firmware since 0x260529 (including) */
+#define GS_CAN_FEATURE_ELM_DEV_FLAG_SEND_USB_BLOBS BIT(15)
+/* supported by Elmue firmware 0x260528 only */
+#define GS_CAN_FEATURE_ELM_DEV_FLAG_SEND_USB_BLOBS_260528 BIT(16)
+#define GS_CAN_FEATURE_MASK GENMASK(16, 0)
 
 /* internal quirks - keep in GS_CAN_FEATURE space for now */
 

-- 
2.53.0

[PATCH can-next 04/13] can: gs_usb: document GS_CAN_FEATURE_FILTER

[Marc Kleine-Budde]

Document the feature ("GS_CAN_FEATURE_FILTER") that indicates that the CAN
controller supports querying CAN filter capabilities with the
GS_USB_BREQ_GET_FILTER control message and setting CAN filter with the
GS_USB_BREQ_SET_FILTER control message.

Link: https://github.com/candle-usb/candleLight_fw/pull/204
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 57062983b148..82be2db76fe7 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -72,8 +72,8 @@ enum gs_usb_breq {
 	GS_USB_BREQ_SET_TERMINATION,
 	GS_USB_BREQ_GET_TERMINATION,
 	GS_USB_BREQ_GET_STATE,
-	__GS_USB_BREQ_PLACEHOLDER_15,
-	__GS_USB_BREQ_PLACEHOLDER_16,
+	GS_USB_BREQ_SET_FILTER,
+	GS_USB_BREQ_GET_FILTER,
 	__GS_USB_BREQ_PLACEHOLDER_17,
 	__GS_USB_BREQ_PLACEHOLDER_18,
 	__GS_USB_BREQ_PLACEHOLDER_19,
@@ -120,6 +120,10 @@ enum gs_can_termination_state {
 #define GS_USB_TERMINATION_DISABLED CAN_TERMINATION_DISABLED
 #define GS_USB_TERMINATION_ENABLED 120
 
+enum gs_device_filter_dev {
+	GS_DEVICE_FILTER_DEV_BXCAN = 1,         /* bxcan, 14 filters */
+};
+
 /* data types passed between host and device */
 
 /* The firmware on the original USB2CAN by Geschwister Schneider
@@ -171,6 +175,27 @@ struct gs_device_termination_state {
 	__le32 state;
 } __packed;
 
+struct gs_device_filter_info {
+	u8 dev;		/* enum gs_device_filter_dev */
+	u8 reserved[3];
+}  __packed __aligned(4);
+
+struct gs_device_filter_bxcan {
+	__le32 fs1r;
+	__le32 fm1r;
+	__le32 ffa1r;
+	__le32 fa1r;
+	__le32 fr1[14];
+	__le32 fr2[14];
+} __packed __aligned(4);
+
+struct gs_device_filter {
+	struct gs_device_filter_info info;
+	union {
+		struct gs_device_filter_bxcan bxcan;
+	};
+} __packed __aligned(4);
+
 #define GS_CAN_FEATURE_LISTEN_ONLY BIT(0)
 #define GS_CAN_FEATURE_LOOP_BACK BIT(1)
 #define GS_CAN_FEATURE_TRIPLE_SAMPLE BIT(2)
@@ -192,6 +217,7 @@ struct gs_device_termination_state {
 #define GS_CAN_FEATURE_ELM_DEV_FLAG_SEND_USB_BLOBS BIT(15)
 /* supported by Elmue firmware 0x260528 only */
 #define GS_CAN_FEATURE_ELM_DEV_FLAG_SEND_USB_BLOBS_260528 BIT(16)
+#define GS_CAN_FEATURE_FILTER BIT(16)
 #define GS_CAN_FEATURE_MASK GENMASK(16, 0)
 
 /* internal quirks - keep in GS_CAN_FEATURE space for now */

-- 
2.53.0

[PATCH can-next 05/13] 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 82be2db76fe7..d049ae20b53d 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -1355,7 +1355,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;
 
@@ -1495,6 +1494,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 06/13] 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 d049ae20b53d..d77f0940c035 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -1420,6 +1420,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;
 
@@ -1432,6 +1434,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
@@ -1440,27 +1447,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
@@ -1484,11 +1470,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
 	 */
@@ -1522,6 +1503,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 07/13] 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 d77f0940c035..473f125e20d8 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -649,10 +649,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);
 
@@ -705,7 +701,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)
 				return;
@@ -719,6 +719,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)
 				return;
@@ -735,6 +737,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",
@@ -763,6 +768,9 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 	}
 
 	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

Re: [PATCH can-next 07/13] can: gs_usb: gs_usb_receive_bulk_callback(): reduce scope of several variables

[sashiko-bot]

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

Pre-existing issues:
- [High] Returning directly on SKB allocation failure drops the USB URB instead of resubmitting it, permanently reducing the RX URB pool and potentially stalling the receiver entirely.
--

--- Patch [7]: [PATCH can-next 07/13] can: gs_usb: gs_usb_receive_bulk_callback(): reduce scope of several variables ---
commit 2f6b205703c7b1ac4e58ba90c1e5b7687566e6e9
Author: Marc Kleine-Budde <mkl@pengutronix.de>

can: gs_usb: gs_usb_receive_bulk_callback(): reduce scope of several variables

This patch improves code readability by reducing the scope of variables
txc, cf, cfd, and skb in gs_usb_receive_bulk_callback().

> 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
[ ... ]
> @@ -705,7 +701,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)
>  				return;

[Severity: High]
This is a pre-existing issue, but does returning directly on skb allocation
failure drop the USB URB instead of resubmitting it?

If enough skb allocations fail under memory pressure, it seems the RX URB pool
will permanently decrease. Could this eventually stall the receiver entirely,
requiring the interface to be brought down and back up to recover? 

Should this failure path go to the resubmit_urb label instead?

> @@ -719,6 +719,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)
>  				return;

[Severity: High]
This is also a pre-existing issue, but does this allocation failure also leak
the URB by returning directly rather than resubmitting?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-gs_usb-new-features-v1-0-427a8013c380@pengutronix.de?part=7

Re: [PATCH can-next 07/13] can: gs_usb: gs_usb_receive_bulk_callback(): reduce scope of several variables

[Marc Kleine-Budde]

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

On 20.07.2026 14:00:23, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Pre-existing issues:
> - [High] Returning directly on SKB allocation failure drops the USB
> URB instead of resubmitting it, permanently reducing the RX URB pool
> and potentially stalling the receiver entirely.

FTR: this will be fixed with
https://lore.kernel.org/all/20260709-gs_usb-resubmit-urb-v1-1-4dd40030cc84@pengutronix.de/

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 --]

[PATCH can-next 08/13] can: gs_usb: gs_update_state(): convert CAN state handling to can_change_state()

[Marc Kleine-Budde]

The gs_usb compatible firmware signals CAN state changes via CAN error
frames to the kernel. As the kernel tracks the state of the CAN device, the
driver has to parse the CAN state from these CAN error frames.

Since the driver has been written, the can_change_state() helper was
introduced to the kernel, in order to send more uniform CAN state error
frames to the user space. Convert the driver to make use of the
can_change_state() helper.

Implement gs_usb_state_get_by_hf() to figure TX and RX state by parsing the
received struct gs_host_frame.

If the CAN error counters are not 0, some firmware repeatedly send CAN
state error frames, even if the CAN state does not change. Implement
gs_usb_should_handle_can_error() to drop these, but keep other CAN error
frames.

Update gs_update_state() to make use of can_change_state(), clear all CAN
state information from the CAN frame (which will be send to user space) and
let can_change_state() populate it consistently from the previously parsed
CAN TX and RX state information.

Some firmware does automatically CAN bus off recovery, without signaling
this via CAN_ERR_RESTARTED. If the CAN state changes from Bus Off to Error
Active, Warning or Passive, treat this also as a restart of the CAN
controller.

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

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 473f125e20d8..e41f91b56ab8 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -529,29 +529,31 @@ static void gs_usb_timestamp_stop(struct gs_usb *parent)
 	cancel_delayed_work_sync(&parent->timestamp);
 }
 
-static void gs_update_state(struct gs_can *dev, struct can_frame *cf)
+static void gs_update_state(struct gs_can *dev, struct can_frame *cf,
+			    enum can_state tx_state, enum can_state rx_state)
 {
 	struct can_device_stats *can_stats = &dev->can.can_stats;
 
-	if (cf->can_id & CAN_ERR_RESTARTED) {
-		dev->can.state = CAN_STATE_ERROR_ACTIVE;
+	/* clear CAN state information, will be set consistently by can_change_state() below */
+	cf->can_id &= ~(CAN_ERR_CRTL | CAN_ERR_BUSOFF | CAN_ERR_RESTARTED);
+	cf->data[1] &= ~(CAN_ERR_CRTL_RX_WARNING | CAN_ERR_CRTL_TX_WARNING |
+			 CAN_ERR_CRTL_RX_PASSIVE | CAN_ERR_CRTL_TX_PASSIVE |
+			 CAN_ERR_CRTL_ACTIVE);
+
+	const enum can_state new_state = max(rx_state, tx_state);
+
+	if (new_state == dev->can.state)
+		return;
+
+	/* some firmware does automatically CAN bus off recovery, account for this */
+	if (cf->can_id & CAN_ERR_RESTARTED ||
+	    (dev->can.state == CAN_STATE_BUS_OFF && new_state < CAN_STATE_BUS_OFF)) {
 		can_stats->restarts++;
-	} else if (cf->can_id & CAN_ERR_BUSOFF) {
-		dev->can.state = CAN_STATE_BUS_OFF;
-		can_stats->bus_off++;
-	} else if (cf->can_id & CAN_ERR_CRTL) {
-		if ((cf->data[1] & CAN_ERR_CRTL_TX_WARNING) ||
-		    (cf->data[1] & CAN_ERR_CRTL_RX_WARNING)) {
-			dev->can.state = CAN_STATE_ERROR_WARNING;
-			can_stats->error_warning++;
-		} else if ((cf->data[1] & CAN_ERR_CRTL_TX_PASSIVE) ||
-			   (cf->data[1] & CAN_ERR_CRTL_RX_PASSIVE)) {
-			dev->can.state = CAN_STATE_ERROR_PASSIVE;
-			can_stats->error_passive++;
-		} else {
-			dev->can.state = CAN_STATE_ERROR_ACTIVE;
-		}
+		/* some firmware doesn't set CAN_ERR_RESTARTED, fixup */
+		cf->can_id |= CAN_ERR_RESTARTED;
 	}
+
+	can_change_state(dev->can.dev, cf, tx_state, rx_state);
 }
 
 static u32 gs_usb_set_timestamp(struct gs_can *dev, struct sk_buff *skb,
@@ -640,6 +642,75 @@ gs_usb_get_minimum_rx_length(const struct gs_can *dev, const struct gs_host_fram
 	return minimum_length;
 }
 
+static void
+gs_usb_state_get_by_hf(const struct gs_can *dev, const struct gs_host_frame *hf,
+		       enum can_state *tx_state_p, enum can_state *rx_state_p)
+{
+	enum can_state tx_state, rx_state;
+
+	/* extract CAN state from frame */
+	if (hf->can_id & cpu_to_le32(CAN_ERR_CRTL | CAN_ERR_BUSOFF)) {
+		const u8 err_crtl = hf->classic_can->data[1];
+
+		if (hf->can_id & cpu_to_le32(CAN_ERR_BUSOFF)) {
+			tx_state = CAN_STATE_BUS_OFF;
+			rx_state = CAN_STATE_BUS_OFF;
+		} else {
+			if (err_crtl & CAN_ERR_CRTL_RX_PASSIVE)
+				rx_state = CAN_STATE_ERROR_PASSIVE;
+			else if (err_crtl & CAN_ERR_CRTL_RX_WARNING)
+				rx_state = CAN_STATE_ERROR_WARNING;
+			else
+				rx_state = CAN_STATE_ERROR_ACTIVE;
+
+			if (err_crtl & CAN_ERR_CRTL_TX_PASSIVE)
+				tx_state = CAN_STATE_ERROR_PASSIVE;
+			else if (err_crtl & CAN_ERR_CRTL_TX_WARNING)
+				tx_state = CAN_STATE_ERROR_WARNING;
+			else
+				tx_state = CAN_STATE_ERROR_ACTIVE;
+		}
+	} else {
+		tx_state = dev->can.state;
+		rx_state = dev->can.state;
+	}
+
+	*tx_state_p = tx_state;
+	*rx_state_p = rx_state;
+}
+
+static bool
+gs_usb_should_handle_can_error(const struct gs_can *dev, const struct gs_host_frame *hf,
+			       enum can_state *tx_state_p, enum can_state *rx_state_p)
+{
+	gs_usb_state_get_by_hf(dev, hf, tx_state_p, rx_state_p);
+
+	/* If the CAN error counters are not 0, some firmware repeatedly send CAN state error
+	 * frames, even if the CAN state does not change. Handle changed CAN states.
+	 */
+	const enum can_state new_state = max(*tx_state_p, *rx_state_p);
+
+	if (new_state != dev->can.state)
+		return true;
+
+	/* handle RX and TX overflow */
+	const u8 err_crtl = hf->classic_can->data[1];
+
+	if ((hf->can_id & cpu_to_le32(CAN_ERR_CRTL)) &&
+	    err_crtl & (CAN_ERR_CRTL_RX_OVERFLOW | CAN_ERR_CRTL_TX_OVERFLOW))
+		return true;
+
+	/* handle TX timeout */
+	if (hf->can_id & cpu_to_le32(CAN_ERR_TX_TIMEOUT))
+		return true;
+
+	/* handle CAN bus errors */
+	if (hf->can_id & cpu_to_le32(CAN_ERR_LOSTARB | CAN_ERR_PROT | CAN_ERR_TRX | CAN_ERR_ACK))
+		return true;
+
+	return false;
+}
+
 static void gs_usb_receive_bulk_callback(struct urb *urb)
 {
 	struct gs_usb *parent = urb->context;
@@ -719,8 +790,13 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 
 			memcpy(cfd->data, hf->canfd->data, data_length);
 		} else {
+			enum can_state tx_state, rx_state;
 			struct can_frame *cf;
 
+			if (hf->can_id & cpu_to_le32(CAN_ERR_FLAG) &&
+			    !gs_usb_should_handle_can_error(dev, hf, &tx_state, &rx_state))
+				goto resubmit_urb;
+
 			skb = alloc_can_skb(netdev, &cf);
 			if (!skb)
 				return;
@@ -732,7 +808,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 
 			/* ERROR frames tell us information about the controller */
 			if (le32_to_cpu(hf->can_id) & CAN_ERR_FLAG)
-				gs_update_state(dev, cf);
+				gs_update_state(dev, cf, tx_state, rx_state);
 		}
 
 		gs_usb_rx_offload(dev, skb, hf);

-- 
2.53.0

Re: [PATCH can-next 08/13] can: gs_usb: gs_update_state(): convert CAN state handling to can_change_state()

[sashiko-bot]

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

New issues:
- [High] The CAN_ERR_RESTARTED flag is unconditionally stripped from cf->can_id before being evaluated, completely ignoring firmware-signaled restart events.
- [High] CAN_ERR_RESTARTED frames are incorrectly dropped by gs_usb_should_handle_can_error() if they do not coincide with a CAN state change.
- [High] Dropping redundant CAN error frames silently bypasses the hardware-level RX overflow check and offload irq finish.
- [Medium] Early return in gs_update_state() leaves CAN_ERR_CRTL cleared, hiding RX/TX overflow errors from userspace.
- [Medium] Parsing CAN error states directly from the USB host frame reads uninitialized memory if the device sends a short error frame.

Pre-existing issues:
- [High] Failing to allocate an SKB causes the driver to return directly without resubmitting the URB, starving the RX pipeline.
--

--- Patch [8]: [PATCH can-next 08/13] can: gs_usb: gs_update_state(): convert CAN state handling to can_change_state() ---
commit 0f07f8fda15070cb595e00084e739e3a1240c93b
Author: Marc Kleine-Budde <mkl@pengutronix.de>

can: gs_usb: gs_update_state(): convert CAN state handling to can_change_state()

The gs_usb compatible firmware signals CAN state changes via CAN error
frames to the kernel. This commit converts the driver to make use of the
can_change_state() helper. It also implements functions to extract CAN
state and drop redundant CAN state error frames.

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 473f125e20d8a..e41f91b56ab82 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -529,29 +529,31 @@ static void gs_usb_timestamp_stop(struct gs_usb *parent)
>  	cancel_delayed_work_sync(&parent->timestamp);
>  }
>  
> -static void gs_update_state(struct gs_can *dev, struct can_frame *cf)
> +static void gs_update_state(struct gs_can *dev, struct can_frame *cf,
> +			    enum can_state tx_state, enum can_state rx_state)
>  {
>  	struct can_device_stats *can_stats = &dev->can.can_stats;
>  
> -	if (cf->can_id & CAN_ERR_RESTARTED) {
> -		dev->can.state = CAN_STATE_ERROR_ACTIVE;
> +	/* clear CAN state information, will be set consistently by can_change_state() below */
> +	cf->can_id &= ~(CAN_ERR_CRTL | CAN_ERR_BUSOFF | CAN_ERR_RESTARTED);
> +	cf->data[1] &= ~(CAN_ERR_CRTL_RX_WARNING | CAN_ERR_CRTL_TX_WARNING |
> +			 CAN_ERR_CRTL_RX_PASSIVE | CAN_ERR_CRTL_TX_PASSIVE |
> +			 CAN_ERR_CRTL_ACTIVE);
> +
> +	const enum can_state new_state = max(rx_state, tx_state);
> +
> +	if (new_state == dev->can.state)
> +		return;

[Severity: Medium]
Does returning early here hide RX/TX overflow errors from userspace? Since
CAN_ERR_CRTL is cleared from cf->can_id just above, if there is no state
change but data[1] contains an overflow error (like CAN_ERR_CRTL_RX_OVERFLOW),
the frame goes to userspace without the CAN_ERR_CRTL bit set, causing
userspace to silently ignore the valid error flags.

> +
> +	/* some firmware does automatically CAN bus off recovery, account for this */
> +	if (cf->can_id & CAN_ERR_RESTARTED ||

[Severity: High]
Will this check for CAN_ERR_RESTARTED ever evaluate to true? The
CAN_ERR_RESTARTED flag is unconditionally cleared from cf->can_id at the start
of this function, so it appears this will silently ignore firmware-signaled
restart events.

> +	    (dev->can.state == CAN_STATE_BUS_OFF && new_state < CAN_STATE_BUS_OFF)) {
>  		can_stats->restarts++;

[ ... ]

> +static void
> +gs_usb_state_get_by_hf(const struct gs_can *dev, const struct gs_host_frame *hf,
> +		       enum can_state *tx_state_p, enum can_state *rx_state_p)
> +{
> +	enum can_state tx_state, rx_state;
> +
> +	/* extract CAN state from frame */
> +	if (hf->can_id & cpu_to_le32(CAN_ERR_CRTL | CAN_ERR_BUSOFF)) {
> +		const u8 err_crtl = hf->classic_can->data[1];

[Severity: Medium]
Could this read uninitialized memory if the device sends a short error frame?
If the USB device sends an error frame with a payload smaller than 2 bytes,
this unconditionally reads from the URB buffer, which might contain stale data
from previous USB transfers.

> +
> +		if (hf->can_id & cpu_to_le32(CAN_ERR_BUSOFF)) {
> +			tx_state = CAN_STATE_BUS_OFF;

[ ... ]

> +static bool
> +gs_usb_should_handle_can_error(const struct gs_can *dev, const struct gs_host_frame *hf,
> +			       enum can_state *tx_state_p, enum can_state *rx_state_p)
> +{
> +	gs_usb_state_get_by_hf(dev, hf, tx_state_p, rx_state_p);
> +
> +	/* If the CAN error counters are not 0, some firmware repeatedly send CAN state error
> +	 * frames, even if the CAN state does not change. Handle changed CAN states.
> +	 */
> +	const enum can_state new_state = max(*tx_state_p, *rx_state_p);
> +
> +	if (new_state != dev->can.state)
> +		return true;
> +
> +	/* handle RX and TX overflow */
> +	const u8 err_crtl = hf->classic_can->data[1];
> +
> +	if ((hf->can_id & cpu_to_le32(CAN_ERR_CRTL)) &&
> +	    err_crtl & (CAN_ERR_CRTL_RX_OVERFLOW | CAN_ERR_CRTL_TX_OVERFLOW))
> +		return true;
> +
> +	/* handle TX timeout */
> +	if (hf->can_id & cpu_to_le32(CAN_ERR_TX_TIMEOUT))
> +		return true;
> +
> +	/* handle CAN bus errors */
> +	if (hf->can_id & cpu_to_le32(CAN_ERR_LOSTARB | CAN_ERR_PROT | CAN_ERR_TRX | CAN_ERR_ACK))
> +		return true;
> +
> +	return false;

[Severity: High]
Are CAN_ERR_RESTARTED frames mistakenly dropped here? If a frame has
CAN_ERR_RESTARTED set but no state change, it falls through to this point.
Since CAN_ERR_RESTARTED is absent from the hardcoded list of accepted error
flags, it seems these frames will be discarded entirely.

> +}
> +
>  static void gs_usb_receive_bulk_callback(struct urb *urb)
>  {

[ ... ]

> @@ -719,8 +790,13 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  
>  			memcpy(cfd->data, hf->canfd->data, data_length);
>  		} else {
> +			enum can_state tx_state, rx_state;
>  			struct can_frame *cf;
>  
> +			if (hf->can_id & cpu_to_le32(CAN_ERR_FLAG) &&
> +			    !gs_usb_should_handle_can_error(dev, hf, &tx_state, &rx_state))
> +				goto resubmit_urb;

[Severity: High]
Does jumping to resubmit_urb here bypass the hardware RX overflow checks? The
check for hf->flags & GS_CAN_FLAG_OVERFLOW and the call to
can_rx_offload_irq_finish() happen later in this function, so dropping
redundant CAN error frames this way appears to silently lose piggybacked
hardware overflow events and delay offload processing.

> +
>  			skb = alloc_can_skb(netdev, &cf);
>  			if (!skb)
>  				return;

[Severity: High]
This isn't a bug introduced by this patch, but doesn't an early return here
leak the URB structure? If alloc_can_skb() fails to allocate memory, the
driver executes an immediate return instead of resubmitting the URB. This
bypasses usb_submit_urb() and depletes the active RX URB pool, which could
permanently stall the RX pipeline.

> @@ -732,7 +808,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  
>  			/* ERROR frames tell us information about the controller */
>  			if (le32_to_cpu(hf->can_id) & CAN_ERR_FLAG)
> -				gs_update_state(dev, cf);
> +				gs_update_state(dev, cf, tx_state, rx_state);
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-gs_usb-new-features-v1-0-427a8013c380@pengutronix.de?part=8

[PATCH can-next 09/13] can: gs_usb: gs_usb_state_get_by_hf(): optionally print recived CAN error frames

[Marc Kleine-Budde]

The previous commit introduced gs_usb_state_get_by_hf() to parse the
current CAN error state from the received CAN error frame.

As there are several gs_usb compatible implementations which slightly
differ in their CAN state and CAN error frame handling add a netdev_dbg()
that prints the received CAN error frame for easier future debugging.

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

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index e41f91b56ab8..2848cb1a75f0 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -670,6 +670,21 @@ gs_usb_state_get_by_hf(const struct gs_can *dev, const struct gs_host_frame *hf,
 			else
 				tx_state = CAN_STATE_ERROR_ACTIVE;
 		}
+
+		const u8 tx_err = hf->classic_can->data[6];
+		const u8 rx_err = hf->classic_can->data[7];
+
+		netdev_dbg(dev->netdev, "host_frame: TX-state='%s/%u' RX-state='%s/%u' %s%s%s%s%s%s%s",
+			   can_get_state_str(tx_state), tx_err,
+			   can_get_state_str(rx_state), rx_err,
+			   hf->can_id & cpu_to_le32(CAN_ERR_BUSOFF) ? "bus-off " : "",
+			   hf->can_id & cpu_to_le32(CAN_ERR_RESTARTED) ?
+			   "restarted-after-bus-off " : "",
+			   err_crtl & CAN_ERR_CRTL_RX_PASSIVE ? "rx-error-passive " : "",
+			   err_crtl & CAN_ERR_CRTL_RX_WARNING ? "rx-error-warning " : "",
+			   err_crtl & CAN_ERR_CRTL_TX_PASSIVE ? "tx-error-passive " : "",
+			   err_crtl & CAN_ERR_CRTL_TX_WARNING ? "tx-error-warning " : "",
+			   err_crtl & CAN_ERR_CRTL_ACTIVE ? "back-to-error-active " : "");
 	} else {
 		tx_state = dev->can.state;
 		rx_state = dev->can.state;

-- 
2.53.0

Re: [PATCH can-next 09/13] can: gs_usb: gs_usb_state_get_by_hf(): optionally print recived CAN error frames

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The driver reads uninitialized kernel heap memory or stale URB data from `data[1]`, `data[6]`, and `data[7]` when processing short CAN error frames.
--

--- Patch [9]: [PATCH can-next 09/13] can: gs_usb: gs_usb_state_get_by_hf(): optionally print recived CAN error frames ---
commit f418a47ba868ccfb1b18e7b941a14de6bd026311
Author: Marc Kleine-Budde <mkl@pengutronix.de>

can: gs_usb: gs_usb_state_get_by_hf(): optionally print recived CAN error frames

This commit introduces a netdev_dbg() statement in gs_usb_state_get_by_hf()
to parse and print the current CAN error state and error frame payload,
making debugging easier across different gs_usb compatible implementations.

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index e41f91b56ab82..2848cb1a75f03 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -670,6 +670,21 @@ gs_usb_state_get_by_hf(const struct gs_can *dev, const struct gs_host_frame *hf,
>  			else
>  				tx_state = CAN_STATE_ERROR_ACTIVE;
>  		}
> +
> +		const u8 tx_err = hf->classic_can->data[6];
> +		const u8 rx_err = hf->classic_can->data[7];

[Severity: Medium]
Could this lead to reading uninitialized kernel heap memory or stale URB data
if a device sends a short CAN error frame?

If a malfunctioning USB device sends an error frame where CAN_ERR_FLAG is set
but specifies a can_dlc of less than 8, gs_usb_get_minimum_rx_length() might
calculate a small minimum_length based on the untrusted can_dlc.

This allows short packets to bypass the bounds checks. When
gs_usb_state_get_by_hf() unconditionally accesses data[6] and data[7] here,
it can read beyond the actual received payload length.

> +
> +		netdev_dbg(dev->netdev, "host_frame: TX-state='%s/%u' RX-state='%s/%u' %s%s%s%s%s%s%s",
> +			   can_get_state_str(tx_state), tx_err,
> +			   can_get_state_str(rx_state), rx_err,
> +			   hf->can_id & cpu_to_le32(CAN_ERR_BUSOFF) ? "bus-off " : "",

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-gs_usb-new-features-v1-0-427a8013c380@pengutronix.de?part=9

[PATCH can-next 10/13] can: gs_usb: gs_update_state(): set CAN_ERR_CNT if device sends CAN error counters

[Marc Kleine-Budde]

Some gs_usb compatible firmware send the CAN error counters in CAN error
frames, but doesn't set CAN_ERR_CNT in can_id.

Fixup the CAN error frame by setting CAN_ERR_CNT, if RX or TX error
counters are greater than 0.

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

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 2848cb1a75f0..186abc95c977 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -540,6 +540,10 @@ static void gs_update_state(struct gs_can *dev, struct can_frame *cf,
 			 CAN_ERR_CRTL_RX_PASSIVE | CAN_ERR_CRTL_TX_PASSIVE |
 			 CAN_ERR_CRTL_ACTIVE);
 
+	/* some firmware sends CAN error counters but doesn't set CAN_ERR_CNT, fixup */
+	if (cf->data[6] || cf->data[7])
+		cf->can_id |= CAN_ERR_CNT;
+
 	const enum can_state new_state = max(rx_state, tx_state);
 
 	if (new_state == dev->can.state)

-- 
2.53.0

Re: [PATCH can-next 10/13] can: gs_usb: gs_update_state(): set CAN_ERR_CNT if device sends CAN error counters

[sashiko-bot]

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

New issues:
- [High] The driver unconditionally reads uninitialized memory from the URB transfer buffer when processing malformed CAN error frames with a small DLC.
- [Medium] Valid CAN error flags (like RX/TX overflow) are silently dropped from userspace if the CAN state does not change.
- [Medium] The CAN_ERR_RESTARTED flag is unconditionally cleared from the CAN ID and then immediately checked, causing explicit restart signals from the firmware to be ignored.

Pre-existing issues:
- [High] If `alloc_can_skb()` or `alloc_canfd_skb()` fails, the driver aborts URB processing without resubmitting the URB, potentially permanently starving the RX path.
--

--- Patch [10]: [PATCH can-next 10/13] can: gs_usb: gs_update_state(): set CAN_ERR_CNT if device sends CAN error counters ---
commit f1e0f1d0c2f80b9c65dc60e4897538baed5336ee
Author: Marc Kleine-Budde <mkl@pengutronix.de>

can: gs_usb: gs_update_state(): set CAN_ERR_CNT if device sends CAN error counters

Some gs_usb compatible firmware send the CAN error counters in CAN error
frames, but doesn't set CAN_ERR_CNT in can_id. Fixup the CAN error frame by
setting CAN_ERR_CNT, if RX or TX error counters are greater than 0.

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index ec9a7cbbbc696..186abc95c9774 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c

[ ... ]

> @@ -495,29 +529,35 @@ static void gs_usb_timestamp_stop(struct gs_usb *parent)
> -static void gs_update_state(struct gs_can *dev, struct can_frame *cf)
> +static void gs_update_state(struct gs_can *dev, struct can_frame *cf,
> +			    enum can_state tx_state, enum can_state rx_state)
>  {
>  	struct can_device_stats *can_stats = &dev->can.can_stats;
>  
> -	if (cf->can_id & CAN_ERR_RESTARTED) {
> -		dev->can.state = CAN_STATE_ERROR_ACTIVE;
> +	/* clear CAN state information, will be set consistently by can_change_state() below */
> +	cf->can_id &= ~(CAN_ERR_CRTL | CAN_ERR_BUSOFF | CAN_ERR_RESTARTED);
> +	cf->data[1] &= ~(CAN_ERR_CRTL_RX_WARNING | CAN_ERR_CRTL_TX_WARNING |
> +			 CAN_ERR_CRTL_RX_PASSIVE | CAN_ERR_CRTL_TX_PASSIVE |
> +			 CAN_ERR_CRTL_ACTIVE);
> +
> +	/* some firmware sends CAN error counters but doesn't set CAN_ERR_CNT, fixup */
> +	if (cf->data[6] || cf->data[7])
> +		cf->can_id |= CAN_ERR_CNT;
> +
> +	const enum can_state new_state = max(rx_state, tx_state);
> +
> +	if (new_state == dev->can.state)
> +		return;

[Severity: Medium]
If the CAN state does not change, we return early from gs_update_state()
here, but we've already stripped the CAN_ERR_CRTL flag from cf->can_id.

Does this silently drop valid CAN error flags, such as RX overflow,
preventing them from reaching userspace?

> +
> +	/* some firmware does automatically CAN bus off recovery, account for this */
> +	if (cf->can_id & CAN_ERR_RESTARTED ||

[Severity: Medium]
Since we unconditionally cleared the CAN_ERR_RESTARTED bit from cf->can_id at
the start of gs_update_state(), won't this check always evaluate to false?

Does this cause explicit restart signals sent by the firmware to be ignored?

> +	    (dev->can.state == CAN_STATE_BUS_OFF && new_state < CAN_STATE_BUS_OFF)) {
>  		can_stats->restarts++;
> -	} else if (cf->can_id & CAN_ERR_BUSOFF) {
> -		dev->can.state = CAN_STATE_BUS_OFF;
> -		can_stats->bus_off++;

[ ... ]

> @@ -606,6 +646,90 @@ gs_usb_get_minimum_rx_length(const struct gs_can *dev, const struct gs_host_fram
> +static void
> +gs_usb_state_get_by_hf(const struct gs_can *dev, const struct gs_host_frame *hf,
> +		       enum can_state *tx_state_p, enum can_state *rx_state_p)
> +{
> +	enum can_state tx_state, rx_state;
> +
> +	/* extract CAN state from frame */
> +	if (hf->can_id & cpu_to_le32(CAN_ERR_CRTL | CAN_ERR_BUSOFF)) {
> +		const u8 err_crtl = hf->classic_can->data[1];
> +
> +		if (hf->can_id & cpu_to_le32(CAN_ERR_BUSOFF)) {
> +			tx_state = CAN_STATE_BUS_OFF;
> +			rx_state = CAN_STATE_BUS_OFF;
> +		} else {
> +			if (err_crtl & CAN_ERR_CRTL_RX_PASSIVE)
> +				rx_state = CAN_STATE_ERROR_PASSIVE;

[ ... ]

> +		}
> +
> +		const u8 tx_err = hf->classic_can->data[6];
> +		const u8 rx_err = hf->classic_can->data[7];

[Severity: High]
Does this code in gs_usb_state_get_by_hf() unconditionally read uninitialized
memory from the kmalloc'ed URB transfer buffer if a malformed CAN error frame
with a small DLC is received?

Since the URB length check only ensures the header is present, accessing fixed
offsets like data[6] and data[7] could read past the actual payload, leaking
uninitialized heap memory to the kernel log and causing erratic state
transitions.

> +
> +		netdev_dbg(dev->netdev, "host_frame: TX-state='%s/%u' RX-state='%s/%u' %s%s%s%s%s%s%s",
> +			   can_get_state_str(tx_state), tx_err,
> +			   can_get_state_str(rx_state), rx_err,

[ ... ]

> @@ -671,7 +791,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)
>  				return;

[Severity: High]
This is a pre-existing issue in gs_usb_receive_bulk_callback(), but if this
allocation fails and we return directly, the URB is never resubmitted.

Could this permanently starve the RX path and kill CAN reception under
systemic memory pressure?

> @@ -685,6 +809,13 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  
>  			memcpy(cfd->data, hf->canfd->data, data_length);
>  		} else {
> +			enum can_state tx_state, rx_state;
> +			struct can_frame *cf;
> +
> +			if (hf->can_id & cpu_to_le32(CAN_ERR_FLAG) &&
> +			    !gs_usb_should_handle_can_error(dev, hf, &tx_state, &rx_state))
> +				goto resubmit_urb;
> +
>  			skb = alloc_can_skb(netdev, &cf);
>  			if (!skb)
>  				return;

[Severity: High]
This is also a pre-existing issue in gs_usb_receive_bulk_callback().
Returning directly here without jumping to the resubmit_urb label means the
URB is permanently dropped from the active queue.

Could this lead to the same RX starvation issue mentioned above?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-gs_usb-new-features-v1-0-427a8013c380@pengutronix.de?part=10

[PATCH can-next 11/13] can: gs_usb: implement CAN_CTRLMODE_BERR_REPORTING for devices without native support

[Marc Kleine-Budde]

RX or TX problems on the CAN bus are signaled by the controller in the form
of CAN bus error. Each bus error increases the CAN error
counters (successful RX or TX decreases the counters). When certain limits
are crossed the controller changes into a different state.

As CAN bus errors might come at a very high rate (>1 KHz), they are
disabled in Linux by default.

Commit 2f3cdad1c616 ("can: gs_usb: add ability to enable / disable berr
reporting") added support to disable CAN bus error reporting if the
firmware supports it. The candlelight firmware recently added support for
this.

Implement CAN_CTRLMODE_BERR_REPORTING for devices that don't support
disabling CAN bus error reporting.

In gs_usb_receive_bulk_callback(), check if the driver received a CAN error
message. If CAN bus error reporting is disabled, drop CAN bus errors, but
process CAN state changes.

Unconditionally announce that the driver supports CAN bus error reporting
by setting CAN_CTRLMODE_BERR_REPORTING in ctrlmode_supported. But only
request CAN bus error reporting from the device if the firmware supports
it and the user space has configured it.

Link: https://github.com/candle-usb/candleLight_fw/pull/303
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 186abc95c977..bc69e6b3c512 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -724,7 +724,8 @@ gs_usb_should_handle_can_error(const struct gs_can *dev, const struct gs_host_fr
 		return true;
 
 	/* handle CAN bus errors */
-	if (hf->can_id & cpu_to_le32(CAN_ERR_LOSTARB | CAN_ERR_PROT | CAN_ERR_TRX | CAN_ERR_ACK))
+	if (dev->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING &&
+	    hf->can_id & cpu_to_le32(CAN_ERR_LOSTARB | CAN_ERR_PROT | CAN_ERR_TRX | CAN_ERR_ACK))
 		return true;
 
 	return false;
@@ -1182,7 +1183,7 @@ static int gs_can_open(struct net_device *netdev)
 	if (ctrlmode & CAN_CTRLMODE_ONE_SHOT)
 		flags |= GS_CAN_FEATURE_ONE_SHOT;
 
-	if (ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
+	if (ctrlmode & CAN_CTRLMODE_BERR_REPORTING && dev->feature & GS_CAN_FEATURE_BERR_REPORTING)
 		flags |= GS_CAN_FEATURE_BERR_REPORTING;
 
 	if (ctrlmode & CAN_CTRLMODE_FD)
@@ -1519,7 +1520,8 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 	dev->can.clock.freq = le32_to_cpu(bt_const.fclk_can);
 	dev->can.bittiming_const = &dev->bt_const;
 
-	dev->can.ctrlmode_supported = CAN_CTRLMODE_CC_LEN8_DLC;
+	dev->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING |
+		CAN_CTRLMODE_CC_LEN8_DLC;
 
 	feature = le32_to_cpu(bt_const.feature);
 	dev->feature = FIELD_GET(GS_CAN_FEATURE_MASK, feature);
@@ -1621,9 +1623,6 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 		}
 	}
 
-	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;
 

-- 
2.53.0

Re: [PATCH can-next 11/13] can: gs_usb: implement CAN_CTRLMODE_BERR_REPORTING for devices without native support

[sashiko-bot]

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

New issues:
- [High] Out-of-bounds read and use of uninitialized heap memory when processing CAN error frames with missing data payload.
- [High] The CAN error filtering logic silently drops CAN_ERR_RESTARTED frames if they lack a state change flag.
- [Medium] The new CAN error filtering logic drops hardware RX overflow events by bypassing the overflow check.

Pre-existing issues:
- [High] Failing to allocate an SKB drops the USB RX URB permanently, eventually halting reception.
--

--- Patch [11]: [PATCH can-next 11/13] can: gs_usb: implement CAN_CTRLMODE_BERR_REPORTING for devices without native support ---
commit 53e85c22d303a201e9d9f47a5e3240bd4a9af629
Author: Marc Kleine-Budde <mkl@pengutronix.de>

can: gs_usb: implement CAN_CTRLMODE_BERR_REPORTING for devices without native support

This commit implements software filtering for CAN bus errors when hardware
support for disabling bus error reporting is unavailable. It adds a check in
the bulk receive callback to drop bus error frames if reporting is disabled,
while still processing CAN state changes. It also unconditionally advertises
bus error reporting support in ctrlmode_supported.

Link: https://github.com/candle-usb/candleLight_fw/pull/303

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 186abc95c9774..bc69e6b3c512c 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -724,7 +724,8 @@ gs_usb_should_handle_can_error(const struct gs_can *dev, const struct gs_host_fr
>  		return true;
>  
>  	/* handle CAN bus errors */
> -	if (hf->can_id & cpu_to_le32(CAN_ERR_LOSTARB | CAN_ERR_PROT | CAN_ERR_TRX | CAN_ERR_ACK))
> +	if (dev->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING &&
> +	    hf->can_id & cpu_to_le32(CAN_ERR_LOSTARB | CAN_ERR_PROT | CAN_ERR_TRX | CAN_ERR_ACK))
>  		return true;
>  
>  	return false;

[Severity: High]
If a frame only contains CAN_ERR_RESTARTED but no state change flag, this
appears to fall through and return false. Does this silently drop the
automatic bus-off recovery frame, preventing gs_update_state() from handling
it and leaving the interface stuck in the BUS_OFF state?

[Severity: Medium]
When this returns false, gs_usb_receive_bulk_callback() executes a goto to
resubmit_urb. Does this bypass the hardware overflow check
if (hf->flags & GS_CAN_FLAG_OVERFLOW) at the end of the function,
causing the driver to silently drop hardware RX queue overflow notifications?

[Severity: High]
If the USB transfer is shorter than the expected payload (e.g. can_dlc is 0),
minimum_length allows processing just the header. Does accessing
hf->classic_can->data[1] during the state check in gs_usb_state_get_by_hf()
read out-of-bounds or use uninitialized heap memory from the URB buffer since
it bypasses the padding and zeroing that alloc_can_skb() used to provide?

[Severity: High]
This is a pre-existing issue, but returning directly in
gs_usb_receive_bulk_callback() when alloc_can_skb() fails leaks the URB:

gs_usb_receive_bulk_callback()
    skb = alloc_can_skb(netdev, &cf);
    if (!skb)
        return;

Can this lead to permanent loss of RX URBs if memory allocation fails,
eventually halting CAN reception until the interface is reset?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-gs_usb-new-features-v1-0-427a8013c380@pengutronix.de?part=11

[PATCH can-next 12/13] can: gs_usb: implement Transceiver Delay Compensation

[Marc Kleine-Budde]

For CAN-FD with data bitrate of more than 1...2 mbit/s, a transceiver delay
compensation (TDC) must be configured. Recent candleLight firmware added
support to control TDC via USB.

Extend gs_make_candev(): if the device signals TDC support with
GS_CAN_FEATURE_TDC, get struct gs_device_tdc_const from the device with the
USB request GS_USB_BREQ_GET_TDC_CONST and pass it to CAN device layer.

In gs_can_open() configure the TDC with USB request GS_USB_BREQ_SET_TDC and
activate TDC by setting flags GS_CAN_FEATURE_TDC.

Add the gs_usb_get_auto_tdcv() function that reads the current TDCV value
from the device (USB request GS_USB_BREQ_GET_TDC) and pass it to the CAN
device layer.

Link: https://github.com/candle-usb/candleLight_fw/pull/318
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 117 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 113 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index bc69e6b3c512..6355d05e4a16 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -74,9 +74,9 @@ enum gs_usb_breq {
 	GS_USB_BREQ_GET_STATE,
 	GS_USB_BREQ_SET_FILTER,
 	GS_USB_BREQ_GET_FILTER,
-	__GS_USB_BREQ_PLACEHOLDER_17,
-	__GS_USB_BREQ_PLACEHOLDER_18,
-	__GS_USB_BREQ_PLACEHOLDER_19,
+	GS_USB_BREQ_GET_TDC_CONST,
+	GS_USB_BREQ_SET_TDC,
+	GS_USB_BREQ_GET_TDC,
 	GS_USB_BREQ_ELM_GET_BOARDINFO = 20,
 	GS_USB_BREQ_ELM_SET_FILTER,
 	GS_USB_BREQ_ELM_GET_LASTERROR,
@@ -196,6 +196,27 @@ struct gs_device_filter {
 	};
 } __packed __aligned(4);
 
+#define GS_CAN_TDC_MODE_OFF BIT(0)
+#define GS_CAN_TDC_MODE_AUTO BIT(1)
+#define GS_CAN_TDC_MODE_MANUAL BIT(2)
+
+struct gs_device_tdc_const {
+	__le32 tdcv_min;
+	__le32 tdcv_max;
+	__le32 tdco_min;
+	__le32 tdco_max;
+	__le32 tdcf_min;
+	__le32 tdcf_max;
+	__le32 mode;
+} __packed __aligned(4);
+
+struct gs_device_tdc {
+	__le32 tdcv;
+	__le32 tdco;
+	__le32 tdcf;
+	__le32 mode;
+} __packed __aligned(4);
+
 #define GS_CAN_FEATURE_LISTEN_ONLY BIT(0)
 #define GS_CAN_FEATURE_LOOP_BACK BIT(1)
 #define GS_CAN_FEATURE_TRIPLE_SAMPLE BIT(2)
@@ -218,7 +239,8 @@ struct gs_device_filter {
 /* supported by Elmue firmware 0x260528 only */
 #define GS_CAN_FEATURE_ELM_DEV_FLAG_SEND_USB_BLOBS_260528 BIT(16)
 #define GS_CAN_FEATURE_FILTER BIT(16)
-#define GS_CAN_FEATURE_MASK GENMASK(16, 0)
+#define GS_CAN_FEATURE_TDC BIT(17)
+#define GS_CAN_FEATURE_MASK GENMASK(17, 0)
 
 /* internal quirks - keep in GS_CAN_FEATURE space for now */
 
@@ -345,6 +367,7 @@ struct gs_can {
 	struct usb_device *udev;
 
 	struct can_bittiming_const bt_const, data_bt_const;
+	struct can_tdc_const tdc_const;
 	unsigned int channel;	/* channel number */
 
 	u32 feature;
@@ -1086,6 +1109,30 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
+static int gs_usb_set_tdc(const struct gs_can *dev)
+{
+	const struct can_tdc *tdc = &dev->can.fd.tdc;
+	struct gs_device_tdc device_tdc = {
+		.tdcv = cpu_to_le32(tdc->tdcv),
+		.tdco = cpu_to_le32(tdc->tdco),
+		.tdcf = cpu_to_le32(tdc->tdcf),
+	};
+
+	const u32 ctrlmode = dev->can.ctrlmode;
+
+	if (ctrlmode & CAN_CTRLMODE_TDC_AUTO)
+		device_tdc.mode = cpu_to_le32(GS_CAN_TDC_MODE_AUTO);
+	else if (ctrlmode & CAN_CTRLMODE_TDC_MANUAL)
+		device_tdc.mode = cpu_to_le32(GS_CAN_TDC_MODE_MANUAL);
+	else
+		device_tdc.mode = cpu_to_le32(GS_CAN_TDC_MODE_OFF);
+
+	return usb_control_msg_send(dev->udev, 0, GS_USB_BREQ_SET_TDC,
+				    USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
+				    dev->channel, 0, &device_tdc, sizeof(device_tdc),
+				    USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
+}
+
 static int gs_can_open(struct net_device *netdev)
 {
 	struct gs_can *dev = netdev_priv(netdev);
@@ -1193,6 +1240,9 @@ static int gs_can_open(struct net_device *netdev)
 	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
 		flags |= GS_CAN_FEATURE_HW_TIMESTAMP;
 
+	if (dev->feature & GS_CAN_FEATURE_TDC)
+		flags |= GS_CAN_FEATURE_TDC;
+
 	rc = gs_usb_set_bittiming(dev);
 	if (rc) {
 		netdev_err(netdev, "failed to set bittiming: %pe\n", ERR_PTR(rc));
@@ -1207,6 +1257,14 @@ static int gs_can_open(struct net_device *netdev)
 		}
 	}
 
+	if (dev->feature & GS_CAN_FEATURE_TDC) {
+		rc = gs_usb_set_tdc(dev);
+		if (rc) {
+			netdev_err(netdev, "failed to set TDC: %pe\n", ERR_PTR(rc));
+			goto out_usb_kill_anchored_urbs;
+		}
+	}
+
 	/* finally start device */
 	dev->can.state = CAN_STATE_ERROR_ACTIVE;
 	dm.flags = cpu_to_le32(flags);
@@ -1452,6 +1510,25 @@ static const u16 gs_usb_termination_const[] = {
 	GS_USB_TERMINATION_ENABLED
 };
 
+static int gs_usb_get_auto_tdcv(const struct net_device *netdev, u32 *tdcv)
+{
+	struct gs_can *dev = netdev_priv(netdev);
+	struct gs_device_tdc tdc;
+	int rc;
+
+	rc = usb_control_msg_recv(dev->udev, 0, GS_USB_BREQ_GET_TDC,
+				  USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
+				  dev->channel, 0,
+				  &tdc, sizeof(tdc), USB_CTRL_GET_TIMEOUT,
+				  GFP_KERNEL);
+	if (rc)
+		return rc;
+
+	*tdcv = le32_to_cpu(tdc.tdcv);
+
+	return 0;
+}
+
 static struct gs_can *gs_make_candev(unsigned int channel,
 				     struct usb_interface *intf,
 				     struct gs_device_config *dconf)
@@ -1626,6 +1703,38 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 	if (feature & GS_CAN_FEATURE_GET_STATE)
 		dev->can.do_get_berr_counter = gs_usb_can_get_berr_counter;
 
+	if (feature & GS_CAN_FEATURE_TDC) {
+		struct gs_device_tdc_const tdc_const;
+
+		rc = usb_control_msg_recv(interface_to_usbdev(intf), 0,
+					  GS_USB_BREQ_GET_TDC_CONST,
+					  USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
+					  channel, 0, &tdc_const,
+					  sizeof(tdc_const),
+					  1000, GFP_KERNEL);
+		if (rc) {
+			dev_err(&intf->dev,
+				"Couldn't get TDC const for channel %d (%pe)\n",
+				channel, ERR_PTR(rc));
+			goto out_free_candev;
+		}
+
+		dev->tdc_const.tdcv_min = le32_to_cpu(tdc_const.tdcv_min);
+		dev->tdc_const.tdcv_max = le32_to_cpu(tdc_const.tdcv_max);
+		dev->tdc_const.tdco_min = le32_to_cpu(tdc_const.tdco_min);
+		dev->tdc_const.tdco_max = le32_to_cpu(tdc_const.tdco_max);
+		dev->tdc_const.tdcf_min = le32_to_cpu(tdc_const.tdcf_min);
+		dev->tdc_const.tdcf_max = le32_to_cpu(tdc_const.tdcf_max);
+
+		if (tdc_const.mode & cpu_to_le32(GS_CAN_TDC_MODE_MANUAL))
+			dev->can.ctrlmode_supported |= CAN_CTRLMODE_TDC_MANUAL;
+		if (tdc_const.mode & cpu_to_le32(GS_CAN_TDC_MODE_AUTO))
+			dev->can.ctrlmode_supported |= CAN_CTRLMODE_TDC_AUTO;
+
+		dev->can.fd.tdc_const = &dev->tdc_const;
+		dev->can.fd.do_get_auto_tdcv = gs_usb_get_auto_tdcv;
+	}
+
 	can_rx_offload_add_manual(netdev, &dev->offload, GS_NAPI_WEIGHT);
 	SET_NETDEV_DEV(netdev, &intf->dev);
 

-- 
2.53.0

[PATCH can-next 13/13] can: gs_usb: implement CAN bus off recovery

[Marc Kleine-Budde]

Traditionally gs_usb compatible CAN devices implement automatic CAN bus off
recovery. Either supported by the CAN IP core in hardware or in the device
firmware.

This turned out to be inflexible, the USB host and thus the user space has
no control over the CAN bus recovery of behaviour. The gs_usb has not
implemented the struct can_priv::do_set_mode that's used to restart the CAN
controller after a CAN bus off.

Recent candleLight firmware closes the gap by implementing the
GS_CAN_FEATURE_BUS_OFF_RECOVERY feature. If the host sets
GS_CAN_FEATURE_BUS_OFF_RECOVERY flag is set device disables automatic CAN
bus off recovery, a manual recovery can be started with the USB request
GS_USB_BREQ_BUS_OFF_RECOVERY.

Implement gs_usb_set_mode() to recover the device via the
GS_USB_BREQ_BUS_OFF_RECOVERY USB request.

Extend gs_make_candev(): if the device signals
GS_CAN_FEATURE_BUS_OFF_RECOVERY support, set the struct
can_priv::do_set_mode callback. In gs_can_open() activate
GS_CAN_FEATURE_BUS_OFF_RECOVERY if supported.

Update gs_update_state() to use the default can_bus_off() handler if the
device goes bus off and GS_CAN_FEATURE_BUS_OFF_RECOVERY.

Link: https://github.com/candle-usb/candleLight_fw/pull/317
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 47 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 6355d05e4a16..e4ee9a1ef3f6 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -89,6 +89,7 @@ enum gs_usb_breq {
 	__GS_USB_BREQ_ELM_PLACEHOLDER_29,
 	__GS_USB_BREQ_ELM_PLACEHOLDER_30,
 	__GS_USB_BREQ_ELM_PLACEHOLDER_31,
+	GS_USB_BREQ_BUS_OFF_RECOVERY = 32,
 };
 
 enum gs_can_mode {
@@ -217,6 +218,10 @@ struct gs_device_tdc {
 	__le32 mode;
 } __packed __aligned(4);
 
+struct gs_device_bus_off_recovery {
+	u32 unused;
+} __packed __aligned(4);
+
 #define GS_CAN_FEATURE_LISTEN_ONLY BIT(0)
 #define GS_CAN_FEATURE_LOOP_BACK BIT(1)
 #define GS_CAN_FEATURE_TRIPLE_SAMPLE BIT(2)
@@ -240,7 +245,8 @@ struct gs_device_tdc {
 #define GS_CAN_FEATURE_ELM_DEV_FLAG_SEND_USB_BLOBS_260528 BIT(16)
 #define GS_CAN_FEATURE_FILTER BIT(16)
 #define GS_CAN_FEATURE_TDC BIT(17)
-#define GS_CAN_FEATURE_MASK GENMASK(17, 0)
+#define GS_CAN_FEATURE_BUS_OFF_RECOVERY BIT(18)
+#define GS_CAN_FEATURE_MASK GENMASK(18, 0)
 
 /* internal quirks - keep in GS_CAN_FEATURE space for now */
 
@@ -573,14 +579,22 @@ static void gs_update_state(struct gs_can *dev, struct can_frame *cf,
 		return;
 
 	/* some firmware does automatically CAN bus off recovery, account for this */
-	if (cf->can_id & CAN_ERR_RESTARTED ||
-	    (dev->can.state == CAN_STATE_BUS_OFF && new_state < CAN_STATE_BUS_OFF)) {
+	if (!(dev->feature & GS_CAN_FEATURE_BUS_OFF_RECOVERY) &&
+	    (cf->can_id & CAN_ERR_RESTARTED ||
+	     (dev->can.state == CAN_STATE_BUS_OFF && new_state < CAN_STATE_BUS_OFF))) {
 		can_stats->restarts++;
 		/* some firmware doesn't set CAN_ERR_RESTARTED, fixup */
 		cf->can_id |= CAN_ERR_RESTARTED;
 	}
 
 	can_change_state(dev->can.dev, cf, tx_state, rx_state);
+
+	/* If device supports explicit CAN bus off recovery by the host, use default CAN bus off
+	 * handler. Otherwise the device will recover itself.
+	 */
+	if (new_state == CAN_STATE_BUS_OFF &&
+	    dev->feature & GS_CAN_FEATURE_BUS_OFF_RECOVERY)
+		can_bus_off(dev->netdev);
 }
 
 static u32 gs_usb_set_timestamp(struct gs_can *dev, struct sk_buff *skb,
@@ -1243,6 +1257,9 @@ static int gs_can_open(struct net_device *netdev)
 	if (dev->feature & GS_CAN_FEATURE_TDC)
 		flags |= GS_CAN_FEATURE_TDC;
 
+	if (dev->feature & GS_CAN_FEATURE_BUS_OFF_RECOVERY)
+		flags |= GS_CAN_FEATURE_BUS_OFF_RECOVERY;
+
 	rc = gs_usb_set_bittiming(dev);
 	if (rc) {
 		netdev_err(netdev, "failed to set bittiming: %pe\n", ERR_PTR(rc));
@@ -1330,6 +1347,27 @@ static int gs_usb_get_state(const struct net_device *netdev,
 	return 0;
 }
 
+static int gs_usb_set_mode(struct net_device *netdev, enum can_mode mode)
+{
+	struct gs_can *dev = netdev_priv(netdev);
+
+	switch (mode) {
+	case CAN_MODE_START: {
+		struct gs_device_bus_off_recovery bus_off_recovery = { 0 };
+
+		return usb_control_msg_send(dev->udev, 0, GS_USB_BREQ_BUS_OFF_RECOVERY,
+					    USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
+					    dev->channel, 0,
+					    &bus_off_recovery, sizeof(bus_off_recovery),
+					    USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
+	}
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int gs_usb_can_get_berr_counter(const struct net_device *netdev,
 				       struct can_berr_counter *bec)
 {
@@ -1735,6 +1773,9 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 		dev->can.fd.do_get_auto_tdcv = gs_usb_get_auto_tdcv;
 	}
 
+	if (feature & GS_CAN_FEATURE_BUS_OFF_RECOVERY)
+		dev->can.do_set_mode = gs_usb_set_mode;
+
 	can_rx_offload_add_manual(netdev, &dev->offload, GS_NAPI_WEIGHT);
 	SET_NETDEV_DEV(netdev, &intf->dev);
 

-- 
2.53.0

Re: [PATCH can-next 13/13] can: gs_usb: implement CAN bus off recovery

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The CAN_ERR_RESTARTED bit check in gs_update_state() is always false because the bit is unconditionally cleared at the beginning of the function.
--

--- Patch [13]: [PATCH can-next 13/13] can: gs_usb: implement CAN bus off recovery ---
commit 7b5dd8a47b695ccb806d9d65e91f427368f07215
Author: Marc Kleine-Budde <mkl@pengutronix.de>

can: gs_usb: implement CAN bus off recovery

This patch implements manual CAN bus off recovery for gs_usb devices that
support the GS_CAN_FEATURE_BUS_OFF_RECOVERY feature. It allows the USB host
to control bus off recovery instead of relying on the hardware's automatic
recovery.

Link: https://github.com/candle-usb/candleLight_fw/pull/317

> 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

[ ... ]

> @@ -573,14 +579,22 @@ static void gs_update_state(struct gs_can *dev, struct can_frame *cf,
>  		return;
>  
>  	/* some firmware does automatically CAN bus off recovery, account for this */
> -	if (cf->can_id & CAN_ERR_RESTARTED ||
> -	    (dev->can.state == CAN_STATE_BUS_OFF && new_state < CAN_STATE_BUS_OFF)) {
> +	if (!(dev->feature & GS_CAN_FEATURE_BUS_OFF_RECOVERY) &&
> +	    (cf->can_id & CAN_ERR_RESTARTED ||
> +	     (dev->can.state == CAN_STATE_BUS_OFF && new_state < CAN_STATE_BUS_OFF))) {

[Severity: Medium]
Will the check for CAN_ERR_RESTARTED ever evaluate to true in this
condition?

Looking earlier in gs_update_state(), the CAN_ERR_RESTARTED bit is
unconditionally cleared from cf->can_id:

	cf->can_id &= ~(CAN_ERR_CRTL | CAN_ERR_BUSOFF | CAN_ERR_RESTARTED);

Because of this, the check for CAN_ERR_RESTARTED here appears to be
structurally guaranteed to be false. Could this cause the driver to miss
bus-off restart events if the cached state was not already CAN_STATE_BUS_OFF?

>  		can_stats->restarts++;
>  		/* some firmware doesn't set CAN_ERR_RESTARTED, fixup */
>  		cf->can_id |= CAN_ERR_RESTARTED;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-gs_usb-new-features-v1-0-427a8013c380@pengutronix.de?part=13