[PATCH 0/2] can: mcp251x: fix error handling and error frames bug

[PATCH 0/2] can: mcp251x: fix error handling and error frames bug

[Marc Kleine-Budde]

Hello,

this series of patches fixes two bugs, one introduced by me in the other 
mcp251x series.

Please test, especially the error handling, i.e. wrong bitrate, shortcut in
CAN bus. I'm also interested in tests with non i.MX SPI can cores.

cheers, Marc

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

[Marc Kleine-Budde]

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)
 #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) {
-- 
1.7.0.4

[PATCH 2/2] can: mcp251x: fix generation of error frames

[Marc Kleine-Budde]

The function "mcp251x_error_skb" is used to generate error frames.
They are identified by the "CAN_ERR_FLAG" in can_id. The function
overwrites the can_id so that the frames show up as normal frames instead
of error frames.

This patch fixes the problem by or'ing the can_id instead of overwriting it.

Signed-off-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Tested-by: Jargalan Nermunkh <jargalan.nermunkh-wZX4cNJlHJ2sVWG7oymsAA@public.gmane.org>
---
 drivers/net/can/mcp251x.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c
index 59f40bc..6aadc3e 100644
--- a/drivers/net/can/mcp251x.c
+++ b/drivers/net/can/mcp251x.c
@@ -705,7 +705,7 @@ static void mcp251x_error_skb(struct net_device *net, int can_id, int data1)
 
 	skb = alloc_can_err_skb(net, &frame);
 	if (skb) {
-		frame->can_id = can_id;
+		frame->can_id |= can_id;
 		frame->data[1] = data1;
 		netif_rx_ni(skb);
 	} else {
-- 
1.7.0.4

Re: [PATCH 0/2] can: mcp251x: fix error handling and error frames bug

[Wolfgang Grandegger]

On 10/20/2010 12:02 PM, Marc Kleine-Budde wrote:
> Hello,
> 
> this series of patches fixes two bugs, one introduced by me in the other 
> mcp251x series.
> 
> Please test, especially the error handling, i.e. wrong bitrate, shortcut in
> CAN bus. I'm also interested in tests with non i.MX SPI can cores.

You can add my Acked-by but more Tested-by's are definitely more useful.
Unfortunately, I do not have that hardware at hand.

Wolfgang.

Re: [PATCH 0/2] can: mcp251x: fix error handling and error frames bug

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 798 bytes --]

On 10/20/2010 12:11 PM, Wolfgang Grandegger wrote:
>> this series of patches fixes two bugs, one introduced by me in the other 
>> mcp251x series.
>>
>> Please test, especially the error handling, i.e. wrong bitrate, shortcut in
>> CAN bus. I'm also interested in tests with non i.MX SPI can cores.
> 
> You can add my Acked-by but more Tested-by's are definitely more useful.

Thanks and ACK :)

> Unfortunately, I do not have that hardware at hand.

I'm sure Jargalan is going to test it.

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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

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

[David Miller]

From: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Date: Wed, 20 Oct 2010 12:02:25 +0200

> 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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Applied.

Re: [PATCH 2/2] can: mcp251x: fix generation of error frames

[David Miller]

From: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Date: Wed, 20 Oct 2010 12:02:26 +0200

> The function "mcp251x_error_skb" is used to generate error frames.
> They are identified by the "CAN_ERR_FLAG" in can_id. The function
> overwrites the can_id so that the frames show up as normal frames instead
> of error frames.
> 
> This patch fixes the problem by or'ing the can_id instead of overwriting it.
> 
> Signed-off-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Tested-by: Jargalan Nermunkh <jargalan.nermunkh-wZX4cNJlHJ2sVWG7oymsAA@public.gmane.org>

Applied.

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

[David Jander]

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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  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?

Best regards,

-- 
David Jander
Protonic Holland.

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

[Marc Kleine-Budde]

[-- 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 --]