Re: [PATCH] dmaengine: pl330: report a failed transfer through callback_result

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] dmaengine: pl330: out-of-order completion in fill_queue()
  corrupts pch->result
It also noticed 2 related pre-existing issues this patch does not
introduce:
- [High] dmaengine: pl330: synchronous pl330_tasklet() call in
  pl330_issue_pending() [1]
- [Critical] dmaengine: pl330: desc_pool list corruption in
  pl330_terminate_all() [2]
--

commit 5e7b59c03f80710bfb7ee7db9e4f312f02ac2fb2
Author: Sungho Kim <sungho.kim@furiosa.ai>

dmaengine: pl330: report a failed transfer through callback_result

This commit records descriptor transfer failures in the PL330 driver and
reports DMA_TRANS_ABORTED through callback_result when the last descriptor
of a transaction completes.

> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 977a9fde80adb..72e83dfdcecbb 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c

[ ... ]

> @@ -2059,6 +2069,7 @@ static inline void fill_queue(struct dma_pl330_chan *pch)
>  		} else {
>  			/* Unacceptable request */
>  			desc->status = DONE;
> +			desc->result = DMA_TRANS_ABORTED;
>  			dev_err(pch->dmac->ddma.dev, "%s:%d Bad Desc(%d)\n",
>  					__func__, __LINE__, desc->txd.cookie);
>  			tasklet_schedule(&pch->task);
> @@ -2101,12 +2112,26 @@ static void pl330_tasklet(struct tasklet_struct *t)
>  
>  	while (!list_empty(&pch->completed_list)) {
>  		struct dmaengine_desc_callback cb;
> +		struct dmaengine_result res = { .residue = 0 };
>  
>  		desc = list_first_entry(&pch->completed_list,
>  					struct dma_pl330_desc, node);
>  
>  		dmaengine_desc_get_callback(&desc->txd, &cb);
>  
> +		/*
> +		 * callback_result is only on the last descriptor of a transaction
> +		 * (tx_submit copies just callback to the periods of a cyclic
> +		 * one), so a failure of an earlier descriptor is held on the
> +		 * channel until the last one completes.
> +		 */
> +		if (pch->result == DMA_TRANS_NOERROR)
> +			pch->result = desc->result;
> +		res.result = pch->result;
> +		if (desc->last)
> +			pch->result = DMA_TRANS_NOERROR;
> +		desc->result = DMA_TRANS_NOERROR;

[Severity: Medium]
Can an out-of-order failure in fill_queue() misattribute pch->result across
descriptors?

When pl330_submit_req() fails with an error other than -EAGAIN (for example
-ENOMEM when a request exceeds the microcode buffer), fill_queue() marks
that descriptor DONE with DMA_TRANS_ABORTED while earlier descriptors on
pch->work_list are still BUSY in the hardware thread:

drivers/dma/pl330.c:pl330_tasklet() {
	list_for_each_entry_safe(desc, _dt, &pch->work_list, node)
		if (desc->status == DONE) {
			if (!pch->cyclic)
				dma_cookie_complete(&desc->txd);
			list_move_tail(&desc->node, &pch->completed_list);
		}
}

pl330_tasklet() moves DONE descriptors to pch->completed_list ahead of
earlier BUSY descriptors.  If an earlier transaction's last descriptor is
still BUSY when the first descriptor of the next transaction is rejected by
fill_queue(), the rejected descriptor completes first and sets
pch->result = DMA_TRANS_ABORTED, which is then reported and cleared when
the earlier transaction finishes without error.  Similarly, within a
multi-descriptor transaction, if the last descriptor is rejected while an
earlier descriptor is BUSY, callback_result runs before the hardware stops
and resets pch->result to DMA_TRANS_NOERROR; if the in-flight descriptor
later faults, pch->result is left stuck at DMA_TRANS_ABORTED for the next
transaction.

>  		if (pch->cyclic) {
>  			desc->status = PREP;
>  			list_move_tail(&desc->node, &pch->work_list);
> @@ -2126,7 +2151,7 @@ static void pl330_tasklet(struct tasklet_struct *t)
>  
>  		if (dmaengine_desc_callback_valid(&cb)) {
>  			spin_unlock_irqrestore(&pch->lock, flags);
> -			dmaengine_desc_callback_invoke(&cb, NULL);
> +			dmaengine_desc_callback_invoke(&cb, &res);
>  			spin_lock_irqsave(&pch->lock, flags);
>  		}
>  	}

[Severity: High]
This problem wasn't introduced by this patch, but pl330_issue_pending()
calls pl330_tasklet() directly in the caller's context [1], running
completion callbacks synchronously inside dma_async_issue_pending():

drivers/dma/pl330.c:pl330_issue_pending() {
	...
	list_splice_tail_init(&pch->submitted_list, &pch->work_list);
	spin_unlock_irqrestore(&pch->lock, flags);

	pl330_tasklet(&pch->task);
}

If a descriptor on pch->work_list was marked DONE by dma_pl330_rqcb() or
rejected by fill_queue(), a client calling dma_async_issue_pending() under
its own lock deadlocks when its completion callback re-acquires that lock:

  spin_lock(&client->lock)
  <hardirq>
    dma_pl330_rqcb()
      desc->status = DONE
      tasklet_schedule(&pch->task)
  dma_async_issue_pending()
    pl330_issue_pending()
      pl330_tasklet(&pch->task)
        dmaengine_desc_callback_invoke()
          client_complete_cb()
            spin_lock(&client->lock)  <-- deadlock

The direct pl330_tasklet() call also races with the scheduled tasklet when
pch->work_list becomes empty, allowing both contexts to set
power_down = true and call pm_runtime_put_autosuspend(), underflowing the
controller's runtime PM usage counter.

[ ... ]

> @@ -2295,6 +2321,7 @@ static int pl330_terminate_all(struct dma_chan *chan)
>  
>  	power_down = pch->active;
>  	pch->active = false;
> +	pch->result = DMA_TRANS_NOERROR;
>  
>  	/* Mark all desc done */
>  	list_for_each_entry(desc, &pch->submitted_list, node) {

[Severity: Critical]
This problem wasn't introduced by this patch, but pl330_terminate_all() and
pl330_free_chan_resources() splice descriptors back into pl330->desc_pool
without holding pl330->pool_lock [2]:

drivers/dma/pl330.c:pl330_terminate_all() {
	...
	list_splice_tail_init(&pch->submitted_list, &pl330->desc_pool);
	list_splice_tail_init(&pch->work_list, &pl330->desc_pool);
	list_splice_tail_init(&pch->completed_list, &pl330->desc_pool);
	spin_unlock_irqrestore(&pch->lock, flags);
	...
}

drivers/dma/pl330.c:pl330_free_chan_resources() {
	...
	spin_lock_irqsave(&pl330->lock, flags);
	...
	if (pch->cyclic)
		list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool);

	spin_unlock_irqrestore(&pl330->lock, flags);
	...
}

Can this corrupt pl330->desc_pool when another channel concurrently
allocates or frees descriptors under pl330->pool_lock via pluck_desc() or
add_desc()?

CPU0                              CPU1
pl330_terminate_all()             pl330_get_desc()
  spin_lock_irqsave(&pch->lock)     pluck_desc(&pl330->desc_pool,
                                               &pl330->pool_lock)
                                      spin_lock_irqsave(&pl330->pool_lock)
  list_splice_tail_init(...,          list_del_init(&desc->node)
    &pl330->desc_pool)                  <-- concurrent list mutation
                                      spin_unlock_irqrestore(&pl330->pool_lock)
  spin_unlock_irqrestore(&pch->lock)

[1] https://sashiko.dev/#/bug/linux-130356b0-b34d-4ec8-b462-9a2ed651bcf4
[2] https://sashiko.dev/#/bug/linux-ecddac4a-242e-4dfb-895d-8ed9e5e87844

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111152109.1551738-1-sungho.kim@furiosa.ai?part=1