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

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

[Thomas Abraham]

Hi Javi,

On 6 October 2011 05:10, Javi Merino <javi.merino@arm.com> wrote:
> If two requests have been submitted and one of them is running, if you
> call pl330_chan_ctrl(ch_id, PL330_OP_START), there's a window of time
> between the spin_lock_irqsave() and the _state() check in which the
> running transaction may finish.  In that case, we don't receive the
> interrupt (because they are disabled), but _start() sees that the DMA
> is stopped, so it starts it.  The problem is that it sends the
> transaction that has just finished again, because pl330_update()
> hasn't mark it as done yet.
>
> This patch fixes this race condition by not calling _start() if the
> DMA is already executing transactions.  When interrupts are reenabled,
> pl330_update() will call _start().
>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> Acked-by: Jassi Brar <jassi.brar@samsung.com>
> ---
>  arch/arm/common/pl330.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
> index 97912fa..7129cfb 100644
> --- a/arch/arm/common/pl330.c
> +++ b/arch/arm/common/pl330.c
> @@ -1546,7 +1546,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
>
>                /* Start the next */
>        case PL330_OP_START:
> -               if (!_start(thrd))
> +               if (!_thrd_active(thrd) && !_start(thrd))
>                        ret = -EIO;
>                break;

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.

I have not debugged this much but if there is something that I am
missing, please let me know. Meanwhile, I will continue checking on
this.

Thanks,
Thomas.


>
> --
> 1.7.0.4
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

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

[Javi Merino]

On 05/11/11 19:05, Thomas Abraham wrote:
> Hi Javi,
> 
> On 6 October 2011 05:10, Javi Merino <javi.merino@arm.com> wrote:
>> If two requests have been submitted and one of them is running, if you
>> call pl330_chan_ctrl(ch_id, PL330_OP_START), there's a window of time
>> between the spin_lock_irqsave() and the _state() check in which the
>> running transaction may finish.  In that case, we don't receive the
>> interrupt (because they are disabled), but _start() sees that the DMA
>> is stopped, so it starts it.  The problem is that it sends the
>> transaction that has just finished again, because pl330_update()
>> hasn't mark it as done yet.
>>
>> This patch fixes this race condition by not calling _start() if the
>> DMA is already executing transactions.  When interrupts are reenabled,
>> pl330_update() will call _start().
>>
>> Signed-off-by: Javi Merino <javi.merino@arm.com>
>> Acked-by: Jassi Brar <jassi.brar@samsung.com>
>> ---
>>  arch/arm/common/pl330.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
>> index 97912fa..7129cfb 100644
>> --- a/arch/arm/common/pl330.c
>> +++ b/arch/arm/common/pl330.c
>> @@ -1546,7 +1546,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
>>
>>                /* Start the next */
>>        case PL330_OP_START:
>> -               if (!_start(thrd))
>> +               if (!_thrd_active(thrd) && !_start(thrd))
>>                        ret = -EIO;
>>                break;
> 
> 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.

If _thrd_active(thrd) returns '1', that means there is an active
transfer still running or, if it has finished, you haven't called
pl330_update() to acknowledge that.  pl330_update() calls _start() as
soon as it can.

drivers/dma/pl330.c registers the irq handler in pl330_probe(), so when
the transaction finishes, pl330_update() should clear it and call
_start().  If there is any outstanding transaction, it should start
straight away. If there isn't, it would mark the channel as free, so
_thrd_active() should return '0'.  If _thrd_active() is still '1', then
something has gone wrong in the way.

Does this shed some light?

Cheers,
Javi

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

[Thomas Abraham]

On 7 November 2011 16:18, Javi Merino <javi.merino@arm.com> wrote:
> On 05/11/11 19:05, Thomas Abraham wrote:
>> Hi Javi,
>>
>> On 6 October 2011 05:10, Javi Merino <javi.merino@arm.com> wrote:
>>> If two requests have been submitted and one of them is running, if you
>>> call pl330_chan_ctrl(ch_id, PL330_OP_START), there's a window of time
>>> between the spin_lock_irqsave() and the _state() check in which the
>>> running transaction may finish.  In that case, we don't receive the
>>> interrupt (because they are disabled), but _start() sees that the DMA
>>> is stopped, so it starts it.  The problem is that it sends the
>>> transaction that has just finished again, because pl330_update()
>>> hasn't mark it as done yet.
>>>
>>> This patch fixes this race condition by not calling _start() if the
>>> DMA is already executing transactions.  When interrupts are reenabled,
>>> pl330_update() will call _start().
>>>
>>> Signed-off-by: Javi Merino <javi.merino@arm.com>
>>> Acked-by: Jassi Brar <jassi.brar@samsung.com>
>>> ---
>>>  arch/arm/common/pl330.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
>>> index 97912fa..7129cfb 100644
>>> --- a/arch/arm/common/pl330.c
>>> +++ b/arch/arm/common/pl330.c
>>> @@ -1546,7 +1546,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
>>>
>>>                /* Start the next */
>>>        case PL330_OP_START:
>>> -               if (!_start(thrd))
>>> +               if (!_thrd_active(thrd) && !_start(thrd))
>>>                        ret = -EIO;
>>>                break;
>>
>> 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.
>
> If _thrd_active(thrd) returns '1', that means there is an active
> transfer still running or, if it has finished, you haven't called
> pl330_update() to acknowledge that.  pl330_update() calls _start() as
> soon as it can.
>
> drivers/dma/pl330.c registers the irq handler in pl330_probe(), so when
> the transaction finishes, pl330_update() should clear it and call
> _start().  If there is any outstanding transaction, it should start
> straight away. If there isn't, it would mark the channel as free, so
> _thrd_active() should return '0'.  If _thrd_active() is still '1', then
> something has gone wrong in the way.
>
> Does this shed some light?

Thanks for the detailed explanation. I will check again on the lines
you have mentioned above. Also, the i2s audio playback on exynos had
been working prior to this change. So that might imply that
pl330_update was working as you have mentioned. Anyway, I will check
again.

Thanks,
Thomas.

>
> Cheers,
> Javi
>
>

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

[Boojin Kim]

Javi Merino 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.
>
> If _thrd_active(thrd) returns '1', that means there is an active
> transfer still running or, if it has finished, you haven't called
> pl330_update() to acknowledge that.  pl330_update() calls _start() as
> soon as it can.
>
> drivers/dma/pl330.c registers the irq handler in pl330_probe(), so
> when
> the transaction finishes, pl330_update() should clear it and call
> _start().  If there is any outstanding transaction, it should start
> straight away. If there isn't, it would mark the channel as free, so
> _thrd_active() should return '0'.  If _thrd_active() is still '1', then
> something has gone wrong in the way.
>
> Does this shed some light?
>

Your patch makes the memcpy operation on dmatest.c and net DMA be frozen too
as well as Samsung audio playback.
With our patch, common DMA memcpy sequence may be changed.
I think..
it's needed to find the other way not to bring these side effects.

> Cheers,
> Javi
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Javi Merino]

On 28/11/11 08:23, Boojin Kim wrote:
> Javi Merino 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.
>>
>> If _thrd_active(thrd) returns '1', that means there is an active
>> transfer still running or, if it has finished, you haven't called
>> pl330_update() to acknowledge that.  pl330_update() calls _start() as
>> soon as it can.
>>
>> drivers/dma/pl330.c registers the irq handler in pl330_probe(), so
>> when
>> the transaction finishes, pl330_update() should clear it and call
>> _start().  If there is any outstanding transaction, it should start
>> straight away. If there isn't, it would mark the channel as free, so
>> _thrd_active() should return '0'.  If _thrd_active() is still '1', then
>> something has gone wrong in the way.
>>
>> Does this shed some light?
>>
> 
> Your patch makes the memcpy operation on dmatest.c and net DMA be frozen too
> as well as Samsung audio playback.

Is the IRQ correctly registered in drivers/dma/pl330.c:pl330_probe()?
Do you get interrupts when the transfer finish?

The only way I can think of that
ee3f615819404a9438b2dd01b7a39f276d2737f2 can break break something is
that you aren't calling pl330_update() at all.

> With our patch, common DMA memcpy sequence may be changed.

Which one is "our patch"?  I'm sorry, I'm not subscribed to any of the
lists so I may have missed something.

Cheers,
Javi

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

[Boojin Kim]

Javi Merino 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.
> >>
> >> If _thrd_active(thrd) returns '1', that means there is an active
> >> transfer still running or, if it has finished, you haven't called
> >> pl330_update() to acknowledge that.  pl330_update() calls _start()
> as
> >> soon as it can.
> >>
> >> drivers/dma/pl330.c registers the irq handler in pl330_probe(), so
> >> when
> >> the transaction finishes, pl330_update() should clear it and call
> >> _start().  If there is any outstanding transaction, it should start
> >> straight away. If there isn't, it would mark the channel as free,
> so
> >> _thrd_active() should return '0'.  If _thrd_active() is still '1',
> then
> >> something has gone wrong in the way.
> >>
> >> Does this shed some light?
> >>
> >
> > Your patch makes the memcpy operation on dmatest.c and net DMA be
> frozen too
> > as well as Samsung audio playback.
>
> Is the IRQ correctly registered in drivers/dma/pl330.c:pl330_probe()?
> Do you get interrupts when the transfer finish?
Sure. IRQ works well.

>
> The only way I can think of that
> ee3f615819404a9438b2dd01b7a39f276d2737f2 can break break something is
> that you aren't calling pl330_update() at all.
>
> > With our patch, common DMA memcpy sequence may be changed.
>
> Which one is "our patch"?  I'm sorry, I'm not subscribed to any of the
> lists so I may have missed something.

Sorry to make you confused.
I mean.. "With your patch, common DMA memcpy sequence may be changed."
Your patch brings the side effect that freezes dma transmits.

>
> Cheers,
> Javi
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Javi Merino]

On 29/11/11 03:41, Boojin Kim wrote:
> Javi Merino 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.
>>>>
>>>> If _thrd_active(thrd) returns '1', that means there is an active
>>>> transfer still running or, if it has finished, you haven't called
>>>> pl330_update() to acknowledge that.  pl330_update() calls _start()
>> as
>>>> soon as it can.
>>>>
>>>> drivers/dma/pl330.c registers the irq handler in pl330_probe(), so
>>>> when
>>>> the transaction finishes, pl330_update() should clear it and call
>>>> _start().  If there is any outstanding transaction, it should start
>>>> straight away. If there isn't, it would mark the channel as free,
>> so
>>>> _thrd_active() should return '0'.  If _thrd_active() is still '1',
>> then
>>>> something has gone wrong in the way.
>>>>
>>>> Does this shed some light?
>>>>
>>>
>>> Your patch makes the memcpy operation on dmatest.c and net DMA be
>> frozen too
>>> as well as Samsung audio playback.
>>
>> Is the IRQ correctly registered in drivers/dma/pl330.c:pl330_probe()?
>> Do you get interrupts when the transfer finish?
> Sure. IRQ works well.

Ok, so can you check if pl330_update() is correctly marking the request
as free?  Do you know if there is another request in the queue when that
happens?

Thomas, you said in a previous email that _thrd_active() always returned
'1'.  Was that after a request in req[0] finished?
  - If so, can you check that MARK_FREE was actually called for that
    request in pl330_update()?
  - If it was after a request in req[1] finished and there was a
    request already waiting in req[0], can you debug why _start()
    didn't activate it.

Cheers,
Javi

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

[Jassi Brar]

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.
>>>>>
>>>>> If _thrd_active(thrd) returns '1', that means there is an active
>>>>> transfer still running or, if it has finished, you haven't called
>>>>> pl330_update() to acknowledge that.  pl330_update() calls _start()
>>> as
>>>>> soon as it can.
>>>>>
>>>>> drivers/dma/pl330.c registers the irq handler in pl330_probe(), so
>>>>> when
>>>>> the transaction finishes, pl330_update() should clear it and call
>>>>> _start().  If there is any outstanding transaction, it should start
>>>>> straight away. If there isn't, it would mark the channel as free,
>>> so
>>>>> _thrd_active() should return '0'.  If _thrd_active() is still '1',
>>> then
>>>>> something has gone wrong in the way.
>>>>>
>>>>> Does this shed some light?
>>>>>
>>>>
>>>> Your patch makes the memcpy operation on dmatest.c and net DMA be
>>> frozen too
>>>> as well as Samsung audio playback.
>>>
>>> Is the IRQ correctly registered in drivers/dma/pl330.c:pl330_probe()?
>>> Do you get interrupts when the transfer finish?
>> Sure. IRQ works well.
>
> Ok, so can you check if pl330_update() is correctly marking the request
> as free?  Do you know if there is another request in the queue when that
> happens?
>
> Thomas, you said in a previous email that _thrd_active() always returned
> '1'.  Was that after a request in req[0] finished?
>  - If so, can you check that MARK_FREE was actually called for that
>    request in pl330_update()?
>  - If it was after a request in req[1] finished and there was a
>    request already waiting in req[0], can you debug why _start()
>    didn't activate it.
>
Javi, could you please check if you too get the memcpy failure with dmatest ?
Thanks.

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

[Kukjin Kim]

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.
> >>>>>
> >>>>> If _thrd_active(thrd) returns '1', that means there is an active
> >>>>> transfer still running or, if it has finished, you haven't called
> >>>>> pl330_update() to acknowledge that.  pl330_update() calls _start()
> >>> as
> >>>>> soon as it can.
> >>>>>
> >>>>> drivers/dma/pl330.c registers the irq handler in pl330_probe(), so
> >>>>> when
> >>>>> the transaction finishes, pl330_update() should clear it and call
> >>>>> _start().  If there is any outstanding transaction, it should start
> >>>>> straight away. If there isn't, it would mark the channel as free,
> >>> so
> >>>>> _thrd_active() should return '0'.  If _thrd_active() is still '1',
> >>> then
> >>>>> something has gone wrong in the way.
> >>>>>
> >>>>> Does this shed some light?
> >>>>>
> >>>>
> >>>> Your patch makes the memcpy operation on dmatest.c and net DMA be
> >>> frozen too
> >>>> as well as Samsung audio playback.
> >>>
> >>> Is the IRQ correctly registered in drivers/dma/pl330.c:pl330_probe()?
> >>> Do you get interrupts when the transfer finish?
> >> Sure. IRQ works well.
> >
> > Ok, so can you check if pl330_update() is correctly marking the request
> > as free?  Do you know if there is another request in the queue when that
> > happens?
> >
> > Thomas, you said in a previous email that _thrd_active() always returned
> > '1'.  Was that after a request in req[0] finished?
> >  - If so, can you check that MARK_FREE was actually called for that
> >    request in pl330_update()?
> >  - If it was after a request in req[1] finished and there was a
> >    request already waiting in req[0], can you debug why _start()
> >    didn't activate it.
> >
> Javi, could you please check if you too get the memcpy failure with
> dmatest ?

(Please adding me in this thread)

Hi Javi,
How was going on the test? If any update, please let us know.

Vinod, I think, if the issue cannot be fixed soon, the patch should be
reverted until clear the issue. Because according to Boojin Kim's test, it
does not affect only Samsung stuff.

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

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

[Javi Merino]

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.
>>>>>>>
>>>>>>> If _thrd_active(thrd) returns '1', that means there is an active
>>>>>>> transfer still running or, if it has finished, you haven't called
>>>>>>> pl330_update() to acknowledge that.  pl330_update() calls _start()
>>>>> as
>>>>>>> soon as it can.
>>>>>>>
>>>>>>> drivers/dma/pl330.c registers the irq handler in pl330_probe(), so
>>>>>>> when
>>>>>>> the transaction finishes, pl330_update() should clear it and call
>>>>>>> _start().  If there is any outstanding transaction, it should start
>>>>>>> straight away. If there isn't, it would mark the channel as free,
>>>>> so
>>>>>>> _thrd_active() should return '0'.  If _thrd_active() is still '1',
>>>>> then
>>>>>>> something has gone wrong in the way.
>>>>>>>
>>>>>>> Does this shed some light?
>>>>>>>
>>>>>>
>>>>>> Your patch makes the memcpy operation on dmatest.c and net DMA be
>>>>> frozen too
>>>>>> as well as Samsung audio playback.
>>>>>
>>>>> Is the IRQ correctly registered in drivers/dma/pl330.c:pl330_probe()?
>>>>> Do you get interrupts when the transfer finish?
>>>> Sure. IRQ works well.
>>>
>>> Ok, so can you check if pl330_update() is correctly marking the request
>>> as free?  Do you know if there is another request in the queue when that
>>> happens?
>>>
>>> Thomas, you said in a previous email that _thrd_active() always returned
>>> '1'.  Was that after a request in req[0] finished?
>>>  - If so, can you check that MARK_FREE was actually called for that
>>>    request in pl330_update()?
>>>  - If it was after a request in req[1] finished and there was a
>>>    request already waiting in req[0], can you debug why _start()
>>>    didn't activate it.
>>>
>> Javi, could you please check if you too get the memcpy failure with
>> dmatest ?
> 
> (Please adding me in this thread)
> 
> Hi Javi,
> How was going on the test? If any update, please let us know.

I'm currently very busy at work, I'll run it as soon as I can.  This
appears to happen only in exynos, can't anybody else help with the
debugging?

Cheers,
Javi

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

[Javi Merino]

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.
>>>>>>>>
>>>>>>>> If _thrd_active(thrd) returns '1', that means there is an active
>>>>>>>> transfer still running or, if it has finished, you haven't called
>>>>>>>> pl330_update() to acknowledge that.  pl330_update() calls _start()
>>>>>> as
>>>>>>>> soon as it can.
>>>>>>>>
>>>>>>>> drivers/dma/pl330.c registers the irq handler in pl330_probe(), so
>>>>>>>> when
>>>>>>>> the transaction finishes, pl330_update() should clear it and call
>>>>>>>> _start().  If there is any outstanding transaction, it should start
>>>>>>>> straight away. If there isn't, it would mark the channel as free,
>>>>>> so
>>>>>>>> _thrd_active() should return '0'.  If _thrd_active() is still '1',
>>>>>> then
>>>>>>>> something has gone wrong in the way.
>>>>>>>>
>>>>>>>> Does this shed some light?
>>>>>>>>
>>>>>>>
>>>>>>> Your patch makes the memcpy operation on dmatest.c and net DMA be
>>>>>> frozen too
>>>>>>> as well as Samsung audio playback.
>>>>>>
>>>>>> Is the IRQ correctly registered in drivers/dma/pl330.c:pl330_probe()?
>>>>>> Do you get interrupts when the transfer finish?
>>>>> Sure. IRQ works well.
>>>>
>>>> Ok, so can you check if pl330_update() is correctly marking the request
>>>> as free?  Do you know if there is another request in the queue when that
>>>> happens?
>>>>
>>>> Thomas, you said in a previous email that _thrd_active() always returned
>>>> '1'.  Was that after a request in req[0] finished?
>>>>  - If so, can you check that MARK_FREE was actually called for that
>>>>    request in pl330_update()?
>>>>  - If it was after a request in req[1] finished and there was a
>>>>    request already waiting in req[0], can you debug why _start()
>>>>    didn't activate it.
>>>>
>>> Javi, could you please check if you too get the memcpy failure with
>>> dmatest ?
>>
>> (Please adding me in this thread)
>>
>> Hi Javi,
>> How was going on the test? If any update, please let us know.
> 
> I'm currently very busy at work, I'll run it as soon as I can.

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 ;)

Cheers,
Javi

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

[Javi Merino]

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

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

[Jassi Brar]

Hi Javi,

On 9 December 2011 17:28, Javi Merino <javi.merino@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.  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.
>
There should be nothing wrong in that.
It is but natural to prefer 0 if both 0 and 1 are available.

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

Then we can decide upon one of the following 2 options (I *think*
either should fix the issue)

diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index 97912fa..f550d46 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -846,7 +846,7 @@ static inline bool _req_active(struct pl330_thread *thrd,
 	if (IS_FREE(req))
 		return false;

-	return (pc >= buf && pc <= buf + req->mc_len) ? true : false;
+	return (pc >= buf && pc < buf + req->mc_len) ? true : false;
 }

--OR--

diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index 97912fa..ad85a75 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -233,7 +233,7 @@
 			} while (0)

 /* If the _pl330_req is available to the client */
-#define IS_FREE(req)	(*((u8 *)((req)->mc_cpu)) == CMD_DMAEND)
+#define IS_FREE(req)	((req)->mc_len == 0)

 /* Use this _only_ to wait on transient states */
 #define UNTIL(t, s)	while (!(_state(t) & (s))) cpu_relax();


Thanks.

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

[Javi Merino]

On 09/12/11 13:04, Jassi Brar wrote:
> Hi Javi,
>
> On 9 December 2011 17:28, Javi Merino <javi.merino@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.

> Then we can decide upon one of the following 2 options (I *think*
> either should fix the issue)
>
> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
> index 97912fa..f550d46 100644
> --- a/arch/arm/common/pl330.c
> +++ b/arch/arm/common/pl330.c
> @@ -846,7 +846,7 @@ static inline bool _req_active(struct pl330_thread *thrd,
>       if (IS_FREE(req))
>               return false;
>
> -     return (pc >= buf && pc <= buf + req->mc_len) ? true : false;
> +     return (pc >= buf && pc < buf + req->mc_len) ? true : false;
>  }

This reintroduces the race condition.  The DMA thread finishes and sends
the IRQ just after pl330_chan_ctrl() disables interrupts.  With this
patch, _thrd_active() returns false and _start() reissues the
just-finished transaction.

> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
> index 97912fa..ad85a75 100644
> --- a/arch/arm/common/pl330.c
> +++ b/arch/arm/common/pl330.c
> @@ -233,7 +233,7 @@
>                       } while (0)
>
>  /* If the _pl330_req is available to the client */
> -#define IS_FREE(req) (*((u8 *)((req)->mc_cpu)) == CMD_DMAEND)
> +#define IS_FREE(req) ((req)->mc_len == 0)

No, this doesn't fix it.  pl330_submit_req() introduces a valid request,
IS_FREE() returns false as it should.  The problem is that the DMA pc is
pointing at buf + req->mc_len .

>> The problem is that pl330_submit_req() always puts the request in buffer 0 if
>> > both buffers are free.
>> >
> There should be nothing wrong in that.
> It is but natural to prefer 0 if both 0 and 1 are available.

On second thoughts, with the solution I proposed in the previous email
the DMA will freeze if you call pl330_submit_req() twice before calling
pl330_chan_ctrl().  Can that happen?  I'd assume that each call to
pl330_submit_req() is always followed by a pl330_chan_ctrl(PL330_OP_START)

Another way of solving this would be to revert
ee3f615819404a9438b2dd01b7a39f276d2737f2 and go back to my original
patch (in the beginning of this thread):

http://article.gmane.org/gmane.linux.ports.arm.kernel/133110

Cheers,
Javi

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

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

[Jassi Brar]

On Fri, Dec 9, 2011 at 7:11 PM, Javi Merino <javi.merino@arm.com> wrote:
> On 09/12/11 13:04, Jassi Brar wrote:
>> Hi Javi,
>>
>> On 9 December 2011 17:28, Javi Merino <javi.merino@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.

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

[Javi Merino]

On 09/12/11 14:15, Jassi Brar wrote:
> On Fri, Dec 9, 2011 at 7:11 PM, Javi Merino <javi.merino@arm.com> wrote:
>> On 09/12/11 13:04, Jassi Brar wrote:
>>> Hi Javi,
>>>
>>> On 9 December 2011 17:28, Javi Merino <javi.merino@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;

No, see my last comment in the previous email.  I think this freezes the
DMA in the following scenario:

pl330_submit_req()
pl330_chan_ctrl(PL33O_OP_START)
... wait for the transfer to finish ...
pl330_update()
...
pl330_submit_req()
pl330_submit_req()
pl330_chan_ctrl(PL330_OP_START)

The pl330 won't start because of the same reason, we have a request in
buffer 0 and _thrd_active() would say that it is active.  This can
happen if drivers/dma/pl330.c:fill_queues() introduces two requests
before calling pl330_chan_ctrl(), which I'm not entirely sure that it
can't happen.

I think the best solution would be to revert
ee3f615819404a9438b2dd01b7a39f276d2737f2 and go back to my original
patch (in the beginning of this thread):

http://article.gmane.org/gmane.linux.ports.arm.kernel/133110

What do you think?  Thanks,
Javi

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

[Jassi Brar]

On Fri, Dec 9, 2011 at 8:22 PM, Javi Merino <javi.merino@arm.com> wrote:
>
> I think the best solution would be to revert
> ee3f615819404a9438b2dd01b7a39f276d2737f2 and go back to my original
> patch (in the beginning of this thread):
>
> http://article.gmane.org/gmane.linux.ports.arm.kernel/133110
>
> What do you think?

Well, we have to resort to that if we can't find something simpler.

What do you think about ...

diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index f407a6b..3a51cdd 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -1546,7 +1546,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)

 		/* Start the next */
 	case PL330_OP_START:
-		if (!_thrd_active(thrd) && !_start(thrd))
+		if (_state(thrd) == PL330_STATE_STOPPED && !_start(thrd))
 			ret = -EIO;
 		break;

Thanks
-jassi

[Sorry I don't have any pl330 platform handy]

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

[Javi Merino]

On 09/12/11 16:50, Jassi Brar wrote:
> What do you think about ...
> 
> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
> index f407a6b..3a51cdd 100644
> --- a/arch/arm/common/pl330.c
> +++ b/arch/arm/common/pl330.c
> @@ -1546,7 +1546,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
> 
>  		/* Start the next */
>  	case PL330_OP_START:
> -		if (!_thrd_active(thrd) && !_start(thrd))
> +		if (_state(thrd) == PL330_STATE_STOPPED && !_start(thrd))

Reintroduces the race condition, but you shorten the window:

  * pl330_submit_req()
  * pl330_chan_ctrl(PL330_OP_START)
  * pl330_submit_req()
  * pl330_chan_ctrl():spin_lock_irqsave()
  * Transfer 1 finishes, interrupt raised (but pl330_update() is not
    called as interrupts are disabled)
  * _state(thrd) is PL330_STATE_STOPPED , _start() reissues the first
    transaction.
  * pl330_chan_ctrl():spin_lock_irqrestore()
  * pl330_update() called for the first transaction, but it is running
    again!

What about properly tracking what we have sent to the DMA?  Something
like the following (warning *ugly* and untested code ahead, may eat your
kitten):

diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index f407a6b..3652c4b 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -303,6 +303,7 @@ struct pl330_thread {
 	struct _pl330_req req[2];
 	/* Index of the last submitted request */
 	unsigned lstenq;
+	int req_running;
 };

 enum pl330_dmac_state {
@@ -836,31 +837,6 @@ static inline u32 _state(struct pl330_thread *thrd)
 	}
 }

-/* If the request 'req' of thread 'thrd' is currently active */
-static inline bool _req_active(struct pl330_thread *thrd,
-		struct _pl330_req *req)
-{
-	void __iomem *regs = thrd->dmac->pinfo->base;
-	u32 buf = req->mc_bus, pc = readl(regs + CPC(thrd->id));
-
-	if (IS_FREE(req))
-		return false;
-
-	return (pc >= buf && pc <= buf + req->mc_len) ? true : false;
-}
-
-/* Returns 0 if the thread is inactive, ID of active req + 1 otherwise */
-static inline unsigned _thrd_active(struct pl330_thread *thrd)
-{
-	if (_req_active(thrd, &thrd->req[0]))
-		return 1; /* First req active */
-
-	if (_req_active(thrd, &thrd->req[1]))
-		return 2; /* Second req active */
-
-	return 0;
-}
-
 static void _stop(struct pl330_thread *thrd)
 {
 	void __iomem *regs = thrd->dmac->pinfo->base;
@@ -897,11 +873,13 @@ static bool _trigger(struct pl330_thread *thrd)
 	if (_state(thrd) != PL330_STATE_STOPPED)
 		return true;

-	if (!IS_FREE(&thrd->req[1 - thrd->lstenq]))
+	if (!IS_FREE(&thrd->req[1 - thrd->lstenq])) {
 		req = &thrd->req[1 - thrd->lstenq];
-	else if (!IS_FREE(&thrd->req[thrd->lstenq]))
+		thrd->req_running = 2 - thrd->lstenq;
+	} else if (!IS_FREE(&thrd->req[thrd->lstenq])) {
 		req = &thrd->req[thrd->lstenq];
-	else
+		thrd->req_running = 1 + thrd->lstenq;
+	} else
 		req = NULL;

 	/* Return if no request */
@@ -1384,6 +1362,7 @@ static void pl330_dotask(unsigned long data)
 			thrd->req[1].r = NULL;
 			MARK_FREE(&thrd->req[0]);
 			MARK_FREE(&thrd->req[1]);
+			thrd->req_running = 0;

 			/* Clear the reset flag */
 			pl330->dmac_tbd.reset_chan &= ~(1 << i);
@@ -1461,7 +1440,7 @@ int pl330_update(const struct pl330_info *pi)

 			thrd = &pl330->channels[id];

-			active = _thrd_active(thrd);
+			active = thrd->req_running;
 			if (!active) /* Aborted */
 				continue;

@@ -1469,6 +1448,7 @@ int pl330_update(const struct pl330_info *pi)

 			rqdone = &thrd->req[active];
 			MARK_FREE(rqdone);
+			thrd->req_running = 0;

 			/* Get going again ASAP */
 			_start(thrd);
@@ -1527,10 +1507,11 @@ int pl330_chan_ctrl(void *ch_id, enum
pl330_chan_op op)
 		thrd->req[1].r = NULL;
 		MARK_FREE(&thrd->req[0]);
 		MARK_FREE(&thrd->req[1]);
+		thrd->req_running = 0;
 		break;

 	case PL330_OP_ABORT:
-		active = _thrd_active(thrd);
+		active = thrd->req_running;

 		/* Make sure the channel is stopped */
 		_stop(thrd);
@@ -1543,10 +1524,11 @@ int pl330_chan_ctrl(void *ch_id, enum
pl330_chan_op op)

 		thrd->req[active].r = NULL;
 		MARK_FREE(&thrd->req[active]);
+		thrd->req_running = 0;

 		/* Start the next */
 	case PL330_OP_START:
-		if (!_thrd_active(thrd) && !_start(thrd))
+		if (!thrd->req_running && !_start(thrd))
 			ret = -EIO;
 		break;

@@ -1587,7 +1569,7 @@ int pl330_chan_status(void *ch_id, struct
pl330_chanstatus *pstatus)
 	else
 		pstatus->faulting = false;

-	active = _thrd_active(thrd);
+	active = thrd->req_running;

 	if (!active) {
 		/* Indicate that the thread is not running */
@@ -1662,6 +1644,7 @@ void *pl330_request_channel(const struct
pl330_info *pi)
 				MARK_FREE(&thrd->req[0]);
 				thrd->req[1].r = NULL;
 				MARK_FREE(&thrd->req[1]);
+				thrd->req_running = 0;
 				break;
 			}
 		}
@@ -1775,6 +1758,8 @@ static inline void _reset_thread(struct
pl330_thread *thrd)
 				+ pi->mcbufsz / 2;
 	thrd->req[1].r = NULL;
 	MARK_FREE(&thrd->req[1]);
+
+	thrd->req_running = 0;
 }

 static int dmac_alloc_threads(struct pl330_dmac *pl330)



> [Sorry I don't have any pl330 platform handy]

It's all right, I can do the testing.  However, I'd like that somebody
with an exynos could test whatever patch we come up with in the end.  I
don't want to break that platform again O:-)

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

[Jassi Brar]

On Sat, Dec 10, 2011 at 1:20 AM, Javi Merino <javi.merino@arm.com> wrote:
>
> What about properly tracking what we have sent to the DMA?  Something
> like the following (warning *ugly* and untested code ahead, may eat your
> kitten):
>
> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
> index f407a6b..3652c4b 100644
> --- a/arch/arm/common/pl330.c
> +++ b/arch/arm/common/pl330.c
> @@ -303,6 +303,7 @@ struct pl330_thread {
>        struct _pl330_req req[2];
>        /* Index of the last submitted request */
>        unsigned lstenq;
> +       int req_running;
>  };
>
>  enum pl330_dmac_state {
> @@ -836,31 +837,6 @@ static inline u32 _state(struct pl330_thread *thrd)
>        }
>  }
>
> -/* If the request 'req' of thread 'thrd' is currently active */
> -static inline bool _req_active(struct pl330_thread *thrd,
> -               struct _pl330_req *req)
> -{
> -       void __iomem *regs = thrd->dmac->pinfo->base;
> -       u32 buf = req->mc_bus, pc = readl(regs + CPC(thrd->id));
> -
> -       if (IS_FREE(req))
> -               return false;
> -
> -       return (pc >= buf && pc <= buf + req->mc_len) ? true : false;
> -}
> -
> -/* Returns 0 if the thread is inactive, ID of active req + 1 otherwise */
> -static inline unsigned _thrd_active(struct pl330_thread *thrd)
> -{
> -       if (_req_active(thrd, &thrd->req[0]))
> -               return 1; /* First req active */
> -
> -       if (_req_active(thrd, &thrd->req[1]))
> -               return 2; /* Second req active */
> -
> -       return 0;
> -}
> -
>  static void _stop(struct pl330_thread *thrd)
>  {
>        void __iomem *regs = thrd->dmac->pinfo->base;
> @@ -897,11 +873,13 @@ static bool _trigger(struct pl330_thread *thrd)
>        if (_state(thrd) != PL330_STATE_STOPPED)
>                return true;
>
> -       if (!IS_FREE(&thrd->req[1 - thrd->lstenq]))
> +       if (!IS_FREE(&thrd->req[1 - thrd->lstenq])) {
>                req = &thrd->req[1 - thrd->lstenq];
> -       else if (!IS_FREE(&thrd->req[thrd->lstenq]))
> +               thrd->req_running = 2 - thrd->lstenq;
> +       } else if (!IS_FREE(&thrd->req[thrd->lstenq])) {
>                req = &thrd->req[thrd->lstenq];
> -       else
> +               thrd->req_running = 1 + thrd->lstenq;
> +       } else
>                req = NULL;
>
>        /* Return if no request */
> @@ -1384,6 +1362,7 @@ static void pl330_dotask(unsigned long data)
>                        thrd->req[1].r = NULL;
>                        MARK_FREE(&thrd->req[0]);
>                        MARK_FREE(&thrd->req[1]);
> +                       thrd->req_running = 0;
>
>                        /* Clear the reset flag */
>                        pl330->dmac_tbd.reset_chan &= ~(1 << i);
> @@ -1461,7 +1440,7 @@ int pl330_update(const struct pl330_info *pi)
>
>                        thrd = &pl330->channels[id];
>
> -                       active = _thrd_active(thrd);
> +                       active = thrd->req_running;
>                        if (!active) /* Aborted */
>                                continue;
>
> @@ -1469,6 +1448,7 @@ int pl330_update(const struct pl330_info *pi)
>
>                        rqdone = &thrd->req[active];
>                        MARK_FREE(rqdone);
> +                       thrd->req_running = 0;
>
>                        /* Get going again ASAP */
>                        _start(thrd);
> @@ -1527,10 +1507,11 @@ int pl330_chan_ctrl(void *ch_id, enum
> pl330_chan_op op)
>                thrd->req[1].r = NULL;
>                MARK_FREE(&thrd->req[0]);
>                MARK_FREE(&thrd->req[1]);
> +               thrd->req_running = 0;
>                break;
>
>        case PL330_OP_ABORT:
> -               active = _thrd_active(thrd);
> +               active = thrd->req_running;
>
>                /* Make sure the channel is stopped */
>                _stop(thrd);
> @@ -1543,10 +1524,11 @@ int pl330_chan_ctrl(void *ch_id, enum
> pl330_chan_op op)
>
>                thrd->req[active].r = NULL;
>                MARK_FREE(&thrd->req[active]);
> +               thrd->req_running = 0;
>
>                /* Start the next */
>        case PL330_OP_START:
> -               if (!_thrd_active(thrd) && !_start(thrd))
> +               if (!thrd->req_running && !_start(thrd))
>                        ret = -EIO;
>                break;
>
> @@ -1587,7 +1569,7 @@ int pl330_chan_status(void *ch_id, struct
> pl330_chanstatus *pstatus)
>        else
>                pstatus->faulting = false;
>
> -       active = _thrd_active(thrd);
> +       active = thrd->req_running;
>
>        if (!active) {
>                /* Indicate that the thread is not running */
> @@ -1662,6 +1644,7 @@ void *pl330_request_channel(const struct
> pl330_info *pi)
>                                MARK_FREE(&thrd->req[0]);
>                                thrd->req[1].r = NULL;
>                                MARK_FREE(&thrd->req[1]);
> +                               thrd->req_running = 0;
>                                break;
>                        }
>                }
> @@ -1775,6 +1758,8 @@ static inline void _reset_thread(struct
> pl330_thread *thrd)
>                                + pi->mcbufsz / 2;
>        thrd->req[1].r = NULL;
>        MARK_FREE(&thrd->req[1]);
> +
> +       thrd->req_running = 0;
>  }
>
>  static int dmac_alloc_threads(struct pl330_dmac *pl330)
>
Yeah, this is like I said 'marker' method. Though we can clean it up a bit.
1) Pack req_running and lstenq together. Make lsteng return invalid value
should there be no buff programmed, otherwise 0 or 1.
2) Try to merge req_running modification as part of MARK_FREE.

Thanks.

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

[Javi Merino]

On 11/12/11 10:51, Jassi Brar wrote:
> On Sat, Dec 10, 2011 at 1:20 AM, Javi Merino <javi.merino@arm.com> wrote:
>>
>> What about properly tracking what we have sent to the DMA?  Something
>> like the following (warning *ugly* and untested code ahead, may eat your
>> kitten):
>>
> Yeah, this is like I said 'marker' method. Though we can clean it up a bit.
> 1) Pack req_running and lstenq together. Make lsteng return invalid value
> should there be no buff programmed, otherwise 0 or 1.

This can lead to starvation.  If lstenq is -1 when the DMA hasn't been
programmed yet, in _trigger() you don't know which buffer is the
"oldest", so you may end up always starting the new buffer and
forgetting about the other one.  lstenq as it is right now prevents that.

> 2) Try to merge req_running modification as part of MARK_FREE.

Yes, I thought about that, but I didn't want to code a proper solution
and then receive a "no, we shouldn't go down this road".

I'll clean it up and send a proper patch.

Cheers,
Javi

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

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

[Jassi Brar]

On 11 December 2011 20:39, Javi Merino <javi.merino@arm.com> wrote:
>>>
>>> What about properly tracking what we have sent to the DMA?  Something
>>> like the following (warning *ugly* and untested code ahead, may eat your
>>> kitten):
>>>
>> Yeah, this is like I said 'marker' method. Though we can clean it up a bit.
>> 1) Pack req_running and lstenq together. Make lsteng return invalid value
>> should there be no buff programmed, otherwise 0 or 1.
>
> This can lead to starvation.  If lstenq is -1 when the DMA hasn't been
> programmed yet, in _trigger() you don't know which buffer is the
> "oldest", so you may end up always starting the new buffer and
> forgetting about the other one.  lstenq as it is right now prevents that.
>
Sorry I don't understand. If lstenq is -1 that means there's no req programmed
so trigger need not do anything. I didn't say we don't need to do anything else.
Though it's just an idea I haven't implemented and it may not work out.
Just please give it a try if you can. Thanks.

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

[Javi Merino]

On 11/12/11 17:10, Jassi Brar wrote:
> On 11 December 2011 20:39, Javi Merino <javi.merino@arm.com> wrote:
>>>>
>>>> What about properly tracking what we have sent to the DMA?  Something
>>>> like the following (warning *ugly* and untested code ahead, may eat your
>>>> kitten):
>>>>
>>> Yeah, this is like I said 'marker' method. Though we can clean it up a bit.
>>> 1) Pack req_running and lstenq together. Make lsteng return invalid value
>>> should there be no buff programmed, otherwise 0 or 1.
>>
>> This can lead to starvation.  If lstenq is -1 when the DMA hasn't been
>> programmed yet, in _trigger() you don't know which buffer is the
>> "oldest", so you may end up always starting the new buffer and
>> forgetting about the other one.  lstenq as it is right now prevents that.
>>
> Sorry I don't understand. If lstenq is -1 that means there's no req programmed
> so trigger need not do anything. I didn't say we don't need to do anything else.

Currently lstenq tracks the last request that pl330_submit_req() has
enqueued.  I think we should add a req_running (or any other name) that
tracks what _trigger() has sent to the DMA.  If we pack them together we
lose some information and I don't see a way of packing them safely.

> Though it's just an idea I haven't implemented and it may not work out.

I was trying to implement it and got stuck in _trigger() thinking "ok,
so which buffer am I supposed to trigger now..."

> Just please give it a try if you can. Thanks.

I have a patch that seems to be working.  Let me test it a little bit
more and I'll send it to the list.

Cheers,
Javi

[PATCH] ARM: PL330: Fix driver freeze

[Javi Merino]

Add a req_running field to the pl330_thread to track which request (if
any) has been submitted to the DMA.  This mechanism replaces the old
one in which we tried to guess the same by looking at the PC of the
DMA, which could prevent the driver from sending more requests if it
didn't guess correctly.

Signed-off-by: Javi Merino <javi.merino@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
---
 arch/arm/common/pl330.c |  116 ++++++++++++++++++++---------------------------
 1 files changed, 49 insertions(+), 67 deletions(-)

diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index f407a6b..8d8df74 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -221,17 +221,6 @@
  */
 #define MCODE_BUFF_PER_REQ	256
 
-/*
- * Mark a _pl330_req as free.
- * We do it by writing DMAEND as the first instruction
- * because no valid request is going to have DMAEND as
- * its first instruction to execute.
- */
-#define MARK_FREE(req)	do { \
-				_emit_END(0, (req)->mc_cpu); \
-				(req)->mc_len = 0; \
-			} while (0)
-
 /* If the _pl330_req is available to the client */
 #define IS_FREE(req)	(*((u8 *)((req)->mc_cpu)) == CMD_DMAEND)
 
@@ -301,8 +290,10 @@ struct pl330_thread {
 	struct pl330_dmac *dmac;
 	/* Only two at a time */
 	struct _pl330_req req[2];
-	/* Index of the last submitted request */
+	/* Index of the last enqueued request */
 	unsigned lstenq;
+	/* Index of the last submitted request or -1 if the DMA is stopped */
+	int req_running;
 };
 
 enum pl330_dmac_state {
@@ -778,6 +769,22 @@ static inline void _execute_DBGINSN(struct pl330_thread *thrd,
 	writel(0, regs + DBGCMD);
 }
 
+/*
+ * Mark a _pl330_req as free.
+ * We do it by writing DMAEND as the first instruction
+ * because no valid request is going to have DMAEND as
+ * its first instruction to execute.
+ */
+static void mark_free(struct pl330_thread *thrd, int idx)
+{
+	struct _pl330_req *req = &thrd->req[idx];
+
+	_emit_END(0, req->mc_cpu);
+	req->mc_len = 0;
+
+	thrd->req_running = -1;
+}
+
 static inline u32 _state(struct pl330_thread *thrd)
 {
 	void __iomem *regs = thrd->dmac->pinfo->base;
@@ -836,31 +843,6 @@ static inline u32 _state(struct pl330_thread *thrd)
 	}
 }
 
-/* If the request 'req' of thread 'thrd' is currently active */
-static inline bool _req_active(struct pl330_thread *thrd,
-		struct _pl330_req *req)
-{
-	void __iomem *regs = thrd->dmac->pinfo->base;
-	u32 buf = req->mc_bus, pc = readl(regs + CPC(thrd->id));
-
-	if (IS_FREE(req))
-		return false;
-
-	return (pc >= buf && pc <= buf + req->mc_len) ? true : false;
-}
-
-/* Returns 0 if the thread is inactive, ID of active req + 1 otherwise */
-static inline unsigned _thrd_active(struct pl330_thread *thrd)
-{
-	if (_req_active(thrd, &thrd->req[0]))
-		return 1; /* First req active */
-
-	if (_req_active(thrd, &thrd->req[1]))
-		return 2; /* Second req active */
-
-	return 0;
-}
-
 static void _stop(struct pl330_thread *thrd)
 {
 	void __iomem *regs = thrd->dmac->pinfo->base;
@@ -892,17 +874,22 @@ static bool _trigger(struct pl330_thread *thrd)
 	struct _arg_GO go;
 	unsigned ns;
 	u8 insn[6] = {0, 0, 0, 0, 0, 0};
+	int idx;
 
 	/* Return if already ACTIVE */
 	if (_state(thrd) != PL330_STATE_STOPPED)
 		return true;
 
-	if (!IS_FREE(&thrd->req[1 - thrd->lstenq]))
-		req = &thrd->req[1 - thrd->lstenq];
-	else if (!IS_FREE(&thrd->req[thrd->lstenq]))
-		req = &thrd->req[thrd->lstenq];
-	else
-		req = NULL;
+	idx = 1 - thrd->lstenq;
+	if (!IS_FREE(&thrd->req[idx]))
+		req = &thrd->req[idx];
+	else {
+		idx = thrd->lstenq;
+		if (!IS_FREE(&thrd->req[idx]))
+			req = &thrd->req[idx];
+		else
+			req = NULL;
+	}
 
 	/* Return if no request */
 	if (!req || !req->r)
@@ -933,6 +920,8 @@ static bool _trigger(struct pl330_thread *thrd)
 	/* Only manager can execute GO */
 	_execute_DBGINSN(thrd, insn, true);
 
+	thrd->req_running = idx;
+
 	return true;
 }
 
@@ -1382,8 +1371,8 @@ static void pl330_dotask(unsigned long data)
 
 			thrd->req[0].r = NULL;
 			thrd->req[1].r = NULL;
-			MARK_FREE(&thrd->req[0]);
-			MARK_FREE(&thrd->req[1]);
+			mark_free(thrd, 0);
+			mark_free(thrd, 1);
 
 			/* Clear the reset flag */
 			pl330->dmac_tbd.reset_chan &= ~(1 << i);
@@ -1461,14 +1450,12 @@ int pl330_update(const struct pl330_info *pi)
 
 			thrd = &pl330->channels[id];
 
-			active = _thrd_active(thrd);
-			if (!active) /* Aborted */
+			active = thrd->req_running;
+			if (active == -1) /* Aborted */
 				continue;
 
-			active -= 1;
-
 			rqdone = &thrd->req[active];
-			MARK_FREE(rqdone);
+			mark_free(thrd, active);
 
 			/* Get going again ASAP */
 			_start(thrd);
@@ -1509,7 +1496,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
 	struct pl330_thread *thrd = ch_id;
 	struct pl330_dmac *pl330;
 	unsigned long flags;
-	int ret = 0, active;
+	int ret = 0, active = thrd->req_running;
 
 	if (!thrd || thrd->free || thrd->dmac->state == DYING)
 		return -EINVAL;
@@ -1525,28 +1512,24 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
 
 		thrd->req[0].r = NULL;
 		thrd->req[1].r = NULL;
-		MARK_FREE(&thrd->req[0]);
-		MARK_FREE(&thrd->req[1]);
+		mark_free(thrd, 0);
+		mark_free(thrd, 1);
 		break;
 
 	case PL330_OP_ABORT:
-		active = _thrd_active(thrd);
-
 		/* Make sure the channel is stopped */
 		_stop(thrd);
 
 		/* ABORT is only for the active req */
-		if (!active)
+		if (active == -1)
 			break;
 
-		active--;
-
 		thrd->req[active].r = NULL;
-		MARK_FREE(&thrd->req[active]);
+		mark_free(thrd, active);
 
 		/* Start the next */
 	case PL330_OP_START:
-		if (!_thrd_active(thrd) && !_start(thrd))
+		if ((active == -1) && !_start(thrd))
 			ret = -EIO;
 		break;
 
@@ -1587,14 +1570,13 @@ int pl330_chan_status(void *ch_id, struct pl330_chanstatus *pstatus)
 	else
 		pstatus->faulting = false;
 
-	active = _thrd_active(thrd);
+	active = thrd->req_running;
 
-	if (!active) {
+	if (active == -1) {
 		/* Indicate that the thread is not running */
 		pstatus->top_req = NULL;
 		pstatus->wait_req = NULL;
 	} else {
-		active--;
 		pstatus->top_req = thrd->req[active].r;
 		pstatus->wait_req = !IS_FREE(&thrd->req[1 - active])
 					? thrd->req[1 - active].r : NULL;
@@ -1659,9 +1641,9 @@ void *pl330_request_channel(const struct pl330_info *pi)
 				thrd->free = false;
 				thrd->lstenq = 1;
 				thrd->req[0].r = NULL;
-				MARK_FREE(&thrd->req[0]);
+				mark_free(thrd, 0);
 				thrd->req[1].r = NULL;
-				MARK_FREE(&thrd->req[1]);
+				mark_free(thrd, 1);
 				break;
 			}
 		}
@@ -1767,14 +1749,14 @@ static inline void _reset_thread(struct pl330_thread *thrd)
 	thrd->req[0].mc_bus = pl330->mcode_bus
 				+ (thrd->id * pi->mcbufsz);
 	thrd->req[0].r = NULL;
-	MARK_FREE(&thrd->req[0]);
+	mark_free(thrd, 0);
 
 	thrd->req[1].mc_cpu = thrd->req[0].mc_cpu
 				+ pi->mcbufsz / 2;
 	thrd->req[1].mc_bus = thrd->req[0].mc_bus
 				+ pi->mcbufsz / 2;
 	thrd->req[1].r = NULL;
-	MARK_FREE(&thrd->req[1]);
+	mark_free(thrd, 1);
 }
 
 static int dmac_alloc_threads(struct pl330_dmac *pl330)
-- 
1.7.0.4

Re: [PATCH] ARM: PL330: Fix driver freeze

[Javi Merino]

On 11/12/11 19:27, Javi Merino wrote:
> Add a req_running field to the pl330_thread to track which request (if
> any) has been submitted to the DMA.  This mechanism replaces the old
> one in which we tried to guess the same by looking at the PC of the
> DMA, which could prevent the driver from sending more requests if it
> didn't guess correctly.
> 
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> Cc: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  arch/arm/common/pl330.c |  116 ++++++++++++++++++++---------------------------
>  1 files changed, 49 insertions(+), 67 deletions(-)
> 
> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
> index f407a6b..8d8df74 100644
> --- a/arch/arm/common/pl330.c
> +++ b/arch/arm/common/pl330.c
> @@ -221,17 +221,6 @@
>   */
>  #define MCODE_BUFF_PER_REQ	256
>  
> -/*
> - * Mark a _pl330_req as free.
> - * We do it by writing DMAEND as the first instruction
> - * because no valid request is going to have DMAEND as
> - * its first instruction to execute.
> - */
> -#define MARK_FREE(req)	do { \
> -				_emit_END(0, (req)->mc_cpu); \
> -				(req)->mc_len = 0; \
> -			} while (0)
> -
>  /* If the _pl330_req is available to the client */
>  #define IS_FREE(req)	(*((u8 *)((req)->mc_cpu)) == CMD_DMAEND)
>  
> @@ -301,8 +290,10 @@ struct pl330_thread {
>  	struct pl330_dmac *dmac;
>  	/* Only two at a time */
>  	struct _pl330_req req[2];
> -	/* Index of the last submitted request */
> +	/* Index of the last enqueued request */
>  	unsigned lstenq;
> +	/* Index of the last submitted request or -1 if the DMA is stopped */
> +	int req_running;
>  };
>  
>  enum pl330_dmac_state {
> @@ -778,6 +769,22 @@ static inline void _execute_DBGINSN(struct pl330_thread *thrd,
>  	writel(0, regs + DBGCMD);
>  }
>  
> +/*
> + * Mark a _pl330_req as free.
> + * We do it by writing DMAEND as the first instruction
> + * because no valid request is going to have DMAEND as
> + * its first instruction to execute.
> + */
> +static void mark_free(struct pl330_thread *thrd, int idx)
> +{
> +	struct _pl330_req *req = &thrd->req[idx];
> +
> +	_emit_END(0, req->mc_cpu);
> +	req->mc_len = 0;
> +
> +	thrd->req_running = -1;
> +}
> +
>  static inline u32 _state(struct pl330_thread *thrd)
>  {
>  	void __iomem *regs = thrd->dmac->pinfo->base;
> @@ -836,31 +843,6 @@ static inline u32 _state(struct pl330_thread *thrd)
>  	}
>  }
>  
> -/* If the request 'req' of thread 'thrd' is currently active */
> -static inline bool _req_active(struct pl330_thread *thrd,
> -		struct _pl330_req *req)
> -{
> -	void __iomem *regs = thrd->dmac->pinfo->base;
> -	u32 buf = req->mc_bus, pc = readl(regs + CPC(thrd->id));
> -
> -	if (IS_FREE(req))
> -		return false;
> -
> -	return (pc >= buf && pc <= buf + req->mc_len) ? true : false;
> -}
> -
> -/* Returns 0 if the thread is inactive, ID of active req + 1 otherwise */
> -static inline unsigned _thrd_active(struct pl330_thread *thrd)
> -{
> -	if (_req_active(thrd, &thrd->req[0]))
> -		return 1; /* First req active */
> -
> -	if (_req_active(thrd, &thrd->req[1]))
> -		return 2; /* Second req active */
> -
> -	return 0;
> -}
> -
>  static void _stop(struct pl330_thread *thrd)
>  {
>  	void __iomem *regs = thrd->dmac->pinfo->base;
> @@ -892,17 +874,22 @@ static bool _trigger(struct pl330_thread *thrd)
>  	struct _arg_GO go;
>  	unsigned ns;
>  	u8 insn[6] = {0, 0, 0, 0, 0, 0};
> +	int idx;
>  
>  	/* Return if already ACTIVE */
>  	if (_state(thrd) != PL330_STATE_STOPPED)
>  		return true;
>  
> -	if (!IS_FREE(&thrd->req[1 - thrd->lstenq]))
> -		req = &thrd->req[1 - thrd->lstenq];
> -	else if (!IS_FREE(&thrd->req[thrd->lstenq]))
> -		req = &thrd->req[thrd->lstenq];
> -	else
> -		req = NULL;
> +	idx = 1 - thrd->lstenq;
> +	if (!IS_FREE(&thrd->req[idx]))
> +		req = &thrd->req[idx];
> +	else {
> +		idx = thrd->lstenq;
> +		if (!IS_FREE(&thrd->req[idx]))
> +			req = &thrd->req[idx];
> +		else
> +			req = NULL;
> +	}
>  
>  	/* Return if no request */
>  	if (!req || !req->r)
> @@ -933,6 +920,8 @@ static bool _trigger(struct pl330_thread *thrd)
>  	/* Only manager can execute GO */
>  	_execute_DBGINSN(thrd, insn, true);
>  
> +	thrd->req_running = idx;
> +
>  	return true;
>  }
>  
> @@ -1382,8 +1371,8 @@ static void pl330_dotask(unsigned long data)
>  
>  			thrd->req[0].r = NULL;
>  			thrd->req[1].r = NULL;
> -			MARK_FREE(&thrd->req[0]);
> -			MARK_FREE(&thrd->req[1]);
> +			mark_free(thrd, 0);
> +			mark_free(thrd, 1);
>  
>  			/* Clear the reset flag */
>  			pl330->dmac_tbd.reset_chan &= ~(1 << i);
> @@ -1461,14 +1450,12 @@ int pl330_update(const struct pl330_info *pi)
>  
>  			thrd = &pl330->channels[id];
>  
> -			active = _thrd_active(thrd);
> -			if (!active) /* Aborted */
> +			active = thrd->req_running;
> +			if (active == -1) /* Aborted */
>  				continue;
>  
> -			active -= 1;
> -
>  			rqdone = &thrd->req[active];
> -			MARK_FREE(rqdone);
> +			mark_free(thrd, active);
>  
>  			/* Get going again ASAP */
>  			_start(thrd);
> @@ -1509,7 +1496,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
>  	struct pl330_thread *thrd = ch_id;
>  	struct pl330_dmac *pl330;
>  	unsigned long flags;
> -	int ret = 0, active;
> +	int ret = 0, active = thrd->req_running;
>  
>  	if (!thrd || thrd->free || thrd->dmac->state == DYING)
>  		return -EINVAL;
> @@ -1525,28 +1512,24 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
>  
>  		thrd->req[0].r = NULL;
>  		thrd->req[1].r = NULL;
> -		MARK_FREE(&thrd->req[0]);
> -		MARK_FREE(&thrd->req[1]);
> +		mark_free(thrd, 0);
> +		mark_free(thrd, 1);
>  		break;
>  
>  	case PL330_OP_ABORT:
> -		active = _thrd_active(thrd);
> -
>  		/* Make sure the channel is stopped */
>  		_stop(thrd);
>  
>  		/* ABORT is only for the active req */
> -		if (!active)
> +		if (active == -1)
>  			break;
>  
> -		active--;
> -
>  		thrd->req[active].r = NULL;
> -		MARK_FREE(&thrd->req[active]);
> +		mark_free(thrd, active);
>  
>  		/* Start the next */
>  	case PL330_OP_START:
> -		if (!_thrd_active(thrd) && !_start(thrd))
> +		if ((active == -1) && !_start(thrd))
>  			ret = -EIO;
>  		break;
>  
> @@ -1587,14 +1570,13 @@ int pl330_chan_status(void *ch_id, struct pl330_chanstatus *pstatus)
>  	else
>  		pstatus->faulting = false;
>  
> -	active = _thrd_active(thrd);
> +	active = thrd->req_running;
>  
> -	if (!active) {
> +	if (active == -1) {
>  		/* Indicate that the thread is not running */
>  		pstatus->top_req = NULL;
>  		pstatus->wait_req = NULL;
>  	} else {
> -		active--;
>  		pstatus->top_req = thrd->req[active].r;
>  		pstatus->wait_req = !IS_FREE(&thrd->req[1 - active])
>  					? thrd->req[1 - active].r : NULL;
> @@ -1659,9 +1641,9 @@ void *pl330_request_channel(const struct pl330_info *pi)
>  				thrd->free = false;
>  				thrd->lstenq = 1;
>  				thrd->req[0].r = NULL;
> -				MARK_FREE(&thrd->req[0]);
> +				mark_free(thrd, 0);
>  				thrd->req[1].r = NULL;
> -				MARK_FREE(&thrd->req[1]);
> +				mark_free(thrd, 1);
>  				break;
>  			}
>  		}
> @@ -1767,14 +1749,14 @@ static inline void _reset_thread(struct pl330_thread *thrd)
>  	thrd->req[0].mc_bus = pl330->mcode_bus
>  				+ (thrd->id * pi->mcbufsz);
>  	thrd->req[0].r = NULL;
> -	MARK_FREE(&thrd->req[0]);
> +	mark_free(thrd, 0);
>  
>  	thrd->req[1].mc_cpu = thrd->req[0].mc_cpu
>  				+ pi->mcbufsz / 2;
>  	thrd->req[1].mc_bus = thrd->req[0].mc_bus
>  				+ pi->mcbufsz / 2;
>  	thrd->req[1].r = NULL;
> -	MARK_FREE(&thrd->req[1]);
> +	mark_free(thrd, 1);
>  }
>  
>  static int dmac_alloc_threads(struct pl330_dmac *pl330)

I've done some testing and I haven't been able to break it.  Can
somebody please test this patch with the workload that failed (audio in
exynos I believe)?

Thanks,
Javi

Re: [PATCH] ARM: PL330: Fix driver freeze

[Tushar Behera]

On 12/15/2011 11:18 PM, Javi Merino wrote:
> On 11/12/11 19:27, Javi Merino wrote:
>> Add a req_running field to the pl330_thread to track which request (if
>> any) has been submitted to the DMA.  This mechanism replaces the old
>> one in which we tried to guess the same by looking at the PC of the
>> DMA, which could prevent the driver from sending more requests if it
>> didn't guess correctly.
>>
>> Signed-off-by: Javi Merino<javi.merino@arm.com>
>> Cc: Jassi Brar<jaswinder.singh@linaro.org>
>> ---
>>   arch/arm/common/pl330.c |  116 ++++++++++++++++++++---------------------------
>>   1 files changed, 49 insertions(+), 67 deletions(-)
>>
>> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
>> index f407a6b..8d8df74 100644
>> --- a/arch/arm/common/pl330.c
>> +++ b/arch/arm/common/pl330.c
>> @@ -221,17 +221,6 @@
>>    */
>>   #define MCODE_BUFF_PER_REQ	256
>>
>> -/*
>> - * Mark a _pl330_req as free.
>> - * We do it by writing DMAEND as the first instruction
>> - * because no valid request is going to have DMAEND as
>> - * its first instruction to execute.
>> - */
>> -#define MARK_FREE(req)	do { \
>> -				_emit_END(0, (req)->mc_cpu); \
>> -				(req)->mc_len = 0; \
>> -			} while (0)
>> -
>>   /* If the _pl330_req is available to the client */
>>   #define IS_FREE(req)	(*((u8 *)((req)->mc_cpu)) == CMD_DMAEND)
>>
>> @@ -301,8 +290,10 @@ struct pl330_thread {
>>   	struct pl330_dmac *dmac;
>>   	/* Only two at a time */
>>   	struct _pl330_req req[2];
>> -	/* Index of the last submitted request */
>> +	/* Index of the last enqueued request */
>>   	unsigned lstenq;
>> +	/* Index of the last submitted request or -1 if the DMA is stopped */
>> +	int req_running;
>>   };
>>
>>   enum pl330_dmac_state {
>> @@ -778,6 +769,22 @@ static inline void _execute_DBGINSN(struct pl330_thread *thrd,
>>   	writel(0, regs + DBGCMD);
>>   }
>>
>> +/*
>> + * Mark a _pl330_req as free.
>> + * We do it by writing DMAEND as the first instruction
>> + * because no valid request is going to have DMAEND as
>> + * its first instruction to execute.
>> + */
>> +static void mark_free(struct pl330_thread *thrd, int idx)
>> +{
>> +	struct _pl330_req *req =&thrd->req[idx];
>> +
>> +	_emit_END(0, req->mc_cpu);
>> +	req->mc_len = 0;
>> +
>> +	thrd->req_running = -1;
>> +}
>> +
>>   static inline u32 _state(struct pl330_thread *thrd)
>>   {
>>   	void __iomem *regs = thrd->dmac->pinfo->base;
>> @@ -836,31 +843,6 @@ static inline u32 _state(struct pl330_thread *thrd)
>>   	}
>>   }
>>
>> -/* If the request 'req' of thread 'thrd' is currently active */
>> -static inline bool _req_active(struct pl330_thread *thrd,
>> -		struct _pl330_req *req)
>> -{
>> -	void __iomem *regs = thrd->dmac->pinfo->base;
>> -	u32 buf = req->mc_bus, pc = readl(regs + CPC(thrd->id));
>> -
>> -	if (IS_FREE(req))
>> -		return false;
>> -
>> -	return (pc>= buf&&  pc<= buf + req->mc_len) ? true : false;
>> -}
>> -
>> -/* Returns 0 if the thread is inactive, ID of active req + 1 otherwise */
>> -static inline unsigned _thrd_active(struct pl330_thread *thrd)
>> -{
>> -	if (_req_active(thrd,&thrd->req[0]))
>> -		return 1; /* First req active */
>> -
>> -	if (_req_active(thrd,&thrd->req[1]))
>> -		return 2; /* Second req active */
>> -
>> -	return 0;
>> -}
>> -
>>   static void _stop(struct pl330_thread *thrd)
>>   {
>>   	void __iomem *regs = thrd->dmac->pinfo->base;
>> @@ -892,17 +874,22 @@ static bool _trigger(struct pl330_thread *thrd)
>>   	struct _arg_GO go;
>>   	unsigned ns;
>>   	u8 insn[6] = {0, 0, 0, 0, 0, 0};
>> +	int idx;
>>
>>   	/* Return if already ACTIVE */
>>   	if (_state(thrd) != PL330_STATE_STOPPED)
>>   		return true;
>>
>> -	if (!IS_FREE(&thrd->req[1 - thrd->lstenq]))
>> -		req =&thrd->req[1 - thrd->lstenq];
>> -	else if (!IS_FREE(&thrd->req[thrd->lstenq]))
>> -		req =&thrd->req[thrd->lstenq];
>> -	else
>> -		req = NULL;
>> +	idx = 1 - thrd->lstenq;
>> +	if (!IS_FREE(&thrd->req[idx]))
>> +		req =&thrd->req[idx];
>> +	else {
>> +		idx = thrd->lstenq;
>> +		if (!IS_FREE(&thrd->req[idx]))
>> +			req =&thrd->req[idx];
>> +		else
>> +			req = NULL;
>> +	}
>>
>>   	/* Return if no request */
>>   	if (!req || !req->r)
>> @@ -933,6 +920,8 @@ static bool _trigger(struct pl330_thread *thrd)
>>   	/* Only manager can execute GO */
>>   	_execute_DBGINSN(thrd, insn, true);
>>
>> +	thrd->req_running = idx;
>> +
>>   	return true;
>>   }
>>
>> @@ -1382,8 +1371,8 @@ static void pl330_dotask(unsigned long data)
>>
>>   			thrd->req[0].r = NULL;
>>   			thrd->req[1].r = NULL;
>> -			MARK_FREE(&thrd->req[0]);
>> -			MARK_FREE(&thrd->req[1]);
>> +			mark_free(thrd, 0);
>> +			mark_free(thrd, 1);
>>
>>   			/* Clear the reset flag */
>>   			pl330->dmac_tbd.reset_chan&= ~(1<<  i);
>> @@ -1461,14 +1450,12 @@ int pl330_update(const struct pl330_info *pi)
>>
>>   			thrd =&pl330->channels[id];
>>
>> -			active = _thrd_active(thrd);
>> -			if (!active) /* Aborted */
>> +			active = thrd->req_running;
>> +			if (active == -1) /* Aborted */
>>   				continue;
>>
>> -			active -= 1;
>> -
>>   			rqdone =&thrd->req[active];
>> -			MARK_FREE(rqdone);
>> +			mark_free(thrd, active);
>>
>>   			/* Get going again ASAP */
>>   			_start(thrd);
>> @@ -1509,7 +1496,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
>>   	struct pl330_thread *thrd = ch_id;
>>   	struct pl330_dmac *pl330;
>>   	unsigned long flags;
>> -	int ret = 0, active;
>> +	int ret = 0, active = thrd->req_running;
>>
>>   	if (!thrd || thrd->free || thrd->dmac->state == DYING)
>>   		return -EINVAL;
>> @@ -1525,28 +1512,24 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
>>
>>   		thrd->req[0].r = NULL;
>>   		thrd->req[1].r = NULL;
>> -		MARK_FREE(&thrd->req[0]);
>> -		MARK_FREE(&thrd->req[1]);
>> +		mark_free(thrd, 0);
>> +		mark_free(thrd, 1);
>>   		break;
>>
>>   	case PL330_OP_ABORT:
>> -		active = _thrd_active(thrd);
>> -
>>   		/* Make sure the channel is stopped */
>>   		_stop(thrd);
>>
>>   		/* ABORT is only for the active req */
>> -		if (!active)
>> +		if (active == -1)
>>   			break;
>>
>> -		active--;
>> -
>>   		thrd->req[active].r = NULL;
>> -		MARK_FREE(&thrd->req[active]);
>> +		mark_free(thrd, active);
>>
>>   		/* Start the next */
>>   	case PL330_OP_START:
>> -		if (!_thrd_active(thrd)&&  !_start(thrd))
>> +		if ((active == -1)&&  !_start(thrd))
>>   			ret = -EIO;
>>   		break;
>>
>> @@ -1587,14 +1570,13 @@ int pl330_chan_status(void *ch_id, struct pl330_chanstatus *pstatus)
>>   	else
>>   		pstatus->faulting = false;
>>
>> -	active = _thrd_active(thrd);
>> +	active = thrd->req_running;
>>
>> -	if (!active) {
>> +	if (active == -1) {
>>   		/* Indicate that the thread is not running */
>>   		pstatus->top_req = NULL;
>>   		pstatus->wait_req = NULL;
>>   	} else {
>> -		active--;
>>   		pstatus->top_req = thrd->req[active].r;
>>   		pstatus->wait_req = !IS_FREE(&thrd->req[1 - active])
>>   					? thrd->req[1 - active].r : NULL;
>> @@ -1659,9 +1641,9 @@ void *pl330_request_channel(const struct pl330_info *pi)
>>   				thrd->free = false;
>>   				thrd->lstenq = 1;
>>   				thrd->req[0].r = NULL;
>> -				MARK_FREE(&thrd->req[0]);
>> +				mark_free(thrd, 0);
>>   				thrd->req[1].r = NULL;
>> -				MARK_FREE(&thrd->req[1]);
>> +				mark_free(thrd, 1);
>>   				break;
>>   			}
>>   		}
>> @@ -1767,14 +1749,14 @@ static inline void _reset_thread(struct pl330_thread *thrd)
>>   	thrd->req[0].mc_bus = pl330->mcode_bus
>>   				+ (thrd->id * pi->mcbufsz);
>>   	thrd->req[0].r = NULL;
>> -	MARK_FREE(&thrd->req[0]);
>> +	mark_free(thrd, 0);
>>
>>   	thrd->req[1].mc_cpu = thrd->req[0].mc_cpu
>>   				+ pi->mcbufsz / 2;
>>   	thrd->req[1].mc_bus = thrd->req[0].mc_bus
>>   				+ pi->mcbufsz / 2;
>>   	thrd->req[1].r = NULL;
>> -	MARK_FREE(&thrd->req[1]);
>> +	mark_free(thrd, 1);
>>   }
>>
>>   static int dmac_alloc_threads(struct pl330_dmac *pl330)
>
> I've done some testing and I haven't been able to break it.  Can
> somebody please test this patch with the workload that failed (audio in
> exynos I believe)?
>
Tested audio on Origen board, based on EXYNOS4210. Working fine with my 
limited tests. (command line aplay).


> Thanks,
> Javi
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Tushar Behera

Re: [PATCH] ARM: PL330: Fix driver freeze

[Jassi Brar]

On 12 December 2011 00:57, Javi Merino <javi.merino@arm.com> wrote:
> Add a req_running field to the pl330_thread to track which request (if
> any) has been submitted to the DMA.  This mechanism replaces the old
> one in which we tried to guess the same by looking at the PC of the
> DMA, which could prevent the driver from sending more requests if it
> didn't guess correctly.
>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> Cc: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  arch/arm/common/pl330.c |  116 ++++++++++++++++++++---------------------------
>  1 files changed, 49 insertions(+), 67 deletions(-)
>
> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
> index f407a6b..8d8df74 100644
> --- a/arch/arm/common/pl330.c
> +++ b/arch/arm/common/pl330.c
> @@ -221,17 +221,6 @@
>  */
>  #define MCODE_BUFF_PER_REQ     256
>
> -/*
> - * Mark a _pl330_req as free.
> - * We do it by writing DMAEND as the first instruction
> - * because no valid request is going to have DMAEND as
> - * its first instruction to execute.
> - */
> -#define MARK_FREE(req) do { \
> -                               _emit_END(0, (req)->mc_cpu); \
> -                               (req)->mc_len = 0; \
> -                       } while (0)
> -
>  /* If the _pl330_req is available to the client */
>  #define IS_FREE(req)   (*((u8 *)((req)->mc_cpu)) == CMD_DMAEND)
>
> @@ -301,8 +290,10 @@ struct pl330_thread {
>        struct pl330_dmac *dmac;
>        /* Only two at a time */
>        struct _pl330_req req[2];
> -       /* Index of the last submitted request */
> +       /* Index of the last enqueued request */
>        unsigned lstenq;
> +       /* Index of the last submitted request or -1 if the DMA is stopped */
> +       int req_running;
>  };
>
>  enum pl330_dmac_state {
> @@ -778,6 +769,22 @@ static inline void _execute_DBGINSN(struct pl330_thread *thrd,
>        writel(0, regs + DBGCMD);
>  }
>
> +/*
> + * Mark a _pl330_req as free.
> + * We do it by writing DMAEND as the first instruction
> + * because no valid request is going to have DMAEND as
> + * its first instruction to execute.
> + */
> +static void mark_free(struct pl330_thread *thrd, int idx)
> +{
> +       struct _pl330_req *req = &thrd->req[idx];
> +
> +       _emit_END(0, req->mc_cpu);
> +       req->mc_len = 0;
> +
> +       thrd->req_running = -1;
> +}
> +
>  static inline u32 _state(struct pl330_thread *thrd)
>  {
>        void __iomem *regs = thrd->dmac->pinfo->base;
> @@ -836,31 +843,6 @@ static inline u32 _state(struct pl330_thread *thrd)
>        }
>  }
>
> -/* If the request 'req' of thread 'thrd' is currently active */
> -static inline bool _req_active(struct pl330_thread *thrd,
> -               struct _pl330_req *req)
> -{
> -       void __iomem *regs = thrd->dmac->pinfo->base;
> -       u32 buf = req->mc_bus, pc = readl(regs + CPC(thrd->id));
> -
> -       if (IS_FREE(req))
> -               return false;
> -
> -       return (pc >= buf && pc <= buf + req->mc_len) ? true : false;
> -}
> -
> -/* Returns 0 if the thread is inactive, ID of active req + 1 otherwise */
> -static inline unsigned _thrd_active(struct pl330_thread *thrd)
> -{
> -       if (_req_active(thrd, &thrd->req[0]))
> -               return 1; /* First req active */
> -
> -       if (_req_active(thrd, &thrd->req[1]))
> -               return 2; /* Second req active */
> -
> -       return 0;
> -}
> -
>  static void _stop(struct pl330_thread *thrd)
>  {
>        void __iomem *regs = thrd->dmac->pinfo->base;
> @@ -892,17 +874,22 @@ static bool _trigger(struct pl330_thread *thrd)
>        struct _arg_GO go;
>        unsigned ns;
>        u8 insn[6] = {0, 0, 0, 0, 0, 0};
> +       int idx;
>
>        /* Return if already ACTIVE */
>        if (_state(thrd) != PL330_STATE_STOPPED)
>                return true;
>
> -       if (!IS_FREE(&thrd->req[1 - thrd->lstenq]))
> -               req = &thrd->req[1 - thrd->lstenq];
> -       else if (!IS_FREE(&thrd->req[thrd->lstenq]))
> -               req = &thrd->req[thrd->lstenq];
> -       else
> -               req = NULL;
> +       idx = 1 - thrd->lstenq;
> +       if (!IS_FREE(&thrd->req[idx]))
> +               req = &thrd->req[idx];
> +       else {
> +               idx = thrd->lstenq;
> +               if (!IS_FREE(&thrd->req[idx]))
> +                       req = &thrd->req[idx];
> +               else
> +                       req = NULL;
> +       }
>
>        /* Return if no request */
>        if (!req || !req->r)
> @@ -933,6 +920,8 @@ static bool _trigger(struct pl330_thread *thrd)
>        /* Only manager can execute GO */
>        _execute_DBGINSN(thrd, insn, true);
>
> +       thrd->req_running = idx;
> +
>        return true;
>  }
>
> @@ -1382,8 +1371,8 @@ static void pl330_dotask(unsigned long data)
>
>                        thrd->req[0].r = NULL;
>                        thrd->req[1].r = NULL;
> -                       MARK_FREE(&thrd->req[0]);
> -                       MARK_FREE(&thrd->req[1]);
> +                       mark_free(thrd, 0);
> +                       mark_free(thrd, 1);
>
>                        /* Clear the reset flag */
>                        pl330->dmac_tbd.reset_chan &= ~(1 << i);
> @@ -1461,14 +1450,12 @@ int pl330_update(const struct pl330_info *pi)
>
>                        thrd = &pl330->channels[id];
>
> -                       active = _thrd_active(thrd);
> -                       if (!active) /* Aborted */
> +                       active = thrd->req_running;
> +                       if (active == -1) /* Aborted */
>                                continue;
>
> -                       active -= 1;
> -
>                        rqdone = &thrd->req[active];
> -                       MARK_FREE(rqdone);
> +                       mark_free(thrd, active);
>
>                        /* Get going again ASAP */
>                        _start(thrd);
> @@ -1509,7 +1496,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
>        struct pl330_thread *thrd = ch_id;
>        struct pl330_dmac *pl330;
>        unsigned long flags;
> -       int ret = 0, active;
> +       int ret = 0, active = thrd->req_running;
>
>        if (!thrd || thrd->free || thrd->dmac->state == DYING)
>                return -EINVAL;
> @@ -1525,28 +1512,24 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
>
>                thrd->req[0].r = NULL;
>                thrd->req[1].r = NULL;
> -               MARK_FREE(&thrd->req[0]);
> -               MARK_FREE(&thrd->req[1]);
> +               mark_free(thrd, 0);
> +               mark_free(thrd, 1);
>                break;
>
>        case PL330_OP_ABORT:
> -               active = _thrd_active(thrd);
> -
>                /* Make sure the channel is stopped */
>                _stop(thrd);
>
>                /* ABORT is only for the active req */
> -               if (!active)
> +               if (active == -1)
>                        break;
>
> -               active--;
> -
>                thrd->req[active].r = NULL;
> -               MARK_FREE(&thrd->req[active]);
> +               mark_free(thrd, active);
>
>                /* Start the next */
>        case PL330_OP_START:
> -               if (!_thrd_active(thrd) && !_start(thrd))
> +               if ((active == -1) && !_start(thrd))
>                        ret = -EIO;
>                break;
>
> @@ -1587,14 +1570,13 @@ int pl330_chan_status(void *ch_id, struct pl330_chanstatus *pstatus)
>        else
>                pstatus->faulting = false;
>
> -       active = _thrd_active(thrd);
> +       active = thrd->req_running;
>
> -       if (!active) {
> +       if (active == -1) {
>                /* Indicate that the thread is not running */
>                pstatus->top_req = NULL;
>                pstatus->wait_req = NULL;
>        } else {
> -               active--;
>                pstatus->top_req = thrd->req[active].r;
>                pstatus->wait_req = !IS_FREE(&thrd->req[1 - active])
>                                        ? thrd->req[1 - active].r : NULL;
> @@ -1659,9 +1641,9 @@ void *pl330_request_channel(const struct pl330_info *pi)
>                                thrd->free = false;
>                                thrd->lstenq = 1;
>                                thrd->req[0].r = NULL;
> -                               MARK_FREE(&thrd->req[0]);
> +                               mark_free(thrd, 0);
>                                thrd->req[1].r = NULL;
> -                               MARK_FREE(&thrd->req[1]);
> +                               mark_free(thrd, 1);
>                                break;
>                        }
>                }
> @@ -1767,14 +1749,14 @@ static inline void _reset_thread(struct pl330_thread *thrd)
>        thrd->req[0].mc_bus = pl330->mcode_bus
>                                + (thrd->id * pi->mcbufsz);
>        thrd->req[0].r = NULL;
> -       MARK_FREE(&thrd->req[0]);
> +       mark_free(thrd, 0);
>
>        thrd->req[1].mc_cpu = thrd->req[0].mc_cpu
>                                + pi->mcbufsz / 2;
>        thrd->req[1].mc_bus = thrd->req[0].mc_bus
>                                + pi->mcbufsz / 2;
>        thrd->req[1].r = NULL;
> -       MARK_FREE(&thrd->req[1]);
> +       mark_free(thrd, 1);
>  }
>
>  static int dmac_alloc_threads(struct pl330_dmac *pl330)
> --
Seems ok, except that 'unsigned lstenq' and 'int req_running' occupy
real estate more than they should for their purpose. Though otherwise
the necessary helpers would bloat the code too.

Hoping you have done testing of enough corner cases...
Acked-by: Jassi Brar <jaswinder.singh@linaro.org>

Thanks.