netdev - Re: [PATCH] flexcan: fix NAPI for bus errors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4CEBE019.7070401@pengutronix.de>
Date:	Tue, 23 Nov 2010 16:39:05 +0100
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	John Ogness <john.ogness@...utronix.de>
CC:	Wolfgang Grandegger <wg@...ndegger.com>,
	socketcan-core@...ts.berlios.de, netdev@...r.kernel.org,
	kernel@...gutronix.de
Subject: Re: [PATCH] flexcan: fix NAPI for bus errors

Hello,

On 11/23/2010 03:34 PM, John Ogness wrote:
> If bus error reporting is disabled and bus errors occur, the flexcan
> driver will hog the system because it continually re-enables the IRQs
> (work_done = 0). This patch changes the driver to only re-enable the
> IRQs if some work was actually done. This allows the features of NAPI to
> be used for bus errors as well (when bus error reporting is disabled).

Good idea, however the chip has IMHO a bug:

The problem with the error interrupt is, when disabled the can core
doesn't issue any can bus warning or bus passive interrupts.

Can you ensure that you get both error warning and error passive can
error messages with disabled BERR and you patch applied?

> This patch is against Linux next-20101123.
> 
> Signed-off-by: John Ogness <john.ogness@...utronix.de>
> ---
>  drivers/net/can/flexcan.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> --- next-20101123-a/drivers/net/can/flexcan.c
> +++ next-20101123-b/drivers/net/can/flexcan.c
> @@ -535,9 +535,12 @@ static int flexcan_poll(struct napi_stru
>  
>  	if (work_done < quota) {
>  		napi_complete(napi);
> -		/* enable IRQs */
> -		writel(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
> -		writel(priv->reg_ctrl_default, &regs->ctrl);
> +
> +		if (work_done > 0) {
> +			/* enable IRQs */
> +			writel(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
> +			writel(priv->reg_ctrl_default, &regs->ctrl);
> +		}
>  	}
>  
>  	return work_done;

If a bus error occurs and bus error reporting is disabled, work_done
will stay 0, so both the RX and the ERR interrupt stay disabled (which
is done in flexcan_irq). I think we should always enable the RX
interrupt. (However, if the bus is working again the chip can probably
send messages again, so we get a TX-complete IRQ, but this depends on
the application.)

Having a look at flexcan_irq:

> 	/*
> 	 * schedule NAPI in case of:
> 	 * - rx IRQ
> 	 * - state change IRQ
> 	 * - bus error IRQ and bus error reporting is activated
> 	 */

This mean with disabled BERR NAPI should only be scheduled in case of a
RX or a state change interrupt. Both of these interrupts should generate
a can_frame in flexcan_poll and this the work_done shoud be > 0, modulo
out of memory situations.

> 	if ((reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) ||
> 	    (reg_esr & FLEXCAN_ESR_ERR_STATE) ||
> 	    flexcan_has_and_handle_berr(priv, reg_esr)) {
> 		/*
> 		 * The error bits are cleared on read,
> 		 * save them for later use.
> 		 */
> 		priv->reg_esr = reg_esr & FLEXCAN_ESR_ERR_BUS;
> 		writel(FLEXCAN_IFLAG_DEFAULT & ~FLEXCAN_IFLAG_RX_FIFO_AVAILABLE,
> 		       &regs->imask1);
> 		writel(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
> 		       &regs->ctrl);
> 		napi_schedule(&priv->napi);
> 	}

So thinking about the problem I don't see how your patch works. But
there might be a bug in the driver or my logic.

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


Download attachment "signature.asc" of type "application/pgp-signature" (263 bytes)