Re: [PATCH can] can: gs_usb: add workarounds for HScanT USB to CAN adapter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: gs_usb: incorrect RX URB size calculation causes USB transfer hang
--

commit d72414dd12485ded5e69be85c30c0da0f32c460c
Author: Marc Kleine-Budde <mkl@pengutronix.de>
can: gs_usb: add workarounds for HScanT USB to CAN adapter

This commit adds workarounds for the HScanT RISC-V based USB to CAN adapter.
It corrects the channel count, binds channels to USB interface 0, and
implements a quirk to allocate 512-byte IN URBs for transfers.

Link: https://github.com/cherry-embedded/HSCanT-hardware

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 3b9b2f104d868..ec8256b46646b 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c

[ ... ]

> @@ -261,6 +274,10 @@ struct canfd_quirk {
>  	u8 quirk;
>  } __packed;
>  
> +struct quirk_hscant {
> +	u8 quirk[512];
> +} __packed;
> +

[Severity: High]
Because quirk is defined as a fixed 512-byte array rather than a flexible
array member (like u8 quirk[]), sizeof(struct quirk_hscant) evaluates to
512 bytes. Will this cause issues when used with struct_size() below?

>  /* struct gs_host_frame::echo_id == GS_HOST_FRAME_ECHO_ID_RX indicates
>   * a regular RX'ed CAN frame
>   */
> @@ -284,6 +301,7 @@ struct gs_host_frame {
>  		DECLARE_FLEX_ARRAY(struct canfd, canfd);
>  		DECLARE_FLEX_ARRAY(struct canfd_ts, canfd_ts);
>  		DECLARE_FLEX_ARRAY(struct canfd_quirk, canfd_quirk);
> +		DECLARE_FLEX_ARRAY(struct quirk_hscant, quirk_hscant);
>  	};
>  } __packed;

[ ... ]

> @@ -1604,10 +1704,12 @@ static int gs_usb_probe(struct usb_interface *intf,
>  		}
>  		parent->canch[i]->parent = parent;
>  
> -		/* set RX packet size based on FD and if hardware
> +		/* set RX packet size based on quirks, FD and if hardware
>  		 * timestamps are supported.
>  		 */
> -		if (parent->canch[i]->can.ctrlmode_supported & CAN_CTRLMODE_FD) {
> +		if (parent->canch[i]->feature & GS_CAN_FEATURE_QUIRK_HSCANT_URB_LENGTH) {
> +			hf_size_rx = struct_size(hf, quirk_hscant, 1);

[Severity: High]
Does this calculate the correct size for the RX URB? 

Because struct gs_host_frame has a 12-byte header, struct_size() computes
sizeof(*hf) + sizeof(struct quirk_hscant), which evaluates to 12 + 512 = 524
bytes instead of the intended 512 bytes.

If the host requests 524 bytes for a bulk IN transfer, but the device sends
exactly 512 bytes without a Zero-Length Packet, won't the host wait
indefinitely for the remaining 12 bytes and completely hang the RX path?

> +		} else if (parent->canch[i]->can.ctrlmode_supported & CAN_CTRLMODE_FD) {
>  			if (parent->canch[i]->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
>  				hf_size_rx = struct_size(hf, canfd_ts, 1);
>  			else

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-gs_usb-hscant-v1-1-42638ca0f85f@pengutronix.de?part=1