* [PATCH can 1/3] can: gs_usb: gs_usb_xmit_callback(): fix handling of failed transmitted URBs
2025-11-08 9:01 [PATCH can 0/3] can: gs_usb: fix USB bulk in and out callbacks Marc Kleine-Budde
@ 2025-11-08 9:01 ` Marc Kleine-Budde
2025-11-08 9:01 ` [PATCH can 2/3] can: gs_usb: gs_usb_receive_bulk_callback(): check actual_length before accessing header Marc Kleine-Budde
2025-11-08 9:01 ` [PATCH can 3/3] can: gs_usb: gs_usb_receive_bulk_callback(): check actual_length before accessing data Marc Kleine-Budde
2 siblings, 0 replies; 8+ messages in thread
From: Marc Kleine-Budde @ 2025-11-08 9:01 UTC (permalink / raw)
To: Vincent Mailhol, Maximilian Schneider, Wolfgang Grandegger
Cc: kernel, linux-can, linux-kernel, Marc Kleine-Budde
The driver lacks the cleanup of failed transfers of URBs. This reduces the
number of available URBs per error by 1. This leads to reduced performance
and ultimately to a complete stop of the transmission.
If the sending of a bulk URB fails do proper cleanup:
- increase netdev stats
- mark the echo_sbk as free
- free the driver's context and do accounting
- wake the send queue
Closes: https://github.com/candle-usb/candleLight_fw/issues/187
Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/gs_usb.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 69b8d6da651b..fa9bab8c89ae 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -750,8 +750,21 @@ static void gs_usb_xmit_callback(struct urb *urb)
struct gs_can *dev = txc->dev;
struct net_device *netdev = dev->netdev;
- if (urb->status)
- netdev_info(netdev, "usb xmit fail %u\n", txc->echo_id);
+ if (!urb->status)
+ return;
+
+ if (urb->status != -ESHUTDOWN && net_ratelimit())
+ netdev_info(netdev, "failed to xmit URB %u: %pe\n",
+ txc->echo_id, ERR_PTR(urb->status));
+
+ netdev->stats.tx_dropped++;
+ netdev->stats.tx_errors++;
+
+ can_free_echo_skb(netdev, txc->echo_id, NULL);
+ gs_free_tx_context(txc);
+ atomic_dec(&dev->active_tx_urbs);
+
+ netif_wake_queue(netdev);
}
static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb,
--
2.51.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH can 2/3] can: gs_usb: gs_usb_receive_bulk_callback(): check actual_length before accessing header
2025-11-08 9:01 [PATCH can 0/3] can: gs_usb: fix USB bulk in and out callbacks Marc Kleine-Budde
2025-11-08 9:01 ` [PATCH can 1/3] can: gs_usb: gs_usb_xmit_callback(): fix handling of failed transmitted URBs Marc Kleine-Budde
@ 2025-11-08 9:01 ` Marc Kleine-Budde
2025-11-08 9:01 ` [PATCH can 3/3] can: gs_usb: gs_usb_receive_bulk_callback(): check actual_length before accessing data Marc Kleine-Budde
2 siblings, 0 replies; 8+ messages in thread
From: Marc Kleine-Budde @ 2025-11-08 9:01 UTC (permalink / raw)
To: Vincent Mailhol, Maximilian Schneider, Wolfgang Grandegger
Cc: kernel, linux-can, linux-kernel, Marc Kleine-Budde
The driver expects to receive a struct gs_host_frame in
gs_usb_receive_bulk_callback().
Use struct_group to describe the header of the struct gs_host_frame and
check that we have at least received the header before accessing any
members of it.
To resubmit the URB, do not dereference the pointer chain
"dev->parent->hf_size_rx" but use "parent->hf_size_rx" instead. Since
"urb->context" contains "parent", it is always defined, while "dev" is not
defined if the URB it too short.
Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/gs_usb.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index fa9bab8c89ae..51f8d694104d 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -262,13 +262,15 @@ struct canfd_quirk {
} __packed;
struct gs_host_frame {
- u32 echo_id;
- __le32 can_id;
+ struct_group(header,
+ u32 echo_id;
+ __le32 can_id;
- u8 can_dlc;
- u8 channel;
- u8 flags;
- u8 reserved;
+ u8 can_dlc;
+ u8 channel;
+ u8 flags;
+ u8 reserved;
+ );
union {
DECLARE_FLEX_ARRAY(struct classic_can, classic_can);
@@ -576,6 +578,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
int rc;
struct net_device_stats *stats;
struct gs_host_frame *hf = urb->transfer_buffer;
+ unsigned int minimum_length;
struct gs_tx_context *txc;
struct can_frame *cf;
struct canfd_frame *cfd;
@@ -594,6 +597,15 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
return;
}
+ minimum_length = sizeof(hf->header);
+ if (urb->actual_length < minimum_length) {
+ dev_err_ratelimited(&parent->udev->dev,
+ "short read (actual_length=%u, minimum_length=%u)\n",
+ urb->actual_length, minimum_length);
+
+ goto resubmit_urb;
+ }
+
/* device reports out of range channel id */
if (hf->channel >= parent->channel_cnt)
goto device_detach;
@@ -687,7 +699,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
resubmit_urb:
usb_fill_bulk_urb(urb, parent->udev,
parent->pipe_in,
- hf, dev->parent->hf_size_rx,
+ hf, parent->hf_size_rx,
gs_usb_receive_bulk_callback, parent);
rc = usb_submit_urb(urb, GFP_ATOMIC);
--
2.51.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH can 3/3] can: gs_usb: gs_usb_receive_bulk_callback(): check actual_length before accessing data
2025-11-08 9:01 [PATCH can 0/3] can: gs_usb: fix USB bulk in and out callbacks Marc Kleine-Budde
2025-11-08 9:01 ` [PATCH can 1/3] can: gs_usb: gs_usb_xmit_callback(): fix handling of failed transmitted URBs Marc Kleine-Budde
2025-11-08 9:01 ` [PATCH can 2/3] can: gs_usb: gs_usb_receive_bulk_callback(): check actual_length before accessing header Marc Kleine-Budde
@ 2025-11-08 9:01 ` Marc Kleine-Budde
2025-11-11 12:31 ` Henrik Brix Andersen
2 siblings, 1 reply; 8+ messages in thread
From: Marc Kleine-Budde @ 2025-11-08 9:01 UTC (permalink / raw)
To: Vincent Mailhol, Maximilian Schneider, Wolfgang Grandegger
Cc: kernel, linux-can, linux-kernel, Marc Kleine-Budde
The URB received in gs_usb_receive_bulk_callback() contains a struct
gs_host_frame. The length of the data after the header depends on the
gs_host_frame hf::flags and the active device features (e.g. time
stamping).
Introduce a new function gs_usb_get_minimum_length() and check that we have
at least received the required amount of data before accessing it. Only
copy the data to that skb that has actually been received.
Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/gs_usb.c | 65 ++++++++++++++++++++++++++++++++++++++++----
1 file changed, 60 insertions(+), 5 deletions(-)
diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 51f8d694104d..8524d423b029 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -261,6 +261,11 @@ struct canfd_quirk {
u8 quirk;
} __packed;
+/* struct gs_host_frame::echo_id == GS_HOST_FRAME_ECHO_ID_RX indicates
+ * a regular RX'ed CAN frame
+ */
+#define GS_HOST_FRAME_ECHO_ID_RX 0xffffffff
+
struct gs_host_frame {
struct_group(header,
u32 echo_id;
@@ -570,6 +575,43 @@ gs_usb_get_echo_skb(struct gs_can *dev, struct sk_buff *skb,
return len;
}
+static unsigned int
+gs_usb_get_minimum_length(const struct gs_can *dev, const struct gs_host_frame *hf,
+ unsigned int *data_length_p)
+{
+ unsigned int minimum_length, data_length;
+
+ /* TX echo only uses the header */
+ if (hf->echo_id != GS_HOST_FRAME_ECHO_ID_RX) {
+ *data_length_p = 0;
+ return sizeof(hf->header);
+ }
+
+ if (hf->flags & GS_CAN_FLAG_FD) {
+ data_length = can_fd_dlc2len(hf->can_dlc);
+
+ if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+ /* timestamp follows data field of max size */
+ minimum_length = struct_size(hf, canfd_ts, 1);
+ else
+ minimum_length = sizeof(hf->header) + data_length;
+ } else {
+ if (hf->can_id & cpu_to_le32(CAN_RTR_FLAG))
+ data_length = 0;
+ else
+ data_length = can_cc_dlc2len(hf->can_dlc);
+
+ if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+ /* timestamp follows data field of max size */
+ minimum_length = struct_size(hf, classic_can_ts, 1);
+ else
+ minimum_length = sizeof(hf->header) + data_length;
+ }
+
+ *data_length_p = data_length;
+ return minimum_length;
+}
+
static void gs_usb_receive_bulk_callback(struct urb *urb)
{
struct gs_usb *parent = urb->context;
@@ -578,7 +620,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
int rc;
struct net_device_stats *stats;
struct gs_host_frame *hf = urb->transfer_buffer;
- unsigned int minimum_length;
+ unsigned int minimum_length, data_length;
struct gs_tx_context *txc;
struct can_frame *cf;
struct canfd_frame *cfd;
@@ -621,20 +663,33 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
if (!netif_running(netdev))
goto resubmit_urb;
- if (hf->echo_id == -1) { /* normal rx */
+ minimum_length = gs_usb_get_minimum_length(dev, hf, &data_length);
+ if (urb->actual_length < minimum_length) {
+ stats->rx_errors++;
+ stats->rx_length_errors++;
+
+ if (net_ratelimit())
+ netdev_err(netdev,
+ "short read (actual_length=%u, minimum_length=%u)\n",
+ urb->actual_length, minimum_length);
+
+ goto resubmit_urb;
+ }
+
+ if (hf->echo_id == GS_HOST_FRAME_ECHO_ID_RX) { /* normal rx */
if (hf->flags & GS_CAN_FLAG_FD) {
skb = alloc_canfd_skb(netdev, &cfd);
if (!skb)
return;
cfd->can_id = le32_to_cpu(hf->can_id);
- cfd->len = can_fd_dlc2len(hf->can_dlc);
+ cfd->len = data_length;
if (hf->flags & GS_CAN_FLAG_BRS)
cfd->flags |= CANFD_BRS;
if (hf->flags & GS_CAN_FLAG_ESI)
cfd->flags |= CANFD_ESI;
- memcpy(cfd->data, hf->canfd->data, cfd->len);
+ memcpy(cfd->data, hf->canfd->data, data_length);
} else {
skb = alloc_can_skb(netdev, &cf);
if (!skb)
@@ -643,7 +698,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
cf->can_id = le32_to_cpu(hf->can_id);
can_frame_set_cc_len(cf, hf->can_dlc, dev->can.ctrlmode);
- memcpy(cf->data, hf->classic_can->data, 8);
+ 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)
--
2.51.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH can 3/3] can: gs_usb: gs_usb_receive_bulk_callback(): check actual_length before accessing data
2025-11-08 9:01 ` [PATCH can 3/3] can: gs_usb: gs_usb_receive_bulk_callback(): check actual_length before accessing data Marc Kleine-Budde
@ 2025-11-11 12:31 ` Henrik Brix Andersen
2025-11-11 13:55 ` Marc Kleine-Budde
0 siblings, 1 reply; 8+ messages in thread
From: Henrik Brix Andersen @ 2025-11-11 12:31 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Vincent Mailhol, Maximilian Schneider, Wolfgang Grandegger,
kernel, linux-can, linux-kernel
Hi,
> On 8 Nov 2025, at 10.01, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> The URB received in gs_usb_receive_bulk_callback() contains a struct
> gs_host_frame. The length of the data after the header depends on the
> gs_host_frame hf::flags and the active device features (e.g. time
> stamping).
>
> Introduce a new function gs_usb_get_minimum_length() and check that we have
> at least received the required amount of data before accessing it. Only
> copy the data to that skb that has actually been received.
>
> Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
> drivers/net/can/usb/gs_usb.c | 65 ++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 60 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 51f8d694104d..8524d423b029 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -261,6 +261,11 @@ struct canfd_quirk {
> u8 quirk;
> } __packed;
>
> +/* struct gs_host_frame::echo_id == GS_HOST_FRAME_ECHO_ID_RX indicates
> + * a regular RX'ed CAN frame
> + */
> +#define GS_HOST_FRAME_ECHO_ID_RX 0xffffffff
> +
> struct gs_host_frame {
> struct_group(header,
> u32 echo_id;
> @@ -570,6 +575,43 @@ gs_usb_get_echo_skb(struct gs_can *dev, struct sk_buff *skb,
> return len;
> }
>
> +static unsigned int
> +gs_usb_get_minimum_length(const struct gs_can *dev, const struct gs_host_frame *hf,
> + unsigned int *data_length_p)
> +{
> + unsigned int minimum_length, data_length;
> +
> + /* TX echo only uses the header */
> + if (hf->echo_id != GS_HOST_FRAME_ECHO_ID_RX) {
> + *data_length_p = 0;
> + return sizeof(hf->header);
> + }
Is this correct? The embedded timestamp is also used in the gs_usb_get_echo_skb() function.
Regards,
Brix
--
Henrik Brix Andersen
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH can 3/3] can: gs_usb: gs_usb_receive_bulk_callback(): check actual_length before accessing data
2025-11-11 12:31 ` Henrik Brix Andersen
@ 2025-11-11 13:55 ` Marc Kleine-Budde
0 siblings, 0 replies; 8+ messages in thread
From: Marc Kleine-Budde @ 2025-11-11 13:55 UTC (permalink / raw)
To: Henrik Brix Andersen
Cc: Vincent Mailhol, Maximilian Schneider, Wolfgang Grandegger,
kernel, linux-can, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 846 bytes --]
On 11.11.2025 13:31:06, Henrik Brix Andersen wrote:
> > +static unsigned int
> > +gs_usb_get_minimum_length(const struct gs_can *dev, const struct gs_host_frame *hf,
> > + unsigned int *data_length_p)
> > +{
> > + unsigned int minimum_length, data_length;
> > +
> > + /* TX echo only uses the header */
> > + if (hf->echo_id != GS_HOST_FRAME_ECHO_ID_RX) {
> > + *data_length_p = 0;
> > + return sizeof(hf->header);
> > + }
>
> Is this correct? The embedded timestamp is also used in the
> gs_usb_get_echo_skb() function.
Right, will update.
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: 488 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread