[PATCH 0/3] dmaengine: dw-axi-dmac: fix termination and pause acknowledgment

[PATCH 0/3] dmaengine: dw-axi-dmac: fix termination and pause acknowledgment

[James Hilliard]

Use the layout-aware channel enable check during termination and retain
transfer resources when the channel does not stop. Keep suspension status
available until the polling pause path observes it, including when an
acknowledgment arrives after the polling timeout or another channel
interrupts concurrently.

Assisted-by: Codex:gpt-6-astra
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
James Hilliard (3):
      dmaengine: dw-axi-dmac: poll the correct enable bit during termination
      dmaengine: dw-axi-dmac: retain descriptors when termination times out
      dmaengine: dw-axi-dmac: preserve suspension status until pause observes it

 drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c | 34 ++++++++++++++++++--------
 1 file changed, 24 insertions(+), 10 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261108-submit-dw-axi-dma-stop-pause-v1-c93f1e767169

Best regards,
--  
James Hilliard <james.hilliard1@gmail.com>

[PATCH 1/3] dmaengine: dw-axi-dmac: poll the correct enable bit during termination

[James Hilliard]

For channels 16 through 31, the channel enable bits occupy bits 32 through
47 of the extended DMAC_CHEN register. dma_chan_terminate_all() only reads
the low word and tests BIT(chan->id), so it can conclude that an upper-bank
channel has stopped while its actual enable bit remains set. Descriptors
can then be freed while hardware still owns them.

Poll axi_chan_is_hw_enable(), which already handles both register banks
and the smaller channel layouts, instead of constructing another mask.

Fixes: 495e18b16e3d ("dmaengine: dw-axi-dmac: Add support DMAX_NUM_CHANNELS > 16")
Cc: stable@vger.kernel.org
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

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 7520c78f55cc..49f65486e862 100644
--- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
+++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
@@ -1181,16 +1181,15 @@ static irqreturn_t dw_axi_dma_interrupt(int irq, void *dev_id)
 static int dma_chan_terminate_all(struct dma_chan *dchan)
 {
 	struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
-	u32 chan_active = BIT(chan->id) << DMAC_CHAN_EN_SHIFT;
 	unsigned long flags;
-	u32 val;
+	bool enabled;
 	int ret;
 	LIST_HEAD(head);
 
 	axi_chan_disable(chan);
 
-	ret = readl_poll_timeout_atomic(chan->chip->regs + DMAC_CHEN, val,
-					!(val & chan_active), 1000, 50000);
+	ret = read_poll_timeout_atomic(axi_chan_is_hw_enable, enabled, !enabled,
+				      1000, 50000, false, chan);
 	if (ret == -ETIMEDOUT)
 		dev_warn(dmaengine_chan_dev(dchan),
 			 "%s failed to stop\n", axi_chan_name(chan));

-- 
2.53.0

[PATCH 2/3] dmaengine: dw-axi-dmac: retain descriptors when termination times out

[James Hilliard]

dma_chan_terminate_all() warns when the channel enable bit does not clear,
but then frees the descriptors and returns success. Its synchronize
callback only drains virtual-channel callbacks, so terminate_sync() can
also report success while hardware still accesses the transfer.

Return the polling error before detaching descriptors or changing the
peripheral routing. This keeps the transfer resources available while
the channel remains enabled and lets the caller retry termination without
treating the timeout as a successful stop.

Hold the channel lock across disabling, polling and descriptor detachment
so an interrupt handler cannot restart the channel between the stop check
and cleanup.

Fixes: 1deb96c0fa58 ("dmaegine: dw-axi-dmac: Support device_prep_dma_cyclic()")
Cc: stable@vger.kernel.org
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

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 49f65486e862..de092f190528 100644
--- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
+++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
@@ -1186,21 +1186,24 @@ static int dma_chan_terminate_all(struct dma_chan *dchan)
 	int ret;
 	LIST_HEAD(head);
 
+	spin_lock_irqsave(&chan->vc.lock, flags);
+
 	axi_chan_disable(chan);
 
 	ret = read_poll_timeout_atomic(axi_chan_is_hw_enable, enabled, !enabled,
 				      1000, 50000, false, chan);
-	if (ret == -ETIMEDOUT)
+	if (ret) {
+		spin_unlock_irqrestore(&chan->vc.lock, flags);
 		dev_warn(dmaengine_chan_dev(dchan),
 			 "%s failed to stop\n", axi_chan_name(chan));
+		return ret;
+	}
 
 	if (chan->direction != DMA_MEM_TO_MEM)
 		dw_axi_dma_set_hw_channel(chan, false);
 	if (chan->direction == DMA_MEM_TO_DEV)
 		dw_axi_dma_set_byte_halfword(chan, false);
 
-	spin_lock_irqsave(&chan->vc.lock, flags);
-
 	vchan_get_all_descriptors(&chan->vc, &head);
 
 	chan->cyclic = false;

-- 
2.53.0

[PATCH 3/3] dmaengine: dw-axi-dmac: preserve suspension status until pause observes it

[James Hilliard]

dma_chan_pause() clears SUSPENDED even when polling times out. Hardware
can set that status after the last poll, including during the final
delay, and the unconditional clear then discards an acknowledgment the
caller never observed. Another channel's interrupt can also consume the
status because the shared IRQ handler clears every channel's status
before taking its lock. Masking the suspend interrupt signal does not
prevent either path from clearing the status used by the polling code.

Leave SUSPENDED to the pause path and acknowledge it only after a
successful poll. A timed-out pause can then be retried without losing a
late acknowledgment. Mark the channel paused only after confirmation,
and let repeated pause calls succeed while it remains paused.

Clear the saved state and any old suspension status when enabling a
channel or completing termination, so a later transfer must obtain its
own acknowledgment. These state changes are serialized with pause by
the channel lock.

Fixes: 1fe20f1b8454 ("dmaengine: Introduce DW AXI DMAC driver")
Cc: stable@vger.kernel.org
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

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 de092f190528..33340f0e7462 100644
--- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
+++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
@@ -226,6 +226,9 @@ static inline void axi_chan_enable(struct axi_dma_chan *chan)
 {
 	u64 val;
 
+	axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
+	chan->is_paused = false;
+
 	if (chan->chip->dw->hdata->nr_channels >= DMAC_CHAN_16) {
 		val = axi_dma_ioread64(chan->chip, DMAC_CHEN);
 		if (chan->id >= DMAC_CHAN_16) {
@@ -1161,6 +1164,8 @@ static irqreturn_t dw_axi_dma_interrupt(int irq, void *dev_id)
 	for (i = 0; i < dw->hdata->nr_channels; i++) {
 		chan = &dw->chan[i];
 		status = axi_chan_irq_read(chan);
+		/* Suspension status is consumed by dma_chan_pause(). */
+		status &= ~DWAXIDMAC_IRQ_SUSPENDED;
 		axi_chan_irq_clear(chan, status);
 
 		dev_vdbg(chip->dev, "%s %u IRQ status: 0x%08x\n",
@@ -1206,6 +1211,8 @@ static int dma_chan_terminate_all(struct dma_chan *dchan)
 
 	vchan_get_all_descriptors(&chan->vc, &head);
 
+	axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
+	chan->is_paused = false;
 	chan->cyclic = false;
 	spin_unlock_irqrestore(&chan->vc.lock, flags);
 
@@ -1225,6 +1232,9 @@ static int dma_chan_pause(struct dma_chan *dchan)
 
 	spin_lock_irqsave(&chan->vc.lock, flags);
 
+	if (chan->is_paused)
+		goto out;
+
 	if (chan->chip->dw->hdata->nr_channels >= DMAC_CHAN_16) {
 		val = axi_dma_ioread64(chan->chip, DMAC_CHSUSPREG);
 		if (chan->id >= DMAC_CHAN_16) {
@@ -1258,10 +1268,12 @@ static int dma_chan_pause(struct dma_chan *dchan)
 		udelay(2);
 	} while (--timeout);
 
-	axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
-
-	chan->is_paused = true;
+	if (timeout) {
+		axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
+		chan->is_paused = true;
+	}
 
+out:
 	spin_unlock_irqrestore(&chan->vc.lock, flags);
 
 	return timeout ? 0 : -EAGAIN;

-- 
2.53.0

Re: [PATCH 1/3] dmaengine: dw-axi-dmac: poll the correct enable bit during termination

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] dmaengine: dw-axi-dmac: use-after-free in
  dma_chan_terminate_all() [1]
--

commit af0999f332d674abe73e6d4b71aee90f2aef6ba8
Author: James Hilliard <james.hilliard1@gmail.com>

dmaengine: dw-axi-dmac: poll the correct enable bit during termination

This commit updates dma_chan_terminate_all() to poll axi_chan_is_hw_enable()
so that upper-bank channel enable bits (channels 16 through 31) are checked
before freeing descriptors.

> 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 7520c78f55ccf..49f65486e8626 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> @@ -1181,16 +1181,15 @@ static irqreturn_t dw_axi_dma_interrupt(int irq, void *dev_id)
>  static int dma_chan_terminate_all(struct dma_chan *dchan)
>  {
>  	struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> -	u32 chan_active = BIT(chan->id) << DMAC_CHAN_EN_SHIFT;
>  	unsigned long flags;
> -	u32 val;
> +	bool enabled;
>  	int ret;
>  	LIST_HEAD(head);
>  
>  	axi_chan_disable(chan);
>  
> -	ret = readl_poll_timeout_atomic(chan->chip->regs + DMAC_CHEN, val,
> -					!(val & chan_active), 1000, 50000);
> +	ret = read_poll_timeout_atomic(axi_chan_is_hw_enable, enabled, !enabled,
> +				      1000, 50000, false, chan);

[Severity: Critical]
This problem wasn't introduced by this patch, but can dma_chan_terminate_all()
race with dma_chan_issue_pending() [1] and free descriptors while the
channel is active?

axi_chan_disable() and the poll loop run before chan->vc.lock is acquired:

CPU0                                  CPU1
dma_chan_terminate_all()
  axi_chan_disable(chan)
  read_poll_timeout_atomic(...)
                                      dma_chan_issue_pending()
                                        spin_lock_irqsave(&chan->vc.lock, ...)
                                        vchan_issue_pending(&chan->vc)
                                        axi_chan_start_first_queued(chan)
                                        spin_unlock_irqrestore(...)
  spin_lock_irqsave(&chan->vc.lock, ...)
  vchan_get_all_descriptors(&chan->vc, &head)
  spin_unlock_irqrestore(...)
  vchan_dma_desc_free_list(&chan->vc, &head)  <-- use-after-free by DMA

Because CPU1 re-enables the channel after the poll finishes, the hardware
continues processing descriptors that CPU0 then collects and frees.

[1] https://sashiko.dev/#/bug/linux-d3e7b3db-d97f-48be-a317-9ef1a4cd9953

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-dw-axi-dma-stop-pause-v1-v1-0-c48ec19e9ee4@gmail.com?part=1

Re: [PATCH 3/3] dmaengine: dw-axi-dmac: preserve suspension status until pause observes it

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] dmaengine: dw-axi-dmac: hardware suspend bit left set when
  is_paused is false
- [High] dmaengine: dw-axi-dmac: axi_chan_enable() clobbers pause state
  in cyclic mode
--

commit 225811462c3128b58ce5d32c533d4264178f3cdc
Author: James Hilliard <james.hilliard1@gmail.com>

dmaengine: dw-axi-dmac: preserve suspension status until pause observes it

This commit preserves the DWAXIDMAC_IRQ_SUSPENDED status bit across the
interrupt handler and failed pause polls so dma_chan_pause() only clears
the status and sets chan->is_paused after observing the hardware
acknowledgment, while resetting both in axi_chan_enable() and
dma_chan_terminate_all().

> 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 de092f190528a..33340f0e7462a 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> @@ -226,6 +226,9 @@ static inline void axi_chan_enable(struct axi_dma_chan *chan)
>  {
>  	u64 val;
>  
> +	axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
> +	chan->is_paused = false;
> +
>  	if (chan->chip->dw->hdata->nr_channels >= DMAC_CHAN_16) {
>  		val = axi_dma_ioread64(chan->chip, DMAC_CHEN);
>  		if (chan->id >= DMAC_CHAN_16) {

[Severity: High]
Can axi_chan_block_xfer_complete() clobber chan->is_paused and
DWAXIDMAC_IRQ_SUSPENDED on a cyclic channel by calling axi_chan_enable()?

In cyclic mode each segment has CH_CTL_H_LLI_LAST set, so segment
completion raises DWAXIDMAC_IRQ_DMA_TRF and calls axi_chan_enable() to
start the next segment:

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:axi_chan_block_xfer_complete() {
	...
	if (chan->cyclic) {
		desc = vd_to_axi_desc(vd);
		if (desc) {
			...
			axi_chan_enable(chan);
		}
	}
}

Because dw_axi_dma_interrupt() reads CH_INTSTATUS before
axi_chan_block_xfer_complete() takes chan->vc.lock, dma_chan_pause() can
run in between:

CPU0                              CPU1
dw_axi_dma_interrupt()
  status = axi_chan_irq_read(chan)
                                  dma_chan_pause()
                                    lock(&chan->vc.lock)
                                    /* sets suspend bit in HW */
                                    chan->is_paused = true;
                                    unlock(&chan->vc.lock)
  axi_chan_block_xfer_complete()
    lock(&chan->vc.lock)
    axi_chan_enable(chan)
      axi_chan_irq_clear(..., DWAXIDMAC_IRQ_SUSPENDED);
      chan->is_paused = false;    <-- clobbers pause state

When axi_chan_enable() clears chan->is_paused while the hardware suspend
bit remains asserted in DMAC_CHSUSPREG or DMAC_CHEN, a later
dma_chan_resume() sees chan->is_paused == false and skips
axi_chan_resume(), leaving the cyclic channel permanently suspended in
hardware.  If dma_chan_pause() instead timed out on CPU1,
axi_chan_enable() also clears the late DWAXIDMAC_IRQ_SUSPENDED
acknowledgment that a retry would look for.

[ ... ]

> @@ -1206,6 +1211,8 @@ static int dma_chan_terminate_all(struct dma_chan *dchan)
>  
>  	vchan_get_all_descriptors(&chan->vc, &head);
>  
> +	axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
> +	chan->is_paused = false;
>  	chan->cyclic = false;
>  	spin_unlock_irqrestore(&chan->vc.lock, flags);
>  
> @@ -1225,6 +1232,9 @@ static int dma_chan_pause(struct dma_chan *dchan)
>  
>  	spin_lock_irqsave(&chan->vc.lock, flags);
>  
> +	if (chan->is_paused)
> +		goto out;
> +
>  	if (chan->chip->dw->hdata->nr_channels >= DMAC_CHAN_16) {
>  		val = axi_dma_ioread64(chan->chip, DMAC_CHSUSPREG);
>  		if (chan->id >= DMAC_CHAN_16) {
> @@ -1258,10 +1268,12 @@ static int dma_chan_pause(struct dma_chan *dchan)
>  		udelay(2);
>  	} while (--timeout);
>  
> -	axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
> -
> -	chan->is_paused = true;
> +	if (timeout) {
> +		axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
> +		chan->is_paused = true;
> +	}
>  
> +out:
>  	spin_unlock_irqrestore(&chan->vc.lock, flags);
>  
>  	return timeout ? 0 : -EAGAIN;

[Severity: High]
Does leaving chan->is_paused false when dma_chan_pause() times out, or
clearing chan->is_paused in dma_chan_terminate_all() and axi_chan_enable(),
leave the hardware channel suspend bit set with no way for
dma_chan_resume() to clear it?

dma_chan_pause() asserts the channel suspend bit in DMAC_CHSUSPREG or
DMAC_CHEN before polling CH_INTSTATUS, and only axi_chan_resume() clears
that bit:

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dma_chan_resume() {
	...
	if (chan->is_paused)
		axi_chan_resume(chan);
	...
}

When the poll in dma_chan_pause() times out, DMAC_CHSUSPREG / DMAC_CHEN
still has the suspend bit set, so the hardware suspends the channel once
the current burst completes, yet chan->is_paused remains false.  Callers
that do not retry dmaengine_pause() before calling dmaengine_resume() (or
that terminate and reuse a paused channel, since neither
dma_chan_terminate_all() nor axi_chan_enable() clears the suspend bit in
DMAC_CHSUSPREG / DMAC_CHEN) will never run axi_chan_resume(), leaving the
channel permanently suspended in hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-dw-axi-dma-stop-pause-v1-v1-0-c48ec19e9ee4@gmail.com?part=3

Re: [PATCH 1/3] dmaengine: dw-axi-dmac: poll the correct enable bit during termination

[James Hilliard]

On Sun, Nov 8, 2026 at 10:10 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found no new issues
> in this patch, but noticed 1 related pre-existing issue it does not
> introduce:
> - [Critical] dmaengine: dw-axi-dmac: use-after-free in
>   dma_chan_terminate_all() [1]
> --
>
> commit af0999f332d674abe73e6d4b71aee90f2aef6ba8
> Author: James Hilliard <james.hilliard1@gmail.com>
>
> dmaengine: dw-axi-dmac: poll the correct enable bit during termination
>
> This commit updates dma_chan_terminate_all() to poll axi_chan_is_hw_enable()
> so that upper-bank channel enable bits (channels 16 through 31) are checked
> before freeing descriptors.
>
> > 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 7520c78f55ccf..49f65486e8626 100644
> > --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> > +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> > @@ -1181,16 +1181,15 @@ static irqreturn_t dw_axi_dma_interrupt(int irq, void *dev_id)
> >  static int dma_chan_terminate_all(struct dma_chan *dchan)
> >  {
> >       struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> > -     u32 chan_active = BIT(chan->id) << DMAC_CHAN_EN_SHIFT;
> >       unsigned long flags;
> > -     u32 val;
> > +     bool enabled;
> >       int ret;
> >       LIST_HEAD(head);
> >
> >       axi_chan_disable(chan);
> >
> > -     ret = readl_poll_timeout_atomic(chan->chip->regs + DMAC_CHEN, val,
> > -                                     !(val & chan_active), 1000, 50000);
> > +     ret = read_poll_timeout_atomic(axi_chan_is_hw_enable, enabled, !enabled,
> > +                                   1000, 50000, false, chan);
>
> [Severity: Critical]
> This problem wasn't introduced by this patch, but can dma_chan_terminate_all()
> race with dma_chan_issue_pending() [1] and free descriptors while the
> channel is active?

Patch 2/3 [0] moves the acquisition of chan->vc.lock before
axi_chan_disable() and holds it through the stop poll and
vchan_get_all_descriptors().

Since dma_chan_issue_pending() takes the same lock, it cannot restart
the channel between the stop check and descriptor detachment as shown
above.

[0] https://lore.kernel.org/r/20261108-submit-dw-axi-dma-stop-pause-v1-v1-2-c48ec19e9ee4@gmail.com/

>
> axi_chan_disable() and the poll loop run before chan->vc.lock is acquired:
>
> CPU0                                  CPU1
> dma_chan_terminate_all()
>   axi_chan_disable(chan)
>   read_poll_timeout_atomic(...)
>                                       dma_chan_issue_pending()
>                                         spin_lock_irqsave(&chan->vc.lock, ...)
>                                         vchan_issue_pending(&chan->vc)
>                                         axi_chan_start_first_queued(chan)
>                                         spin_unlock_irqrestore(...)
>   spin_lock_irqsave(&chan->vc.lock, ...)
>   vchan_get_all_descriptors(&chan->vc, &head)
>   spin_unlock_irqrestore(...)
>   vchan_dma_desc_free_list(&chan->vc, &head)  <-- use-after-free by DMA
>
> Because CPU1 re-enables the channel after the poll finishes, the hardware
> continues processing descriptors that CPU0 then collects and frees.
>
> [1] https://sashiko.dev/#/bug/linux-d3e7b3db-d97f-48be-a317-9ef1a4cd9953
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-dw-axi-dma-stop-pause-v1-v1-0-c48ec19e9ee4@gmail.com?part=1

Re: [PATCH 3/3] dmaengine: dw-axi-dmac: preserve suspension status until pause observes it

[James Hilliard]

On Sun, Nov 8, 2026 at 10:17 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [High] dmaengine: dw-axi-dmac: hardware suspend bit left set when
>   is_paused is false
> - [High] dmaengine: dw-axi-dmac: axi_chan_enable() clobbers pause state
>   in cyclic mode
> --
>
> commit 225811462c3128b58ce5d32c533d4264178f3cdc
> Author: James Hilliard <james.hilliard1@gmail.com>
>
> dmaengine: dw-axi-dmac: preserve suspension status until pause observes it
>
> This commit preserves the DWAXIDMAC_IRQ_SUSPENDED status bit across the
> interrupt handler and failed pause polls so dma_chan_pause() only clears
> the status and sets chan->is_paused after observing the hardware
> acknowledgment, while resetting both in axi_chan_enable() and
> dma_chan_terminate_all().
>
> > 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 de092f190528a..33340f0e7462a 100644
> > --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> > +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> > @@ -226,6 +226,9 @@ static inline void axi_chan_enable(struct axi_dma_chan *chan)
> >  {
> >       u64 val;
> >
> > +     axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
> > +     chan->is_paused = false;
> > +
> >       if (chan->chip->dw->hdata->nr_channels >= DMAC_CHAN_16) {
> >               val = axi_dma_ioread64(chan->chip, DMAC_CHEN);
> >               if (chan->id >= DMAC_CHAN_16) {
>
> [Severity: High]
> Can axi_chan_block_xfer_complete() clobber chan->is_paused and
> DWAXIDMAC_IRQ_SUSPENDED on a cyclic channel by calling axi_chan_enable()?
>
> In cyclic mode each segment has CH_CTL_H_LLI_LAST set, so segment
> completion raises DWAXIDMAC_IRQ_DMA_TRF and calls axi_chan_enable() to
> start the next segment:
>
> drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:axi_chan_block_xfer_complete() {
>         ...
>         if (chan->cyclic) {
>                 desc = vd_to_axi_desc(vd);
>                 if (desc) {
>                         ...
>                         axi_chan_enable(chan);
>                 }
>         }
> }
>
> Because dw_axi_dma_interrupt() reads CH_INTSTATUS before
> axi_chan_block_xfer_complete() takes chan->vc.lock, dma_chan_pause() can
> run in between:
>
> CPU0                              CPU1
> dw_axi_dma_interrupt()
>   status = axi_chan_irq_read(chan)
>                                   dma_chan_pause()
>                                     lock(&chan->vc.lock)
>                                     /* sets suspend bit in HW */
>                                     chan->is_paused = true;
>                                     unlock(&chan->vc.lock)
>   axi_chan_block_xfer_complete()
>     lock(&chan->vc.lock)
>     axi_chan_enable(chan)
>       axi_chan_irq_clear(..., DWAXIDMAC_IRQ_SUSPENDED);
>       chan->is_paused = false;    <-- clobbers pause state
>
> When axi_chan_enable() clears chan->is_paused while the hardware suspend
> bit remains asserted in DMAC_CHSUSPREG or DMAC_CHEN, a later
> dma_chan_resume() sees chan->is_paused == false and skips
> axi_chan_resume(), leaving the cyclic channel permanently suspended in
> hardware.  If dma_chan_pause() instead timed out on CPU1,
> axi_chan_enable() also clears the late DWAXIDMAC_IRQ_SUSPENDED
> acknowledgment that a retry would look for.
>
> [ ... ]
>
> > @@ -1206,6 +1211,8 @@ static int dma_chan_terminate_all(struct dma_chan *dchan)
> >
> >       vchan_get_all_descriptors(&chan->vc, &head);
> >
> > +     axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
> > +     chan->is_paused = false;
> >       chan->cyclic = false;
> >       spin_unlock_irqrestore(&chan->vc.lock, flags);
> >
> > @@ -1225,6 +1232,9 @@ static int dma_chan_pause(struct dma_chan *dchan)
> >
> >       spin_lock_irqsave(&chan->vc.lock, flags);
> >
> > +     if (chan->is_paused)
> > +             goto out;
> > +
> >       if (chan->chip->dw->hdata->nr_channels >= DMAC_CHAN_16) {
> >               val = axi_dma_ioread64(chan->chip, DMAC_CHSUSPREG);
> >               if (chan->id >= DMAC_CHAN_16) {
> > @@ -1258,10 +1268,12 @@ static int dma_chan_pause(struct dma_chan *dchan)
> >               udelay(2);
> >       } while (--timeout);
> >
> > -     axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
> > -
> > -     chan->is_paused = true;
> > +     if (timeout) {
> > +             axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
> > +             chan->is_paused = true;
> > +     }
> >
> > +out:
> >       spin_unlock_irqrestore(&chan->vc.lock, flags);
> >
> >       return timeout ? 0 : -EAGAIN;
>
> [Severity: High]
> Does leaving chan->is_paused false when dma_chan_pause() times out, or
> clearing chan->is_paused in dma_chan_terminate_all() and axi_chan_enable(),
> leave the hardware channel suspend bit set with no way for
> dma_chan_resume() to clear it?
>
> dma_chan_pause() asserts the channel suspend bit in DMAC_CHSUSPREG or
> DMAC_CHEN before polling CH_INTSTATUS, and only axi_chan_resume() clears
> that bit:
>
> drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dma_chan_resume() {
>         ...
>         if (chan->is_paused)
>                 axi_chan_resume(chan);
>         ...
> }
>
> When the poll in dma_chan_pause() times out, DMAC_CHSUSPREG / DMAC_CHEN
> still has the suspend bit set, so the hardware suspends the channel once
> the current burst completes, yet chan->is_paused remains false.  Callers
> that do not retry dmaengine_pause() before calling dmaengine_resume() (or
> that terminate and reuse a paused channel, since neither
> dma_chan_terminate_all() nor axi_chan_enable() clears the suspend bit in
> DMAC_CHSUSPREG / DMAC_CHEN) will never run axi_chan_resume(), leaving the
> channel permanently suspended in hardware.

The failed-pause/resume case is an issue, but successful termination and
reuse are different. The CH_SUSP register description [0] states that
"CH_SUSP is cleared when channel is disabled."

dma_chan_terminate_all() disables the channel and waits for CH_EN to
clear before resetting is_paused. With patch 2/3 applied, a disable
timeout returns an error before reaching that reset. Successful
termination therefore clears the hardware suspend request without
needing axi_chan_resume().

That does not resolve the late-acknowledgment case when the channel
remains enabled after a failed pause.

[0] https://www.scribd.com/document/962771796/DW-Axi-Dmac-Databook

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-dw-axi-dma-stop-pause-v1-v1-0-c48ec19e9ee4@gmail.com?part=3

Re: [PATCH 1/3] dmaengine: dw-axi-dmac: poll the correct enable bit during termination

[Andy Shevchenko]

On Sun, Nov 08, 2026 at 10:02:36PM -0700, James Hilliard wrote:
> For channels 16 through 31, the channel enable bits occupy bits 32 through
> 47 of the extended DMAC_CHEN register. dma_chan_terminate_all() only reads
> the low word and tests BIT(chan->id), so it can conclude that an upper-bank
> channel has stopped while its actual enable bit remains set. Descriptors
> can then be freed while hardware still owns them.
> 
> Poll axi_chan_is_hw_enable(), which already handles both register banks
> and the smaller channel layouts, instead of constructing another mask.

...

> -	ret = readl_poll_timeout_atomic(chan->chip->regs + DMAC_CHEN, val,
> -					!(val & chan_active), 1000, 50000);
> +	ret = read_poll_timeout_atomic(axi_chan_is_hw_enable, enabled, !enabled,
> +				      1000, 50000, false, chan);

With a single argument it's readx_poll_timeout_atomic().
While at it, you may use 1 * USEC_PER_MSEC and 50 * USEC_PER_MSEC instead of
plain numbers.

	ret = readx_poll_timeout_atomic(axi_chan_is_hw_enable, chan,
					enabled, !enabled,
					1 * USEC_PER_MSEC, 50 * USEC_PER_MSEC);

-- 
With Best Regards,
Andy Shevchenko