Re: [PATCH v4 5/5] dmaengine: dw-axi-dmac: preserve pending pause requests

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

On Thu, Nov 12, 2026 at 01:22:54AM -0700, James Hilliard wrote:
> dma_chan_pause() clears SUSPENDED even when polling times out. Hardware
> can set that status after the last poll, 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.
> 
> 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.
> 
> Store the lock-protected pause flags as bitfields in existing padding.

...

> +static bool axi_chan_is_suspended(struct axi_dma_chan *chan)
> +{
> +	if (!axi_chan_is_hw_enable(chan)) {
> +		/* A completed transfer cannot acknowledge a new suspend request. */
> +		axi_chan_disable(chan);
> +		return true;
> +	}
> +
> +	return axi_chan_irq_read(chan) & DWAXIDMAC_IRQ_SUSPENDED;

You can invert the conditional to be positive. This will also make code a bit
neater I think.

	if (axi_chan_is_hw_enable(chan))
		return axi_chan_irq_read(chan) & DWAXIDMAC_IRQ_SUSPENDED;

	/* A completed transfer cannot acknowledge a new suspend request. */
	axi_chan_disable(chan);
	return true;

> +}

...

> static int dma_chan_resume(struct dma_chan *dchan)
>  {
>  	struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
>  	unsigned long flags;
> +	int ret = 0;

Better in terms of maintenance is to make an assignment closer to its user.

>  	spin_lock_irqsave(&chan->vc.lock, flags);
>  
> -	if (chan->is_paused)
> -		axi_chan_resume(chan);

	ret = 0;

> +	if (!chan->pause_requested)
> +		goto out;
> +
> +	/* Hardware ignores resume until the suspend request is acknowledged. */
> +	ret = axi_chan_wait_for_suspend(chan);
> +	if (ret)
> +		goto out;
> +
> +	axi_chan_resume(chan);
> +	if (chan->cyclic_restart) {
> +		chan->cyclic_restart = false;
> +		axi_chan_enable(chan);
> +	} else {
> +		axi_chan_start_first_queued(chan);
> +	}
>  
> +out:
>  	spin_unlock_irqrestore(&chan->vc.lock, flags);
>  
> -	return 0;
> +	return ret;
>  }

...

> struct axi_dma_chan {
>  	void __iomem			*chan_regs;
>  	u8				id;
>  	u8				hw_handshake_num;

+ blank line?

> +	/* Protected by vc.lock. */
> +	bool				pause_requested:1;
> +	bool				is_paused:1;
> +	bool				cyclic_restart:1;
> +
>  	atomic_t			descs_allocated;
>  
>  	struct dma_pool			*desc_pool;

>  	struct axi_dma_desc		*desc;
>  	struct dma_slave_config		config;
>  	enum dma_transfer_direction	direction;
> -	/* these other elements are all protected by vc.lock */
> -	bool				is_paused;
>  };

-- 
With Best Regards,
Andy Shevchenko