netdev - Re: [PATCH 1/2] can: mcp251x: fix endless loop in interrupt handler if CANINTF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4CC03100.6050706@pengutronix.de>
Date:	Thu, 21 Oct 2010 14:24:32 +0200
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	David Jander <david.jander@...tonic.nl>
CC:	socketcan-core@...ts.berlios.de, netdev@...r.kernel.org
Subject: Re: [PATCH 1/2] can: mcp251x: fix endless loop in interrupt handler
 if CANINTF_MERRF is set

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@...gutronix.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   |


Download attachment "signature.asc" of type "application/pgp-signature" (263 bytes)