dmaengine Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] dmaengine: qcom: bam_dma: Fix resource leak
@ 2019-10-17 15:26 Jeffrey Hugo
  2019-10-18  7:58 ` Vinod Koul
  0 siblings, 1 reply; 2+ messages in thread
From: Jeffrey Hugo @ 2019-10-17 15:26 UTC (permalink / raw)
  To: agross, bjorn.andersson, dan.j.williams, vkoul
  Cc: linux-arm-msm, dmaengine, linux-kernel, Jeffrey Hugo

bam_dma_terminate_all() will leak resources if any of the transactions are
committed to the hardware (present in the desc fifo), and not complete.
Since bam_dma_terminate_all() does not cause the hardware to be updated,
the hardware will still operate on any previously committed transactions.
This can cause memory corruption if the memory for the transaction has been
reassigned, and will cause a sync issue between the BAM and its client(s).

Fix this by properly updating the hardware in bam_dma_terminate_all().

Fixes: e7c0fe2a5c84 ("dmaengine: add Qualcomm BAM dma driver")
Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
---
 drivers/dma/qcom/bam_dma.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 8e90a405939d..ef73f65224b1 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -694,6 +694,25 @@ static int bam_dma_terminate_all(struct dma_chan *chan)
 
 	/* remove all transactions, including active transaction */
 	spin_lock_irqsave(&bchan->vc.lock, flag);
+	/*
+	 * If we have transactions queued, then some might be committed to the
+	 * hardware in the desc fifo.  The only way to reset the desc fifo is
+	 * to do a hardware reset (either by pipe or the entire block).
+	 * bam_chan_init_hw() will trigger a pipe reset, and also reinit the
+	 * pipe.  If the pipe is left disabled (default state after pipe reset)
+	 * and is accessed by a connected hardware engine, a fatal error in
+	 * the BAM will occur.  There is a small window where this could happen
+	 * with bam_chan_init_hw(), but it is assumed that the caller has
+	 * stopped activity on any attached hardware engine.  Make sure to do
+	 * this first so that the BAM hardware doesn't cause memory corruption
+	 * by accessing freed resources.
+	 */
+	if (!list_empty(&bchan->desc_list)) {
+		async_desc = list_first_entry(&bchan->desc_list,
+					      struct bam_async_desc, desc_node);
+		bam_chan_init_hw(bchan, async_desc->dir);
+	}
+
 	list_for_each_entry_safe(async_desc, tmp,
 				 &bchan->desc_list, desc_node) {
 		list_add(&async_desc->vd.node, &bchan->vc.desc_issued);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] dmaengine: qcom: bam_dma: Fix resource leak
  2019-10-17 15:26 [PATCH] dmaengine: qcom: bam_dma: Fix resource leak Jeffrey Hugo
@ 2019-10-18  7:58 ` Vinod Koul
  0 siblings, 0 replies; 2+ messages in thread
From: Vinod Koul @ 2019-10-18  7:58 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: agross, bjorn.andersson, dan.j.williams, linux-arm-msm,
	dmaengine, linux-kernel

On 17-10-19, 08:26, Jeffrey Hugo wrote:
> bam_dma_terminate_all() will leak resources if any of the transactions are
> committed to the hardware (present in the desc fifo), and not complete.
> Since bam_dma_terminate_all() does not cause the hardware to be updated,
> the hardware will still operate on any previously committed transactions.
> This can cause memory corruption if the memory for the transaction has been
> reassigned, and will cause a sync issue between the BAM and its client(s).
> 
> Fix this by properly updating the hardware in bam_dma_terminate_all().

Applied and marked stable, thanks

-- 
~Vinod

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-17 15:26 [PATCH] dmaengine: qcom: bam_dma: Fix resource leak Jeffrey Hugo
2019-10-18  7:58 ` Vinod Koul

dmaengine Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dmaengine/0 dmaengine/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dmaengine dmaengine/ https://lore.kernel.org/dmaengine \
		dmaengine@vger.kernel.org
	public-inbox-index dmaengine

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.dmaengine


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git