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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3890 bytes --]

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.....

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.

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: 181 bytes --]