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

[Javi Merino <javi.merino@arm.com>]

On 07/12/11 20:54, Javi Merino wrote:
> On 07/12/11 10:01, Javi Merino wrote:
>> On 07/12/11 07:52, Kukjin Kim wrote:
>>> Jassi Brar wrote:
>>>> On 29 November 2011 15:23, Javi Merino <javi.merino@arm.com> wrote:
>>>>>>>>>> On Samsung's Exynos4 platform, while testing audio playback with
>>>>>>> i2s
>>>>>>>>>> interface, the above change causes the playback to freeze. The
>>>>>>>>>> _thrd_active(thrd) call always returns '1' and hence _start(thrd)
>>>>>>> is
>>>>>>>>>> not getting called.
>>>>>>>>>
>>>>>>>> Your patch makes the memcpy operation on dmatest.c and net DMA be
>>>>>>> frozen too
>>>>>>>> as well as Samsung audio playback.
>>>>>>>
>>>> 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.  I'll have a look at it tomorrow
> and try to answer my own questions ;)

The problem is that pl330_submit_req() always puts the request in buffer 0 if
both buffers are free.

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,
which makes pl330_chan_ctrl() not start the transaction and hence, the DMA
freezes.

I believe something like the following should fix it:

--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -1301,7 +1301,7 @@ int pl330_submit_req(void *ch_id, struct pl330_req *r)
 		goto xfer_exit;
 	}
 
-	idx = IS_FREE(&thrd->req[0]) ? 0 : 1;
+	idx = IS_FREE(&thrd->req[1 - thrd->lstenq]) ? 1 - thrd->lstenq : thrd->lstenq;
 
 	xs.ccr = ccr;
 	xs.r = r;


I have only tested this with a single-channel single-thread dma test.
I'll send a proper patch to the list once I've done more testing.

Cheers,
Javi