[PATCH] dmaengine: dw-axi-dmac: restart the next queued transfer after completion

[PATCH] dmaengine: dw-axi-dmac: restart the next queued transfer after completion

[Baineng Shou]

Commit 333e11bf47fa ("Avoid hw_desc array overrun in dw-axi-dmac") removed
the axi_chan_start_first_queued() call from the non-cyclic completion path,
after observing that a descriptor started there could be dropped by the
non-idle check in axi_chan_block_xfer_start() because the channel has not
fully stopped yet when the transfer-complete interrupt arrives.

That removes more than the race: with more than one descriptor issued on
a channel, the first completion now leaves the remaining descriptors
queued forever - nothing starts them and no further interrupt arrives,
so their callbacks never run.

Fix it by waiting for the channel to actually go idle after the
completion and error paths disable it, restore the start of the next
queued descriptor in the non-cyclic completion path, and skip the start
attempt from issue_pending() while the channel is still busy.

Fixes: 333e11bf47fa ("Avoid hw_desc array overrun in dw-axi-dmac")
Cc: stable@vger.kernel.org
Signed-off-by: Baineng Shou <shoubaineng@gmail.com>
---
 .../dma/dw-axi-dmac/dw-axi-dmac-platform.c    | 31 ++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
index eebed2474210..f3d89f5cc12a 100644
--- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
+++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
@@ -263,6 +263,26 @@ static inline bool axi_chan_is_hw_enable(struct axi_dma_chan *chan)
 		return !!(val & (BIT(chan->id) << DMAC_CHAN_EN_SHIFT));
 }
 
+static int axi_chan_wait_idle(struct axi_dma_chan *chan)
+{
+	unsigned int timeout = 50; /* 50 x 2us = 100us */
+
+	/*
+	 * Writing the channel-disable bit is asynchronous: the hardware
+	 * finishes the current burst, flushes the FIFO and only then
+	 * clears the enable bit. Wait until the channel is really idle
+	 * before (re)starting a transfer, otherwise the non-idle check
+	 * in axi_chan_block_xfer_start() would silently drop it.
+	 */
+	while (axi_chan_is_hw_enable(chan)) {
+		if (!--timeout)
+			return -ETIMEDOUT;
+		udelay(2);
+	}
+
+	return 0;
+}
+
 static void axi_dma_hw_init(struct axi_dma_chip *chip)
 {
 	int ret;
@@ -501,7 +521,7 @@ static void dma_chan_issue_pending(struct dma_chan *dchan)
 	unsigned long flags;
 
 	spin_lock_irqsave(&chan->vc.lock, flags);
-	if (vchan_issue_pending(&chan->vc))
+	if (vchan_issue_pending(&chan->vc) && !axi_chan_is_hw_enable(chan))
 		axi_chan_start_first_queued(chan);
 	spin_unlock_irqrestore(&chan->vc.lock, flags);
 }
@@ -1067,6 +1087,9 @@ static noinline void axi_chan_handle_err(struct axi_dma_chan *chan, u32 status)
 	spin_lock_irqsave(&chan->vc.lock, flags);
 
 	axi_chan_disable(chan);
+	if (axi_chan_wait_idle(chan))
+		dev_warn(chan2dev(chan), "%s failed to go idle\n",
+			 axi_chan_name(chan));
 
 	/* The bad descriptor currently is in the head of vc list */
 	vd = vchan_next_desc(&chan->vc);
@@ -1108,6 +1131,9 @@ static void axi_chan_block_xfer_complete(struct axi_dma_chan *chan)
 		dev_err(chan2dev(chan), "BUG: %s caught DWAXIDMAC_IRQ_DMA_TRF, but channel not idle!\n",
 			axi_chan_name(chan));
 		axi_chan_disable(chan);
+		if (axi_chan_wait_idle(chan))
+			dev_warn(chan2dev(chan), "%s failed to go idle\n",
+				 axi_chan_name(chan));
 	}
 
 	/* The completed descriptor currently is in the head of vc list */
@@ -1141,6 +1167,9 @@ static void axi_chan_block_xfer_complete(struct axi_dma_chan *chan)
 		/* Remove the completed descriptor from issued list before completing */
 		list_del(&vd->node);
 		vchan_cookie_complete(vd);
+
+		/* Restart the next queued descriptor, if any */
+		axi_chan_start_first_queued(chan);
 	}
 
 out:
-- 
2.34.1

Re: [PATCH] dmaengine: dw-axi-dmac: restart the next queued transfer after completion

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dmaengine: dw-axi-dmac: permanent queue stall on axi_chan_wait_idle() timeout
--

commit 9a989fc2edd4f4603b5177a0c01ff0b20e6546b9
Author: Baineng Shou <shoubaineng@gmail.com>

dmaengine: dw-axi-dmac: restart the next queued transfer after completion

Commit 333e11bf47fa ("Avoid hw_desc array overrun in dw-axi-dmac") removed
the axi_chan_start_first_queued() call from the non-cyclic completion path,
after observing that a descriptor started there could be dropped by the
non-idle check in axi_chan_block_xfer_start() because the channel has not
fully stopped yet when the transfer-complete interrupt arrives.

> diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> index eebed2474210d..f3d89f5cc12a7 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c

[ ... ]

> @@ -1067,6 +1087,9 @@ static noinline void axi_chan_handle_err(struct axi_dma_chan *chan, u32 status)
>  	spin_lock_irqsave(&chan->vc.lock, flags);
>  
>  	axi_chan_disable(chan);
> +	if (axi_chan_wait_idle(chan))
> +		dev_warn(chan2dev(chan), "%s failed to go idle\n",
> +			 axi_chan_name(chan));

[Severity: High]
If axi_chan_wait_idle() times out (e.g., due to bus contention extending the
flush time beyond 100us), it returns -ETIMEDOUT. Because the error is ignored
here, execution continues and calls axi_chan_start_first_queued(chan) at the end
of this function.

Since the hardware channel is still active, axi_chan_block_xfer_start() will
evaluate axi_chan_is_hw_enable(chan) as true, log an error, and return without
programming the new transfer. 

The next descriptor will remain in the pending queue, but no hardware transfer
will be started. Without a new transfer, no further completion interrupts will
fire, permanently deadlocking the DMA channel queue.

Would it be better to handle the timeout error and avoid attempting to start
the next queued descriptor while the channel is busy?

>  
>  	/* The bad descriptor currently is in the head of vc list */
>  	vd = vchan_next_desc(&chan->vc);

[ ... ]

> @@ -1108,6 +1131,9 @@ static void axi_chan_block_xfer_complete(struct axi_dma_chan *chan)
>  		dev_err(chan2dev(chan), "BUG: %s caught DWAXIDMAC_IRQ_DMA_TRF, but channel not idle!\n",
>  			axi_chan_name(chan));
>  		axi_chan_disable(chan);
> +		if (axi_chan_wait_idle(chan))
> +			dev_warn(chan2dev(chan), "%s failed to go idle\n",
> +				 axi_chan_name(chan));

[Severity: High]
Does the same unhandled timeout logic apply to the completion handler here?

If axi_chan_wait_idle() times out, we will eventually call
axi_chan_start_first_queued(chan) lower in this function, which could drop the
transfer and result in a permanent queue stall.

>  	}
>  
>  	/* The completed descriptor currently is in the head of vc list */
> @@ -1141,6 +1167,9 @@ static void axi_chan_block_xfer_complete(struct axi_dma_chan *chan)
>  		/* Remove the completed descriptor from issued list before completing */
>  		list_del(&vd->node);
>  		vchan_cookie_complete(vd);
> +
> +		/* Restart the next queued descriptor, if any */
> +		axi_chan_start_first_queued(chan);
>  	}
>  
>  out:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919084732.502247-1-shoubaineng@gmail.com?part=1