Re: [PATCH v2] ARM: pl330: Fix a race condition
On Sat, Dec 10, 2011 at 1:20 AM, Javi Merino javi.mer...@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;
Re: [PATCH v2] ARM: pl330: Fix a race condition
On 11/12/11 10:51, Jassi Brar wrote: On Sat, Dec 10, 2011 at 1:20 AM, Javi Merino javi.mer...@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. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] ARM: pl330: Fix a race condition
On 11 December 2011 20:39, Javi Merino javi.mer...@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. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] ARM: pl330: Fix a race condition
On 11/12/11 17:10, Jassi Brar wrote: On 11 December 2011 20:39, Javi Merino javi.mer...@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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] ARM: pl330: Fix a race condition
On 07/12/11 20:54, Javi Merino wrote: On 07/12/11 10:01, Javi Merino wrote: On 07/12/11 07:52, Kukjin Kim wrote: Jassi Brar wrote: On 29 November 2011 15:23, Javi Merino javi.mer...@arm.com wrote: On Samsung's Exynos4 platform, while testing audio playback with i2s interface, the above change causes the playback to freeze. The _thrd_active(thrd) call always returns '1' and hence _start(thrd) is not getting called. Your patch makes the memcpy operation on dmatest.c and net DMA be frozen too as well as Samsung audio playback. Javi, could you please check if you too get the memcpy failure with dmatest ? Ok, I think I've just reproduced it in my end with the kernel's dmatest module. After the first transaction it looks like the dma test wasn't able to issue any more transactions. I'll have a look at it tomorrow and try to answer my own questions ;) The problem is that pl330_submit_req() always puts the request in buffer 0 if both buffers are free. If you submit a transaction, it finishes and there's nothing else to run, pl330_update() calls _start() but there is nothing to send. This is all right. Then, if another transaction is submitted, pl330_submit_req() will put it in buffer 0 again. This time, the PC of the DMA is in the last instruction of buffer 0 (the DMAEND of the *previous* transaction that finished long ago) so _thrd_active() thinks that this buffer is active, which makes pl330_chan_ctrl() not start the transaction and hence, the DMA freezes. I believe something like the following should fix it: --- a/arch/arm/common/pl330.c +++ b/arch/arm/common/pl330.c @@ -1301,7 +1301,7 @@ int pl330_submit_req(void *ch_id, struct pl330_req *r) goto xfer_exit; } - idx = IS_FREE(thrd-req[0]) ? 0 : 1; + idx = IS_FREE(thrd-req[1 - thrd-lstenq]) ? 1 - thrd-lstenq : thrd-lstenq; xs.ccr = ccr; xs.r = r; I have only tested this with a single-channel single-thread dma test. I'll send a proper patch to the list once I've done more testing. Cheers, Javi -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] ARM: pl330: Fix a race condition
Hi Javi, On 9 December 2011 17:28, Javi Merino javi.mer...@arm.com wrote: Javi, could you please check if you too get the memcpy failure with dmatest ? Ok, I think I've just reproduced it in my end with the kernel's dmatest module. After the first transaction it looks like the dma test wasn't able to issue any more transactions. I'll have a look at it tomorrow and try to answer my own questions ;) The problem is that pl330_submit_req() always puts the request in buffer 0 if both buffers are free. There should be nothing wrong in that. It is but natural to prefer 0 if both 0 and 1 are available. If you submit a transaction, it finishes and there's nothing else to run, pl330_update() calls _start() but there is nothing to send. This is all right. Then, if another transaction is submitted, pl330_submit_req() will put it in buffer 0 again. This time, the PC of the DMA is in the last instruction of buffer 0 (the DMAEND of the *previous* transaction that finished long ago) so _thrd_active() thinks that this buffer is active, Many thanks for the in-depth analysis. Though before the PC check, the IS_FREE() should return true since pl330_update() does MARK_FREE() Could you please check if the client's callback function called successfully for the first submitted transfer ? Then we can decide upon one of the following 2 options (I *think* either should fix the issue) diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c index 97912fa..f550d46 100644 --- a/arch/arm/common/pl330.c +++ b/arch/arm/common/pl330.c @@ -846,7 +846,7 @@ static inline bool _req_active(struct pl330_thread *thrd, if (IS_FREE(req)) return false; - return (pc = buf pc = buf + req-mc_len) ? true : false; + return (pc = buf pc buf + req-mc_len) ? true : false; } --OR-- diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c index 97912fa..ad85a75 100644 --- a/arch/arm/common/pl330.c +++ b/arch/arm/common/pl330.c @@ -233,7 +233,7 @@ } while (0) /* If the _pl330_req is available to the client */ -#define IS_FREE(req) (*((u8 *)((req)-mc_cpu)) == CMD_DMAEND) +#define IS_FREE(req) ((req)-mc_len == 0) /* Use this _only_ to wait on transient states */ #define UNTIL(t, s)while (!(_state(t) (s))) cpu_relax(); Thanks. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] ARM: pl330: Fix a race condition
On Fri, Dec 9, 2011 at 8:22 PM, Javi Merino javi.mer...@arm.com wrote: I think the best solution would be to revert ee3f615819404a9438b2dd01b7a39f276d2737f2 and go back to my original patch (in the beginning of this thread): http://article.gmane.org/gmane.linux.ports.arm.kernel/133110 What do you think? Well, we have to resort to that if we can't find something simpler. What do you think about ... diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c index f407a6b..3a51cdd 100644 --- a/arch/arm/common/pl330.c +++ b/arch/arm/common/pl330.c @@ -1546,7 +1546,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op) /* Start the next */ case PL330_OP_START: - if (!_thrd_active(thrd) !_start(thrd)) + if (_state(thrd) == PL330_STATE_STOPPED !_start(thrd)) ret = -EIO; break; Thanks -jassi [Sorry I don't have any pl330 platform handy] -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] ARM: pl330: Fix a race condition
On 09/12/11 16:50, Jassi Brar wrote:
What do you think about ...
diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index f407a6b..3a51cdd 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -1546,7 +1546,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op)
/* Start the next */
case PL330_OP_START:
- if (!_thrd_active(thrd) !_start(thrd))
+ if (_state(thrd) == PL330_STATE_STOPPED !_start(thrd))
Reintroduces the race condition, but you shorten the window:
* pl330_submit_req()
* pl330_chan_ctrl(PL330_OP_START)
* pl330_submit_req()
* pl330_chan_ctrl():spin_lock_irqsave()
* Transfer 1 finishes, interrupt raised (but pl330_update() is not
called as interrupts are disabled)
* _state(thrd) is PL330_STATE_STOPPED , _start() reissues the first
transaction.
* pl330_chan_ctrl():spin_lock_irqrestore()
* pl330_update() called for the first transaction, but it is running
again!
What about properly tracking what we have sent to the DMA? Something
like the following (warning *ugly* and untested code ahead, may eat your
kitten):
diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index f407a6b..3652c4b 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -303,6 +303,7 @@ struct pl330_thread {
struct _pl330_req req[2];
/* Index of the last submitted request */
unsigned lstenq;
+ int req_running;
};
enum pl330_dmac_state {
@@ -836,31 +837,6 @@ static inline u32 _state(struct pl330_thread *thrd)
}
}
-/* If the request 'req' of thread 'thrd' is currently active */
-static inline bool _req_active(struct pl330_thread *thrd,
- struct _pl330_req *req)
-{
- void __iomem *regs = thrd-dmac-pinfo-base;
- u32 buf = req-mc_bus, pc = readl(regs + CPC(thrd-id));
-
- if (IS_FREE(req))
- return false;
-
- return (pc = buf pc = buf + req-mc_len) ? true : false;
-}
-
-/* Returns 0 if the thread is inactive, ID of active req + 1 otherwise */
-static inline unsigned _thrd_active(struct pl330_thread *thrd)
-{
- if (_req_active(thrd, thrd-req[0]))
- return 1; /* First req active */
-
- if (_req_active(thrd, thrd-req[1]))
- return 2; /* Second req active */
-
- return 0;
-}
-
static void _stop(struct pl330_thread *thrd)
{
void __iomem *regs = thrd-dmac-pinfo-base;
@@ -897,11 +873,13 @@ static bool _trigger(struct pl330_thread *thrd)
if (_state(thrd) != PL330_STATE_STOPPED)
return true;
- if (!IS_FREE(thrd-req[1 - thrd-lstenq]))
+ if (!IS_FREE(thrd-req[1 - thrd-lstenq])) {
req = thrd-req[1 - thrd-lstenq];
- else if (!IS_FREE(thrd-req[thrd-lstenq]))
+ thrd-req_running = 2 - thrd-lstenq;
+ } else if (!IS_FREE(thrd-req[thrd-lstenq])) {
req = thrd-req[thrd-lstenq];
- else
+ thrd-req_running = 1 + thrd-lstenq;
+ } else
req = NULL;
/* Return if no request */
@@ -1384,6 +1362,7 @@ static void pl330_dotask(unsigned long data)
thrd-req[1].r = NULL;
MARK_FREE(thrd-req[0]);
MARK_FREE(thrd-req[1]);
+ thrd-req_running = 0;
/* Clear the reset flag */
pl330-dmac_tbd.reset_chan = ~(1 i);
@@ -1461,7 +1440,7 @@ int pl330_update(const struct pl330_info *pi)
thrd = pl330-channels[id];
- active = _thrd_active(thrd);
+ active = thrd-req_running;
if (!active) /* Aborted */
continue;
@@ -1469,6 +1448,7 @@ int pl330_update(const struct pl330_info *pi)
rqdone = thrd-req[active];
MARK_FREE(rqdone);
+ thrd-req_running = 0;
/* Get going again ASAP */
_start(thrd);
@@ -1527,10 +1507,11 @@ int pl330_chan_ctrl(void *ch_id, enum
pl330_chan_op op)
thrd-req[1].r = NULL;
MARK_FREE(thrd-req[0]);
MARK_FREE(thrd-req[1]);
+ thrd-req_running = 0;
break;
case PL330_OP_ABORT:
- active = _thrd_active(thrd);
+ active = thrd-req_running;
/* Make sure the channel is stopped */
_stop(thrd);
@@ -1543,10 +1524,11 @@ int pl330_chan_ctrl(void *ch_id, enum
pl330_chan_op op)
thrd-req[active].r = NULL;
MARK_FREE(thrd-req[active]);
+ thrd-req_running = 0;
/* Start the next */
case PL330_OP_START:
- if (!_thrd_active(thrd) !_start(thrd))
+ if (!thrd-req_running !_start(thrd))
ret = -EIO;
Re: [PATCH v2] ARM: pl330: Fix a race condition
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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] ARM: pl330: Fix a race condition
On 29 November 2011 15:23, Javi Merino javi.mer...@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. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] ARM: pl330: Fix a race condition
Javi Merino 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] ARM: pl330: Fix a race condition
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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] ARM: pl330: Fix a race condition
Javi Merino 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-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] ARM: pl330: Fix a race condition
On 05/11/11 19:05, Thomas Abraham wrote: Hi Javi, On 6 October 2011 05:10, Javi Merino javi.mer...@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.mer...@arm.com Acked-by: Jassi Brar jassi.b...@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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] ARM: pl330: Fix a race condition
On 7 November 2011 16:18, Javi Merino javi.mer...@arm.com wrote: On 05/11/11 19:05, Thomas Abraham wrote: Hi Javi, On 6 October 2011 05:10, Javi Merino javi.mer...@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.mer...@arm.com Acked-by: Jassi Brar jassi.b...@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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] ARM: pl330: Fix a race condition
Hi Javi, On 6 October 2011 05:10, Javi Merino javi.mer...@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.mer...@arm.com Acked-by: Jassi Brar jassi.b...@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-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
