* Re: Possible infinite-loop bug in drivers/net/can/flexcan.c
[not found] ` <52170C82.9070109@motec.com.au>
@ 2013-08-23 9:05 ` Marc Kleine-Budde
0 siblings, 0 replies; only message in thread
From: Marc Kleine-Budde @ 2013-08-23 9:05 UTC (permalink / raw)
To: Tom Evans; +Cc: lw, Scott Thomson, linux-can@vger.kernel.org
[-- 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(®s->iflag1);
> while (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE &&
> work_done < quota) {
> work_done += flexcan_read_frame(dev);
> reg_iflag1 = flexcan_read(®s->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 --]
^ permalink raw reply [flat|nested] only message in thread