* [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 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
[parent not found: <1351433873-14082-1-git-send-email-digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* 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
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.