[PATCH v2 1/2] can: peak_usb: upgrades the handling of bus state changes

[PATCH v2 1/2] can: peak_usb: upgrades the handling of bus state changes

[Stephane Grosjean]

This patch updates old code by using the functions published since by the
socket-can module. In particular, this new code better manages the change
of bus state by also using the value of the error counters that the driver
now systematically asks for when initializing the channel.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
v2:
 - do the statistics and state update, even if the allocation of the skb
   fails.

 drivers/net/can/usb/peak_usb/pcan_usb.c | 161 ++++++------------------
 1 file changed, 38 insertions(+), 123 deletions(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 7d18bc6911f5..4c3e18ca3181 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -453,146 +453,61 @@ static int pcan_usb_decode_error(struct pcan_usb_msg_context *mc, u8 n,
 {
 	struct sk_buff *skb;
 	struct can_frame *cf;
-	enum can_state new_state;
+	enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
 
 	/* ignore this error until 1st ts received */
 	if (n == PCAN_USB_ERROR_QOVR)
 		if (!mc->pdev->time_ref.tick_count)
 			return 0;
 
-	new_state = mc->pdev->dev.can.state;
-
-	switch (mc->pdev->dev.can.state) {
-	case CAN_STATE_ERROR_ACTIVE:
-		if (n & PCAN_USB_ERROR_BUS_LIGHT) {
-			new_state = CAN_STATE_ERROR_WARNING;
-			break;
-		}
-		fallthrough;
-
-	case CAN_STATE_ERROR_WARNING:
-		if (n & PCAN_USB_ERROR_BUS_HEAVY) {
-			new_state = CAN_STATE_ERROR_PASSIVE;
-			break;
-		}
-		if (n & PCAN_USB_ERROR_BUS_OFF) {
-			new_state = CAN_STATE_BUS_OFF;
-			break;
-		}
-		if (n & ~PCAN_USB_ERROR_BUS) {
-			/*
-			 * trick to bypass next comparison and process other
-			 * errors
-			 */
-			new_state = CAN_STATE_MAX;
-			break;
-		}
-		if ((n & PCAN_USB_ERROR_BUS_LIGHT) == 0) {
-			/* no error (back to active state) */
-			new_state = CAN_STATE_ERROR_ACTIVE;
-			break;
-		}
-		break;
-
-	case CAN_STATE_ERROR_PASSIVE:
-		if (n & PCAN_USB_ERROR_BUS_OFF) {
-			new_state = CAN_STATE_BUS_OFF;
-			break;
-		}
-		if (n & PCAN_USB_ERROR_BUS_LIGHT) {
-			new_state = CAN_STATE_ERROR_WARNING;
-			break;
-		}
-		if (n & ~PCAN_USB_ERROR_BUS) {
-			/*
-			 * trick to bypass next comparison and process other
-			 * errors
-			 */
-			new_state = CAN_STATE_MAX;
-			break;
-		}
-
-		if ((n & PCAN_USB_ERROR_BUS_HEAVY) == 0) {
-			/* no error (back to warning state) */
-			new_state = CAN_STATE_ERROR_WARNING;
-			break;
-		}
-		break;
-
-	default:
-		/* do nothing waiting for restart */
-		return 0;
-	}
-
-	/* donot post any error if current state didn't change */
-	if (mc->pdev->dev.can.state == new_state)
-		return 0;
-
 	/* allocate an skb to store the error frame */
 	skb = alloc_can_err_skb(mc->netdev, &cf);
-	if (!skb)
-		return -ENOMEM;
-
-	switch (new_state) {
-	case CAN_STATE_BUS_OFF:
-		cf->can_id |= CAN_ERR_BUSOFF;
-		mc->pdev->dev.can.can_stats.bus_off++;
-		can_bus_off(mc->netdev);
-		break;
-
-	case CAN_STATE_ERROR_PASSIVE:
-		cf->can_id |= CAN_ERR_CRTL;
-		cf->data[1] = (mc->pdev->bec.txerr > mc->pdev->bec.rxerr) ?
-				CAN_ERR_CRTL_TX_PASSIVE :
-				CAN_ERR_CRTL_RX_PASSIVE;
-		cf->data[6] = mc->pdev->bec.txerr;
-		cf->data[7] = mc->pdev->bec.rxerr;
-
-		mc->pdev->dev.can.can_stats.error_passive++;
-		break;
-
-	case CAN_STATE_ERROR_WARNING:
-		cf->can_id |= CAN_ERR_CRTL;
-		cf->data[1] = (mc->pdev->bec.txerr > mc->pdev->bec.rxerr) ?
-				CAN_ERR_CRTL_TX_WARNING :
-				CAN_ERR_CRTL_RX_WARNING;
-		cf->data[6] = mc->pdev->bec.txerr;
-		cf->data[7] = mc->pdev->bec.rxerr;
-
-		mc->pdev->dev.can.can_stats.error_warning++;
-		break;
 
-	case CAN_STATE_ERROR_ACTIVE:
-		cf->can_id |= CAN_ERR_CRTL;
-		cf->data[1] = CAN_ERR_CRTL_ACTIVE;
-
-		/* sync local copies of rxerr/txerr counters */
-		mc->pdev->bec.txerr = 0;
-		mc->pdev->bec.rxerr = 0;
-		break;
-
-	default:
-		/* CAN_STATE_MAX (trick to handle other errors) */
-		if (n & PCAN_USB_ERROR_TXQFULL)
-			netdev_dbg(mc->netdev, "device Tx queue full)\n");
-
-		if (n & PCAN_USB_ERROR_RXQOVR) {
-			netdev_dbg(mc->netdev, "data overrun interrupt\n");
+	if (n & PCAN_USB_ERROR_RXQOVR) {
+		/* data overrun interrupt */
+		netdev_dbg(mc->netdev, "data overrun interrupt\n");
+		mc->netdev->stats.rx_over_errors++;
+		mc->netdev->stats.rx_errors++;
+		if (skb) {
 			cf->can_id |= CAN_ERR_CRTL;
 			cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
-			mc->netdev->stats.rx_over_errors++;
-			mc->netdev->stats.rx_errors++;
 		}
+	}
 
-		cf->data[6] = mc->pdev->bec.txerr;
-		cf->data[7] = mc->pdev->bec.rxerr;
+	if (n & PCAN_USB_ERROR_TXQFULL)
+		netdev_dbg(mc->netdev, "device Tx queue full)\n");
 
-		new_state = mc->pdev->dev.can.state;
-		break;
+	if (n & PCAN_USB_ERROR_BUS_OFF) {
+		new_state = CAN_STATE_BUS_OFF;
+	} else if (n & PCAN_USB_ERROR_BUS_HEAVY) {
+		new_state = ((mc->pdev->bec.txerr >= 128) ||
+			     (mc->pdev->bec.rxerr >= 128)) ?
+				CAN_STATE_ERROR_PASSIVE :
+				CAN_STATE_ERROR_WARNING;
+	} else {
+		new_state = CAN_STATE_ERROR_ACTIVE;
 	}
 
-	mc->pdev->dev.can.state = new_state;
+	/* handle change of state */
+	if (new_state != mc->pdev->dev.can.state) {
+		if (skb) {
+			enum can_state tx_state =
+				(mc->pdev->bec.txerr >= mc->pdev->bec.rxerr) ?
+					new_state : 0;
+			enum can_state rx_state =
+				(mc->pdev->bec.txerr <= mc->pdev->bec.rxerr) ?
+					new_state : 0;
+
+			can_change_state(mc->netdev, cf, tx_state, rx_state);
+		}
+
+		/* things must be done even in case of OOM */
+		if (new_state == CAN_STATE_BUS_OFF)
+			can_bus_off(mc->netdev);
+	}
 
+	if (!skb)
+		return -ENOMEM;
 	if (status_len & PCAN_USB_STATUSLEN_TIMESTAMP) {
 		struct skb_shared_hwtstamps *hwts = skb_hwtstamps(skb);
 
-- 
2.25.1

[PATCH 2/2] can: peak_usb: Add a "firmware update available" message to old PCAN-USB

[Stephane Grosjean]

When the driver detects that the PCAN-USB device runs an old firmware
(< 4.1) then it prints a message suggesting to contact
<support@peak-system.com> for a possible firmware update.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 4c3e18ca3181..f5edfc1e412e 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -878,6 +878,11 @@ static int pcan_usb_init(struct peak_usb_device *dev)
 		return err;
 	}
 
+	dev_info(dev->netdev->dev.parent,
+		 "PEAK-System %s adapter hwrev %u serial %08X (%u channel)\n",
+		 pcan_usb.name, dev->device_rev, serial_number,
+		 pcan_usb.ctrl_count);
+
 	/* Since rev 4.1, PCAN-USB is able to make single-short as well as
 	 * looped back frames.
 	 */
@@ -886,13 +891,11 @@ static int pcan_usb_init(struct peak_usb_device *dev)
 
 		priv->ctrlmode_supported |= CAN_CTRLMODE_ONE_SHOT |
 					    CAN_CTRLMODE_LOOPBACK;
+	} else {
+		dev_info(dev->netdev->dev.parent,
+			 "Firmware update available. Please contact support@peak-system.com\n");
 	}
 
-	dev_info(dev->netdev->dev.parent,
-		 "PEAK-System %s adapter hwrev %u serial %08X (%u channel)\n",
-		 pcan_usb.name, dev->device_rev, serial_number,
-		 pcan_usb.ctrl_count);
-
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH v2 1/2] can: peak_usb: upgrades the handling of bus state changes

[Marc Kleine-Budde]

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

On 28.06.2021 16:20:42, Stephane Grosjean wrote:
> This patch updates old code by using the functions published since by the
> socket-can module. In particular, this new code better manages the change
> of bus state by also using the value of the error counters that the driver
> now systematically asks for when initializing the channel.
> 
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> ---
> v2:
>  - do the statistics and state update, even if the allocation of the skb
>    fails.

BTW: please send the whole series with the same "v$NUMBER" as the b4
tool wants to have v2 on all patches of the series.

> 
>  drivers/net/can/usb/peak_usb/pcan_usb.c | 161 ++++++------------------
>  1 file changed, 38 insertions(+), 123 deletions(-)
> 
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 7d18bc6911f5..4c3e18ca3181 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -453,146 +453,61 @@ static int pcan_usb_decode_error(struct pcan_usb_msg_context *mc, u8 n,
>  {
>  	struct sk_buff *skb;
>  	struct can_frame *cf;
> -	enum can_state new_state;
> +	enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
>  
>  	/* ignore this error until 1st ts received */
>  	if (n == PCAN_USB_ERROR_QOVR)
>  		if (!mc->pdev->time_ref.tick_count)
>  			return 0;
>  
> -	new_state = mc->pdev->dev.can.state;
> -
> -	switch (mc->pdev->dev.can.state) {
> -	case CAN_STATE_ERROR_ACTIVE:
> -		if (n & PCAN_USB_ERROR_BUS_LIGHT) {
> -			new_state = CAN_STATE_ERROR_WARNING;
> -			break;
> -		}
> -		fallthrough;
> -
> -	case CAN_STATE_ERROR_WARNING:
> -		if (n & PCAN_USB_ERROR_BUS_HEAVY) {
> -			new_state = CAN_STATE_ERROR_PASSIVE;
> -			break;
> -		}
> -		if (n & PCAN_USB_ERROR_BUS_OFF) {
> -			new_state = CAN_STATE_BUS_OFF;
> -			break;
> -		}
> -		if (n & ~PCAN_USB_ERROR_BUS) {
> -			/*
> -			 * trick to bypass next comparison and process other
> -			 * errors
> -			 */
> -			new_state = CAN_STATE_MAX;
> -			break;
> -		}
> -		if ((n & PCAN_USB_ERROR_BUS_LIGHT) == 0) {
> -			/* no error (back to active state) */
> -			new_state = CAN_STATE_ERROR_ACTIVE;
> -			break;
> -		}
> -		break;
> -
> -	case CAN_STATE_ERROR_PASSIVE:
> -		if (n & PCAN_USB_ERROR_BUS_OFF) {
> -			new_state = CAN_STATE_BUS_OFF;
> -			break;
> -		}
> -		if (n & PCAN_USB_ERROR_BUS_LIGHT) {
> -			new_state = CAN_STATE_ERROR_WARNING;
> -			break;
> -		}
> -		if (n & ~PCAN_USB_ERROR_BUS) {
> -			/*
> -			 * trick to bypass next comparison and process other
> -			 * errors
> -			 */
> -			new_state = CAN_STATE_MAX;
> -			break;
> -		}
> -
> -		if ((n & PCAN_USB_ERROR_BUS_HEAVY) == 0) {
> -			/* no error (back to warning state) */
> -			new_state = CAN_STATE_ERROR_WARNING;
> -			break;
> -		}
> -		break;
> -
> -	default:
> -		/* do nothing waiting for restart */
> -		return 0;
> -	}
> -
> -	/* donot post any error if current state didn't change */
> -	if (mc->pdev->dev.can.state == new_state)
> -		return 0;
> -
>  	/* allocate an skb to store the error frame */
>  	skb = alloc_can_err_skb(mc->netdev, &cf);
> -	if (!skb)
> -		return -ENOMEM;
> -
> -	switch (new_state) {
> -	case CAN_STATE_BUS_OFF:
> -		cf->can_id |= CAN_ERR_BUSOFF;
> -		mc->pdev->dev.can.can_stats.bus_off++;
> -		can_bus_off(mc->netdev);
> -		break;
> -
> -	case CAN_STATE_ERROR_PASSIVE:
> -		cf->can_id |= CAN_ERR_CRTL;
> -		cf->data[1] = (mc->pdev->bec.txerr > mc->pdev->bec.rxerr) ?
> -				CAN_ERR_CRTL_TX_PASSIVE :
> -				CAN_ERR_CRTL_RX_PASSIVE;
> -		cf->data[6] = mc->pdev->bec.txerr;
> -		cf->data[7] = mc->pdev->bec.rxerr;

The TX and RX error counters are not supplied anymore, was this intentional?

> -
> -		mc->pdev->dev.can.can_stats.error_passive++;
> -		break;
> -
> -	case CAN_STATE_ERROR_WARNING:
> -		cf->can_id |= CAN_ERR_CRTL;
> -		cf->data[1] = (mc->pdev->bec.txerr > mc->pdev->bec.rxerr) ?
> -				CAN_ERR_CRTL_TX_WARNING :
> -				CAN_ERR_CRTL_RX_WARNING;
> -		cf->data[6] = mc->pdev->bec.txerr;
> -		cf->data[7] = mc->pdev->bec.rxerr;
> -
> -		mc->pdev->dev.can.can_stats.error_warning++;
> -		break;
>  
> -	case CAN_STATE_ERROR_ACTIVE:
> -		cf->can_id |= CAN_ERR_CRTL;
> -		cf->data[1] = CAN_ERR_CRTL_ACTIVE;
> -
> -		/* sync local copies of rxerr/txerr counters */
> -		mc->pdev->bec.txerr = 0;
> -		mc->pdev->bec.rxerr = 0;
> -		break;
> -
> -	default:
> -		/* CAN_STATE_MAX (trick to handle other errors) */
> -		if (n & PCAN_USB_ERROR_TXQFULL)
> -			netdev_dbg(mc->netdev, "device Tx queue full)\n");
> -
> -		if (n & PCAN_USB_ERROR_RXQOVR) {
> -			netdev_dbg(mc->netdev, "data overrun interrupt\n");
> +	if (n & PCAN_USB_ERROR_RXQOVR) {
> +		/* data overrun interrupt */
> +		netdev_dbg(mc->netdev, "data overrun interrupt\n");
> +		mc->netdev->stats.rx_over_errors++;
> +		mc->netdev->stats.rx_errors++;
> +		if (skb) {
>  			cf->can_id |= CAN_ERR_CRTL;
>  			cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
> -			mc->netdev->stats.rx_over_errors++;
> -			mc->netdev->stats.rx_errors++;
>  		}
> +	}
>  
> -		cf->data[6] = mc->pdev->bec.txerr;
> -		cf->data[7] = mc->pdev->bec.rxerr;
> +	if (n & PCAN_USB_ERROR_TXQFULL)
> +		netdev_dbg(mc->netdev, "device Tx queue full)\n");
>  
> -		new_state = mc->pdev->dev.can.state;
> -		break;
> +	if (n & PCAN_USB_ERROR_BUS_OFF) {
> +		new_state = CAN_STATE_BUS_OFF;
> +	} else if (n & PCAN_USB_ERROR_BUS_HEAVY) {
> +		new_state = ((mc->pdev->bec.txerr >= 128) ||
> +			     (mc->pdev->bec.rxerr >= 128)) ?
> +				CAN_STATE_ERROR_PASSIVE :
> +				CAN_STATE_ERROR_WARNING;
> +	} else {
> +		new_state = CAN_STATE_ERROR_ACTIVE;
>  	}
>  
> -	mc->pdev->dev.can.state = new_state;
> +	/* handle change of state */
> +	if (new_state != mc->pdev->dev.can.state) {
> +		if (skb) {
> +			enum can_state tx_state =
> +				(mc->pdev->bec.txerr >= mc->pdev->bec.rxerr) ?
> +					new_state : 0;
> +			enum can_state rx_state =
> +				(mc->pdev->bec.txerr <= mc->pdev->bec.rxerr) ?
> +					new_state : 0;
> +
> +			can_change_state(mc->netdev, cf, tx_state, rx_state);

You have to call can_change_state() even if cf is NULL, because it does
the change state and can handle cf == NULL.

> +		}
> +
> +		/* things must be done even in case of OOM */
> +		if (new_state == CAN_STATE_BUS_OFF)
> +			can_bus_off(mc->netdev);
> +	}
>  
> +	if (!skb)
> +		return -ENOMEM;
>  	if (status_len & PCAN_USB_STATUSLEN_TIMESTAMP) {
>  		struct skb_shared_hwtstamps *hwts = skb_hwtstamps(skb);

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]