| Message ID | CABb+yY0BGHSMA08uDMNkdSehFkQ_9afzzbkdpPkD0cz3owVWBQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 19/09/11 19:07, Jassi Brar wrote: > On Mon, Sep 19, 2011 at 10:41 PM, 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 moves the _state() check out of the critical section, which >> removes the race condition. It also treats PL330_STATE_COMPLETING as >> still executing, because that introduces another race condition now >> that we call _state() with interrupts enabled. Namely, if we read the >> state as "completing" and the DMA sends the interrupt before we >> disable interrupts, pl330_update() starts the next transaction and >> returns. Then the _start() in pl330_chan_ctrl() will patiently wait >> until the just issued transaction finishes (because the state we read >> was PL330_STATE_COMPLETING) and when it does, it _trigger()s the same >> transaction again. >> >> Signed-off-by: Javi Merino <javi.merino@arm.com> >> Cc: Jassi Brar <jassi.brar@samsung.com> >> --- >> arch/arm/common/pl330.c | 12 +++++++----- >> 1 files changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c >> index 97912fa..26b5615 100644 >> --- a/arch/arm/common/pl330.c >> +++ b/arch/arm/common/pl330.c >> @@ -936,9 +936,9 @@ static bool _trigger(struct pl330_thread *thrd) >> return true; >> } >> >> -static bool _start(struct pl330_thread *thrd) >> +static bool _start(struct pl330_thread *thrd, u32 state) >> { >> - switch (_state(thrd)) { >> + switch (state) { >> case PL330_STATE_FAULT_COMPLETING: >> UNTIL(thrd, PL330_STATE_FAULTING | PL330_STATE_KILLING); >> >> @@ -949,7 +949,6 @@ static bool _start(struct pl330_thread *thrd) >> _stop(thrd); >> >> case PL330_STATE_KILLING: >> - case PL330_STATE_COMPLETING: >> UNTIL(thrd, PL330_STATE_STOPPED) >> >> case PL330_STATE_STOPPED: >> @@ -961,6 +960,7 @@ static bool _start(struct pl330_thread *thrd) >> case PL330_STATE_UPDTPC: >> case PL330_STATE_CACHEMISS: >> case PL330_STATE_EXECUTING: >> + case PL330_STATE_COMPLETING: >> return true; >> >> case PL330_STATE_WFE: /* For RESUME, nothing yet */ >> @@ -1471,7 +1471,7 @@ int pl330_update(const struct pl330_info *pi) >> MARK_FREE(rqdone); >> >> /* Get going again ASAP */ >> - _start(thrd); >> + _start(thrd, _state(thrd)); >> >> /* For now, just make a list of callbacks to be done */ >> list_add_tail(&rqdone->rqd, &pl330->req_done); >> @@ -1510,12 +1510,14 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op) >> struct pl330_dmac *pl330; >> unsigned long flags; >> int ret = 0, active; >> + u32 dma_state; >> >> if (!thrd || thrd->free || thrd->dmac->state == DYING) >> return -EINVAL; >> >> pl330 = thrd->dmac; >> >> + dma_state = _state(thrd); >> spin_lock_irqsave(&pl330->lock, flags); >> >> switch (op) { >> @@ -1546,7 +1548,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op) >> >> /* Start the next */ >> case PL330_OP_START: >> - if (!_start(thrd)) >> + if (!_start(thrd, dma_state)) >> ret = -EIO; >> break; >> >> -- > > IIUIC your race scenario should be taken care of simply by doing... > > 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; > My first reaction was this was just moving the race condition, but thinking about it carefully, it looks like a better (simpler) solution. > Could you please test if it fixes the issue? Yes, I'll kick off runs later today and see how it goes. I don't have a way to reproduce the bug consistently, I just do lots of DMA transactions and eventually, I end up hitting it. I'll run it overnight and see if it fixes it or not. Cheers, Javi
On 20/09/11 14:36, Javi Merino wrote: > On 19/09/11 19:07, Jassi Brar wrote: >> IIUIC your race scenario should be taken care of simply by doing... >> >> 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; >> > > My first reaction was this was just moving the race condition, but > thinking about it carefully, it looks like a better (simpler) solution. > >> Could you please test if it fixes the issue? > > Yes, I'll kick off runs later today and see how it goes. I don't have a > way to reproduce the bug consistently, I just do lots of DMA > transactions and eventually, I end up hitting it. I'll run it overnight > and see if it fixes it or not. I have run a lot of tests using your patch and I haven't been able to reproduce the bug so your proposed solution is probably a better (simpler) fix than mine. Cheers, Javi
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 >
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
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 > >
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
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
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
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
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.
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.
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
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
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.
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.
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
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.
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.
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.
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
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
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.
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
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;