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

[Alexander Stein <alexander.stein@systec-electronic.com>]

On Wednesday 08 October 2014 11:56:04, Marc Kleine-Budde wrote:
> On 10/08/2014 11:08 AM, David Jander wrote:
> > 
> > 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?
> 
> Correct analysis :( At least it shows it makes sense to have this code
> in a central place.....

I didn't reviewed that piece of code, I just read David's description. Well, I actually saw this scenario on pch_can where the rx mailboxes are split in lower and upper half. The current upper MB was empty and rx_poll left handling MBs and freed the lower MB. Meanwhile a frame was about beeing inserted in the current upper MB. Upon next interrupt reception started on lower MBs until eventually the remained frame in upper MB was read. But at this time the order is messed up. There was no lockup, because the interrupt signaling worked a bit different.

> If we handle the low_first mailbox, we might have to check if the "old"
> next, or better, if any of the mailboxes >= old_next are pending *and*
> there is a non pending mailbox < "old" next. This should be possible
> with one or two clever bitmasks.

If we detect that (all) MBs before the old one are pending again, we even can't ensure proper CAN frame order. All MBs below and even after old_next coul have been written meanwhile. That's why I hate those MB interfaces and prefer a real FIFO.
Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: alexander.stein@systec-electronic.com
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082