Re: [PATCH v2] can: peak_usb: Add bus error reporting for the PCAN-USB FD family

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: peak_usb: returning -EINVAL for bogus error reports drops valid URB packets
--

commit 6ff05518907e7981e5fff27d40e8a2458d563751
Author: Stéphane Grosjean <s.grosjean@peak-system.fr>

can: peak_usb: Add bus error reporting for the PCAN-USB FD family

CAN bus error reporting is currently missing for all PEAK-System
USB-to-CAN FD devices. Add support for reporting bus errors by enabling
bus error notifications in the firmware for each CAN channel.

Link to v1: https://patch.msgid.link/20260929-peak_usb-canfd-berr-v1-1-7abd585f877f@peak-system.fr

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> index 0d46f4ce5dca7..98f6943469b33 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> @@ -661,17 +661,71 @@ static int pcan_usb_fd_decode_error(struct pcan_usb_fd_if *usb_if,
>  	struct pucan_error_msg *er = (struct pucan_error_msg *)rx_msg;
>  	struct pcan_usb_fd_device *pdev;
>  	struct peak_usb_device *dev;
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +	u8 can_err_tx = 0;
>  
>  	if (pucan_ermsg_get_channel(er) >= ARRAY_SIZE(usb_if->dev))
>  		return -EINVAL;
>  
> +	/* Guard against bogus channel 1 reports from single-channel adapters */
>  	dev = usb_if->dev[pucan_ermsg_get_channel(er)];
> +	if (!dev)
> +		return -EINVAL;

[Severity: High]
Does returning -EINVAL here cause valid URB packets to be dropped?

The caller pcan_usb_fd_decode_buf() handles a negative return value by
aborting the URB decoding entirely and jumping to the fail label:

	case PUCAN_MSG_ERROR:
		err = pcan_usb_fd_decode_error(usb_if, rx_msg);
		if (err < 0)
			goto fail;
		break;

Because USB bulk transfers pack multiple CAN records together, aborting
the loop discards any subsequent valid CAN frames in the same URB.
This appears to lead to silent data loss under normal operation.

Should this return 0 instead of -EINVAL to safely ignore the hardware quirk
without dropping the rest of the buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-peak_usb-canfd-berr-v2-1-1a7005797c38@peak-system.fr?part=1