ARM: pl330: Fix a race condition

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 <[email protected]>

Cc Jassi Brar <[email protected]>

---
?arch/arm/common/pl330.c | ? 12 +++++++-----
?1 files changed, 7 insertions(+), 5 deletions(-)

arch/arm/common/pl330.c +2 -2

index 97912fa..26b5615 100644
@@ -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);

arch/arm/common/pl330.c +0 -0

@@ -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:

arch/arm/common/pl330.c +0 -0

@@ -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 */

arch/arm/common/pl330.c +0 -0

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

arch/arm/common/pl330.c +2 -0

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

--

Quoted 83 lines · Javi Merino

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 <[email protected]>

Cc Jassi Brar <[email protected]>

---
 arch/arm/common/pl330.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

arch/arm/common/pl330.c +6 -4

index 97912fa..26b5615 100644
@@ -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) {

arch/arm/common/pl330.c int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)

Quoted 9 lines

arch/arm/common/pl330.c +1 -2

@@ -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...

arch/arm/common/pl330.c +0 -0

index 97912fa..7129cfb 100644

Could you please test if it fixes the issue?

Quoted 7 lines · Jassi Brar

IIUIC your race scenario should be taken care of simply by doing...

arch/arm/common/pl330.c +0 -0

index 97912fa..7129cfb 100644

arch/arm/common/pl330.c int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op) · line 1552

Quoted 8 lines

arch/arm/common/pl330.c +1 -1

@@ -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.

Quoted 1 line

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.

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 <[email protected]>

Acked-by Jassi Brar <[email protected]>

---
?arch/arm/common/pl330.c | ? ?2 +-
?1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/common/pl330.c +0 -0

index 97912fa..7129cfb 100644
@@ -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;

--
1.7.0.4

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

Hi Javi,

Quoted 25 lines · Javi Merino

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 <[email protected]>

Acked-by Jassi Brar <[email protected]>

---
 arch/arm/common/pl330.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/common/pl330.c +0 -0

index 97912fa..7129cfb 100644
@@ -1546,7 +1546,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
 

arch/arm/common/pl330.c int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op) · line 1551

Quoted 6 lines

arch/arm/common/pl330.c +1 -1

               /* 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.

Quoted 28 lines · Thomas Abraham

Hi Javi,

Quoted 25 lines · Javi Merino

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 <[email protected]>

Acked-by Jassi Brar <[email protected]>

---
?arch/arm/common/pl330.c | ? ?2 +-
?1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/common/pl330.c +0 -0

index 97912fa..7129cfb 100644
@@ -1546,7 +1546,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
 

Quoted 11 lines

Quoted 6 lines

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

Quoted 4 lines

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

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

Quoted 18 lines · Javi Merino

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

On Samsung's Exynos4 platform, while testing audio playback with

i2s

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

interface, the above change causes the playback to freeze. The _thrd_active(thrd) call always returns '1' and hence _start(thrd)

is

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

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

Quoted 7 lines

Quoted 7 lines

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

Quoted 1 line

Quoted 1 line

_thrd_active() should return '0'. If _thrd_active() is still '1',

then

Quoted 5 lines

Quoted 4 lines

something has gone wrong in the way.

Does this shed some light?

Quoted 1 line

Your patch makes the memcpy operation on dmatest.c and net DMA be

frozen too

Quoted 1 line

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.

Quoted 1 line

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

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

Quoted 27 lines · Javi Merino

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

On Samsung's Exynos4 platform, while testing audio playback with

i2s

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

interface, the above change causes the playback to freeze. The _thrd_active(thrd) call always returns '1' and hence _start(thrd)

is

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

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

Quoted 7 lines

Quoted 7 lines

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

Quoted 1 line

Quoted 1 line

_thrd_active() should return '0'. If _thrd_active() is still '1',

then

Quoted 5 lines

Quoted 4 lines

something has gone wrong in the way.

Does this shed some light?

Quoted 6 lines

Quoted 1 line

Your patch makes the memcpy operation on dmatest.c and net DMA be

frozen too

Quoted 1 line

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.

Quoted 26 lines

Quoted 26 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

On Samsung's Exynos4 platform, while testing audio playback with

i2s

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

interface, the above change causes the playback to freeze. The _thrd_active(thrd) call always returns '1' and hence _start(thrd)

is

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

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

Quoted 7 lines

Quoted 7 lines

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

Quoted 1 line

Quoted 1 line

_thrd_active() should return '0'. ?If _thrd_active() is still '1',

then

Quoted 5 lines

Quoted 4 lines

something has gone wrong in the way.

Does this shed some light?

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

Your patch makes the memcpy operation on dmatest.c and net DMA be

frozen too

Quoted 1 line

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.

Quoted 38 lines · Javi Merino

Quoted 26 lines

Quoted 26 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

On Samsung's Exynos4 platform, while testing audio playback with

i2s

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

interface, the above change causes the playback to freeze. The _thrd_active(thrd) call always returns '1' and hence _start(thrd)

is

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

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

Quoted 7 lines

Quoted 7 lines

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

Quoted 1 line

Quoted 1 line

_thrd_active() should return '0'. ?If _thrd_active() is still '1',

then

Quoted 5 lines

Quoted 4 lines

something has gone wrong in the way.

Does this shed some light?

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

Your patch makes the memcpy operation on dmatest.c and net DMA be

frozen too

Quoted 1 line

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?

Quoted 8 lines

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 ?

Quoted 48 lines · Jassi Brar

Quoted 46 lines · Javi Merino

Quoted 26 lines

Quoted 26 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

On Samsung's Exynos4 platform, while testing audio playback with

i2s

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

interface, the above change causes the playback to freeze. The _thrd_active(thrd) call always returns '1' and hence _start(thrd)

is

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

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

Quoted 7 lines

Quoted 7 lines

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

Quoted 1 line

Quoted 1 line

_thrd_active() should return '0'. If _thrd_active() is still '1',

then

Quoted 5 lines

Quoted 4 lines

something has gone wrong in the way.

Does this shed some light?

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

Your patch makes the memcpy operation on dmatest.c and net DMA be

frozen too

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 52 lines · Kukjin Kim

Quoted 50 lines · Jassi Brar

Quoted 38 lines · Javi Merino

Quoted 26 lines

Quoted 26 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

On Samsung's Exynos4 platform, while testing audio playback with

i2s

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

interface, the above change causes the playback to freeze. The _thrd_active(thrd) call always returns '1' and hence _start(thrd)

is

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

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

Quoted 7 lines

Quoted 7 lines

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

Quoted 1 line

Quoted 1 line

_thrd_active() should return '0'. If _thrd_active() is still '1',

then

Quoted 5 lines

Quoted 4 lines

something has gone wrong in the way.

Does this shed some light?

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

Your patch makes the memcpy operation on dmatest.c and net DMA be

frozen too

Quoted 1 line

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?

Quoted 8 lines

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 ?

Quoted 4 lines

(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.

Quoted 14 lines · Javi Merino

Quoted 13 lines · Kukjin Kim

Quoted 12 lines · Jassi Brar

Quoted 11 lines · Javi Merino

Quoted 11 lines

Quoted 11 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

On Samsung's Exynos4 platform, while testing audio playback with

i2s

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

interface, the above change causes the playback to freeze. The _thrd_active(thrd) call always returns '1' and hence _start(thrd)

is

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

not getting called.

Your patch makes the memcpy operation on dmatest.c and net DMA be

frozen too

Quoted 1 line

as well as Samsung audio playback.

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

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

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Javi, could you please check if you too get the memcpy failure with dmatest ?

Quoted 4 lines

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,

Hi Javi,

Quoted 8 lines · Javi Merino

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

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.

Quoted 7 lines

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)

arch/arm/common/pl330.c +0 -0

index 97912fa..f550d46 100644

Quoted 3 lines

The problem is that pl330_submit_req() always puts the request in buffer 0 if

Quoted 2 lines

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.

Quoted 18 lines · Jassi Brar

Hi Javi,

Quoted 8 lines · Javi Merino

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

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.

Quoted 7 lines

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,

Quoted 8 lines

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.

Quoted 28 lines · Javi Merino

Quoted 18 lines · Jassi Brar

Hi Javi,

Quoted 8 lines · Javi Merino

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

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.

Quoted 7 lines

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,

Quoted 8 lines

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 ?

Quoted 7 lines

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.

(quoted diff) +1 -1

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

: thrd->lstenq;

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?

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

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

arch/arm/common/pl330.c +0 -0

index f407a6b..3652c4b 100644
@@ -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 {

arch/arm/common/pl330.c +5 -28

@@ -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 */

arch/arm/common/pl330.c +1 -0

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

arch/arm/common/pl330.c +1 -1

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

arch/arm/common/pl330.c +1 -0

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

arch/arm/common/pl330.c +1 -0

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

arch/arm/common/pl330.c +1 -0

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

arch/arm/common/pl330.c +0 -0

@@ -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 */

arch/arm/common/pl330.c +1 -0

@@ -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;
 ? ? ? ? ? ? ? ? ? ? ? ?}

? ? ? ? ? ? ? ?}

arch/arm/common/pl330.c +2 -0

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

Quoted 5 lines · Javi Merino

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.

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

Quoted 5 lines

Quoted 5 lines

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

Quoted 3 lines

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.

Quoted 9 lines · Javi Merino

Quoted 8 lines

Quoted 5 lines

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.

Quoted 5 lines

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.

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 <[email protected]>

Cc Jassi Brar <[email protected]>

---
 arch/arm/common/pl330.c |  116 ++++++++++++++++++++---------------------------
 1 files changed, 49 insertions(+), 67 deletions(-)

arch/arm/common/pl330.c +48 -66

index f407a6b..8d8df74 100644
@@ -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);
 

Quoted 256 lines · 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<[email protected]>

Cc Jassi Brar<[email protected]>

---
  arch/arm/common/pl330.c |  116 ++++++++++++++++++++---------------------------
  1 files changed, 49 insertions(+), 67 deletions(-)

arch/arm/common/pl330.c +49 -67

index f407a6b..8d8df74 100644
@@ -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);
  }
 

arch/arm/common/pl330.c static inline void _reset_thread(struct pl330_thread *thrd) · line 1762

Quoted 1 line

arch/arm/common/pl330.c +0 -0

  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

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

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 <[email protected]>

Cc Jassi Brar <[email protected]>

---
?arch/arm/common/pl330.c | ?116 ++++++++++++++++++++--------------------------- ?1 files changed, 49 insertions(+), 67 deletions(-)

arch/arm/common/pl330.c +0 -0

index f407a6b..8d8df74 100644
@@ -221,17 +221,6 @@

?*/
?#define MCODE_BUFF_PER_REQ ? ? 256

arch/arm/common/pl330.c +0 -11

-/*
- * 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)

arch/arm/common/pl330.c +0 -0

@@ -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 {

arch/arm/common/pl330.c +17 -25

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

arch/arm/common/pl330.c +0 -0

@@ -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;
?}

arch/arm/common/pl330.c +2 -2

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

arch/arm/common/pl330.c +3 -5

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

arch/arm/common/pl330.c +3 -3

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

arch/arm/common/pl330.c +0 -0

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

arch/arm/common/pl330.c +2 -2

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