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