Re: [PATCH v3] dmaengine: dw-axi-dmac: report paused state and residue in tx_status

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dmaengine: dw-axi-dmac: false 0 residue for queued descriptors due to stale HW pointers
--

commit 1fb45501ae16a4b2f841216b130cbe2ef95cb7a3
Author: Rui Wang <wr574332525@163.com>

dmaengine: dw-axi-dmac: report paused state and residue in tx_status

The driver implements device_pause/device_resume, but device_tx_status
keeps reporting DMA_IN_PROGRESS for a paused channel, so clients cannot
tell a paused channel apart from a running one. This patch addresses
this by properly reporting DMA_PAUSED and updating the residue calculation
to read the hardware pointers directly instead of relying on completed
blocks.

> 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..5653a57126d66 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> @@ -352,33 +352,120 @@ static void vchan_desc_put(struct virt_dma_desc *vdesc)

[ ... ]

>  	vdesc = vchan_find_desc(&chan->vc, cookie);
>  	if (vdesc) {
> -		length = vd_to_axi_desc(vdesc)->length;
> -		completed_blocks = vd_to_axi_desc(vdesc)->completed_blocks;
> -		len = vd_to_axi_desc(vdesc)->hw_desc[0].len;
> -		completed_length = completed_blocks * len;
> -		bytes = length - completed_length;
> +		desc = vd_to_axi_desc(vdesc);
> +		length = desc->length;
> +
> +		if (chan->cyclic) {
> +			completed_length = desc->completed_blocks *
> +					   desc->hw_desc[0].len;
> +		} else if (vdesc == vchan_next_desc(&chan->vc)) {
> +			/*
> +			 * The descriptor is on the hardware: read the progress
> +			 * from its pointer. If the transfer has just finished
> +			 * but the interrupt has not reaped the descriptor yet,
> +			 * the pointer sits at the end and the residue reads 0.
> +			 *
> +			 * The hardware clears the channel enable bit when the
> +			 * transfer completes, so a still-enabled channel cannot
> +			 * be done: right after the start its pointer may still
> +			 * hold the value of a previous transfer, which aliases
> +			 * the end of the new one when the buffer is reused.
> +			 * Never report full completion while the channel runs.
> +			 */
> +			completed_length = axi_chan_get_xferred(chan, desc);
> +			if (completed_length == length &&
> +			    axi_chan_is_hw_enable(chan))
> +				completed_length = length - 1;
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this code handle the case where the descriptor is at the head of
desc_issued but hasn't actually been started on the hardware?

If a transfer completes and a new transfer is queued reusing the same memory
buffer, the new transfer will be at the head of desc_issued. Because the
hardware is idle, axi_chan_is_hw_enable(chan) will be false.

When dma_chan_tx_status() is called, vchan_next_desc() matches this new
unstarted transfer. The hardware pointers CH_SAR and CH_DAR still hold the
stale end-addresses from the previous transfer, which match the new
transfer's boundaries.

This causes axi_chan_get_xferred() to return length, and because the
hardware is idle, the mitigation here is bypassed. Could this result in an
unstarted transfer falsely reporting a residue of 0, indicating full
completion?

> +		} else {
> +			/* Still queued, nothing transferred yet */
> +			completed_length = 0;
> +		}
> +
> +		bytes = length - min_t(u32, completed_length, length);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923044104.3234-1-wr574332525@163.com?part=1