[PATCH v2] ARM: pl330: Fix a race condition

[Jassi Brar]

On Fri, Dec 9, 2011 at 7:11 PM, Javi Merino <javi.merino at arm.com> wrote:
> On 09/12/11 13:04, Jassi Brar wrote:
>> Hi Javi,
>>
>> On 9 December 2011 17:28, Javi Merino <javi.merino at arm.com> wrote:
>>>
>>>>>>> Javi, could you please check if you too get the memcpy failure with
>>>>>>> dmatest ?
>>>>>>
>>>> Ok, I think I've just reproduced it in my end with the kernel's dmatest
>>>> module.  After the first transaction it looks like the dma test wasn't
>>>> able to issue any more transactions.
>>>>
>>> If you submit a transaction, it finishes and there's nothing else to run,
>>> pl330_update() calls _start() but there is nothing to send.  This is all
>>> right.  Then, if another transaction is submitted, pl330_submit_req() will
>>> put it in buffer 0 again.  This time, the PC of the DMA is in the last
>>> instruction of buffer 0 (the DMAEND of the *previous* transaction that
>>> finished long ago) so _thrd_active() thinks that this buffer is active,
>>>
>> Many thanks for the in-depth analysis.
>>
>> Though before the PC check, the IS_FREE() should return true since
>> pl330_update() does MARK_FREE()
>>
>> Could you please check if the client's callback function called
>> successfully for the
>> first submitted transfer ?
>
> Yes, it calls MARK_FREE() and indeed in pl330_update(), _thrd_active()
> returns 0.  The problem comes when, afterwards, pl330_submit_req()
> introduces a new request and chooses the same buffer. Then, IS_FREE()
> returns false (obviously) but the PC of the DMA is at the end of the
> buffer, so _thrd_active() claims that it is active so pl330_chan_ctrl()
> doesn't start it.
>
OK, I see what you mean.
We need to be able to differentiate between 'programmed' state
and 'running' state.
So instead of employing  _state() or another marker, we'd rather
alternate between buff 0 & 1 as a workaround.

That is, I am ok with your following fix.

-       idx = IS_FREE(&thrd->req[0]) ? 0 : 1;
+       idx = IS_FREE(&thrd->req[1 - thrd->lstenq]) ? 1 - thrd->lstenq
: thrd->lstenq;

Thanks.