Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[David Jander <david@protonic.nl>]

Dear Marc,

On Mon, 06 Oct 2014 12:00:03 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 10/06/2014 09:28 AM, David Jander wrote:
> >>> 2.- Since the problem addressed by my patch to at91_can is very similar,
> >>> what about solving these problems in the SocketCAN framework (if that is
> >>> possible)?
> >>
> >> Have you had a look at my rx-fifo branch in
> >> https://gitorious.org/linux-can/linux-can-next? It already tries to
> >> abstract the simulation of the FIFO with the linear mailboxes.
> > 
> > Looks interesting. I think it is a good idea to do this in dev.c, since
> > there are obviously more CAN drivers that can use this. Unfortunately it
> > seems you are still pretending the napi-poll handler to call
> > can_rx_fifo_poll(). Wouldn't it be better to just empty all MBs into a
> > circular buffer or kfifo from the interrupt handler instead?
> 
> Yes probably, I started the rx-fifo patch before you came up with that idea.
> 
> > I still don't understand the results Alexander is getting, though....
> > 
> > What are you going to do with the rx-fifo work? Do you recommend to base my
> > patch on that? In that case, calling can_rx_fifo_poll() from the interrupt
> > handler will look a little awkward... but it should work. Or should I
> > propose an extension to rx-fifo?
> 
> My plans, or rather the points that need to be addressed for the rx-fifo
> are:
> - improve to work with more than 32 mailboxes. 64 are probably enough
>   for everybody :)
> - make it work with the flexcan linear buffers
> - make it work with the ti_hecc driver
> - add option or convert to run from interrupt handler and copy to
>   kfifo/cyclic buffer/...

Can you lend you brain for a sec on this problem... I think I discovered a
race condition in your rx-fifo code, but it might just be me not understanding
it correctly....

	do {
		pending = fifo->read_pending(fifo);
		pending &= fifo->active;

		if (!(pending & BIT_ULL(fifo->next))) {
			/*
			 * Wrap around only if:
			 * - we are in the upper group and
			 * - there is a CAN frame in the first mailbox
			 *   of the lower group.
			 */
			if (can_rx_fifo_ge(fifo, fifo->next, fifo->high_first) &&
			    (pending & BIT_ULL(fifo->low_first))) {
				fifo->next = fifo->low_first;
				fifo->active |= fifo->mask_high;
				fifo->mailbox_enable_mask(fifo, fifo->mask_high);
			} else {
				break;
			}
		}

In this piece of code, suppose that fifo->next is in the upper half and a
message is being written to the MB it is pointing at, but it is still not
pending. The lower half has already been enabled and filled up completely (due
to latency). In that case, fifo-next will jump back to fifo->low_first and
leave a lonely filled MB in the middle of the upper half, that will trigger
and infinite loop between IRQ and can_rx_fifo_poll(). The interrupt will never
get cleared again.
I know this is an extreme case of latency being so high as to fill more than
the complete lower half, but if it strikes, it results in a lock-up. Am I
right, or did I screw up somewhere?

Thanks.

Best regards,

-- 
David Jander
Protonic Holland.