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