[PATCH] to C_CAN driver in linux kernel

[PATCH] to C_CAN driver in linux kernel

[Holger Bechtold]

Hello all

I would like to sumbit 2 patches to the C_CAN driver module file c_can.c.
Both bugs were noticed, fixed and tested on platform
   BeagleBoneBlack with AM335x Texas Instruments ARM Microprocessors
using these 2 kernel versions:

--- Angstroem -----------------------------
# uname -a
Linux beaglebone 3.8.13 #1 SMP Thu Sep 12 10:27:06 CEST 2013 armv7l GNU/Linux

# cat /etc/issue
Angstrom v2012.12 - Kernel
-------------------------------------------

and

--- Ubuntu --------------------------------
# uname -a
Linux ubuntu-armhf 3.8.13-bone20 #1 SMP Wed May 29 10:49:26 UTC 2013 ...
   armv7l GNU/Linux

# cat /etc/issue
Ubuntu 12.04.3 LTS
-------------------------------------------

I could not yet test the patches with the newest kernel sources
   linux-3.12.1
but the differences in file c_can.c are quite small and pertain only to
the control of a CAN-BUS activity LED.

If you will accept patches only for the newest kernel sources, please
drop me a line and I will try to test with newest sources.

Best Regards
Holger Bechtold

---------- patch 1 description -----------------------------------------------

The for-loop cycling through the message objects forming the reception FIFO
was left prematurely when it reached the last object (marked IF_MCONT_EOB).
If a received message was stored in this object, it was not handled
properly, leaving the IF_MCONT_INTPND condition pending. This led to a
processor deadlock and a kernel panic.

---------- patch 1 -----------------------------------------------------------

--- drivers/net/can/c_can/c_can.c.orig	2013-11-20 16:57:44.655806758 +0100
+++ drivers/net/can/c_can/c_can.c	2013-11-20 17:06:31.915937041 +0100
@@ -810,9 +807,6 @@ static int c_can_do_rx_poll(struct net_d
 			msg_ctrl_save = priv->read_reg(priv,
 					C_CAN_IFACE(MSGCTRL_REG, 0));
 
-			if (msg_ctrl_save & IF_MCONT_EOB)
-				return num_rx_pkts;
-
 			if (msg_ctrl_save & IF_MCONT_MSGLST) {
 				c_can_handle_lost_msg_obj(dev, 0, msg_obj);
 				num_rx_pkts++;

---------- patch 2 description -----------------------------------------------

The number of bytes transmitted was not updated correctly. Thus programs
like 'ifconfig' showed wrong transmit byte counts.

---------- patch 2 -----------------------------------------------------------

--- drivers/net/can/c_can/c_can.c.orig	2013-11-20 16:57:44.655806758 +0100
+++ drivers/net/can/c_can/c_can.c	2013-11-20 17:06:31.915937041 +0100
@@ -749,11 +749,8 @@ static void c_can_do_tx(struct net_devic
 		msg_obj_no = get_tx_echo_msg_obj(priv);
 		val = c_can_read_reg32(priv, C_CAN_TXRQST1_REG);
 		if (!(val & (1 << (msg_obj_no - 1)))) {
-			can_get_echo_skb(dev,
-					msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
-			stats->tx_bytes += priv->read_reg(priv,
-					C_CAN_IFACE(MSGCTRL_REG, 0))
-					& IF_MCONT_DLC_MASK;
+			stats->tx_bytes += can_get_echo_skb(dev, msg_obj_no -
+				C_CAN_MSG_OBJ_TX_FIRST);
 			stats->tx_packets++;
 			c_can_inval_msg_object(dev, 0, msg_obj_no);
 		} else {

------------------------------------------------------------------------------

Signed-off-by: Holger Bechtold <Holger.Bechtold@gmx.net>

Re: [PATCH] to C_CAN driver in linux kernel

[Marc Kleine-Budde]

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

Hello Holger,

On 11/24/2013 01:15 PM, Holger Bechtold wrote:
> I would like to sumbit 2 patches to the C_CAN driver module file c_can.c.

Thanks for your contribution. Please keep it one patch per Mail, for
further patches please have a look at Documentation/SubmittingPatches in
the Kernel tree.

> Both bugs were noticed, fixed and tested on platform
>    BeagleBoneBlack with AM335x Texas Instruments ARM Microprocessors
> using these 2 kernel versions:

> Linux beaglebone 3.8.13 #1 SMP Thu Sep 12 10:27:06 CEST 2013 armv7l GNU/Linux
> Linux ubuntu-armhf 3.8.13-bone20 #1 SMP Wed May 29 10:49:26 UTC 2013 ...

> I could not yet test the patches with the newest kernel sources
>    linux-3.12.1
> but the differences in file c_can.c are quite small and pertain only to
> the control of a CAN-BUS activity LED.
> 
> If you will accept patches only for the newest kernel sources, please
> drop me a line and I will try to test with newest sources.
> 
> Best Regards
> Holger Bechtold
> 
> ---------- patch 1 description -----------------------------------------------
> 
> The for-loop cycling through the message objects forming the reception FIFO
> was left prematurely when it reached the last object (marked IF_MCONT_EOB).
> If a received message was stored in this object, it was not handled
> properly, leaving the IF_MCONT_INTPND condition pending. This led to a
> processor deadlock and a kernel panic.

This issue has been fixed by Markus Pargmann and is already upstream in

    5d0f801 can: c_can: Fix RX message handling, handle lost message
before EOB

Which is included in v3.13-rc1~175^2~1^2~1, this means v3.13-rc1 was the
first release containing this patch. It's not in v3.12.1 and wasn't
queued for -stable (i.e. v3.12.2) yet, but should be. I've triggered
David about the status of the patch.

[...]

> ---------- patch 2 description -----------------------------------------------
> 
> The number of bytes transmitted was not updated correctly. Thus programs
> like 'ifconfig' showed wrong transmit byte counts.
> 
> ---------- patch 2 -----------------------------------------------------------
> 
> --- drivers/net/can/c_can/c_can.c.orig	2013-11-20 16:57:44.655806758 +0100
> +++ drivers/net/can/c_can/c_can.c	2013-11-20 17:06:31.915937041 +0100
> @@ -749,11 +749,8 @@ static void c_can_do_tx(struct net_devic
>  		msg_obj_no = get_tx_echo_msg_obj(priv);
>  		val = c_can_read_reg32(priv, C_CAN_TXRQST1_REG);
>  		if (!(val & (1 << (msg_obj_no - 1)))) {
> -			can_get_echo_skb(dev,
> -					msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
> -			stats->tx_bytes += priv->read_reg(priv,
> -					C_CAN_IFACE(MSGCTRL_REG, 0))
> -					& IF_MCONT_DLC_MASK;
> +			stats->tx_bytes += can_get_echo_skb(dev, msg_obj_no -
> +				C_CAN_MSG_OBJ_TX_FIRST);

There is a problem with the return value of can_get_echo_skb(), I will
return 0 if the echoing has been switched off. However the flexcan
driver uses this function, too. Maybe it's time to cleanup the echo
handling a bit.

What's wrong with reading the number of bytes from the hardware? Can you
fix the read instead? (Or even better, fix the echoing :)

>  			stats->tx_packets++;
>  			c_can_inval_msg_object(dev, 0, msg_obj_no);
>  		} else {
> 
> ------------------------------------------------------------------------------
> 
> Signed-off-by: Holger Bechtold <Holger.Bechtold@gmx.net>

regards,
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: 259 bytes --]