On Thu, Jun 20, 2024 at 10:53:53AM +0800, Jie Hai wrote:
> List desc_allocated was introduced for the case of a transfer
> submitted multiple times. But elegating descriptors on the list
> causes other problems.
>
> For example, in the multi-thread scenario, which tasks are
> continuously created and submitted by each thread. If one of
> the threads calls dmaengine_terminate_all, for dirvers using
> vchan_get_all_descriptors, all descriptors will be freed. If
> there's another thread submitting a transfer A by
> vchan_tx_submit, the following results may be generated:
> 1. desc A is freeing -> visit wrong address of node prep/next.
> 2. desc A is freed -> visit invalid address of A.
>
> In the above case, calltrace is generated and the system is
> suspended. This can be tested by dmatest.
What's test steps to reproduce this problem?
Frank
>
> This patch removes desc_allocated from vchan_get_all_descriptors,
> and add new function 'vchan_get_all_allocated_descs' to get all
> descriptors ever allocated.
>
> And apply vchan_get_all_allocated_descs to free chan resource and
> vchan_get_all_descriptors to terminate all transfers, respectively.
> This avoids freeing up descriptors in use by other threads.
>
> Signed-off-by: Jie Hai <haijie1@xxxxxxxxxx>
> ---
> drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c | 2 +-
> drivers/dma/fsl-edma-common.c | 2 +-
> drivers/dma/fsl-qdma.c | 2 +-
> drivers/dma/sf-pdma/sf-pdma.c | 2 +-
> drivers/dma/virt-dma.h | 20 ++++++++++++++++++--
> 5 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c b/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
> index 36384d019263..efdecf15e1b3 100644
> --- a/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
> +++ b/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
> @@ -71,7 +71,7 @@ static void dpaa2_qdma_free_chan_resources(struct dma_chan *chan)
> LIST_HEAD(head);
>
> spin_lock_irqsave(&dpaa2_chan->vchan.lock, flags);
> - vchan_get_all_descriptors(&dpaa2_chan->vchan, &head);
> + vchan_get_all_allocated_descs(&dpaa2_chan->vchan, &head);
> spin_unlock_irqrestore(&dpaa2_chan->vchan.lock, flags);
>
> vchan_dma_desc_free_list(&dpaa2_chan->vchan, &head);
> diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
> index 3af430787315..1e0ad87eb7fa 100644
> --- a/drivers/dma/fsl-edma-common.c
> +++ b/drivers/dma/fsl-edma-common.c
> @@ -828,7 +828,7 @@ void fsl_edma_free_chan_resources(struct dma_chan *chan)
> if (edma->drvdata->dmamuxs)
> fsl_edma_chan_mux(fsl_chan, 0, false);
> fsl_chan->edesc = NULL;
> - vchan_get_all_descriptors(&fsl_chan->vchan, &head);
> + vchan_get_all_allocated_descs(&fsl_chan->vchan, &head);
> fsl_edma_unprep_slave_dma(fsl_chan);
> spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags);
>
> diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c
> index 5005e138fc23..7af428db404e 100644
> --- a/drivers/dma/fsl-qdma.c
> +++ b/drivers/dma/fsl-qdma.c
> @@ -316,7 +316,7 @@ static void fsl_qdma_free_chan_resources(struct dma_chan *chan)
> LIST_HEAD(head);
>
> spin_lock_irqsave(&fsl_chan->vchan.lock, flags);
> - vchan_get_all_descriptors(&fsl_chan->vchan, &head);
> + vchan_get_all_allocated_descs(&fsl_chan->vchan, &head);
> spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags);
>
> vchan_dma_desc_free_list(&fsl_chan->vchan, &head);
> diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
> index 428473611115..4dc8a8c8ad80 100644
> --- a/drivers/dma/sf-pdma/sf-pdma.c
> +++ b/drivers/dma/sf-pdma/sf-pdma.c
> @@ -147,7 +147,7 @@ static void sf_pdma_free_chan_resources(struct dma_chan *dchan)
> sf_pdma_disable_request(chan);
> kfree(chan->desc);
> chan->desc = NULL;
> - vchan_get_all_descriptors(&chan->vchan, &head);
> + vchan_get_all_allocated_descs(&chan->vchan, &head);
> sf_pdma_disclaim_chan(chan);
> spin_unlock_irqrestore(&chan->vchan.lock, flags);
> vchan_dma_desc_free_list(&chan->vchan, &head);
> diff --git a/drivers/dma/virt-dma.h b/drivers/dma/virt-dma.h
> index 59d9eabc8b67..4492641b79f6 100644
> --- a/drivers/dma/virt-dma.h
> +++ b/drivers/dma/virt-dma.h
> @@ -187,13 +187,29 @@ static inline void vchan_get_all_descriptors(struct virt_dma_chan *vc,
> {
> lockdep_assert_held(&vc->lock);
>
> - list_splice_tail_init(&vc->desc_allocated, head);
> 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);
> }
>
> +/**
> + * vchan_get_all_allocated_descs - obtain all descriptors
> + * @vc: virtual channel to get descriptors from
> + * @head: list of descriptors found
> + *
> + * vc.lock must be held by caller
> + *
> + * Removes all descriptors from internal lists, and provides a list of all
> + * descriptors found
> + */
> +static inline void vchan_get_all_allocated_descs(struct virt_dma_chan *vc,
> + struct list_head *head)
> +{
> + list_splice_tail_init(&vc->desc_allocated, head);
> + vchan_get_all_descriptors(vc, head);
> +}
> +
> static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
> {
> struct virt_dma_desc *vd;
> @@ -201,7 +217,7 @@ static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
> LIST_HEAD(head);
>
> spin_lock_irqsave(&vc->lock, flags);
> - vchan_get_all_descriptors(vc, &head);
> + vchan_get_all_allocated_descs(vc, &head);
> list_for_each_entry(vd, &head, node)
> dmaengine_desc_clear_reuse(&vd->tx);
> spin_unlock_irqrestore(&vc->lock, flags);
> --
> 2.33.0
>
