diff mbox series

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

Message ID 20191210123352.7555-2-s.hauer@pengutronix.de (mailing list archive)
State Changes Requested
Headers show
Series virt-dma and i.MX SDMA fixes | expand

Commit Message

Sascha Hauer Dec. 10, 2019, 12:33 p.m. UTC
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(-)

Comments

Peter Ujfalusi Dec. 10, 2019, 1:12 p.m. UTC | #1
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
Sascha Hauer Dec. 10, 2019, 2:19 p.m. UTC | #2
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
Peter Ujfalusi Dec. 10, 2019, 3:23 p.m. UTC | #3
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
diff mbox series

Patch

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