Re: [PATCH 3/7] can: m_can: m_can_handle_state_errors(): fix CAN state transition to Error Active

["Markus Schneider-Pargmann" <msp@baylibre.com>]

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

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)


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