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

On 10/08/2014 12:36 PM, Alexander Stein wrote:
[...]
>>> 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

ACK, this is why I suggested the "non pending MB in front of old next"
check.

> after old_next coul have been written meanwhile. That's why I hate
> those MB interfaces and prefer a real FIFO.

If you have a FIFO, which is big enough....(for no various definitions
of big enough) of course a FIFO is preferred.

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