Re: [PATCH] to C_CAN driver in linux kernel

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

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