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

[sashiko-bot@kernel.org]

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