Re: [PATCH 1/2] can: mcp251x: fix endless loop in interrupt handler if CANINTF_MERRF is set

[Marc Kleine-Budde <mkl@pengutronix.de>]

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

On 10/21/2010 02:08 PM, David Jander wrote:
> On Wednesday 20 October 2010 12:02:25 pm Marc Kleine-Budde wrote:
>> Commit d3cd15657516141adce387810be8cb377baf020e introduced a bug, the
>> interrupt handler would loop endlessly if the CANINTF_MERRF bit is set,
>> because it's not cleared.
>>
>> This patch fixes the problem by masking out the CANINTF_MERRF and all other
>> non interesting bits.
>>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>>  drivers/net/can/mcp251x.c |   14 +++++++++-----
>>  1 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c
>> index c664be2..59f40bc 100644
>> --- a/drivers/net/can/mcp251x.c
>> +++ b/drivers/net/can/mcp251x.c
>> @@ -125,8 +125,9 @@
>>  #  define CANINTF_TX0IF 0x04
>>  #  define CANINTF_RX1IF 0x02
>>  #  define CANINTF_RX0IF 0x01
>> -#  define CANINTF_ERR_TX \
>> -	(CANINTF_ERRIF | CANINTF_TX2IF | CANINTF_TX1IF | CANINTF_TX0IF)
>> +#  define CANINTF_RX (CANINTF_RX0IF | CANINTF_RX1IF)
>> +#  define CANINTF_TX (CANINTF_TX2IF | CANINTF_TX1IF | CANINTF_TX0IF)
>> +#  define CANINTF_ERR (CANINTF_ERRIF)
> 
> Is it just me, or do you still miss MERRF?
> 
>>  #define EFLG	      0x2d
>>  #  define EFLG_EWARN	0x01
>>  #  define EFLG_RXWAR	0x02
>> @@ -790,6 +791,9 @@ static irqreturn_t mcp251x_can_ist(int irq, void
>>  *dev_id)
>>
>>  		mcp251x_read_2regs(spi, CANINTF, &intf, &eflag);
>>
>> +		/* mask out flags we don't care about */
>> +		intf &= CANINTF_RX | CANINTF_TX | CANINTF_ERR;
>> +
>>  		/* receive buffer 0 */
>>  		if (intf & CANINTF_RX0IF) {
>>  			mcp251x_hw_rx(spi, 0);
>> @@ -810,8 +814,8 @@ static irqreturn_t mcp251x_can_ist(int irq, void
>>  *dev_id) }
>>
>>  		/* any error or tx interrupt we need to clear? */
>> -		if (intf & CANINTF_ERR_TX)
>> -			clear_intf |= intf & CANINTF_ERR_TX;
>> +		if (intf & (CANINTF_ERR | CANINTF_TX))
>> +			clear_intf |= intf & (CANINTF_ERR | CANINTF_TX);
>>  		if (clear_intf)
>>  			mcp251x_write_bits(spi, CANINTF, clear_intf, 0x00);
>>
>> @@ -887,7 +891,7 @@ static irqreturn_t mcp251x_can_ist(int irq, void
>>  *dev_id) if (intf == 0)
>>  			break;
>>
>> -		if (intf & (CANINTF_TX2IF | CANINTF_TX1IF | CANINTF_TX0IF)) {
>> +		if (intf & CANINTF_TX) {
>>  			net->stats.tx_packets++;
>>  			net->stats.tx_bytes += priv->tx_len - 1;
>>  			if (priv->tx_len) {
> 
> I've been staring at the changes for quite some time now, but I still don't 
> understand how an "CANINTF" value of "0x80" is ever cleared (MERRF set).
> Granted, you don't loop anymore, but shouldn't that flag be handled properly?

No need to clear the bit, because we don't get en interrupt when it's
set. Because we don't enable it to trigger an interrupt:

> 	/* Enable interrupts */
> 	mcp251x_write_reg(spi, CANINTE,
> 			  CANINTE_ERRIE | CANINTE_TX2IE | CANINTE_TX1IE |
> 			  CANINTE_TX0IE | CANINTE_RX1IE | CANINTE_RX0IE);

cheers, Marc

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]