Re: Possible infinite-loop bug in drivers/net/can/flexcan.c

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

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

On 08/23/2013 09:17 AM, Tom Evans wrote:
> I don't know where best to report this, so I'm sending it upstream to
> our Kernel supplier and to the last author in the file header.

Use the official linux-can mailinglist (Cc'ed).

> I'm changing the FlexCAN driver on our i.MX53 product as the kernel
> can't guarantee the NAPI service time and it is dropping CAN packets.
> I've been measuring 19 MILLISECONDS between the CAN interrupt and NAPI
> reading, so it is dropping a huge number of packets.
> 
> Anyway, while rewriting, I think I spotted an "infinite loop" on a
> buffer allocation failure:
> 
> /*
>  * flexcan.c - FLEXCAN CAN controller driver
>  *
>  * Copyright (c) 2005-2006 Varma Electronics Oy
>  * Copyright (c) 2009 Sascha Hauer, Pengutronix
>  * Copyright (c) 2010 Marc Kleine-Budde, Pengutronix
>  *
>  * Based on code originally by Andrey Volkov <avolkov@varma-el.com>
> ...
> static int flexcan_poll(struct napi_struct *napi, int quota)
> {
>         ...
>         reg_iflag1 = flexcan_read(&regs->iflag1);
>         while (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE &&
>                work_done < quota) {
>                 work_done += flexcan_read_frame(dev);
>                 reg_iflag1 = flexcan_read(&regs->iflag1);
>         }
> 
> where...
> 
> static int flexcan_read_frame(struct net_device *dev)
> {
>         struct net_device_stats *stats = &dev->stats;
>         struct can_frame *cf;
>         struct sk_buff *skb;
> 
>         skb = alloc_can_skb(dev, &cf);
>         if (unlikely(!skb)) {
>                 stats->rx_dropped++;
>                 return 0;
>         }
> 
> So if alloc_can_skb() returns NULL, it fails to read the FlexCAN
> registers, doesn't clear FLEXCAN_IFLAG_RX_FIFO_AVAILABLE and returns ZERO.
> 
> The "while" loop in flexcan_poll() only exits when the FlexCAN FIFO is
> empty or when "work_done" counts out.
> 
> I've put some test code in flexcan_read_frame() to stop allocating skbs
> after 10,000 calls, and after than many CAN messages the whole thing
> resets as the watchdog resets it.
> 
> The simplest fix is to have flexcan_read_frame() return "1" on drop.

How does the driver behave if flexcan_read_frame() returns "1" here?

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