Skip to contentSkip to conversation

to C_CAN driver in linux kernel

PATCH In review Holger Bechtold 2 participants 2 messages Nov 24, 2013

Subsystems (2)
CAN network drivers CAN network layer
lore
Share & tools
lore

Holger Bechtold

author
#0 lore raw

PATCH 1/1 to C_CAN driver in linux kernel

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

net/can/c_can/c_can.c renamed from net/can/c_can/c_can.c.orig +0 -3
@@ -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 -----------------------------------------------------------

net/can/c_can/c_can.c renamed from net/can/c_can/c_can.c.orig +2 -5
@@ -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 <[email protected]>
maintainer
#1 lore raw

Hello Holger,

Quoted 1 line · Holger Bechtold

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.

Quoted 18 lines

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

Quoted 7 lines

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

[...]

Quoted 10 lines

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

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

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

net/can/c_can/c_can.c static void c_can_do_tx(struct net_devic · line 753

Quoted 10 lines
net/can/c_can/c_can.c renamed from net/can/c_can/c_can.c.orig +2 -5
@@ -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 :)

Quoted 7 lines
net/can/c_can/c_can.c renamed from net/can/c_can/c_can.c.orig +0 -1
 			stats->tx_packets++;
 			c_can_inval_msg_object(dev, 0, msg_obj_no);
 		} else {
 
------------------------------------------------------------------------------
Signed-off-by Holger Bechtold <[email protected]>

regards,
Marc

Signature · 4 lines
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   |