[PATCH can-next 0/7] can: peak_usb: fixes, cleanups, bus error reporting and loopback mode

[PATCH can-next 0/7] can: peak_usb: fixes, cleanups, bus error reporting and loopback mode

[Marc Kleine-Budde]

Hello,

this series aggregates the peak_usb patches waiting for net-next by Stefan
Günther and Stéphane Grosjean, plus 2 cleanup patches by me.

This series adds the missing CAN_ERR_FLAG when reporting error counters,
the cleans up the driver and adds bus error reporting and loopback mode.

regards,
Marc

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Marc Kleine-Budde (2):
      can: peak_usb: add missing includes
      can: peak_usb: sort struct peak_usb_adapter::ctrlmode_supported by value

Stefan Günther (1):
      can: peak_usb: fix missing CAN_ERR_FLAG when reporting error counters

Stéphane Grosjean (4):
      can: peak_usb: Add PCAN-USB bus errors reporting
      can: peak_usb: Sort CAN_CTRLMODE flags by value
      can: peak_usb: Add bus error reporting for the PCAN-USB FD family
      can: peak_usb: Add support of loopback mode to the PCAN-USB FD family

 drivers/net/can/usb/peak_usb/pcan_usb.c      | 120 +++++++++++++++++++++----
 drivers/net/can/usb/peak_usb/pcan_usb_core.h |   6 ++
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c   | 125 +++++++++++++++++++++++----
 include/linux/can/dev/peak_canfd.h           |  15 +++-
 4 files changed, 228 insertions(+), 38 deletions(-)
---
base-commit: eb0c18404c8943356f4aa164e5db9067b79ea93c
change-id: 20261001-peak_usb_enhancements-f4a35b7bfd76

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

[PATCH can-next 1/7] can: peak_usb: fix missing CAN_ERR_FLAG when reporting error counters

[Marc Kleine-Budde]

From: Stefan Günther <stefangun@pm.me>

When the device reports error counters, the driver incorrectly uses an
assignment (=) instead of a bitwise OR (|=) for CAN_ERR_CNT. This wipes
out the CAN_ERR_FLAG and CAN_ERR_CRTL flags, causing the error frame to
be sent to userspace as a regular CAN frame with ID 0x200.

Fix this by using a bitwise OR to preserve the flags.

Signed-off-by: Stefan Günther <stefangun@pm.me>
Cc: stable@vger.kernel.org
Fixes: 3e5c291c7942 ("can: add CAN_ERR_CNT flag to notify availability of error counter")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/peak_usb/pcan_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 8fd058c32856..785224e00c4e 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -526,7 +526,7 @@ static int pcan_usb_decode_error(struct pcan_usb_msg_context *mc, u8 n,
 			/* Supply TX/RX error counters in case of
 			 * controller error.
 			 */
-			cf->can_id = CAN_ERR_CNT;
+			cf->can_id |= CAN_ERR_CNT;
 			cf->data[6] = mc->pdev->bec.txerr;
 			cf->data[7] = mc->pdev->bec.rxerr;
 		}

-- 
2.53.0

[PATCH can-next 2/7] can: peak_usb: add missing includes

[Marc Kleine-Budde]

To make the header self contained, so that LSP servers don't throw errors,
make the header file self contained and include all needed header files.

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

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.h b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
index 65999f04f4b7..9ad7d31d2720 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
@@ -11,6 +11,12 @@
 #ifndef PCAN_USB_CORE_H
 #define PCAN_USB_CORE_H
 
+#include <linux/can/dev.h>
+#include <linux/can/netlink.h>
+#include <linux/skbuff.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+
 /* PEAK-System vendor id. */
 #define PCAN_USB_VENDOR_ID		0x0c72
 

-- 
2.53.0

[PATCH can-next 3/7] can: peak_usb: sort struct peak_usb_adapter::ctrlmode_supported by value

[Marc Kleine-Budde]

To support readability and maintainability sort the struct
peak_usb_adapter::ctrlmode_supported by their value.

No functional change intended.

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

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 785224e00c4e..169c00d93463 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -1016,7 +1016,8 @@ const struct peak_usb_adapter pcan_usb = {
 	.name = "PCAN-USB",
 	.device_id = PCAN_USB_PRODUCT_ID,
 	.ctrl_count = 1,
-	.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
+	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+			      CAN_CTRLMODE_3_SAMPLES |
 			      CAN_CTRLMODE_CC_LEN8_DLC,
 	.clock = {
 		.freq = PCAN_USB_CRYSTAL_HZ / 2,

-- 
2.53.0

[PATCH can-next 4/7] can: peak_usb: Add PCAN-USB bus errors reporting

[Marc Kleine-Budde]

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

The good old PCAN-USB was missing CAN bus error reporting. This patch fixes
that.

Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/peak_usb/pcan_usb.c | 115 +++++++++++++++++++++++++++-----
 1 file changed, 99 insertions(+), 16 deletions(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 169c00d93463..e71ed7879c04 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -115,6 +115,7 @@
 #define PCAN_USB_REC_BUSEVT		5
 
 /* CAN bus events notifications selection mask */
+#define PCAN_USB_ERR_ECC		0x01	/* ask for BERR */
 #define PCAN_USB_ERR_RXERR		0x02	/* ask for rxerr counter */
 #define PCAN_USB_ERR_TXERR		0x04	/* ask for txerr counter */
 
@@ -122,11 +123,20 @@
  * In other words, its interest is to know which side among rx and tx is
  * responsible of the change of the bus state.
  */
-#define PCAN_USB_BERR_MASK	(PCAN_USB_ERR_RXERR | PCAN_USB_ERR_TXERR)
+#define PCAN_USB_BERR_MASK	(PCAN_USB_ERR_ECC | \
+				 PCAN_USB_ERR_RXERR | PCAN_USB_ERR_TXERR)
 
-/* identify bus event packets with rx/tx error counters */
-#define PCAN_USB_ERR_CNT_DEC		0x00	/* counters are decreasing */
-#define PCAN_USB_ERR_CNT_INC		0x80	/* counters are increasing */
+/* SJA1000 ECC register */
+#define PCAN_SJA1000_ECC_SEG		0x1f
+#define PCAN_SJA1000_ECC_DIR		0x20
+#define PCAN_SJA1000_ECC_ERR		6
+#define PCAN_SJA1000_ECC_BIT		0x00
+#define PCAN_SJA1000_ECC_FORM		0x40
+#define PCAN_SJA1000_ECC_STUFF		0x80
+#define PCAN_SJA1000_ECC_MASK		0xc0
+
+/* SJA1000 Bus Error Interrupt */
+#define PCAN_SJA1000_IRQ_BEI		0x80
 
 /* private to PCAN-USB adapter */
 struct pcan_usb {
@@ -550,23 +560,95 @@ static int pcan_usb_decode_error(struct pcan_usb_msg_context *mc, u8 n,
 /* decode bus event usb packet: first byte contains rxerr while 2nd one contains
  * txerr.
  */
-static int pcan_usb_handle_bus_evt(struct pcan_usb_msg_context *mc, u8 ir)
+static int pcan_usb_handle_bus_evt(struct pcan_usb_msg_context *mc, u8 ir,
+				   u8 status_len)
 {
 	struct pcan_usb *pdev = mc->pdev;
+	u8 rec_len = status_len & PCAN_USB_STATUSLEN_DLC;
 
-	/* according to the content of the packet */
-	switch (ir) {
-	case PCAN_USB_ERR_CNT_DEC:
-	case PCAN_USB_ERR_CNT_INC:
+	/* Check for potential out-of-bound accesses
+	 * ("end" is a misnomer; it is not a pointer to the last valid byte,
+	 * but rather to the one following it)
+	 */
+	if (!rec_len || (mc->ptr + rec_len) > mc->end)
+		return -EINVAL;
 
-		/* save rx/tx error counters from in the device context */
+	/* 1st byte is ECC (if BEI), 2nd one is rxerr, 3rd one is txerr:
+	 * save rx/tx error counters from record data bytes first, so that
+	 * device error counters are always up-to-date.
+	 */
+	if (rec_len > 1) {
 		pdev->bec.rxerr = mc->ptr[1];
-		pdev->bec.txerr = mc->ptr[2];
-		break;
+		if (rec_len > 2)
+			pdev->bec.txerr = mc->ptr[2];
+	}
 
-	default:
-		/* reserved */
-		break;
+	/* Then process bus error interrupt (if any) */
+	if (ir & PCAN_SJA1000_IRQ_BEI) {
+		u8 ecc = mc->ptr[0];
+
+		/* create a "bus-error frame" skb if any bit is set in ECC */
+		if (ecc) {
+			struct net_device_stats *stats = &mc->netdev->stats;
+			struct sk_buff *skb;
+			struct can_frame *cf;
+			u8 can_err_tx = 0;
+
+			pdev->dev.can.can_stats.bus_error++;
+
+			/* Error occurred during reception? */
+			if (ecc & PCAN_SJA1000_ECC_DIR) {
+				stats->rx_errors++;
+			} else {
+				stats->tx_errors++;
+				can_err_tx = CAN_ERR_PROT_TX;
+			}
+
+			/* if berr-reporting is off, stop here */
+			if (!(pdev->dev.can.ctrlmode &
+						CAN_CTRLMODE_BERR_REPORTING))
+				return 0;
+
+			/* allocate an skb to store the error frame */
+			skb = alloc_can_err_skb(mc->netdev, &cf);
+			if (!skb)
+				return -ENOMEM;
+
+			cf->can_id |= CAN_ERR_CNT | CAN_ERR_PROT |
+				      CAN_ERR_BUSERROR;
+			cf->data[2] |= can_err_tx;
+
+			/* set error type according to 1st data byte (ECC) */
+			switch (ecc & PCAN_SJA1000_ECC_MASK) {
+			case PCAN_SJA1000_ECC_BIT:
+				cf->data[2] |= CAN_ERR_PROT_BIT;
+				break;
+			case PCAN_SJA1000_ECC_FORM:
+				cf->data[2] |= CAN_ERR_PROT_FORM;
+				break;
+			case PCAN_SJA1000_ECC_STUFF:
+				cf->data[2] |= CAN_ERR_PROT_STUFF;
+				break;
+			default:
+				break;
+			}
+
+			/* set error location */
+			cf->data[3] = ecc & PCAN_SJA1000_ECC_SEG;
+
+			cf->data[6] = pdev->bec.txerr;
+			cf->data[7] = pdev->bec.rxerr;
+
+			if (status_len & PCAN_USB_STATUSLEN_TIMESTAMP) {
+				struct skb_shared_hwtstamps *hwts =
+					skb_hwtstamps(skb);
+
+				peak_usb_get_ts_time(&mc->pdev->time_ref,
+						     mc->ts16, &hwts->hwtstamp);
+			}
+
+			netif_rx(skb);
+		}
 	}
 
 	return 0;
@@ -627,7 +709,7 @@ static int pcan_usb_decode_status(struct pcan_usb_msg_context *mc,
 
 	case PCAN_USB_REC_BUSEVT:
 		/* bus event notifications (get rxerr/txerr) */
-		err = pcan_usb_handle_bus_evt(mc, n);
+		err = pcan_usb_handle_bus_evt(mc, n, status_len);
 		if (err)
 			return err;
 		break;
@@ -1018,6 +1100,7 @@ const struct peak_usb_adapter pcan_usb = {
 	.ctrl_count = 1,
 	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
 			      CAN_CTRLMODE_3_SAMPLES |
+			      CAN_CTRLMODE_BERR_REPORTING |
 			      CAN_CTRLMODE_CC_LEN8_DLC,
 	.clock = {
 		.freq = PCAN_USB_CRYSTAL_HZ / 2,

-- 
2.53.0

Re: [PATCH can-next 4/7] can: peak_usb: Add PCAN-USB bus errors reporting

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch quietly fixes an existing slab out-of-bounds read
  in pcan_usb_handle_bus_evt()…
- [Medium] The patch adds PCAN_USB_ERR_ECC (0x01, which the patch itself
  documents as 'ask for BERR') to PCAN_USB_BERR_MASK.
- [Low] Several comments in drivers/net/can/usb/peak_usb/pcan_usb.c are
  stale or contradictory after this patch.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  pcan_usb_decode_msg() (drivers/net/can/usb/peak_usb/pcan_usb.c, line…

--

can: peak_usb: Add PCAN-USB bus errors reporting

This patch adds CAN bus error reporting to the PCAN-USB driver. It asks the
firmware for SJA1000 ECC reports and turns them into CAN_ERR_BUSERROR
frames in pcan_usb_handle_bus_evt(). It also advertises
CAN_CTRLMODE_BERR_REPORTING in ctrlmode_supported.

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 169c00d93463c..e71ed7879c04e 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -115,6 +115,7 @@
>  #define PCAN_USB_REC_BUSEVT		5
>
>  /* CAN bus events notifications selection mask */
> +#define PCAN_USB_ERR_ECC		0x01	/* ask for BERR */
>  #define PCAN_USB_ERR_RXERR		0x02	/* ask for rxerr counter */
>  #define PCAN_USB_ERR_TXERR		0x04	/* ask for txerr counter */
>
> @@ -122,11 +123,20 @@
>   * In other words, its interest is to know which side among rx and tx is
>   * responsible of the change of the bus state.
>   */
> -#define PCAN_USB_BERR_MASK	(PCAN_USB_ERR_RXERR | PCAN_USB_ERR_TXERR)
> +#define PCAN_USB_BERR_MASK	(PCAN_USB_ERR_ECC | \
> +				 PCAN_USB_ERR_RXERR | PCAN_USB_ERR_TXERR)

[Severity: Medium]
Can berr-reporting be turned off on the device side with this change?

pcan_usb_start() sends this mask to the device on every open and does not
look at ctrlmode:

pcan_usb_start() {
    ...
	err = pcan_usb_set_err_frame(dev, PCAN_USB_BERR_MASK);
    ...
}

The patch also adds CAN_CTRLMODE_BERR_REPORTING to ctrlmode_supported.
However, the only place that flag is checked is pcan_usb_handle_bus_evt().
By then the record has already crossed USB, and can_stats.bus_error and
rx_errors/tx_errors have already been incremented.

For comparison, set_normal_mode() in drivers/net/can/sja1000/sja1000.c
masks the interrupt source when reporting is off:

	if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
		priv->write_reg(priv, SJA1000_IER, IRQ_ALL);
	else
		priv->write_reg(priv, SJA1000_IER,
				IRQ_ALL & ~IRQ_BEI);

Consider a bus error storm, for example a single node with no ACK. Once the
controller is error-passive the old RXERR|TXERR mask stopped producing
records, because TEC stopped changing. With ECC always requested, the
firmware could send a record for every retransmission. The driver then
throws those records away when berr-reporting is off.

Could pcan_usb_start() build the mask as RXERR|TXERR, and add ECC only when
CAN_CTRLMODE_BERR_REPORTING is set? ctrlmode cannot change while the
interface is up, so making the decision at start time should be enough.

[ ... ]

> @@ -550,23 +560,95 @@ static int pcan_usb_decode_error(struct pcan_usb_msg_context *mc, u8 n,
>  /* decode bus event usb packet: first byte contains rxerr while 2nd one contains
>   * txerr.
>   */

[Severity: Low]
This isn't a bug, but a few comments look stale after this change.

The header comment above pcan_usb_handle_bus_evt() still says the first
byte is rxerr and the second is txerr. The new comment inside the function
says "1st byte is ECC (if BEI), 2nd one is rxerr, 3rd one is txerr". The
code reads ECC, rxerr and txerr from ptr[0], ptr[1] and ptr[2].

The comment above PCAN_USB_BERR_MASK still says:

  /* This mask generates an usb packet each time the state of the bus changes.
   * In other words, its interest is to know which side among rx and tx is
   * responsible of the change of the bus state.
   */

With PCAN_USB_ERR_ECC added, the mask now also requests a report for every
bus error.

The PCAN_USB_REC_BUSEVT case in pcan_usb_decode_status() still reads
"bus event notifications (get rxerr/txerr)". That path now also builds
CAN_ERR_BUSERROR frames.

> -static int pcan_usb_handle_bus_evt(struct pcan_usb_msg_context *mc, u8 ir)
> +static int pcan_usb_handle_bus_evt(struct pcan_usb_msg_context *mc, u8 ir,
> +				   u8 status_len)
>  {
>  	struct pcan_usb *pdev = mc->pdev;
> +	u8 rec_len = status_len & PCAN_USB_STATUSLEN_DLC;
>
> -	/* according to the content of the packet */
> -	switch (ir) {
> -	case PCAN_USB_ERR_CNT_DEC:
> -	case PCAN_USB_ERR_CNT_INC:
> +	/* Check for potential out-of-bound accesses
> +	 * ("end" is a misnomer; it is not a pointer to the last valid byte,
> +	 * but rather to the one following it)
> +	 */
> +	if (!rec_len || (mc->ptr + rec_len) > mc->end)
> +		return -EINVAL;
>
> -		/* save rx/tx error counters from in the device context */
> +	/* 1st byte is ECC (if BEI), 2nd one is rxerr, 3rd one is txerr:
> +	 * save rx/tx error counters from record data bytes first, so that
> +	 * device error counters are always up-to-date.
> +	 */
> +	if (rec_len > 1) {
>  		pdev->bec.rxerr = mc->ptr[1];
> -		pdev->bec.txerr = mc->ptr[2];
> -		break;
> +		if (rec_len > 2)
> +			pdev->bec.txerr = mc->ptr[2];
> +	}

[Severity: Medium]
This new bounds check, together with the rec_len checks around ptr[1] and
ptr[2], appears to also fix an out-of-bounds read that existed before this
patch. Should that fix be a separate patch with a Fixes: tag?

Before this patch, pcan_usb_handle_bus_evt() read mc->ptr[1] and
mc->ptr[2] for ir == 0x00 or 0x80 without checking them against mc->end.
The caller, pcan_usb_decode_status(), only checks the func/num bytes
before the call. The rec_len check runs after the handler returns:

	/* check whether function and number can be read */
	if ((mc->ptr + 2) > mc->end)
		return -EINVAL;
	...
	case PCAN_USB_REC_BUSEVT:
		err = pcan_usb_handle_bus_evt(mc, n);
	...
	if ((mc->ptr + rec_len) > mc->end)
		return -EINVAL;

The RX buffer is a 64 byte kmalloc (PCAN_USB_RX_BUFFER_SIZE). A transfer
that ends right after a BUSEVT header can therefore make the driver read
2-3 bytes past the slab object.

The commit message only says:

  The good old PCAN-USB was missing CAN bus error reporting. This patch
  fixes that.

These reads came from ea8b33bde76c ("can: pcan_usb: add support of
rxerr/txerr counters"). They got their current form in 590eb2b7d8cf
("can: peak_usb: pcan_usb_handle_bus_evt(): fix reading rxerr/txerr
values"), which was Cc'd to stable.

Without a Fixes: tag or a note in the changelog, stable trees have no
reason to pick this up. Could it be split into its own fix, or at least
described in the commit message?

[ ... ]

> @@ -627,7 +709,7 @@ static int pcan_usb_decode_status(struct pcan_usb_msg_context *mc,
>
>  	case PCAN_USB_REC_BUSEVT:
>  		/* bus event notifications (get rxerr/txerr) */
> -		err = pcan_usb_handle_bus_evt(mc, n);
> +		err = pcan_usb_handle_bus_evt(mc, n, status_len);
>  		if (err)
>  			return err;
>  		break;

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. I
noticed it while looking at this parsing path: can pcan_usb_decode_msg()
read past the end of ibuf[]?

pcan_usb_decode_msg() {
	struct pcan_usb_msg_context mc = {
		.rec_cnt = ibuf[1],
    ...
	for (err = 0; mc.rec_idx < mc.rec_cnt && !err; mc.rec_idx++) {
		u8 sl = *mc.ptr++;
    ...
}

The record count comes from the device, and each status byte is read
without checking that mc.ptr < mc.end.

Take a 64 byte transfer with a 2 byte header that claims 5 records,
followed by BUSEVT records of 18, 18, 18 and 8 bytes. It passes every
bounds check and leaves mc.ptr == mc.end. The 5th iteration then reads
ibuf[64], one byte past the buffer.

In pcan_usb_decode_data() there is also an unchecked increment for the SRR
flag:

		if (can_id_flags & PCAN_USB_TX_SRR)
			mc->ptr++;

That increment can move ptr to end + 1 before the next status byte is
read.

Later checks reject the record, so the overread is limited to 1-2 bytes.
This loop dates back to 46be265d3388 ("can: usb: PEAK-System Technik
PCAN-USB specific part").

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-peak_usb_enhancements-v1-0-50e965755c06%40pengutronix.de

[PATCH can-next 5/7] can: peak_usb: Sort CAN_CTRLMODE flags by value

[Marc Kleine-Budde]

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

Sort the CAN_CTRLMODE flags in ctrlmode_supported by ascending bit value.

No functional change intended.

Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 32 +++++++++++++++++++-----------
 1 file changed, 20 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..82502594a409 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -1198,9 +1198,11 @@ const struct peak_usb_adapter pcan_usb_fd = {
 	.name = "PCAN-USB FD",
 	.device_id = PCAN_USBFD_PRODUCT_ID,
 	.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,
+	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+			      CAN_CTRLMODE_3_SAMPLES |
+			      CAN_CTRLMODE_ONE_SHOT |
+			      CAN_CTRLMODE_FD |
+			      CAN_CTRLMODE_CC_LEN8_DLC,
 	.clock = {
 		.freq = PCAN_UFD_CRYSTAL_HZ,
 	},
@@ -1274,9 +1276,11 @@ const struct peak_usb_adapter pcan_usb_chip = {
 	.name = "PCAN-Chip USB",
 	.device_id = PCAN_USBCHIP_PRODUCT_ID,
 	.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,
+	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+			      CAN_CTRLMODE_3_SAMPLES |
+			      CAN_CTRLMODE_ONE_SHOT |
+			      CAN_CTRLMODE_FD |
+			      CAN_CTRLMODE_CC_LEN8_DLC,
 	.clock = {
 		.freq = PCAN_UFD_CRYSTAL_HZ,
 	},
@@ -1350,9 +1354,11 @@ const struct peak_usb_adapter pcan_usb_pro_fd = {
 	.name = "PCAN-USB Pro FD",
 	.device_id = PCAN_USBPROFD_PRODUCT_ID,
 	.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,
+	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+			      CAN_CTRLMODE_3_SAMPLES |
+			      CAN_CTRLMODE_ONE_SHOT |
+			      CAN_CTRLMODE_FD |
+			      CAN_CTRLMODE_CC_LEN8_DLC,
 	.clock = {
 		.freq = PCAN_UFD_CRYSTAL_HZ,
 	},
@@ -1426,9 +1432,11 @@ const struct peak_usb_adapter pcan_usb_x6 = {
 	.name = "PCAN-USB X6",
 	.device_id = PCAN_USBX6_PRODUCT_ID,
 	.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,
+	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+			      CAN_CTRLMODE_3_SAMPLES |
+			      CAN_CTRLMODE_ONE_SHOT |
+			      CAN_CTRLMODE_FD |
+			      CAN_CTRLMODE_CC_LEN8_DLC,
 	.clock = {
 		.freq = PCAN_UFD_CRYSTAL_HZ,
 	},

-- 
2.53.0

[PATCH can-next 6/7] can: peak_usb: Add bus error reporting for the PCAN-USB FD family

[Marc Kleine-Budde]

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. Parsing of
the entire URB is aborted if the firmware reports an invalid channel.

Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 83 +++++++++++++++++++++++++++---
 include/linux/can/dev/peak_canfd.h         | 15 +++++-
 2 files changed, 90 insertions(+), 8 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 82502594a409..9081f30e3d35 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,73 @@ 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.
+	 * Treat the entire URB as invalid in that case.
+	 */
 	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 +955,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 +969,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 +1017,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;
@@ -1201,6 +1266,7 @@ const struct peak_usb_adapter pcan_usb_fd = {
 	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
 			      CAN_CTRLMODE_3_SAMPLES |
 			      CAN_CTRLMODE_ONE_SHOT |
+			      CAN_CTRLMODE_BERR_REPORTING |
 			      CAN_CTRLMODE_FD |
 			      CAN_CTRLMODE_CC_LEN8_DLC,
 	.clock = {
@@ -1279,6 +1345,7 @@ const struct peak_usb_adapter pcan_usb_chip = {
 	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
 			      CAN_CTRLMODE_3_SAMPLES |
 			      CAN_CTRLMODE_ONE_SHOT |
+			      CAN_CTRLMODE_BERR_REPORTING |
 			      CAN_CTRLMODE_FD |
 			      CAN_CTRLMODE_CC_LEN8_DLC,
 	.clock = {
@@ -1357,6 +1424,7 @@ const struct peak_usb_adapter pcan_usb_pro_fd = {
 	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
 			      CAN_CTRLMODE_3_SAMPLES |
 			      CAN_CTRLMODE_ONE_SHOT |
+			      CAN_CTRLMODE_BERR_REPORTING |
 			      CAN_CTRLMODE_FD |
 			      CAN_CTRLMODE_CC_LEN8_DLC,
 	.clock = {
@@ -1435,6 +1503,7 @@ const struct peak_usb_adapter pcan_usb_x6 = {
 	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
 			      CAN_CTRLMODE_3_SAMPLES |
 			      CAN_CTRLMODE_ONE_SHOT |
+			      CAN_CTRLMODE_BERR_REPORTING |
 			      CAN_CTRLMODE_FD |
 			      CAN_CTRLMODE_CC_LEN8_DLC,
 	.clock = {
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;

-- 
2.53.0

Re: [PATCH can-next 6/7] can: peak_usb: Add bus error reporting for the PCAN-USB FD family

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The invalid-channel guard is incomplete.
- [Medium] pcan_usb_fd_start() now calls `err =
  pcan_usb_fd_set_options(dev, 1, PUCAN_OPTION_ERROR, usb_opts);` (line…
- [Low] The new `if (!dev)` check in pcan_usb_fd_decode_error() fixes an
  existing NULL pointer dereference, but the fix is folded into a…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but when
  peak_usb_start() fails at dev_set_bus(dev, 1) after…

--

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

This patch adds CAN bus error reporting for the PEAK-System USB-to-CAN FD
devices. pcan_usb_fd_start() now enables PUCAN_OPTION_ERROR for each
channel, pcan_usb_fd_decode_error() turns firmware error records into CAN
error frames, and the adapters now advertise CAN_CTRLMODE_BERR_REPORTING.
It also adds a NULL check for an invalid channel in
pcan_usb_fd_decode_error().

> 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 82502594a4092..9081f30e3d356 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,73 @@ 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.
> +	 * Treat the entire URB as invalid in that case.
> +	 */
>  	dev = usb_if->dev[pucan_ermsg_get_channel(er)];
> +	if (!dev)
> +		return -EINVAL;
> +

[Severity: High]
This check covers pcan_usb_fd_decode_error(). Do the other decoders that
read a channel from the same URB need it too?

On PCAN-USB FD and PCAN-Chip USB, ctrl_count is PCAN_USBFD_CHANNEL_COUNT
(1). pcan_usb_fd_init() therefore sets only usb_if->dev[0], and dev[1]
stays NULL. The ARRAY_SIZE(usb_if->dev) bound is PCAN_USB_MAX_CHANNEL (2),
so channel 1 passes that check.

The other three decoders still dereference without a NULL check:

pcan_usb_fd_decode_canmsg():
	dev = usb_if->dev[pucan_msg_get_channel(rm)];
	netdev = dev->netdev;

pcan_usb_fd_decode_status():
	dev = usb_if->dev[pucan_stmsg_get_channel(sm)];
	pdev = container_of(dev, struct pcan_usb_fd_device, dev);
	netdev = dev->netdev;

pcan_usb_fd_decode_overrun():
	dev = usb_if->dev[pufd_omsg_get_channel(ov)];
	netdev = dev->netdev;

Suppose the firmware sends the bogus channel 1 described in the new comment
on a CAN_RX, STATUS or OVERRUN record. Wouldn't that dereference NULL in
peak_usb_read_bulk_callback()->pcan_usb_fd_decode_buf()?

The commit message says "Parsing of the entire URB is aborted if the
firmware reports an invalid channel". As far as I can tell, that only holds
for error records. The later loopback patch in this series doesn't seem to
change these decoders either.

[Severity: Low]
The code itself is fine, but this check also fixes an existing NULL pointer
dereference. Before this patch, pcan_usb_fd_decode_error() did:

	dev = usb_if->dev[pucan_ermsg_get_channel(er)];
	pdev = container_of(dev, struct pcan_usb_fd_device, dev);
	pdev->bec.txerr = er->tx_err_cnt;

PUCAN_OPTION_ERROR was already enabled on first open, so a channel 1 error
record on a single-channel adapter could already fault there.

Could this fix be split out with a Fixes: tag and Cc: stable, as was done
for the earlier "fix missing CAN_ERR_FLAG ..." patch in this series? That
way stable trees would pick it up.

[ ... ]

> @@ -912,12 +969,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++;

[Severity: Medium]
Can dev_opened_count end up one too high here?

pcan_usb_fd_set_options() now runs on every open. If it fails, the counter
is still incremented before err is returned. peak_usb_start() then takes
the err_adapter path:

	if (dev->adapter->dev_start) {
		err = dev->adapter->dev_start(dev);
		if (err)
			goto err_adapter;
	}

err_adapter frees the tx URBs and kills the rx URBs, but it never calls
adapter->dev_stop(). ndo_stop isn't called after a failed open either.

With the count stuck too high:

- the == 0 check here no longer resets time_ref or re-enables calibration
  on the next first open
- the == 1 check in pcan_usb_fd_stop() never matches, so calibration
  messages are never turned off
- the > 0 check in pcan_usb_fd_exit() gives the wrong result

Before this patch, a non-first open could only fail in
pcan_usb_fd_set_filter_std(), which returns before the increment. Should
the increment be skipped when pcan_usb_fd_set_options() fails?

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but a
related path exists when pcan_usb_fd_start() succeeds and bus-on then fails
in peak_usb_start():

	err = dev->adapter->dev_set_bus(dev, 1);
	if (err)
		goto err_adapter;

err_adapter doesn't call dev_stop() here either. dev_opened_count stays
incremented and the firmware notification options stay enabled. With this
patch, the per-channel PUCAN_OPTION_ERROR is also left armed in that case.

Should peak_usb_start() undo dev_start() on this path?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-peak_usb_enhancements-v1-0-50e965755c06%40pengutronix.de

[PATCH can-next 7/7] can: peak_usb: Add support of loopback mode to the PCAN-USB FD family

[Marc Kleine-Budde]

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

This commit adds loopback mode support for all PEAK-System USB-to-CAN FD
interfaces.

The implementation follows the same approach as the peak_pciefd driver.
The only difference is local echo handling: on USB devices, transmit
confirmations are provided through USB write acknowledgments rather than
through the reception of a controller-generated loopback frame.

Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 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 9081f30e3d35..59debe009f92 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -934,6 +934,12 @@ static int pcan_usb_fd_encode_msg(struct peak_usb_device *dev,
 	if (dev->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
 		tx_msg_flags |= PUCAN_MSG_SINGLE_SHOT;
 
+	/* Request controller loopback and mark the frame as self-received,
+	 * matching the behavior of the peak_pciefd driver.
+	 */
+	if (dev->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
+		tx_msg_flags |= PUCAN_MSG_LOOPED_BACK | PUCAN_MSG_SELF_RECEIVE;
+
 	tx_msg->flags = cpu_to_le16(tx_msg_flags);
 	tx_msg->channel_dlc = PUCAN_MSG_CHANNEL_DLC(dev->ctrl_idx, dlc);
 	memcpy(tx_msg->d, cfd->data, cfd->len);
@@ -1263,7 +1269,8 @@ const struct peak_usb_adapter pcan_usb_fd = {
 	.name = "PCAN-USB FD",
 	.device_id = PCAN_USBFD_PRODUCT_ID,
 	.ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
-	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+	.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
+			      CAN_CTRLMODE_LISTENONLY |
 			      CAN_CTRLMODE_3_SAMPLES |
 			      CAN_CTRLMODE_ONE_SHOT |
 			      CAN_CTRLMODE_BERR_REPORTING |
@@ -1342,7 +1349,8 @@ const struct peak_usb_adapter pcan_usb_chip = {
 	.name = "PCAN-Chip USB",
 	.device_id = PCAN_USBCHIP_PRODUCT_ID,
 	.ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
-	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+	.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
+			      CAN_CTRLMODE_LISTENONLY |
 			      CAN_CTRLMODE_3_SAMPLES |
 			      CAN_CTRLMODE_ONE_SHOT |
 			      CAN_CTRLMODE_BERR_REPORTING |
@@ -1421,7 +1429,8 @@ const struct peak_usb_adapter pcan_usb_pro_fd = {
 	.name = "PCAN-USB Pro FD",
 	.device_id = PCAN_USBPROFD_PRODUCT_ID,
 	.ctrl_count = PCAN_USBPROFD_CHANNEL_COUNT,
-	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+	.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
+			      CAN_CTRLMODE_LISTENONLY |
 			      CAN_CTRLMODE_3_SAMPLES |
 			      CAN_CTRLMODE_ONE_SHOT |
 			      CAN_CTRLMODE_BERR_REPORTING |
@@ -1500,7 +1509,8 @@ const struct peak_usb_adapter pcan_usb_x6 = {
 	.name = "PCAN-USB X6",
 	.device_id = PCAN_USBX6_PRODUCT_ID,
 	.ctrl_count = PCAN_USBPROFD_CHANNEL_COUNT,
-	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+	.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
+			      CAN_CTRLMODE_LISTENONLY |
 			      CAN_CTRLMODE_3_SAMPLES |
 			      CAN_CTRLMODE_ONE_SHOT |
 			      CAN_CTRLMODE_BERR_REPORTING |

-- 
2.53.0

Re: [PATCH can-next 7/7] can: peak_usb: Add support of loopback mode to the PCAN-USB FD family

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch adds CAN_CTRLMODE_LOOPBACK to the static
  .ctrlmode_supported of all four FD-family adapters: pcan_usb_fd,…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but a malicious or
  faulty USB device that presents itself as a single-channel PCAN-USB FD…

--

can: peak_usb: Add support of loopback mode to the PCAN-USB FD family

This commit adds CAN_CTRLMODE_LOOPBACK to the supported modes of the
PCAN-USB FD, PCAN-Chip USB, PCAN-USB Pro FD and PCAN-USB X6 adapters. When
loopback is on, pcan_usb_fd_encode_msg() sets PUCAN_MSG_LOOPED_BACK and
PUCAN_MSG_SELF_RECEIVE in the uCAN TX message flags. This follows the
peak_pciefd driver, but local echo still comes from USB write completions.

> 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 9081f30e3d356..59debe009f922 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> @@ -934,6 +934,12 @@ static int pcan_usb_fd_encode_msg(struct peak_usb_device *dev,
>  	if (dev->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
>  		tx_msg_flags |= PUCAN_MSG_SINGLE_SHOT;
>  
> +	/* Request controller loopback and mark the frame as self-received,
> +	 * matching the behavior of the peak_pciefd driver.
> +	 */
> +	if (dev->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> +		tx_msg_flags |= PUCAN_MSG_LOOPED_BACK | PUCAN_MSG_SELF_RECEIVE;
> +
>  	tx_msg->flags = cpu_to_le16(tx_msg_flags);
>  	tx_msg->channel_dlc = PUCAN_MSG_CHANNEL_DLC(dev->ctrl_idx, dlc);
>  	memcpy(tx_msg->d, cfd->data, cfd->len);
> @@ -1263,7 +1269,8 @@ const struct peak_usb_adapter pcan_usb_fd = {
>  	.name = "PCAN-USB FD",
>  	.device_id = PCAN_USBFD_PRODUCT_ID,
>  	.ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
> -	.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
> +	.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> +			      CAN_CTRLMODE_LISTENONLY |
>  			      CAN_CTRLMODE_3_SAMPLES |
>  			      CAN_CTRLMODE_ONE_SHOT |
>  			      CAN_CTRLMODE_BERR_REPORTING |

[Severity: Low]
Does every firmware revision of these adapters support loopback?

LOOPBACK is added statically here, and also in pcan_usb_chip,
pcan_usb_pro_fd and pcan_usb_x6. All the new mode does is set
PUCAN_MSG_LOOPED_BACK | PUCAN_MSG_SELF_RECEIVE in pcan_usb_fd_encode_msg().
The host does not program any controller mode, and
pcan_usb_fd_decode_canmsg() never reads these flags. Whether the mode works
is therefore up to the firmware.

pcan_usb_fd_init() already gates modes that depend on the firmware:

	if (fw_info->fw_version[0] >= 2) {
		/* firmware >= 2.x supports ISO/non-ISO switching */
		dev->can.ctrlmode_supported |= CAN_CTRLMODE_FD_NON_ISO;

If a firmware revision ignored these TX flags, "ip link set canX type can
loopback on" would be accepted but would do nothing.

This may well be fine. ONE_SHOT is already advertised statically, and it
is also just a uCAN TX flag (PUCAN_MSG_SINGLE_SHOT). The non-FD driver's
pcan_usb_init() does gate LOOPBACK on device_rev >= 41, but it gates
ONE_SHOT the same way, so that comparison may not apply here. The commit
message also says all USB-to-CAN FD interfaces support this mode.

Is there any FD firmware version that should be excluded here?

[ ... ]

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Can a
single-channel adapter cause a NULL pointer dereference in the bulk-IN
decode path?

PCAN-USB FD and PCAN-Chip USB use ctrl_count = PCAN_USBFD_CHANNEL_COUNT
(1). struct pcan_usb_fd_if, however, has dev[PCAN_USB_MAX_CHANNEL] (2).
pcan_usb_fd_init() only fills the slot of the controller being registered:

	pdev->usb_if->dev[dev->ctrl_idx] = dev;

On those adapters, usb_if->dev[1] stays NULL.

Before dereferencing, pcan_usb_fd_decode_canmsg() only checks the channel
against the array size:

	if (pucan_msg_get_channel(rm) >= ARRAY_SIZE(usb_if->dev))
		return -ENOMEM;

	dev = usb_if->dev[pucan_msg_get_channel(rm)];
	netdev = dev->netdev;

A faulty or malicious device could send a PUCAN_MSG_CAN_RX record with
channel 1. That record passes this check, and the code then dereferences
NULL in URB completion context.

pcan_usb_fd_decode_status() and pcan_usb_fd_decode_overrun() use the same
pattern, for PUCAN_MSG_STATUS and PCAN_UFD_MSG_OVERRUN records.

pcan_usb_fd_decode_error() already handles this case. The guard came in
with "can: peak_usb: Add bus error reporting for the PCAN-USB FD family",
but it was added only to this decoder:

	/* Guard against bogus channel 1 reports from single-channel adapters.
	 * Treat the entire URB as invalid in that case.
	 */
	dev = usb_if->dev[pucan_ermsg_get_channel(er)];
	if (!dev)
		return -EINVAL;

Should the CAN_RX, STATUS and OVERRUN decoders get the same NULL check?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-peak_usb_enhancements-v1-0-50e965755c06%40pengutronix.de