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

[Sascha Hauer <s.hauer@pengutronix.de>]

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 |