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

[PATCH 0/5] 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.

Sascha

Sascha Hauer (5):
  dmaengine: virt-dma: Add missing locking around list operations
  dmaengine: virt-dma: Do not call desc_free() under a spin_lock
  dmaengine: imx-sdma: rename function
  dmaengine: imx-sdma: find desc first in sdma_tx_status
  dmaengine: imx-sdma: Fix memory leak

 drivers/dma/imx-sdma.c | 37 ++++++++++++++++++++++---------------
 drivers/dma/virt-dma.c |  1 +
 drivers/dma/virt-dma.h | 26 ++++++++++----------------
 3 files changed, 33 insertions(+), 31 deletions(-)

-- 
2.24.0

[PATCH 1/5] 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 ab158bac03a7..41883ee2c29f 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 2/5] 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.

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

diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c
index ec4adf4260a0..87d5bd53c98b 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_aborted);
 
 	tasklet_init(&vc->task, vchan_complete, (unsigned long)vc);
 
diff --git a/drivers/dma/virt-dma.h b/drivers/dma/virt-dma.h
index 41883ee2c29f..6cae93624f0d 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_aborted;
 
 	struct virt_dma_desc *cyclic;
-	struct virt_dma_desc *vd_terminated;
 };
 
 static inline struct virt_dma_chan *to_virt_chan(struct dma_chan *chan)
@@ -146,11 +146,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_aborted);
 
-	vc->vd_terminated = vd;
 	if (vc->cyclic == vd)
 		vc->cyclic = NULL;
 }
@@ -184,6 +181,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_aborted, head);
 }
 
 static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
@@ -212,16 +210,7 @@ static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
  */
 static inline void vchan_synchronize(struct virt_dma_chan *vc)
 {
-	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;
-	}
-	spin_unlock_irqrestore(&vc->lock, flags);
 }
 
 #endif
-- 
2.24.0

[PATCH 3/5] 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/5] 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/5] 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 1/5] dmaengine: virt-dma: Add missing locking around list operations

[Peter Ujfalusi]

Hi Sascha,

On 06/12/2019 15.53, 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.

At this commit the vhcan_vdesc_fini() _is_ called when the lock is held
via vchan_terminate_vdesc() which must be called with the lock held...

Swap patch 1 and 2.

> 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 ab158bac03a7..41883ee2c29f 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);
> +	}
>  }
>  
>  /**
> 

- Péter

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

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

[Peter Ujfalusi]

Hi Sascha,


On 06/12/2019 15.53, 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.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/dma/virt-dma.c |  1 +
>  drivers/dma/virt-dma.h | 17 +++--------------
>  2 files changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c
> index ec4adf4260a0..87d5bd53c98b 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_aborted);

Can we keep the terminated term instead of aborted: 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 41883ee2c29f..6cae93624f0d 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_aborted;
>  
>  	struct virt_dma_desc *cyclic;
> -	struct virt_dma_desc *vd_terminated;
>  };
>  
>  static inline struct virt_dma_chan *to_virt_chan(struct dma_chan *chan)
> @@ -146,11 +146,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_aborted);
>  
> -	vc->vd_terminated = vd;
>  	if (vc->cyclic == vd)
>  		vc->cyclic = NULL;
>  }
> @@ -184,6 +181,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_aborted, head);
>  }
>  
>  static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
> @@ -212,16 +210,7 @@ static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
>   */
>  static inline void vchan_synchronize(struct virt_dma_chan *vc)
>  {
> -	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;
> -	}
> -	spin_unlock_irqrestore(&vc->lock, flags);

We don't want the terminated descriptors to accumulate until the channel
is freed up.

spin_lock_irqsave(&vc->lock, flags);
list_splice_tail_init(&vc->desc_terminated, &head);
spin_unlock_irqrestore(&vc->lock, flags);

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


>  }
>  
>  #endif
> 

- Péter

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

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

[Peter Ujfalusi]

On 09/12/2019 9.48, Peter Ujfalusi wrote:
> Hi Sascha,
> 
> 
> On 06/12/2019 15.53, 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.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> ---
>>  drivers/dma/virt-dma.c |  1 +
>>  drivers/dma/virt-dma.h | 17 +++--------------
>>  2 files changed, 4 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c
>> index ec4adf4260a0..87d5bd53c98b 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_aborted);
> 
> Can we keep the terminated term instead of aborted: 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 41883ee2c29f..6cae93624f0d 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_aborted;
>>  
>>  	struct virt_dma_desc *cyclic;
>> -	struct virt_dma_desc *vd_terminated;
>>  };
>>  
>>  static inline struct virt_dma_chan *to_virt_chan(struct dma_chan *chan)
>> @@ -146,11 +146,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_aborted);
>>  
>> -	vc->vd_terminated = vd;
>>  	if (vc->cyclic == vd)
>>  		vc->cyclic = NULL;
>>  }
>> @@ -184,6 +181,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_aborted, head);
>>  }
>>  
>>  static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
>> @@ -212,16 +210,7 @@ static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
>>   */
>>  static inline void vchan_synchronize(struct virt_dma_chan *vc)
>>  {
>> -	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;
>> -	}
>> -	spin_unlock_irqrestore(&vc->lock, flags);
> 
> We don't want the terminated descriptors to accumulate until the channel
> is freed up.

Well, most DMA driver will clean it up in their terminate_all, but it is
better to do it in synchronize as well.

> 
> spin_lock_irqsave(&vc->lock, flags);
> list_splice_tail_init(&vc->desc_terminated, &head);
> spin_unlock_irqrestore(&vc->lock, flags);
> 
> list_for_each_entry_safe(vd, _vd, &head, node) {
> 	list_del(&vd->node);
> 	vchan_vdesc_fini(vd);
> }
> 
> 
>>  }
>>  
>>  #endif
>>
> 
> - Péter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

- Péter

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

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

[Sascha Hauer]

On Mon, Dec 09, 2019 at 09:38:17AM +0200, Peter Ujfalusi wrote:
> Hi Sascha,
> 
> On 06/12/2019 15.53, 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.
> 
> At this commit the vhcan_vdesc_fini() _is_ called when the lock is held
> via vchan_terminate_vdesc() which must be called with the lock held...
> 
> Swap patch 1 and 2.

Right, will do.

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 2/5] dmaengine: virt-dma: Do not call desc_free() under a spin_lock

[Sascha Hauer]

On Mon, Dec 09, 2019 at 09:48:22AM +0200, Peter Ujfalusi wrote:
> Hi Sascha,
> 
> 
> On 06/12/2019 15.53, 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.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/dma/virt-dma.c |  1 +
> >  drivers/dma/virt-dma.h | 17 +++--------------
> >  2 files changed, 4 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c
> > index ec4adf4260a0..87d5bd53c98b 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_aborted);
> 
> Can we keep the terminated term instead of aborted: desc_terminated

Sure.

> >  	tasklet_kill(&vc->task);
> > -
> > -	spin_lock_irqsave(&vc->lock, flags);
> > -	if (vc->vd_terminated) {
> > -		vchan_vdesc_fini(vc->vd_terminated);
> > -		vc->vd_terminated = NULL;
> > -	}
> > -	spin_unlock_irqrestore(&vc->lock, flags);
> 
> We don't want the terminated descriptors to accumulate until the channel
> is freed up.

Nothing easier than that. I just have to revert to an earlier version
before I decided that it's better to free up the descriptors at the end ;)

I'll send a v2 shortly.

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 |