Re: [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
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 Both patches looks good to me. Acked-by: Laxman Dewangan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
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 Looks good to me. Acked-by: Laxman Dewangan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
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 Osipenkodig...@gmail.com Looks good to me. Acked-by: Laxman Dewangan ldewan...@nvidia.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
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 Osipenkodig...@gmail.com Both patches looks good to me. Acked-by: Laxman Dewangan ldewan...@nvidia.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
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(>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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
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 --- 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(>lock, flags); list_splice_init(>pending_sg_req, _req_list); -- 1.7.12 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] dma: tegra20-apbdma: channel freeing correction
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 dig...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
