1. Patchew
  2. linux
  3. [PATCH can-next v2 0/2] can: sja1000: clean up CAN state handling
  4. View patch

[PATCH can-next v2 2/2] can: sja1000: sja1000_err(): use error counter for error state

Marc Kleine-Budde posted 2 patches 2 weeks, 3 days ago
[PATCH can-next v2 2/2] can: sja1000: sja1000_err(): use error counter for error state
Posted by Marc Kleine-Budde 2 weeks, 3 days ago 
From: Michael Tretter <m.tretter@pengutronix.de>

The CAN controller sends the EPI interrupt whenever it reaches the error
passive status or enters the error active status from the error passive
status.

Instead of keeping track of the controller status in the driver, read the
txerr and rxerr counters and use can_state_get_by_berr_counter() to
determine the state of the CAN controller.

Suggested-by: Achim Baumgartner <abaumgartner@topcon.com>
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/sja1000/sja1000.c | 33 +++++++--------------------------
 1 file changed, 7 insertions(+), 26 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index ae3244b63b65..67fccc15ed20 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -407,10 +407,9 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
 {
 	struct sja1000_priv *priv = netdev_priv(dev);
 	struct net_device_stats *stats = &dev->stats;
+	enum can_state state, rx_state, tx_state;
 	struct can_frame *cf;
 	struct sk_buff *skb;
-	enum can_state state = priv->can.state;
-	enum can_state rx_state, tx_state;
 	struct can_berr_counter bec;
 	uint8_t ecc, alc;
 	int ret = 0;
@@ -418,6 +417,12 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
 	skb = alloc_can_err_skb(dev, &cf);
 
 	sja1000_get_berr_counter(dev, &bec);
+	can_state_get_by_berr_counter(dev, &bec, &tx_state, &rx_state);
+
+	if (status & SR_BS)
+		rx_state = CAN_STATE_BUS_OFF;
+
+	state = max(tx_state, rx_state);
 
 	if (isrc & IRQ_DOI) {
 		/* data overrun interrupt */
@@ -440,18 +445,6 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
 		if (priv->flags & SJA1000_QUIRK_RESET_ON_OVERRUN)
 			ret = IRQ_WAKE_THREAD;
 	}
-
-	if (isrc & IRQ_EI) {
-		/* error warning interrupt */
-		netdev_dbg(dev, "error warning interrupt\n");
-
-		if (status & SR_BS)
-			state = CAN_STATE_BUS_OFF;
-		else if (status & SR_ES)
-			state = CAN_STATE_ERROR_WARNING;
-		else
-			state = CAN_STATE_ERROR_ACTIVE;
-	}
 	if (state != CAN_STATE_BUS_OFF && skb) {
 		cf->can_id |= CAN_ERR_CNT;
 		cf->data[6] = bec.txerr;
@@ -493,15 +486,6 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
 			stats->rx_errors++;
 		}
 	}
-	if (isrc & IRQ_EPI) {
-		/* error passive interrupt */
-		netdev_dbg(dev, "error passive interrupt\n");
-
-		if (state == CAN_STATE_ERROR_PASSIVE)
-			state = CAN_STATE_ERROR_WARNING;
-		else
-			state = CAN_STATE_ERROR_PASSIVE;
-	}
 	if (isrc & IRQ_ALI) {
 		/* arbitration lost interrupt */
 		netdev_dbg(dev, "arbitration lost interrupt\n");
@@ -514,9 +498,6 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
 	}
 
 	if (state != priv->can.state) {
-		tx_state = bec.txerr >= bec.rxerr ? state : 0;
-		rx_state = bec.txerr <= bec.rxerr ? state : 0;
-
 		can_change_state(dev, cf, tx_state, rx_state);
 
 		if(state == CAN_STATE_BUS_OFF)

-- 
2.51.0
Re: [PATCH can-next v2 2/2] can: sja1000: sja1000_err(): use error counter for error state
Posted by Michael Tretter 2 weeks, 3 days ago 
On Fri, 23 Jan 2026 11:16:27 +0100, Marc Kleine-Budde wrote:
> From: Michael Tretter <m.tretter@pengutronix.de>
> 
> The CAN controller sends the EPI interrupt whenever it reaches the error
> passive status or enters the error active status from the error passive
> status.
> 
> Instead of keeping track of the controller status in the driver, read the
> txerr and rxerr counters and use can_state_get_by_berr_counter() to
> determine the state of the CAN controller.
> 
> Suggested-by: Achim Baumgartner <abaumgartner@topcon.com>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/can/sja1000/sja1000.c | 33 +++++++--------------------------
>  1 file changed, 7 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index ae3244b63b65..67fccc15ed20 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -407,10 +407,9 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
>  {
>  	struct sja1000_priv *priv = netdev_priv(dev);
>  	struct net_device_stats *stats = &dev->stats;
> +	enum can_state state, rx_state, tx_state;
>  	struct can_frame *cf;
>  	struct sk_buff *skb;
> -	enum can_state state = priv->can.state;
> -	enum can_state rx_state, tx_state;
>  	struct can_berr_counter bec;
>  	uint8_t ecc, alc;
>  	int ret = 0;
> @@ -418,6 +417,12 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
>  	skb = alloc_can_err_skb(dev, &cf);
>  
>  	sja1000_get_berr_counter(dev, &bec);
> +	can_state_get_by_berr_counter(dev, &bec, &tx_state, &rx_state);
> +
> +	if (status & SR_BS)
> +		rx_state = CAN_STATE_BUS_OFF;

I was wondering why you dropped the check for (status & SR_ES). SR_ES
just indicates that one of the error counters exceeded the warning
limit, which is handled by can_state_get_by_berr_counter(). Thus, it's
fine to drop the check.

Thanks!

Michael

> +
> +	state = max(tx_state, rx_state);
>  
>  	if (isrc & IRQ_DOI) {
>  		/* data overrun interrupt */
> @@ -440,18 +445,6 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
>  		if (priv->flags & SJA1000_QUIRK_RESET_ON_OVERRUN)
>  			ret = IRQ_WAKE_THREAD;
>  	}
> -
> -	if (isrc & IRQ_EI) {
> -		/* error warning interrupt */
> -		netdev_dbg(dev, "error warning interrupt\n");
> -
> -		if (status & SR_BS)
> -			state = CAN_STATE_BUS_OFF;
> -		else if (status & SR_ES)
> -			state = CAN_STATE_ERROR_WARNING;
> -		else
> -			state = CAN_STATE_ERROR_ACTIVE;
> -	}
>  	if (state != CAN_STATE_BUS_OFF && skb) {
>  		cf->can_id |= CAN_ERR_CNT;
>  		cf->data[6] = bec.txerr;
> @@ -493,15 +486,6 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
>  			stats->rx_errors++;
>  		}
>  	}
> -	if (isrc & IRQ_EPI) {
> -		/* error passive interrupt */
> -		netdev_dbg(dev, "error passive interrupt\n");
> -
> -		if (state == CAN_STATE_ERROR_PASSIVE)
> -			state = CAN_STATE_ERROR_WARNING;
> -		else
> -			state = CAN_STATE_ERROR_PASSIVE;
> -	}
>  	if (isrc & IRQ_ALI) {
>  		/* arbitration lost interrupt */
>  		netdev_dbg(dev, "arbitration lost interrupt\n");
> @@ -514,9 +498,6 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
>  	}
>  
>  	if (state != priv->can.state) {
> -		tx_state = bec.txerr >= bec.rxerr ? state : 0;
> -		rx_state = bec.txerr <= bec.rxerr ? state : 0;
> -
>  		can_change_state(dev, cf, tx_state, rx_state);
>  
>  		if(state == CAN_STATE_BUS_OFF)
> 
> -- 
> 2.51.0
> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.