The CAN Error State is determined by the receive and transmit error
counters. The CAN error counters decrease when reception/transmission
is successful, so that a status transition back to the Error Active
status is possible. This transition is not handled by
m_can_handle_state_errors().

Add the missing detection of the Error Active state to
m_can_handle_state_errors() and extend the handling of this state in
m_can_handle_state_change().

Fixes: e0d1f4816f2a ("can: m_can: add Bosch M_CAN controller support")
Fixes: cd0d83eab2e0 ("can: m_can: m_can_handle_state_change(): fix state change")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/m_can/m_can.c | 48 ++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index a51dc0bb8124..b485d2f3d971 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -812,6 +812,10 @@ static int m_can_handle_state_change(struct net_device *dev,
 	u32 timestamp = 0;
 
 	switch (new_state) {
+	case CAN_STATE_ERROR_ACTIVE:
+		/* error active state */
+		cdev->can.state = CAN_STATE_ERROR_ACTIVE;
+		break;
 	case CAN_STATE_ERROR_WARNING:
 		/* error warning state */
 		cdev->can.can_stats.error_warning++;
@@ -841,6 +845,13 @@ static int m_can_handle_state_change(struct net_device *dev,
 	__m_can_get_berr_counter(dev, &bec);
 
 	switch (new_state) {
+	case CAN_STATE_ERROR_ACTIVE:
+		/* error active state */
+		cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
+		cf->data[1] = CAN_ERR_CRTL_ACTIVE;
+		cf->data[6] = bec.txerr;
+		cf->data[7] = bec.rxerr;
+		break;
 	case CAN_STATE_ERROR_WARNING:
 		/* error warning state */
 		cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
@@ -877,30 +888,29 @@ static int m_can_handle_state_change(struct net_device *dev,
 	return 1;
 }
 
+static enum can_state
+m_can_can_state_get_by_psr(const u32 psr)
+{
+	if (psr & PSR_BO)
+		return CAN_STATE_BUS_OFF;
+	if (psr & PSR_EP)
+		return CAN_STATE_ERROR_PASSIVE;
+	if (psr & PSR_EW)
+		return CAN_STATE_ERROR_WARNING;
+
+	return CAN_STATE_ERROR_ACTIVE;
+}
+
 static int m_can_handle_state_errors(struct net_device *dev, u32 psr)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
-	int work_done = 0;
+	enum can_state new_state;
 
-	if (psr & PSR_EW && cdev->can.state != CAN_STATE_ERROR_WARNING) {
-		netdev_dbg(dev, "entered error warning state\n");
-		work_done += m_can_handle_state_change(dev,
-						       CAN_STATE_ERROR_WARNING);
-	}
+	new_state = m_can_can_state_get_by_psr(psr);
+	if (new_state == cdev->can.state)
+		return 0;
 
-	if (psr & PSR_EP && cdev->can.state != CAN_STATE_ERROR_PASSIVE) {
-		netdev_dbg(dev, "entered error passive state\n");
-		work_done += m_can_handle_state_change(dev,
-						       CAN_STATE_ERROR_PASSIVE);
-	}
-
-	if (psr & PSR_BO && cdev->can.state != CAN_STATE_BUS_OFF) {
-		netdev_dbg(dev, "entered error bus off state\n");
-		work_done += m_can_handle_state_change(dev,
-						       CAN_STATE_BUS_OFF);
-	}
-
-	return work_done;
+	return m_can_handle_state_change(dev, new_state);
 }
 
 static void m_can_handle_other_err(struct net_device *dev, u32 irqstatus)

-- 
2.50.1

Hi,

On Tue Aug 12, 2025 at 7:36 PM CEST, Marc Kleine-Budde wrote:
> The CAN Error State is determined by the receive and transmit error
> counters. The CAN error counters decrease when reception/transmission
> is successful, so that a status transition back to the Error Active
> status is possible. This transition is not handled by
> m_can_handle_state_errors().
>
> Add the missing detection of the Error Active state to
> m_can_handle_state_errors() and extend the handling of this state in
> m_can_handle_state_change().
>
> Fixes: e0d1f4816f2a ("can: m_can: add Bosch M_CAN controller support")
> Fixes: cd0d83eab2e0 ("can: m_can: m_can_handle_state_change(): fix state change")
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/can/m_can/m_can.c | 48 ++++++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index a51dc0bb8124..b485d2f3d971 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -812,6 +812,10 @@ static int m_can_handle_state_change(struct net_device *dev,
>  	u32 timestamp = 0;
>  
>  	switch (new_state) {
> +	case CAN_STATE_ERROR_ACTIVE:
> +		/* error active state */

This comment and the one below don't really help IMHO.

> +		cdev->can.state = CAN_STATE_ERROR_ACTIVE;
> +		break;
>  	case CAN_STATE_ERROR_WARNING:
>  		/* error warning state */
>  		cdev->can.can_stats.error_warning++;
> @@ -841,6 +845,13 @@ static int m_can_handle_state_change(struct net_device *dev,
>  	__m_can_get_berr_counter(dev, &bec);
>  
>  	switch (new_state) {
> +	case CAN_STATE_ERROR_ACTIVE:
> +		/* error active state */
> +		cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
> +		cf->data[1] = CAN_ERR_CRTL_ACTIVE;
> +		cf->data[6] = bec.txerr;
> +		cf->data[7] = bec.rxerr;
> +		break;
>  	case CAN_STATE_ERROR_WARNING:
>  		/* error warning state */
>  		cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
> @@ -877,30 +888,29 @@ static int m_can_handle_state_change(struct net_device *dev,
>  	return 1;
>  }
>  
> +static enum can_state
> +m_can_can_state_get_by_psr(const u32 psr)
> +{
> +	if (psr & PSR_BO)
> +		return CAN_STATE_BUS_OFF;
> +	if (psr & PSR_EP)
> +		return CAN_STATE_ERROR_PASSIVE;
> +	if (psr & PSR_EW)
> +		return CAN_STATE_ERROR_WARNING;

Why should m_can_handle_state_errors() should be called if none of these
flags are set?

m_can_handle_state_errors() seems to only be called if IR_ERR_STATE
which is defined as:
  #define IR_ERR_STATE	(IR_BO | IR_EW | IR_EP)

This is the for the interrupt register but will the PSR register bits be
set without the interrupt register being set?

Best
Markus

> +
> +	return CAN_STATE_ERROR_ACTIVE;
> +}
> +
>  static int m_can_handle_state_errors(struct net_device *dev, u32 psr)
>  {
>  	struct m_can_classdev *cdev = netdev_priv(dev);
> -	int work_done = 0;
> +	enum can_state new_state;
>  
> -	if (psr & PSR_EW && cdev->can.state != CAN_STATE_ERROR_WARNING) {
> -		netdev_dbg(dev, "entered error warning state\n");
> -		work_done += m_can_handle_state_change(dev,
> -						       CAN_STATE_ERROR_WARNING);
> -	}
> +	new_state = m_can_can_state_get_by_psr(psr);
> +	if (new_state == cdev->can.state)
> +		return 0;
>  
> -	if (psr & PSR_EP && cdev->can.state != CAN_STATE_ERROR_PASSIVE) {
> -		netdev_dbg(dev, "entered error passive state\n");
> -		work_done += m_can_handle_state_change(dev,
> -						       CAN_STATE_ERROR_PASSIVE);
> -	}
> -
> -	if (psr & PSR_BO && cdev->can.state != CAN_STATE_BUS_OFF) {
> -		netdev_dbg(dev, "entered error bus off state\n");
> -		work_done += m_can_handle_state_change(dev,
> -						       CAN_STATE_BUS_OFF);
> -	}
> -
> -	return work_done;
> +	return m_can_handle_state_change(dev, new_state);
>  }
>  
>  static void m_can_handle_other_err(struct net_device *dev, u32 irqstatus)

On 20.08.2025 10:49:35, Markus Schneider-Pargmann wrote:
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -812,6 +812,10 @@ static int m_can_handle_state_change(struct net_device *dev,
> >  	u32 timestamp = 0;
> >  
> >  	switch (new_state) {
> > +	case CAN_STATE_ERROR_ACTIVE:
> > +		/* error active state */
> 
> This comment and the one below don't really help IMHO.

both removed

> > +		cdev->can.state = CAN_STATE_ERROR_ACTIVE;
> > +		break;
> >  	case CAN_STATE_ERROR_WARNING:
> >  		/* error warning state */
> >  		cdev->can.can_stats.error_warning++;
> > @@ -841,6 +845,13 @@ static int m_can_handle_state_change(struct net_device *dev,
> >  	__m_can_get_berr_counter(dev, &bec);
> >  
> >  	switch (new_state) {
> > +	case CAN_STATE_ERROR_ACTIVE:
> > +		/* error active state */
> > +		cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
> > +		cf->data[1] = CAN_ERR_CRTL_ACTIVE;
> > +		cf->data[6] = bec.txerr;
> > +		cf->data[7] = bec.rxerr;
> > +		break;
> >  	case CAN_STATE_ERROR_WARNING:
> >  		/* error warning state */
> >  		cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
> > @@ -877,30 +888,29 @@ static int m_can_handle_state_change(struct net_device *dev,
> >  	return 1;
> >  }
> >  
> > +static enum can_state
> > +m_can_can_state_get_by_psr(const u32 psr)
> > +{
> > +	if (psr & PSR_BO)
> > +		return CAN_STATE_BUS_OFF;
> > +	if (psr & PSR_EP)
> > +		return CAN_STATE_ERROR_PASSIVE;
> > +	if (psr & PSR_EW)
> > +		return CAN_STATE_ERROR_WARNING;
> 
> Why should m_can_handle_state_errors() should be called if none of these
> flags are set?
> 
> m_can_handle_state_errors() seems to only be called if IR_ERR_STATE
> which is defined as:
>   #define IR_ERR_STATE	(IR_BO | IR_EW | IR_EP)
> 
> This is the for the interrupt register but will the PSR register bits be
> set without the interrupt register being set?

The IR_ERR_STATE will be set if there was a _change_ in corresponding
PSR register's bits.

With successful transmission/reception the corresponding error counter
decrease and if a threshold is crossed the state changes.

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   |

On Wed Aug 20, 2025 at 10:49 AM CEST, Markus Schneider-Pargmann wrote:
> Hi,
>
> On Tue Aug 12, 2025 at 7:36 PM CEST, Marc Kleine-Budde wrote:
>> The CAN Error State is determined by the receive and transmit error
>> counters. The CAN error counters decrease when reception/transmission
>> is successful, so that a status transition back to the Error Active
>> status is possible. This transition is not handled by
>> m_can_handle_state_errors().
>>
>> Add the missing detection of the Error Active state to
>> m_can_handle_state_errors() and extend the handling of this state in
>> m_can_handle_state_change().
>>
>> Fixes: e0d1f4816f2a ("can: m_can: add Bosch M_CAN controller support")
>> Fixes: cd0d83eab2e0 ("can: m_can: m_can_handle_state_change(): fix state change")
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>>  drivers/net/can/m_can/m_can.c | 48 ++++++++++++++++++++++++++-----------------
>>  1 file changed, 29 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index a51dc0bb8124..b485d2f3d971 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -812,6 +812,10 @@ static int m_can_handle_state_change(struct net_device *dev,
>>  	u32 timestamp = 0;
>>  
>>  	switch (new_state) {
>> +	case CAN_STATE_ERROR_ACTIVE:
>> +		/* error active state */
>
> This comment and the one below don't really help IMHO.
>
>> +		cdev->can.state = CAN_STATE_ERROR_ACTIVE;
>> +		break;
>>  	case CAN_STATE_ERROR_WARNING:
>>  		/* error warning state */
>>  		cdev->can.can_stats.error_warning++;
>> @@ -841,6 +845,13 @@ static int m_can_handle_state_change(struct net_device *dev,
>>  	__m_can_get_berr_counter(dev, &bec);
>>  
>>  	switch (new_state) {
>> +	case CAN_STATE_ERROR_ACTIVE:
>> +		/* error active state */
>> +		cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
>> +		cf->data[1] = CAN_ERR_CRTL_ACTIVE;
>> +		cf->data[6] = bec.txerr;
>> +		cf->data[7] = bec.rxerr;
>> +		break;
>>  	case CAN_STATE_ERROR_WARNING:
>>  		/* error warning state */
>>  		cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
>> @@ -877,30 +888,29 @@ static int m_can_handle_state_change(struct net_device *dev,
>>  	return 1;
>>  }
>>  
>> +static enum can_state
>> +m_can_can_state_get_by_psr(const u32 psr)
>> +{
>> +	if (psr & PSR_BO)
>> +		return CAN_STATE_BUS_OFF;
>> +	if (psr & PSR_EP)
>> +		return CAN_STATE_ERROR_PASSIVE;
>> +	if (psr & PSR_EW)
>> +		return CAN_STATE_ERROR_WARNING;
>
> Why should m_can_handle_state_errors() should be called if none of these
> flags are set?
>
> m_can_handle_state_errors() seems to only be called if IR_ERR_STATE
> which is defined as:
>   #define IR_ERR_STATE	(IR_BO | IR_EW | IR_EP)
>
> This is the for the interrupt register but will the PSR register bits be
> set without the interrupt register being set?

After reading the other users of the above function, I do see why this
was added. I am still wondering if there is a way to return to
ERROR_ACTIVE once the errors are cleared from the error register.

Also looking at all the users added for the function above, could you
read the register inside the function? Currently you are adding a
reg variable and a read call for each call to this function.
m_can_handle_state_errors() also doesn't need the psr value with your
refactoring.

Best
Markus

>
> Best
> Markus
>
>> +
>> +	return CAN_STATE_ERROR_ACTIVE;
>> +}
>> +
>>  static int m_can_handle_state_errors(struct net_device *dev, u32 psr)
>>  {
>>  	struct m_can_classdev *cdev = netdev_priv(dev);
>> -	int work_done = 0;
>> +	enum can_state new_state;
>>  
>> -	if (psr & PSR_EW && cdev->can.state != CAN_STATE_ERROR_WARNING) {
>> -		netdev_dbg(dev, "entered error warning state\n");
>> -		work_done += m_can_handle_state_change(dev,
>> -						       CAN_STATE_ERROR_WARNING);
>> -	}
>> +	new_state = m_can_can_state_get_by_psr(psr);
>> +	if (new_state == cdev->can.state)
>> +		return 0;
>>  
>> -	if (psr & PSR_EP && cdev->can.state != CAN_STATE_ERROR_PASSIVE) {
>> -		netdev_dbg(dev, "entered error passive state\n");
>> -		work_done += m_can_handle_state_change(dev,
>> -						       CAN_STATE_ERROR_PASSIVE);
>> -	}
>> -
>> -	if (psr & PSR_BO && cdev->can.state != CAN_STATE_BUS_OFF) {
>> -		netdev_dbg(dev, "entered error bus off state\n");
>> -		work_done += m_can_handle_state_change(dev,
>> -						       CAN_STATE_BUS_OFF);
>> -	}
>> -
>> -	return work_done;
>> +	return m_can_handle_state_change(dev, new_state);
>>  }
>>  
>>  static void m_can_handle_other_err(struct net_device *dev, u32 irqstatus)

On 20.08.2025 11:09:16, Markus Schneider-Pargmann wrote:
> >> --- a/drivers/net/can/m_can/m_can.c
> >> +++ b/drivers/net/can/m_can/m_can.c

[...]

> >> +static enum can_state
> >> +m_can_can_state_get_by_psr(const u32 psr)
> >> +{
> >> +	if (psr & PSR_BO)
> >> +		return CAN_STATE_BUS_OFF;
> >> +	if (psr & PSR_EP)
> >> +		return CAN_STATE_ERROR_PASSIVE;
> >> +	if (psr & PSR_EW)
> >> +		return CAN_STATE_ERROR_WARNING;
> >
> > Why should m_can_handle_state_errors() should be called if none of these
> > flags are set?
> >
> > m_can_handle_state_errors() seems to only be called if IR_ERR_STATE
> > which is defined as:
> >   #define IR_ERR_STATE	(IR_BO | IR_EW | IR_EP)
> >
> > This is the for the interrupt register but will the PSR register bits be
> > set without the interrupt register being set?
> 
> After reading the other users of the above function, I do see why this
> was added. I am still wondering if there is a way to return to
> ERROR_ACTIVE once the errors are cleared from the error register.

Sorry, I don't get what you mean.

> Also looking at all the users added for the function above, could you
> read the register inside the function? Currently you are adding a
> reg variable and a read call for each call to this function.
> m_can_handle_state_errors() also doesn't need the psr value with your
> refactoring.

That makes sense.

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   |

On 09.09.2025 16:28:48, Marc Kleine-Budde wrote:
> On 20.08.2025 11:09:16, Markus Schneider-Pargmann wrote:
> > >> --- a/drivers/net/can/m_can/m_can.c
> > >> +++ b/drivers/net/can/m_can/m_can.c
> 
> [...]
> 
> > >> +static enum can_state
> > >> +m_can_can_state_get_by_psr(const u32 psr)
> > >> +{
> > >> +	if (psr & PSR_BO)
> > >> +		return CAN_STATE_BUS_OFF;
> > >> +	if (psr & PSR_EP)
> > >> +		return CAN_STATE_ERROR_PASSIVE;
> > >> +	if (psr & PSR_EW)
> > >> +		return CAN_STATE_ERROR_WARNING;
> > >
> > > Why should m_can_handle_state_errors() should be called if none of these
> > > flags are set?
> > >
> > > m_can_handle_state_errors() seems to only be called if IR_ERR_STATE
> > > which is defined as:
> > >   #define IR_ERR_STATE	(IR_BO | IR_EW | IR_EP)
> > >
> > > This is the for the interrupt register but will the PSR register bits be
> > > set without the interrupt register being set?
> > 
> > After reading the other users of the above function, I do see why this
> > was added. I am still wondering if there is a way to return to
> > ERROR_ACTIVE once the errors are cleared from the error register.
> 
> Sorry, I don't get what you mean.
> 
> > Also looking at all the users added for the function above, could you
> > read the register inside the function? Currently you are adding a
> > reg variable and a read call for each call to this function.
> > m_can_handle_state_errors() also doesn't need the psr value with your
> > refactoring.
> 
> That makes sense.

I'm also preparing the driver to have error handling for all register
reads...so I'll have to return in case of an error and pass the state
via a pointer.

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   |

On 09.09.2025 16:31:29, Marc Kleine-Budde wrote:
> > > Also looking at all the users added for the function above, could you
> > > read the register inside the function? Currently you are adding a
> > > reg variable and a read call for each call to this function.
> > > m_can_handle_state_errors() also doesn't need the psr value with your
> > > refactoring.
> > 
> > That makes sense.
> 
> I'm also preparing the driver to have error handling for all register
> reads...so I'll have to return in case of an error and pass the state
> via a pointer.

This has to wait until a later patch.

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   |