[PATCH v2 0/6] virt-dma and i.MX SDMA fixes

[PATCH v2 0/6] virt-dma and i.MX SDMA fixes

[Sascha Hauer]

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 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

*** BLURB HERE ***
Sascha Hauer (6):
  dmaengine: virt-dma: Do not call desc_free() under a spin_lock
  dmaengine: virt-dma: Add missing locking around list operations
  dmaengine: imx-sdma: rename function
  dmaengine: imx-sdma: find desc first in sdma_tx_status
  dmaengine: imx-sdma: Fix memory leak
  ARM: imx_v6_v7_defconfig: Enable NFS_V4_1 and NFS_V4_2 support

 arch/arm/configs/imx_v6_v7_defconfig |  2 ++
 drivers/dma/imx-sdma.c               | 37 +++++++++++++++++-----------
 drivers/dma/virt-dma.c               |  1 +
 drivers/dma/virt-dma.h               | 27 +++++++++++---------
 4 files changed, 41 insertions(+), 26 deletions(-)

-- 
2.24.0

[PATCH 1/6] 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 ec4adf4260a0..02c0a8885a53 100644
--- a/drivers/dma/virt-dma.c
+++ b/drivers/dma/virt-dma.c
@@ -135,6 +135,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 2/6] 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 3/6] 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 4/6] 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 5/6] 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

[PATCH 6/6] ARM: imx_v6_v7_defconfig: Enable NFS_V4_1 and NFS_V4_2 support

[Sascha Hauer]

Enable NFS_V4_1 and NFS_V4_2 to support NFS servers providing that
protocol.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/configs/imx_v6_v7_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index 26d6dee67aa6..23a26e786ae9 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -442,6 +442,8 @@ CONFIG_UBIFS_FS=y
 CONFIG_NFS_FS=y
 CONFIG_NFS_V3_ACL=y
 CONFIG_NFS_V4=y
+CONFIG_NFS_V4_1=y
+CONFIG_NFS_V4_2=y
 CONFIG_ROOT_NFS=y
 CONFIG_NLS_DEFAULT="cp437"
 CONFIG_NLS_CODEPAGE_437=y
-- 
2.24.0

Re: [PATCH 1/6] dmaengine: virt-dma: Do not call desc_free() under a spin_lock

[Peter Ujfalusi]

On 10/12/2019 14.33, Sascha Hauer wrote:
> 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 ec4adf4260a0..02c0a8885a53 100644
> --- a/drivers/dma/virt-dma.c
> +++ b/drivers/dma/virt-dma.c
> @@ -135,6 +135,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);

My only issue with the vchan_dma_desc_free_list() is that it prints with
dev_dbg() for each descriptor it is freeing up.
The 'stuck' descriptor happens quite frequently if you start/stop audio
for example.

This is why I proposed a local

list_for_each_entry_safe(vd, _vd, &head, node) {
	list_del(&vd->node);
	vchan_vdesc_fini(vd);
}

On the other hand what vchan_dma_desc_free_list() is doing is exactly
the same thing as this loop is doing with the addition of the debug print.

I'm not sure how useful that debug print is, not sure if anyone would
miss if it is gone?

If not, than see my comment on patch 2.

>  }
>  
>  #endif
> 

- Péter

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

Re: [PATCH 2/6] dmaengine: virt-dma: Add missing locking around list operations

[Peter Ujfalusi]

On 10/12/2019 14.33, Sascha Hauer wrote:
> 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 {

If we add:
		list_del(&vd->node);

here, then the list_del() can be removed from vchan_complete() before
vchan_vdesc_fini() is called and as a plus the
vchan_dma_desc_free_list() can be as simple as:

list_for_each_entry_safe(vd, _vd, head, node)
	vchan_vdesc_fini(vd);

But only if we don't care about the debug print in there, if we care
then the vchan_synchronize() could use the very same simple loop to free
up only the descriptors from the desc_terminated list.


>  		vc->desc_free(vd);
> +	}
>  }
>  
>  /**
> 

- Péter

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

Re: [PATCH 6/6] ARM: imx_v6_v7_defconfig: Enable NFS_V4_1 and NFS_V4_2 support

[Sascha Hauer]

On Tue, Dec 10, 2019 at 01:33:52PM +0100, Sascha Hauer wrote:
> Enable NFS_V4_1 and NFS_V4_2 to support NFS servers providing that
> protocol.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm/configs/imx_v6_v7_defconfig | 2 ++
>  1 file changed, 2 insertions(+)

Ignore this one, it shouldn't be part of this series.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/6] dmaengine: virt-dma: Do not call desc_free() under a spin_lock

[Sascha Hauer]

On Tue, Dec 10, 2019 at 03:12:47PM +0200, Peter Ujfalusi wrote:
> 
> 
> On 10/12/2019 14.33, Sascha Hauer wrote:
> > 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 ec4adf4260a0..02c0a8885a53 100644
> > --- a/drivers/dma/virt-dma.c
> > +++ b/drivers/dma/virt-dma.c
> > @@ -135,6 +135,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);
> 
> My only issue with the vchan_dma_desc_free_list() is that it prints with
> dev_dbg() for each descriptor it is freeing up.
> The 'stuck' descriptor happens quite frequently if you start/stop audio
> for example.

if we consider the message useful then I would say it's equally useful
both for the 'stuck' descriptor and for the regular case.

IMO the debug message only makes sense if we make sure it is printed
each time a descriptor is freed. Currently it's printed when the
descriptor is freed from vchan_dma_desc_free_list(), but not when it's
freed from vchan_vdesc_fini(). This is confusing as looking at the dmesg
suggests that we lose descriptors.

> 
> This is why I proposed a local
> 
> list_for_each_entry_safe(vd, _vd, &head, node) {
> 	list_del(&vd->node);
> 	vchan_vdesc_fini(vd);
> }
> 
> On the other hand what vchan_dma_desc_free_list() is doing is exactly
> the same thing as this loop is doing with the addition of the debug print.
> 
> I'm not sure how useful that debug print is, not sure if anyone would
> miss if it is gone?
> 
> If not, than see my comment on patch 2.

We could add the dev_dbg into vchan_vdesc_fini() as well and still
implement your suggestion on patch 2...

Anyway, I don't care much if the dev_dbg is there or not. I'll happily
add a patch removing it for the next round if that's what we agree upon.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/6] dmaengine: virt-dma: Do not call desc_free() under a spin_lock

[Peter Ujfalusi]

On 10/12/2019 16.19, Sascha Hauer wrote:
> On Tue, Dec 10, 2019 at 03:12:47PM +0200, Peter Ujfalusi wrote:
>>>  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);
>>
>> My only issue with the vchan_dma_desc_free_list() is that it prints with
>> dev_dbg() for each descriptor it is freeing up.
>> The 'stuck' descriptor happens quite frequently if you start/stop audio
>> for example.
> 
> if we consider the message useful then I would say it's equally useful
> both for the 'stuck' descriptor and for the regular case.

The case with the 'stuck' descriptor is not really a stuck descriptor.
It is the cyclic descriptor which is by principle never completes so the
vchan_complete() would be never called for it -> we leaked descriptors
in audio start/stop/start/...

Printing about it up is like printing before each vc->desc_free() call
at each completion.

> IMO the debug message only makes sense if we make sure it is printed
> each time a descriptor is freed. Currently it's printed when the
> descriptor is freed from vchan_dma_desc_free_list(), but not when it's
> freed from vchan_vdesc_fini(). This is confusing as looking at the dmesg
> suggests that we lose descriptors.

The only case I can think when it is usable is when client prepared
several transfers, but decides to terminate the channel, or if several
descriptors are in the issued list waiting and the client terminates the
channel.

Enabling the debug for all free operation would easily make the device
overwhelmed with prints during boot (MMC rootfs w/ system DMA?).

I'm questioning the sole usefulness of the print. I don't think it adds
any value or information.

>> This is why I proposed a local
>>
>> list_for_each_entry_safe(vd, _vd, &head, node) {
>> 	list_del(&vd->node);
>> 	vchan_vdesc_fini(vd);
>> }
>>
>> On the other hand what vchan_dma_desc_free_list() is doing is exactly
>> the same thing as this loop is doing with the addition of the debug print.
>>
>> I'm not sure how useful that debug print is, not sure if anyone would
>> miss if it is gone?
>>
>> If not, than see my comment on patch 2.
> 
> We could add the dev_dbg into vchan_vdesc_fini() as well and still
> implement your suggestion on patch 2...

That would be a bad idea. imho.

> Anyway, I don't care much if the dev_dbg is there or not. I'll happily
> add a patch removing it for the next round if that's what we agree upon.

I would just drop the debug print ;)

Vinod?

> 
> Sascha
> 
> 

- Péter

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