Re: [PATCH v2 3/3] dmaengine: dw-axi-dmac: preserve pending pause requests

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Nov 09, 2026 at 08:06:02PM -0700, James Hilliard wrote:
> 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.
> 
> Leave SUSPENDED to the pause and resume paths. Track the suspend request
> separately from its acknowledgment so a timeout can be followed by either
> a pause retry or a resume. Wait for acknowledgment before clearing the
> hardware request, since hardware ignores an earlier clear. If that wait
> also times out, return -EAGAIN with the request still pending. Use the
> atomic polling helper so an acknowledgment during the final delay is
> observed.
> 
> A completed transfer is also quiescent, but its completion handler may
> still need to advance a cyclic descriptor. Defer that restart while a
> pause is pending, and resume it without reprogramming the first LLI.
> Track the active descriptor until its completion handler runs so that
> resume and issue_pending cannot restart a completed cyclic segment or
> reissue a completed noncyclic descriptor while its IRQ is pending.
> Start queued work after the active descriptor is retired, respecting the
> pause request.
> 
> Clear stale suspension status at the start of a new descriptor rather
> than at each cyclic restart. Reset the software state after successful
> termination and when allocating a channel for a new client.

...

> +/* Called with vc.lock held. A timed-out request can still be acknowledged. */

We have context checkers, please use __must_hold() or alike annotations.

> +static int axi_chan_wait_for_suspend(struct axi_dma_chan *chan)
> +{
> +	bool suspended;
> +	int ret;
> +
> +	if (chan->is_paused)
> +		return 0;
> +
> +	ret = readx_poll_timeout_atomic(axi_chan_is_suspended, chan, suspended, suspended,
> +				       2, 40);

Again, fix indentation, reformat and split logically.

	ret = readx_poll_timeout_atomic(axi_chan_is_suspended, chan,
					suspended, suspended, 2, 40);

(note no leading spaces in the second line, only tabs fit).

> +	if (ret)
> +		return -EAGAIN;
> +
> +	axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
> +	chan->is_paused = true;
> +	return 0;
> +}

...

> -	do  {
> -		if (axi_chan_irq_read(chan) & DWAXIDMAC_IRQ_SUSPENDED)
> -			break;
> -
> -		udelay(2);
> -	} while (--timeout);
> -
> -	axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
> -
> -	chan->is_paused = true;
> -
> +wait:
> +	ret = axi_chan_wait_for_suspend(chan);

You may split the conversion to readx_poll_timeout() into a separate patch.

...

> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac.h
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac.h
> @@ -52,7 +52,9 @@ struct axi_dma_chan {
>  	enum dma_transfer_direction	direction;
>  	bool				cyclic;
>  	/* these other elements are all protected by vc.lock */
> +	bool				pause_requested;
>  	bool				is_paused;
> +	bool				cyclic_restart;

Probably making them bitfields will be a good idea to save some memory:

	bool				pause_requested:1;
	bool				is_paused:1;
	bool				cyclic_restart:1;

>  };

Just run `pahole` before and after (won't probably show you the gain,
but before might show better position for "cyclic" member, which can
be amended later).

-- 
With Best Regards,
Andy Shevchenko