Re: [PATCH 09/15] can: rx-fifo: Add can_rx_fifo_reset() function

[David Jander <david@protonic.nl>]

On Mon, 03 Nov 2014 12:16:24 +0100
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 10/10/2014 05:46 PM, David Jander wrote:
> > This function needs to be called every time the CAN controller is reset
> > and before interrupts are enabled. Otherwise the irq-offload loop gets
> > out of sync. Detect this and BUG() to the user if he forgot.
> > 
> > Signed-off-by: David Jander <david@protonic.nl>
> > ---
> >  drivers/net/can/dev.c   | 20 ++++++++++++++++++--
> >  include/linux/can/dev.h |  1 +
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> > index 3e1160a..fc35d28 100644
> > --- a/drivers/net/can/dev.c
> > +++ b/drivers/net/can/dev.c
> > @@ -487,8 +487,17 @@ int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo)
> >  		fifo->second_first = true;
> >  	}
> >  
> > -	if (received)
> > -		napi_schedule(&fifo->napi);
> > +	if (received) {
> > +		can_rx_fifo_napi_schedule(fifo);
> > +	} else {
> > +		/*
> > +		 * This should only happen if the CAN conroller was
> > reset, but
> > +		 * can_rx_fifo_reset() was not called. BUG();
> > +		 */
> > +		netdev_warn(fifo->dev, "%s: No messages found,"
> > +			    " RX-FIFO out of sync?\n", __func__);
> > +		BUG();
> 
> Can you replace the BUG() (which crashes the system) by something less
> fatal? E.g. by some of the WARN function [1]? Does it make sense to call
> the fifo_reset() function after this? We'll loose up to a FIFO size
> amount of CAN frames, but the system stays hopefully working.

Ok. We just need to make enough noise, so that the potential driver's author
sees it before release... a WARN*() should do.

> Marc
> 
> [1] http://lxr.free-electrons.com/source/include/asm-generic/bug.h#L84
> 

Best regards,

-- 
David Jander
Protonic Holland.