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

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

[Sungho Kim]

A channel fault still completes the descriptor as a success, and the
callback is invoked with a NULL result. Record the outcome and pass
DMA_TRANS_ABORTED to callback_result on a fault, an abort or a rejected
request, reporting it on the last descriptor of the transaction.

Signed-off-by: Sungho Kim <sungho.kim@furiosa.ai>
---
 drivers/dma/pl330.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index c2d0b473b..d479d73cb 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -458,6 +458,12 @@ struct dma_pl330_chan {
 	/* for cyclic capability */
 	bool cyclic;
 
+	/*
+	 * First failure among the completed descriptors of a transaction,
+	 * reported once its last descriptor completes.
+	 */
+	enum dmaengine_tx_result result;
+
 	/* for runtime pm tracking */
 	bool active;
 };
@@ -534,6 +540,8 @@ struct dma_pl330_desc {
 	struct pl330_reqcfg rqcfg;
 
 	enum desc_status status;
+	/* How the transfer of this descriptor ended */
+	enum dmaengine_tx_result result;
 
 	int bytes_requested;
 	bool last;
@@ -1572,6 +1580,8 @@ static void dma_pl330_rqcb(struct dma_pl330_desc *desc, enum pl330_op_err err)
 	spin_lock_irqsave(&pch->lock, flags);
 
 	desc->status = DONE;
+	if (err != PL330_ERR_NONE)
+		desc->result = DMA_TRANS_ABORTED;
 
 	spin_unlock_irqrestore(&pch->lock, flags);
 
@@ -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;
+
 		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);
 		}
 	}
@@ -2167,6 +2192,7 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan)
 
 	dma_cookie_init(chan);
 	pch->cyclic = false;
+	pch->result = DMA_TRANS_NOERROR;
 
 	pch->thread = pl330_request_channel(pl330);
 	if (!pch->thread) {
@@ -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) {
@@ -2579,6 +2606,7 @@ static struct dma_pl330_desc *pluck_desc(struct list_head *pool,
 		list_del_init(&desc->node);
 
 		desc->status = PREP;
+		desc->result = DMA_TRANS_NOERROR;
 		desc->txd.callback = NULL;
 		desc->txd.callback_result = NULL;
 	}
-- 
2.53.0

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

[sashiko-bot]

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