[PATCH v3 0/9] dmaengine: virt-dma related fixes

[PATCH v3 0/9] dmaengine: virt-dma related fixes

[Sascha Hauer]

Several drivers call vchan_dma_desc_free_list() or vchan_vdesc_fini() under
&vc->lock. This is not allowed and the first two patches fix this up. If you
are a DMA driver maintainer, the first two patches are the reason you are on
Cc.

The remaining patches are the original purpose of this series:
The i.MX SDMA driver leaks memory when a currently running descriptor is
aborted. Calling vchan_terminate_vdesc() on it to fix this revealed that
the virt-dma support calls the desc_free with the spin_lock held. This
doesn't work for the SDMA driver because it calls dma_free_coherent in
its desc_free hook. This series aims to fix that up.

Changes since v2:
- change drivers to not call vchan_dma_desc_free_list() under spin_lock
- remove debug message from vchan_dma_desc_free_list()

Changes since v1:
- rebase on v5.5-rc1
- Swap patches 1 and 2 for bisectablity
- Rename desc_aborted to desc_terminated
- Free up terminated descriptors immediately instead of letting them accumulate

Sascha Hauer (9):
  dmaengine: bcm2835: do not call vchan_vdesc_fini() with lock held
  dmaengine: virt-dma: Add missing locking
  dmaengine: virt-dma: remove debug message
  dmaengine: virt-dma: Do not call desc_free() under a spin_lock
  dmaengine: virt-dma: Add missing locking around list operations
  dmaengine: virt-dma: use vchan_vdesc_fini() to free descriptors
  dmaengine: imx-sdma: rename function
  dmaengine: imx-sdma: find desc first in sdma_tx_status
  dmaengine: imx-sdma: Fix memory leak

 drivers/dma/bcm2835-dma.c                     |  5 +--
 .../dma/dw-axi-dmac/dw-axi-dmac-platform.c    |  8 +---
 drivers/dma/imx-sdma.c                        | 37 +++++++++++--------
 drivers/dma/mediatek/mtk-uart-apdma.c         |  3 +-
 drivers/dma/owl-dma.c                         |  3 +-
 drivers/dma/s3c24xx-dma.c                     | 22 +++++------
 drivers/dma/sf-pdma/sf-pdma.c                 |  4 +-
 drivers/dma/sun4i-dma.c                       |  3 +-
 drivers/dma/virt-dma.c                        | 10 ++---
 drivers/dma/virt-dma.h                        | 27 ++++++++------
 10 files changed, 63 insertions(+), 59 deletions(-)

-- 
2.24.0

[PATCH 1/9] dmaengine: bcm2835: do not call vchan_vdesc_fini() with lock held

[Sascha Hauer]

vchan_vdesc_fini() can't be called locked. Instead, call
vchan_terminate_vdesc() which delays the freeing of the descriptor to
vchan_synchronize().

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/dma/bcm2835-dma.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index e4c593f48575..4768ef26013b 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -797,10 +797,7 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
 
 	/* stop DMA activity */
 	if (c->desc) {
-		if (c->desc->vd.tx.flags & DMA_PREP_INTERRUPT)
-			vchan_terminate_vdesc(&c->desc->vd);
-		else
-			vchan_vdesc_fini(&c->desc->vd);
+		vchan_terminate_vdesc(&c->desc->vd);
 		c->desc = NULL;
 		bcm2835_dma_abort(c);
 	}
-- 
2.24.0

[PATCH 2/9] dmaengine: virt-dma: Add missing locking

[Sascha Hauer]

Originally freeing descriptors was split into a locked and an unlocked
part. The locked part in vchan_get_all_descriptors() collected all
descriptors on a separate list_head. This was done to allow iterating
over that new list in vchan_dma_desc_free_list() without a lock held.

This became broken in 13bb26ae8850 ("dmaengine: virt-dma: don't always
free descriptor upon completion"). With this commit
vchan_dma_desc_free_list() no longer exclusively operates on the
separate list, but starts to put descriptors which can be reused back on
&vc->desc_allocated. This list operation should have been locked, but
wasn't.
In the mean time drivers started to call vchan_dma_desc_free_list() with
their lock held so that we now have the situation that
vchan_dma_desc_free_list() is called locked from some drivers and
unlocked from others.
To clean this up we have to do two things:

1. Add missing locking in vchan_dma_desc_free_list()
2. Make sure drivers call vchan_dma_desc_free_list() unlocked

This needs to be done atomically, so in this patch the locking is added
and all drivers are fixed.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 .../dma/dw-axi-dmac/dw-axi-dmac-platform.c    |  8 ++-----
 drivers/dma/mediatek/mtk-uart-apdma.c         |  3 ++-
 drivers/dma/owl-dma.c                         |  3 ++-
 drivers/dma/s3c24xx-dma.c                     | 22 +++++++++----------
 drivers/dma/sf-pdma/sf-pdma.c                 |  4 ++--
 drivers/dma/sun4i-dma.c                       |  3 ++-
 drivers/dma/virt-dma.c                        |  4 ++++
 7 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
index a1ce307c502f..14c1ac26f866 100644
--- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
+++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
@@ -636,14 +636,10 @@ static int dma_chan_terminate_all(struct dma_chan *dchan)
 
 	vchan_get_all_descriptors(&chan->vc, &head);
 
-	/*
-	 * As vchan_dma_desc_free_list can access to desc_allocated list
-	 * we need to call it in vc.lock context.
-	 */
-	vchan_dma_desc_free_list(&chan->vc, &head);
-
 	spin_unlock_irqrestore(&chan->vc.lock, flags);
 
+	vchan_dma_desc_free_list(&chan->vc, &head);
+
 	dev_vdbg(dchan2dev(dchan), "terminated: %s\n", axi_chan_name(chan));
 
 	return 0;
diff --git a/drivers/dma/mediatek/mtk-uart-apdma.c b/drivers/dma/mediatek/mtk-uart-apdma.c
index c20e6bd4e298..29f1223b285a 100644
--- a/drivers/dma/mediatek/mtk-uart-apdma.c
+++ b/drivers/dma/mediatek/mtk-uart-apdma.c
@@ -430,9 +430,10 @@ static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
 
 	spin_lock_irqsave(&c->vc.lock, flags);
 	vchan_get_all_descriptors(&c->vc, &head);
-	vchan_dma_desc_free_list(&c->vc, &head);
 	spin_unlock_irqrestore(&c->vc.lock, flags);
 
+	vchan_dma_desc_free_list(&c->vc, &head);
+
 	return 0;
 }
 
diff --git a/drivers/dma/owl-dma.c b/drivers/dma/owl-dma.c
index 023f951189a7..c683051257fd 100644
--- a/drivers/dma/owl-dma.c
+++ b/drivers/dma/owl-dma.c
@@ -674,10 +674,11 @@ static int owl_dma_terminate_all(struct dma_chan *chan)
 	}
 
 	vchan_get_all_descriptors(&vchan->vc, &head);
-	vchan_dma_desc_free_list(&vchan->vc, &head);
 
 	spin_unlock_irqrestore(&vchan->vc.lock, flags);
 
+	vchan_dma_desc_free_list(&vchan->vc, &head);
+
 	return 0;
 }
 
diff --git a/drivers/dma/s3c24xx-dma.c b/drivers/dma/s3c24xx-dma.c
index 43da8eeb18ef..1ed5dc1f597c 100644
--- a/drivers/dma/s3c24xx-dma.c
+++ b/drivers/dma/s3c24xx-dma.c
@@ -519,15 +519,6 @@ static void s3c24xx_dma_start_next_txd(struct s3c24xx_dma_chan *s3cchan)
 	s3c24xx_dma_start_next_sg(s3cchan, txd);
 }
 
-static void s3c24xx_dma_free_txd_list(struct s3c24xx_dma_engine *s3cdma,
-				struct s3c24xx_dma_chan *s3cchan)
-{
-	LIST_HEAD(head);
-
-	vchan_get_all_descriptors(&s3cchan->vc, &head);
-	vchan_dma_desc_free_list(&s3cchan->vc, &head);
-}
-
 /*
  * Try to allocate a physical channel.  When successful, assign it to
  * this virtual channel, and initiate the next descriptor.  The
@@ -709,8 +700,9 @@ static int s3c24xx_dma_terminate_all(struct dma_chan *chan)
 {
 	struct s3c24xx_dma_chan *s3cchan = to_s3c24xx_dma_chan(chan);
 	struct s3c24xx_dma_engine *s3cdma = s3cchan->host;
+	LIST_HEAD(head);
 	unsigned long flags;
-	int ret = 0;
+	int ret;
 
 	spin_lock_irqsave(&s3cchan->vc.lock, flags);
 
@@ -734,7 +726,15 @@ static int s3c24xx_dma_terminate_all(struct dma_chan *chan)
 	}
 
 	/* Dequeue jobs not yet fired as well */
-	s3c24xx_dma_free_txd_list(s3cdma, s3cchan);
+
+	vchan_get_all_descriptors(&s3cchan->vc, &head);
+
+	spin_unlock_irqrestore(&s3cchan->vc.lock, flags);
+
+	vchan_dma_desc_free_list(&s3cchan->vc, &head);
+
+	return 0;
+
 unlock:
 	spin_unlock_irqrestore(&s3cchan->vc.lock, flags);
 
diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
index 465256fe8b1f..6d0bec947636 100644
--- a/drivers/dma/sf-pdma/sf-pdma.c
+++ b/drivers/dma/sf-pdma/sf-pdma.c
@@ -155,9 +155,9 @@ static void sf_pdma_free_chan_resources(struct dma_chan *dchan)
 	kfree(chan->desc);
 	chan->desc = NULL;
 	vchan_get_all_descriptors(&chan->vchan, &head);
-	vchan_dma_desc_free_list(&chan->vchan, &head);
 	sf_pdma_disclaim_chan(chan);
 	spin_unlock_irqrestore(&chan->vchan.lock, flags);
+	vchan_dma_desc_free_list(&chan->vchan, &head);
 }
 
 static size_t sf_pdma_desc_residue(struct sf_pdma_chan *chan,
@@ -220,8 +220,8 @@ static int sf_pdma_terminate_all(struct dma_chan *dchan)
 	chan->desc = NULL;
 	chan->xfer_err = false;
 	vchan_get_all_descriptors(&chan->vchan, &head);
-	vchan_dma_desc_free_list(&chan->vchan, &head);
 	spin_unlock_irqrestore(&chan->vchan.lock, flags);
+	vchan_dma_desc_free_list(&chan->vchan, &head);
 
 	return 0;
 }
diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c
index e397a50058c8..4e1575e731d8 100644
--- a/drivers/dma/sun4i-dma.c
+++ b/drivers/dma/sun4i-dma.c
@@ -885,12 +885,13 @@ static int sun4i_dma_terminate_all(struct dma_chan *chan)
 	}
 
 	spin_lock_irqsave(&vchan->vc.lock, flags);
-	vchan_dma_desc_free_list(&vchan->vc, &head);
 	/* Clear these so the vchan is usable again */
 	vchan->processing = NULL;
 	vchan->pchan = NULL;
 	spin_unlock_irqrestore(&vchan->vc.lock, flags);
 
+	vchan_dma_desc_free_list(&vchan->vc, &head);
+
 	return 0;
 }
 
diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c
index ec4adf4260a0..660267ca5e42 100644
--- a/drivers/dma/virt-dma.c
+++ b/drivers/dma/virt-dma.c
@@ -116,7 +116,11 @@ void vchan_dma_desc_free_list(struct virt_dma_chan *vc, struct list_head *head)
 
 	list_for_each_entry_safe(vd, _vd, head, node) {
 		if (dmaengine_desc_test_reuse(&vd->tx)) {
+			unsigned long flags;
+
+			spin_lock_irqsave(&vc->lock, flags);
 			list_move_tail(&vd->node, &vc->desc_allocated);
+			spin_unlock_irqrestore(&vc->lock, flags);
 		} else {
 			dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd);
 			list_del(&vd->node);
-- 
2.24.0

[PATCH 3/9] dmaengine: virt-dma: remove debug message

[Sascha Hauer]

vchan_dma_desc_free_list() basically open codes vchan_vdesc_fini() in
the loop body. One difference is an additional debug message. As this
isn't overly useful remove it.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/dma/virt-dma.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c
index 660267ca5e42..7ba712888ac7 100644
--- a/drivers/dma/virt-dma.c
+++ b/drivers/dma/virt-dma.c
@@ -122,7 +122,6 @@ void vchan_dma_desc_free_list(struct virt_dma_chan *vc, struct list_head *head)
 			list_move_tail(&vd->node, &vc->desc_allocated);
 			spin_unlock_irqrestore(&vc->lock, flags);
 		} else {
-			dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd);
 			list_del(&vd->node);
 			vc->desc_free(vd);
 		}
-- 
2.24.0

[PATCH 4/9] dmaengine: virt-dma: Do not call desc_free() under a spin_lock

[Sascha Hauer]

vchan_vdesc_fini() shouldn't be called under a spin_lock. This is done
in two places, once in vchan_terminate_vdesc() and once in
vchan_synchronize(). Instead of freeing the vdesc right away, collect
the aborted vdescs on a separate list and free them along with the other
vdescs. The terminated descs are also freed in vchan_synchronize as done
before this patch.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/dma/virt-dma.c |  1 +
 drivers/dma/virt-dma.h | 18 +++++++++---------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c
index 7ba712888ac7..26e08c7a7465 100644
--- a/drivers/dma/virt-dma.c
+++ b/drivers/dma/virt-dma.c
@@ -138,6 +138,7 @@ void vchan_init(struct virt_dma_chan *vc, struct dma_device *dmadev)
 	INIT_LIST_HEAD(&vc->desc_submitted);
 	INIT_LIST_HEAD(&vc->desc_issued);
 	INIT_LIST_HEAD(&vc->desc_completed);
+	INIT_LIST_HEAD(&vc->desc_terminated);
 
 	tasklet_init(&vc->task, vchan_complete, (unsigned long)vc);
 
diff --git a/drivers/dma/virt-dma.h b/drivers/dma/virt-dma.h
index ab158bac03a7..e213137b6bc1 100644
--- a/drivers/dma/virt-dma.h
+++ b/drivers/dma/virt-dma.h
@@ -31,9 +31,9 @@ struct virt_dma_chan {
 	struct list_head desc_submitted;
 	struct list_head desc_issued;
 	struct list_head desc_completed;
+	struct list_head desc_terminated;
 
 	struct virt_dma_desc *cyclic;
-	struct virt_dma_desc *vd_terminated;
 };
 
 static inline struct virt_dma_chan *to_virt_chan(struct dma_chan *chan)
@@ -141,11 +141,8 @@ static inline void vchan_terminate_vdesc(struct virt_dma_desc *vd)
 {
 	struct virt_dma_chan *vc = to_virt_chan(vd->tx.chan);
 
-	/* free up stuck descriptor */
-	if (vc->vd_terminated)
-		vchan_vdesc_fini(vc->vd_terminated);
+	list_add_tail(&vd->node, &vc->desc_terminated);
 
-	vc->vd_terminated = vd;
 	if (vc->cyclic == vd)
 		vc->cyclic = NULL;
 }
@@ -179,6 +176,7 @@ static inline void vchan_get_all_descriptors(struct virt_dma_chan *vc,
 	list_splice_tail_init(&vc->desc_submitted, head);
 	list_splice_tail_init(&vc->desc_issued, head);
 	list_splice_tail_init(&vc->desc_completed, head);
+	list_splice_tail_init(&vc->desc_terminated, head);
 }
 
 static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
@@ -207,16 +205,18 @@ static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
  */
 static inline void vchan_synchronize(struct virt_dma_chan *vc)
 {
+	LIST_HEAD(head);
 	unsigned long flags;
 
 	tasklet_kill(&vc->task);
 
 	spin_lock_irqsave(&vc->lock, flags);
-	if (vc->vd_terminated) {
-		vchan_vdesc_fini(vc->vd_terminated);
-		vc->vd_terminated = NULL;
-	}
+
+	list_splice_tail_init(&vc->desc_terminated, &head);
+
 	spin_unlock_irqrestore(&vc->lock, flags);
+
+	vchan_dma_desc_free_list(vc, &head);
 }
 
 #endif
-- 
2.24.0

[PATCH 5/9] dmaengine: virt-dma: Add missing locking around list operations

[Sascha Hauer]

All list operations are protected by &vc->lock. As vchan_vdesc_fini()
is called unlocked add the missing locking around the list operations.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/dma/virt-dma.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/virt-dma.h b/drivers/dma/virt-dma.h
index e213137b6bc1..e9f5250fbe4d 100644
--- a/drivers/dma/virt-dma.h
+++ b/drivers/dma/virt-dma.h
@@ -113,10 +113,15 @@ static inline void vchan_vdesc_fini(struct virt_dma_desc *vd)
 {
 	struct virt_dma_chan *vc = to_virt_chan(vd->tx.chan);
 
-	if (dmaengine_desc_test_reuse(&vd->tx))
+	if (dmaengine_desc_test_reuse(&vd->tx)) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&vc->lock, flags);
 		list_add(&vd->node, &vc->desc_allocated);
-	else
+		spin_unlock_irqrestore(&vc->lock, flags);
+	} else {
 		vc->desc_free(vd);
+	}
 }
 
 /**
-- 
2.24.0

[PATCH 6/9] dmaengine: virt-dma: use vchan_vdesc_fini() to free descriptors

[Sascha Hauer]

vchan_dma_desc_free_list() basically open codes vchan_vdesc_fini() in its
loop body. Call it directly rather than duplicating the code.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/dma/virt-dma.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c
index 26e08c7a7465..95dfe431777e 100644
--- a/drivers/dma/virt-dma.c
+++ b/drivers/dma/virt-dma.c
@@ -115,16 +115,8 @@ void vchan_dma_desc_free_list(struct virt_dma_chan *vc, struct list_head *head)
 	struct virt_dma_desc *vd, *_vd;
 
 	list_for_each_entry_safe(vd, _vd, head, node) {
-		if (dmaengine_desc_test_reuse(&vd->tx)) {
-			unsigned long flags;
-
-			spin_lock_irqsave(&vc->lock, flags);
-			list_move_tail(&vd->node, &vc->desc_allocated);
-			spin_unlock_irqrestore(&vc->lock, flags);
-		} else {
-			list_del(&vd->node);
-			vc->desc_free(vd);
-		}
+		list_del(&vd->node);
+		vchan_vdesc_fini(vd);
 	}
 }
 EXPORT_SYMBOL_GPL(vchan_dma_desc_free_list);
-- 
2.24.0

[PATCH 7/9] dmaengine: imx-sdma: rename function

[Sascha Hauer]

Rename sdma_disable_channel_async() after the hook it implements, like
done for all other functions in the SDMA driver.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index c27e206a764c..527f8a81f50b 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1077,7 +1077,7 @@ static void sdma_channel_terminate_work(struct work_struct *work)
 	sdmac->context_loaded = false;
 }
 
-static int sdma_disable_channel_async(struct dma_chan *chan)
+static int sdma_terminate_all(struct dma_chan *chan)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
 
@@ -1324,7 +1324,7 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
 	struct sdma_engine *sdma = sdmac->sdma;
 
-	sdma_disable_channel_async(chan);
+	sdma_terminate_all(chan);
 
 	sdma_channel_synchronize(chan);
 
@@ -2103,7 +2103,7 @@ static int sdma_probe(struct platform_device *pdev)
 	sdma->dma_device.device_prep_slave_sg = sdma_prep_slave_sg;
 	sdma->dma_device.device_prep_dma_cyclic = sdma_prep_dma_cyclic;
 	sdma->dma_device.device_config = sdma_config;
-	sdma->dma_device.device_terminate_all = sdma_disable_channel_async;
+	sdma->dma_device.device_terminate_all = sdma_terminate_all;
 	sdma->dma_device.device_synchronize = sdma_channel_synchronize;
 	sdma->dma_device.src_addr_widths = SDMA_DMA_BUSWIDTHS;
 	sdma->dma_device.dst_addr_widths = SDMA_DMA_BUSWIDTHS;
-- 
2.24.0

[PATCH 8/9] dmaengine: imx-sdma: find desc first in sdma_tx_status

[Sascha Hauer]

In sdma_tx_status() we must first find the current sdma_desc. In cyclic
mode we assume that this can always be found with vchan_find_desc().
This is true because do not remove the current descriptor from the
desc_issued list:

	/*
	 * Do not delete the node in desc_issued list in cyclic mode, otherwise
	 * the desc allocated will never be freed in vchan_dma_desc_free_list
	 */
	if (!(sdmac->flags & IMX_DMA_SG_LOOP))
		list_del(&vd->node);

We will change this in the next step, so check if the current descriptor is
the desired one also for the cyclic case.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 527f8a81f50b..99dbfd9039cf 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1648,7 +1648,7 @@ static enum dma_status sdma_tx_status(struct dma_chan *chan,
 				      struct dma_tx_state *txstate)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
-	struct sdma_desc *desc;
+	struct sdma_desc *desc = NULL;
 	u32 residue;
 	struct virt_dma_desc *vd;
 	enum dma_status ret;
@@ -1659,19 +1659,23 @@ static enum dma_status sdma_tx_status(struct dma_chan *chan,
 		return ret;
 
 	spin_lock_irqsave(&sdmac->vc.lock, flags);
+
 	vd = vchan_find_desc(&sdmac->vc, cookie);
-	if (vd) {
+	if (vd)
 		desc = to_sdma_desc(&vd->tx);
+	else if (sdmac->desc && sdmac->desc->vd.tx.cookie == cookie)
+		desc = sdmac->desc;
+
+	if (desc) {
 		if (sdmac->flags & IMX_DMA_SG_LOOP)
 			residue = (desc->num_bd - desc->buf_ptail) *
 				desc->period_len - desc->chn_real_count;
 		else
 			residue = desc->chn_count - desc->chn_real_count;
-	} else if (sdmac->desc && sdmac->desc->vd.tx.cookie == cookie) {
-		residue = sdmac->desc->chn_count - sdmac->desc->chn_real_count;
 	} else {
 		residue = 0;
 	}
+
 	spin_unlock_irqrestore(&sdmac->vc.lock, flags);
 
 	dma_set_tx_state(txstate, chan->completed_cookie, chan->cookie,
-- 
2.24.0

[PATCH 9/9] dmaengine: imx-sdma: Fix memory leak

[Sascha Hauer]

The current descriptor is not on any list of the virtual DMA channel.
Once sdma_terminate_all() is called when a descriptor is currently
in flight then this one is forgotten to be freed. We have to call
vchan_terminate_vdesc() on this descriptor to re-add it to the lists.
Now that we also free the currently running descriptor we can (and
actually have to) remove the current descriptor from its list also
for the cyclic case.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 99dbfd9039cf..066b21a32232 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -760,12 +760,8 @@ static void sdma_start_desc(struct sdma_channel *sdmac)
 		return;
 	}
 	sdmac->desc = desc = to_sdma_desc(&vd->tx);
-	/*
-	 * Do not delete the node in desc_issued list in cyclic mode, otherwise
-	 * the desc allocated will never be freed in vchan_dma_desc_free_list
-	 */
-	if (!(sdmac->flags & IMX_DMA_SG_LOOP))
-		list_del(&vd->node);
+
+	list_del(&vd->node);
 
 	sdma->channel_control[channel].base_bd_ptr = desc->bd_phys;
 	sdma->channel_control[channel].current_bd_ptr = desc->bd_phys;
@@ -1071,7 +1067,6 @@ static void sdma_channel_terminate_work(struct work_struct *work)
 
 	spin_lock_irqsave(&sdmac->vc.lock, flags);
 	vchan_get_all_descriptors(&sdmac->vc, &head);
-	sdmac->desc = NULL;
 	spin_unlock_irqrestore(&sdmac->vc.lock, flags);
 	vchan_dma_desc_free_list(&sdmac->vc, &head);
 	sdmac->context_loaded = false;
@@ -1080,11 +1075,19 @@ static void sdma_channel_terminate_work(struct work_struct *work)
 static int sdma_terminate_all(struct dma_chan *chan)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
+	unsigned long flags;
+
+	spin_lock_irqsave(&sdmac->vc.lock, flags);
 
 	sdma_disable_channel(chan);
 
-	if (sdmac->desc)
+	if (sdmac->desc) {
+		vchan_terminate_vdesc(&sdmac->desc->vd);
+		sdmac->desc = NULL;
 		schedule_work(&sdmac->terminate_worker);
+	}
+
+	spin_unlock_irqrestore(&sdmac->vc.lock, flags);
 
 	return 0;
 }
-- 
2.24.0

Re: [PATCH v3 0/9] dmaengine: virt-dma related fixes

[Peter Ujfalusi]

On 16/12/2019 12.53, Sascha Hauer wrote:
> Several drivers call vchan_dma_desc_free_list() or vchan_vdesc_fini() under
> &vc->lock. This is not allowed and the first two patches fix this up. If you
> are a DMA driver maintainer, the first two patches are the reason you are on
> Cc.
> 
> The remaining patches are the original purpose of this series:
> The i.MX SDMA driver leaks memory when a currently running descriptor is
> aborted. Calling vchan_terminate_vdesc() on it to fix this revealed that
> the virt-dma support calls the desc_free with the spin_lock held. This
> doesn't work for the SDMA driver because it calls dma_free_coherent in
> its desc_free hook. This series aims to fix that up.

for drivers/dma/virt-dma.* :
Reviewed-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com>


> Changes since v2:
> - change drivers to not call vchan_dma_desc_free_list() under spin_lock
> - remove debug message from vchan_dma_desc_free_list()
> 
> Changes since v1:
> - rebase on v5.5-rc1
> - Swap patches 1 and 2 for bisectablity
> - Rename desc_aborted to desc_terminated
> - Free up terminated descriptors immediately instead of letting them accumulate
> 
> Sascha Hauer (9):
>   dmaengine: bcm2835: do not call vchan_vdesc_fini() with lock held
>   dmaengine: virt-dma: Add missing locking
>   dmaengine: virt-dma: remove debug message
>   dmaengine: virt-dma: Do not call desc_free() under a spin_lock
>   dmaengine: virt-dma: Add missing locking around list operations
>   dmaengine: virt-dma: use vchan_vdesc_fini() to free descriptors
>   dmaengine: imx-sdma: rename function
>   dmaengine: imx-sdma: find desc first in sdma_tx_status
>   dmaengine: imx-sdma: Fix memory leak
> 
>  drivers/dma/bcm2835-dma.c                     |  5 +--
>  .../dma/dw-axi-dmac/dw-axi-dmac-platform.c    |  8 +---
>  drivers/dma/imx-sdma.c                        | 37 +++++++++++--------
>  drivers/dma/mediatek/mtk-uart-apdma.c         |  3 +-
>  drivers/dma/owl-dma.c                         |  3 +-
>  drivers/dma/s3c24xx-dma.c                     | 22 +++++------
>  drivers/dma/sf-pdma/sf-pdma.c                 |  4 +-
>  drivers/dma/sun4i-dma.c                       |  3 +-
>  drivers/dma/virt-dma.c                        | 10 ++---
>  drivers/dma/virt-dma.h                        | 27 ++++++++------
>  10 files changed, 63 insertions(+), 59 deletions(-)
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

RE: [PATCH 9/9] dmaengine: imx-sdma: Fix memory leak

[Robin Gong]

On 2019/16/12 18:53 Sascha Hauer <s.hauer@pengutronix.de> wrote:
> The current descriptor is not on any list of the virtual DMA channel.
> Once sdma_terminate_all() is called when a descriptor is currently in flight then
> this one is forgotten to be freed. We have to call
> vchan_terminate_vdesc() on this descriptor to re-add it to the lists.
> Now that we also free the currently running descriptor we can (and actually
> have to) remove the current descriptor from its list also for the cyclic case.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Thanks Sacha. Now it's better to document or comment somewhere to notice
vchan_terminate_vdesc() should be called in channel(cyclic) terminated unless
dma controller driver free all desc by itself after this patch set applied.
Reviewed-by: Robin Gong <yibin.gong@nxp.com>
Tested-by: Robin Gong <yibin.gong@nxp.com>
> ---
>  drivers/dma/imx-sdma.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index
> 99dbfd9039cf..066b21a32232 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -760,12 +760,8 @@ static void sdma_start_desc(struct sdma_channel
> *sdmac)
>  		return;
>  	}
>  	sdmac->desc = desc = to_sdma_desc(&vd->tx);
> -	/*
> -	 * Do not delete the node in desc_issued list in cyclic mode, otherwise
> -	 * the desc allocated will never be freed in vchan_dma_desc_free_list
> -	 */
> -	if (!(sdmac->flags & IMX_DMA_SG_LOOP))
> -		list_del(&vd->node);
> +
> +	list_del(&vd->node);
> 
>  	sdma->channel_control[channel].base_bd_ptr = desc->bd_phys;
>  	sdma->channel_control[channel].current_bd_ptr = desc->bd_phys; @@
> -1071,7 +1067,6 @@ static void sdma_channel_terminate_work(struct
> work_struct *work)
> 
>  	spin_lock_irqsave(&sdmac->vc.lock, flags);
>  	vchan_get_all_descriptors(&sdmac->vc, &head);
> -	sdmac->desc = NULL;
>  	spin_unlock_irqrestore(&sdmac->vc.lock, flags);
>  	vchan_dma_desc_free_list(&sdmac->vc, &head);
>  	sdmac->context_loaded = false;
> @@ -1080,11 +1075,19 @@ static void sdma_channel_terminate_work(struct
> work_struct *work)  static int sdma_terminate_all(struct dma_chan *chan)  {
>  	struct sdma_channel *sdmac = to_sdma_chan(chan);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&sdmac->vc.lock, flags);
> 
>  	sdma_disable_channel(chan);
> 
> -	if (sdmac->desc)
> +	if (sdmac->desc) {
> +		vchan_terminate_vdesc(&sdmac->desc->vd);
> +		sdmac->desc = NULL;
>  		schedule_work(&sdmac->terminate_worker);
> +	}
> +
> +	spin_unlock_irqrestore(&sdmac->vc.lock, flags);
> 
>  	return 0;
>  }
> --
> 2.24.0

Re: [PATCH v3 0/9] dmaengine: virt-dma related fixes

[Vinod Koul]

On 16-12-19, 11:53, Sascha Hauer wrote:
> Several drivers call vchan_dma_desc_free_list() or vchan_vdesc_fini() under
> &vc->lock. This is not allowed and the first two patches fix this up. If you
> are a DMA driver maintainer, the first two patches are the reason you are on
> Cc.
> 
> The remaining patches are the original purpose of this series:
> The i.MX SDMA driver leaks memory when a currently running descriptor is
> aborted. Calling vchan_terminate_vdesc() on it to fix this revealed that
> the virt-dma support calls the desc_free with the spin_lock held. This
> doesn't work for the SDMA driver because it calls dma_free_coherent in
> its desc_free hook. This series aims to fix that up.

Applied all, thanks

-- 
~Vinod