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

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

[Stéphane Grosjean]

From: Stéphane Grosjean <s.grosjean@peak-system.fr>

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.

Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
Changes in v2:
- Add a defensive check against single-channel adapters reporting an
  invalid channel index.
- Link to v1: https://patch.msgid.link/20260929-peak_usb-canfd-berr-v1-1-7abd585f877f@peak-system.fr

To: Marc Kleine-Budde <mkl@pengutronix.de>
To: Vincent Mailhol <mailhol@kernel.org>
Cc: linux-can@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 89 ++++++++++++++++++++++++++----
 include/linux/can/dev/peak_canfd.h         | 15 ++++-
 2 files changed, 92 insertions(+), 12 deletions(-)

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 0d46f4ce5dca..98f6943469b3 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;
+
 	pdev = container_of(dev, struct pcan_usb_fd_device, dev);
 
 	/* keep a trace of tx and rx error counters for later use */
 	pdev->bec.txerr = er->tx_err_cnt;
 	pdev->bec.rxerr = er->rx_err_cnt;
 
+	/* ignore non-CAN error notifications */
+	if (PUCAN_ERMSG_TYPE(er) > PUCAN_ERMSG_OTHER_ERROR)
+		return 0;
+
+	/* update other errors counters */
+	dev->can.can_stats.bus_error++;
+
+	if (PUCAN_ERMSG_RX(er)) {
+		dev->netdev->stats.rx_errors++;
+	} else {
+		dev->netdev->stats.tx_errors++;
+		can_err_tx = CAN_ERR_PROT_TX;
+	}
+
+	/* if berr-reporting is off, stop here */
+	if (!(dev->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
+		return 0;
+
+	/* otherwise, build the CAN_ERR_xxx frame */
+	skb = alloc_can_err_skb(dev->netdev, &cf);
+	if (!skb)
+		return -ENOMEM;
+
+	cf->can_id |= CAN_ERR_CNT | CAN_ERR_PROT | CAN_ERR_BUSERROR;
+	cf->data[2] |= can_err_tx;
+
+	switch (PUCAN_ERMSG_TYPE(er)) {
+	case PUCAN_ERMSG_BIT_ERROR:
+		cf->data[2] |= CAN_ERR_PROT_BIT;
+		break;
+	case PUCAN_ERMSG_FORM_ERROR:
+		cf->data[2] |= CAN_ERR_PROT_FORM;
+		break;
+	case PUCAN_ERMSG_STUFF_ERROR:
+		cf->data[2] |= CAN_ERR_PROT_STUFF;
+		break;
+	default:
+		break;
+	}
+
+	cf->data[3] = PUCAN_ERMSG_CODE(er);
+
+	cf->data[6] = pdev->bec.txerr;
+	cf->data[7] = pdev->bec.rxerr;
+
+	peak_usb_netif_rx_64(skb, le32_to_cpu(er->ts_low),
+			     le32_to_cpu(er->ts_high));
 	return 0;
 }
 
@@ -899,6 +953,7 @@ static int pcan_usb_fd_start(struct peak_usb_device *dev)
 {
 	struct pcan_usb_fd_device *pdev =
 			container_of(dev, struct pcan_usb_fd_device, dev);
+	u16 usb_opts = 0;
 	int err;
 
 	/* set filter mode: all acceptance */
@@ -912,12 +967,17 @@ static int pcan_usb_fd_start(struct peak_usb_device *dev)
 		peak_usb_init_time_ref(&pdev->usb_if->time_ref,
 				       &pcan_usb_pro_fd);
 
-		/* enable USB calibration messages */
-		err = pcan_usb_fd_set_options(dev, 1,
-					      PUCAN_OPTION_ERROR,
-					      PCAN_UFD_FLTEXT_CALIBRATION);
+		/* enable USB calibration messages (needed only once for the
+		 * entire interface)
+		 */
+		usb_opts |= PCAN_UFD_FLTEXT_CALIBRATION;
 	}
 
+	/* set channel device options: always asks for bus error notifications
+	 * to get (at least) rxerr/txerr, as well as any USB-specific option.
+	 */
+	err = pcan_usb_fd_set_options(dev, 1, PUCAN_OPTION_ERROR, usb_opts);
+
 	pdev->usb_if->dev_opened_count++;
 
 	/* reset cached error counters */
@@ -955,12 +1015,15 @@ static int pcan_usb_fd_stop(struct peak_usb_device *dev)
 {
 	struct pcan_usb_fd_device *pdev =
 			container_of(dev, struct pcan_usb_fd_device, dev);
+	u16 usb_opts = 0;
 
 	/* turn off special msgs for that interface if no other dev opened */
 	if (pdev->usb_if->dev_opened_count == 1)
-		pcan_usb_fd_set_options(dev, 0,
-					PUCAN_OPTION_ERROR,
-					PCAN_UFD_FLTEXT_CALIBRATION);
+		usb_opts |= PCAN_UFD_FLTEXT_CALIBRATION;
+
+	/* turn off bus error option, as well as any USB-specific options */
+	pcan_usb_fd_set_options(dev, 0, PUCAN_OPTION_ERROR, usb_opts);
+
 	pdev->usb_if->dev_opened_count--;
 
 	return 0;
@@ -1200,7 +1263,8 @@ const struct peak_usb_adapter pcan_usb_fd = {
 	.ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
 	.ctrlmode_supported = CAN_CTRLMODE_FD |
 			CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
-			CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
+			CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC |
+			CAN_CTRLMODE_BERR_REPORTING,
 	.clock = {
 		.freq = PCAN_UFD_CRYSTAL_HZ,
 	},
@@ -1276,7 +1340,8 @@ const struct peak_usb_adapter pcan_usb_chip = {
 	.ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
 	.ctrlmode_supported = CAN_CTRLMODE_FD |
 		CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
-		CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
+		CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC |
+		CAN_CTRLMODE_BERR_REPORTING,
 	.clock = {
 		.freq = PCAN_UFD_CRYSTAL_HZ,
 	},
@@ -1352,7 +1417,8 @@ const struct peak_usb_adapter pcan_usb_pro_fd = {
 	.ctrl_count = PCAN_USBPROFD_CHANNEL_COUNT,
 	.ctrlmode_supported = CAN_CTRLMODE_FD |
 			CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
-			CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
+			CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC |
+			CAN_CTRLMODE_BERR_REPORTING,
 	.clock = {
 		.freq = PCAN_UFD_CRYSTAL_HZ,
 	},
@@ -1428,7 +1494,8 @@ const struct peak_usb_adapter pcan_usb_x6 = {
 	.ctrl_count = PCAN_USBPROFD_CHANNEL_COUNT,
 	.ctrlmode_supported = CAN_CTRLMODE_FD |
 			CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
-			CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
+			CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC |
+			CAN_CTRLMODE_BERR_REPORTING,
 	.clock = {
 		.freq = PCAN_UFD_CRYSTAL_HZ,
 	},
diff --git a/include/linux/can/dev/peak_canfd.h b/include/linux/can/dev/peak_canfd.h
index 056e0efa649f..bbcba2711f19 100644
--- a/include/linux/can/dev/peak_canfd.h
+++ b/include/linux/can/dev/peak_canfd.h
@@ -8,6 +8,8 @@
 #ifndef PUCAN_H
 #define PUCAN_H
 
+#include <linux/bitfield.h>
+
 /* uCAN commands opcodes list (low-order 10 bits) */
 #define PUCAN_CMD_NOP			0x000
 #define PUCAN_CMD_RESET_MODE		0x001
@@ -197,7 +199,18 @@ struct __packed pucan_rx_msg {
 #define PUCAN_ERMSG_FORM_ERROR		1
 #define PUCAN_ERMSG_STUFF_ERROR		2
 #define PUCAN_ERMSG_OTHER_ERROR		3
-#define PUCAN_ERMSG_ERR_CNT_DEC		4
+
+#define PUCAN_ERMSG_TYPE_MASK		GENMASK(6, 4)
+#define PUCAN_ERMSG_D_BIT		BIT(7)
+#define PUCAN_ERMSG_TYPE(e) \
+	FIELD_GET(PUCAN_ERMSG_TYPE_MASK, (e)->channel_type_d)
+
+#define PUCAN_ERMSG_RX(e) \
+	FIELD_GET(PUCAN_ERMSG_D_BIT, (e)->channel_type_d)
+
+#define PUCAN_ERMSG_CODE_MASK		GENMASK(6, 0)
+#define PUCAN_ERMSG_CODE(e) \
+	FIELD_GET(PUCAN_ERMSG_CODE_MASK, (e)->code_g)
 
 struct __packed pucan_error_msg {
 	__le16	size;

---
base-commit: a7bfaba4823e3c165bb2004c74eff7c096672bc7
change-id: 20260929-peak_usb-canfd-berr-dee42c0cba00

Best regards,
--  
Stéphane Grosjean <s.grosjean@peak-system.fr>

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

[sashiko-bot]

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

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

[Stéphane Grosjean]

Hi,

The NULL check was added to protect against a malformed PUCAN_MSG_ERROR
record reporting channel 1 on a single-channel adapter.
 
Such a value cannot be generated by compliant PCAN-USB FD firmware and
therefore indicates corrupted or malicious input. The driver already
returns -EINVAL and aborts URB decoding for other protocol violations,
for example when the reported channel index exceeds the bounds of the
channel array. This change follows the same policy.
 
Since the input stream is already invalid at this point, I believe
rejecting the entire URB is the correct behaviour rather than attempting
to continue parsing subsequent records from untrusted data.

Best regards,

-- Stéphane

----- Mail original -----
> 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
> 

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

[Marc Kleine-Budde]

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

On 30.09.2026 09:49:19, Stéphane Grosjean wrote:
> The NULL check was added to protect against a malformed PUCAN_MSG_ERROR
> record reporting channel 1 on a single-channel adapter.
>
> Such a value cannot be generated by compliant PCAN-USB FD firmware and
> therefore indicates corrupted or malicious input. The driver already
> returns -EINVAL and aborts URB decoding for other protocol violations,
> for example when the reported channel index exceeds the bounds of the
> channel array. This change follows the same policy.
>
> Since the input stream is already invalid at this point, I believe
> rejecting the entire URB is the correct behaviour rather than attempting
> to continue parsing subsequent records from untrusted data.

Valid decision, let's document this, see my other mail.

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

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

[Marc Kleine-Budde]

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

On 29.09.2026 16:41:03, Stéphane Grosjean wrote:
> From: Stéphane Grosjean <s.grosjean@peak-system.fr>
>
> 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.

To make it more clear, let's explain your decision to abort parsing if
the FW sends bogus data. What about something like this:

Abort parsing of the URB if the firmware reports an invalid channel.

> Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
> ---
> Changes in v2:
> - Add a defensive check against single-channel adapters reporting an
>   invalid channel index.
> - Link to v1: https://patch.msgid.link/20260929-peak_usb-canfd-berr-v1-1-7abd585f877f@peak-system.fr
>
> To: Marc Kleine-Budde <mkl@pengutronix.de>
> To: Vincent Mailhol <mailhol@kernel.org>
> Cc: linux-can@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 89 ++++++++++++++++++++++++++----
>  include/linux/can/dev/peak_canfd.h         | 15 ++++-
>  2 files changed, 92 insertions(+), 12 deletions(-)
>
> 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 0d46f4ce5dca..98f6943469b3 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 */

A malicious FW can also send invalid channel information on multi
channel adapters, so let's update the comment here, too:

Abort parsing of the URB if the firmware reports an invalid channel.

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