* [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
@ 2012-10-28 14:17 ` Dmitry Osipenko
0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Osipenko @ 2012-10-28 14:17 UTC (permalink / raw)
To: digetx-Re5JQEeQqe8AvxtiuMwx3w
Cc: vinod.koul-ral2JQCrhuEAvxtiuMwx3w,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Fixed channel "lock" after free.
Example: Channel 1 was allocated and prepared as slave_sg, used and freed. Now preparation of cyclic dma on channel 1 will fail with err "DMA
configuration conflict" because tdc->isr_handler still selected to handle_once_dma_done.
This happens because tegra_dma_abort_all() won't be called on channel freeing if pending list is empty.
Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/dma/tegra20-apb-dma.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 4d816be..00c5dee 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1147,6 +1147,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
if (tdc->busy)
tegra_dma_terminate_all(dc);
+ tdc->isr_handler = NULL;
spin_lock_irqsave(&tdc->lock, flags);
list_splice_init(&tdc->pending_sg_req, &sg_req_list);
--
1.7.12
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
@ 2012-10-28 14:17 ` Dmitry Osipenko
0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Osipenko @ 2012-10-28 14:17 UTC (permalink / raw)
To: digetx; +Cc: vinod.koul, linux-tegra, linux-kernel
Fixed channel "lock" after free.
Example: Channel 1 was allocated and prepared as slave_sg, used and freed. Now preparation of cyclic dma on channel 1 will fail with err "DMA
configuration conflict" because tdc->isr_handler still selected to handle_once_dma_done.
This happens because tegra_dma_abort_all() won't be called on channel freeing if pending list is empty.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/dma/tegra20-apb-dma.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 4d816be..00c5dee 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1147,6 +1147,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
if (tdc->busy)
tegra_dma_terminate_all(dc);
+ tdc->isr_handler = NULL;
spin_lock_irqsave(&tdc->lock, flags);
list_splice_init(&tdc->pending_sg_req, &sg_req_list);
--
1.7.12
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
2012-10-28 14:17 ` Dmitry Osipenko
(?)
@ 2012-10-29 15:27 ` Stephen Warren
2012-10-29 23:20 ` [PATCH V2] dma: tegra: avoid channel lock up after free Dmitry Osipenko
-1 siblings, 1 reply; 11+ messages in thread
From: Stephen Warren @ 2012-10-29 15:27 UTC (permalink / raw)
To: Dmitry Osipenko, Laxman Dewangan; +Cc: vinod.koul, linux-tegra, linux-kernel
On 10/28/2012 08:17 AM, Dmitry Osipenko wrote:
> Fixed channel "lock" after free.
>
> Example: Channel 1 was allocated and prepared as slave_sg, used and freed. Now preparation of cyclic dma on channel 1 will fail with err "DMA
> configuration conflict" because tdc->isr_handler still selected to handle_once_dma_done.
>
> This happens because tegra_dma_abort_all() won't be called on channel freeing if pending list is empty.
That commit description isn't correctly wrapped.
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> @@ -1147,6 +1147,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>
> if (tdc->busy)
> tegra_dma_terminate_all(dc);
> + tdc->isr_handler = NULL;
Should we remove that assignment from tegra_dma_abort_all(); perhaps it
is redundant now?
Actually, I wonder if the correct fix isn't to:
a) Always call tegra_dma_terminate_all() from
tegra_dma_free_chan_resources() irrespective of busy state.
b) Make tegra_dma_terminate_all() always call tegra_dma_abort_all()
irrespective of whether list_empty(&tdc->pending_sg_req).
But then I wonder: should tdc->isr_handler get left set to non-NULL in
the scenario mentioned in your commit description at all; should it be
cleared as soon as the channel is idle in all cases, so that it doesn't
need to be cleared when freeing the channel?
I CC'd Laxman, the driver author, to comment here.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH V2] dma: tegra: avoid channel lock up after free
2012-10-29 15:27 ` Stephen Warren
@ 2012-10-29 23:20 ` Dmitry Osipenko
2012-10-29 23:28 ` Dmitry Osipenko
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Dmitry Osipenko @ 2012-10-29 23:20 UTC (permalink / raw)
To: swarren; +Cc: digetx, vinod.koul, linux-tegra, linux-kernel, ldewangan
Fixed channel "lock up" after free.
Lock scenario: Channel 1 was allocated and prepared as slave_sg, used and freed.
Now preparation of cyclic dma on channel 1 will fail with err "DMA configuration
conflict" because tdc->isr_handler still selected to handle_once_dma_done.
This happens because tegra_dma_abort_all() won't be called on channel freeing
if pending list is empty or channel not busy. We need to clear isr_handler
on channel freeing to avoid locking. Also I added small optimization to prepare
functions, so current channel type checked before making allocations.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/dma/tegra20-apb-dma.c | 60 ++++++++++++++++++-------------------------
1 file changed, 25 insertions(+), 35 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 4d816be..5a557af 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -681,11 +681,6 @@ static void tegra_dma_terminate_all(struct dma_chan *dc)
bool was_busy;
spin_lock_irqsave(&tdc->lock, flags);
- if (list_empty(&tdc->pending_sg_req)) {
- spin_unlock_irqrestore(&tdc->lock, flags);
- return;
- }
-
if (!tdc->busy)
goto skip_dma_stop;
@@ -896,6 +891,15 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
return NULL;
}
+ /*
+ * Make sure that mode should not be conflicting with currently
+ * configured mode.
+ */
+ if (tdc->isr_handler && tdc->isr_handler != handle_once_dma_done) {
+ dev_err(tdc2dev(tdc), "DMA configured in cyclic mode\n");
+ return NULL;
+ }
+
ret = get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
&burst_size, &slave_bw);
if (ret < 0)
@@ -968,20 +972,9 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
if (flags & DMA_CTRL_ACK)
dma_desc->txd.flags = DMA_CTRL_ACK;
- /*
- * Make sure that mode should not be conflicting with currently
- * configured mode.
- */
- if (!tdc->isr_handler) {
- tdc->isr_handler = handle_once_dma_done;
- tdc->cyclic = false;
- } else {
- if (tdc->cyclic) {
- dev_err(tdc2dev(tdc), "DMA configured in cyclic mode\n");
- tegra_dma_desc_put(tdc, dma_desc);
- return NULL;
- }
- }
+
+ tdc->isr_handler = handle_once_dma_done;
+ tdc->cyclic = false;
return &dma_desc->txd;
}
@@ -1024,6 +1017,16 @@ struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
}
/*
+ * Make sure that mode should not be conflicting with currently
+ * configured mode.
+ */
+ if (tdc->isr_handler &&
+ tdc->isr_handler != handle_cont_sngl_cycle_dma_done) {
+ dev_err(tdc2dev(tdc), "DMA configuration conflict\n");
+ return NULL;
+ }
+
+ /*
* We only support cycle transfer when buf_len is multiple of
* period_len.
*/
@@ -1097,20 +1100,8 @@ struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
sg_req->last_sg = true;
dma_desc->txd.flags = 0;
- /*
- * Make sure that mode should not be conflicting with currently
- * configured mode.
- */
- if (!tdc->isr_handler) {
- tdc->isr_handler = handle_cont_sngl_cycle_dma_done;
- tdc->cyclic = true;
- } else {
- if (!tdc->cyclic) {
- dev_err(tdc2dev(tdc), "DMA configuration conflict\n");
- tegra_dma_desc_put(tdc, dma_desc);
- return NULL;
- }
- }
+ tdc->isr_handler = handle_cont_sngl_cycle_dma_done;
+ tdc->cyclic = true;
return &dma_desc->txd;
}
@@ -1145,8 +1136,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
dev_dbg(tdc2dev(tdc), "Freeing channel %d\n", tdc->id);
- if (tdc->busy)
- tegra_dma_terminate_all(dc);
+ tegra_dma_terminate_all(dc);
spin_lock_irqsave(&tdc->lock, flags);
list_splice_init(&tdc->pending_sg_req, &sg_req_list);
--
1.7.12
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH V2] dma: tegra: avoid channel lock up after free
2012-10-29 23:20 ` [PATCH V2] dma: tegra: avoid channel lock up after free Dmitry Osipenko
@ 2012-10-29 23:28 ` Dmitry Osipenko
2012-10-30 18:05 ` Stephen Warren
2012-10-31 14:58 ` Laxman Dewangan
2 siblings, 0 replies; 11+ messages in thread
From: Dmitry Osipenko @ 2012-10-29 23:28 UTC (permalink / raw)
To: linux-kernel
Also code looks not thread-safe, is it ok? For example we can protect prepare
functions with a spinlock and add tegra_dma_terminate_all_locked for calling on
channel freeing under spinlock to avoid reschedule. What do you think?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] dma: tegra: avoid channel lock up after free
2012-10-29 23:20 ` [PATCH V2] dma: tegra: avoid channel lock up after free Dmitry Osipenko
2012-10-29 23:28 ` Dmitry Osipenko
@ 2012-10-30 18:05 ` Stephen Warren
2012-10-31 14:58 ` Laxman Dewangan
2 siblings, 0 replies; 11+ messages in thread
From: Stephen Warren @ 2012-10-30 18:05 UTC (permalink / raw)
To: Dmitry Osipenko, ldewangan; +Cc: vinod.koul, linux-tegra, linux-kernel
On 10/29/2012 05:20 PM, Dmitry Osipenko wrote:
> Fixed channel "lock up" after free.
>
> Lock scenario: Channel 1 was allocated and prepared as slave_sg, used and freed.
> Now preparation of cyclic dma on channel 1 will fail with err "DMA configuration
> conflict" because tdc->isr_handler still selected to handle_once_dma_done.
>
> This happens because tegra_dma_abort_all() won't be called on channel freeing
> if pending list is empty or channel not busy. We need to clear isr_handler
> on channel freeing to avoid locking. Also I added small optimization to prepare
> functions, so current channel type checked before making allocations.
Reviewed-by: Stephen Warren <swarren@nvidia.com>
I believe this looks OK. However, I would like Laxman to also ack/review
this since he wrote the driver.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
2012-10-28 14:17 ` Dmitry Osipenko
@ 2012-10-31 14:48 ` Laxman Dewangan
-1 siblings, 0 replies; 11+ messages in thread
From: Laxman Dewangan @ 2012-10-31 14:48 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: vinod.koul-ral2JQCrhuEAvxtiuMwx3w,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Sunday 28 October 2012 07:47 PM, Dmitry Osipenko wrote:
> Fixed channel "lock" after free.
>
> Example: Channel 1 was allocated and prepared as slave_sg, used and freed. Now preparation of cyclic dma on channel 1 will fail with err "DMA
> configuration conflict" because tdc->isr_handler still selected to handle_once_dma_done.
>
> This happens because tegra_dma_abort_all() won't be called on channel freeing if pending list is empty.
>
> Signed-off-by: Dmitry Osipenko<digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Looks good to me.
Acked-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
@ 2012-10-31 14:48 ` Laxman Dewangan
0 siblings, 0 replies; 11+ messages in thread
From: Laxman Dewangan @ 2012-10-31 14:48 UTC (permalink / raw)
To: Dmitry Osipenko; +Cc: vinod.koul, linux-tegra, linux-kernel
On Sunday 28 October 2012 07:47 PM, Dmitry Osipenko wrote:
> Fixed channel "lock" after free.
>
> Example: Channel 1 was allocated and prepared as slave_sg, used and freed. Now preparation of cyclic dma on channel 1 will fail with err "DMA
> configuration conflict" because tdc->isr_handler still selected to handle_once_dma_done.
>
> This happens because tegra_dma_abort_all() won't be called on channel freeing if pending list is empty.
>
> Signed-off-by: Dmitry Osipenko<digetx@gmail.com>
Looks good to me.
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
2012-10-28 14:17 ` Dmitry Osipenko
@ 2012-10-31 14:48 ` Laxman Dewangan
-1 siblings, 0 replies; 11+ messages in thread
From: Laxman Dewangan @ 2012-10-31 14:48 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: vinod.koul-ral2JQCrhuEAvxtiuMwx3w,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Sunday 28 October 2012 07:47 PM, Dmitry Osipenko wrote:
> Fixed channel "lock" after free.
>
> Example: Channel 1 was allocated and prepared as slave_sg, used and freed. Now preparation of cyclic dma on channel 1 will fail with err "DMA
> configuration conflict" because tdc->isr_handler still selected to handle_once_dma_done.
>
> This happens because tegra_dma_abort_all() won't be called on channel freeing if pending list is empty.
>
> Signed-off-by: Dmitry Osipenko<digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Both patches looks good to me.
Acked-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
@ 2012-10-31 14:48 ` Laxman Dewangan
0 siblings, 0 replies; 11+ messages in thread
From: Laxman Dewangan @ 2012-10-31 14:48 UTC (permalink / raw)
To: Dmitry Osipenko; +Cc: vinod.koul, linux-tegra, linux-kernel
On Sunday 28 October 2012 07:47 PM, Dmitry Osipenko wrote:
> Fixed channel "lock" after free.
>
> Example: Channel 1 was allocated and prepared as slave_sg, used and freed. Now preparation of cyclic dma on channel 1 will fail with err "DMA
> configuration conflict" because tdc->isr_handler still selected to handle_once_dma_done.
>
> This happens because tegra_dma_abort_all() won't be called on channel freeing if pending list is empty.
>
> Signed-off-by: Dmitry Osipenko<digetx@gmail.com>
Both patches looks good to me.
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] dma: tegra: avoid channel lock up after free
2012-10-29 23:20 ` [PATCH V2] dma: tegra: avoid channel lock up after free Dmitry Osipenko
2012-10-29 23:28 ` Dmitry Osipenko
2012-10-30 18:05 ` Stephen Warren
@ 2012-10-31 14:58 ` Laxman Dewangan
2 siblings, 0 replies; 11+ messages in thread
From: Laxman Dewangan @ 2012-10-31 14:58 UTC (permalink / raw)
To: Dmitry Osipenko; +Cc: swarren, vinod.koul, linux-tegra, linux-kernel
On Tuesday 30 October 2012 04:50 AM, Dmitry Osipenko wrote:
> Fixed channel "lock up" after free.
>
> Lock scenario: Channel 1 was allocated and prepared as slave_sg, used and freed.
> Now preparation of cyclic dma on channel 1 will fail with err "DMA configuration
> conflict" because tdc->isr_handler still selected to handle_once_dma_done.
>
> This happens because tegra_dma_abort_all() won't be called on channel freeing
> if pending list is empty or channel not busy. We need to clear isr_handler
> on channel freeing to avoid locking. Also I added small optimization to prepare
> functions, so current channel type checked before making allocations.
>
> Signed-off-by: Dmitry Osipenko<digetx@gmail.com>
> ---
Looks good to me.
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-10-31 14:58 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-28 14:17 [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction Dmitry Osipenko
2012-10-28 14:17 ` Dmitry Osipenko
2012-10-29 15:27 ` Stephen Warren
2012-10-29 23:20 ` [PATCH V2] dma: tegra: avoid channel lock up after free Dmitry Osipenko
2012-10-29 23:28 ` Dmitry Osipenko
2012-10-30 18:05 ` Stephen Warren
2012-10-31 14:58 ` Laxman Dewangan
[not found] ` <1351433873-14082-1-git-send-email-digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2012-10-31 14:48 ` [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction Laxman Dewangan
2012-10-31 14:48 ` Laxman Dewangan
2012-10-31 14:48 ` Laxman Dewangan
2012-10-31 14:48 ` Laxman Dewangan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.