Re: [PATCH] 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 2 potential issue(s) to consider:
- [Medium] dmaengine: dw-axi-dmac: torn read of active 64-bit counters using lo_hi_readq()
- [Medium] dmaengine: dw-axi-dmac: completed_length zeroed before IRQ handling
--

commit 00e157d9ceffe7d7b5238031118199d3b1fa0eca
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. Report DMA_PAUSED when
the channel is paused and the cookie is still in flight, and clear the
stale is_paused flag in dma_chan_terminate_all(), which disables the
channel and thus implicitly cancels the paused state.

Also, the residue of an in-flight transfer is currently derived from
the number of completed LLI blocks. Read the current hardware pointer
and walk the descriptor's LLIs to compute how many bytes have actually
been transferred.

> 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..4e1b8654a332e 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
[ ... ]
> @@ -352,19 +352,51 @@ static void vchan_desc_put(struct virt_dma_desc *vdesc)
>  	axi_desc_put(vd_to_axi_desc(vdesc));
>  }
>  
> +/*
> + * Return the number of bytes already transferred by the in-flight
> + * non-cyclic descriptor, based on the current hardware read or write
> + * position: CH_SAR for MEM_TO_DEV and MEM_TO_MEM, CH_DAR for
> + * DEV_TO_MEM. Must be called with vc.lock held and the channel
> + * running or paused.
> + */
> +static u32 axi_chan_get_xferred(struct axi_dma_chan *chan,
> +				struct axi_dma_desc *desc)
> +{
> +	struct axi_dma_hw_desc *hw_desc;
> +	bool dst = chan->direction == DMA_DEV_TO_MEM;
> +	u64 pos, start;
> +	u32 xferred = 0;
> +	int i;
> +
> +	pos = lo_hi_readq(chan->chan_regs + (dst ? CH_DAR : CH_SAR));

[Severity: Medium]
Does this reading of an active 64-bit hardware counter lead to word tearing?

lo_hi_readq() unconditionally performs two separate 32-bit readl() calls on
all architectures. If the DMA transfer crosses a 32-bit boundary (e.g.,
crossing a 4GB address boundary) between the two reads, the combined 64-bit
value will tear, creating a massively corrupted address.

This corrupted address can fail the descriptor bounds check (pos >= start &&
pos <= start + hw_desc->len) later in this function, causing
axi_chan_get_xferred() to conservatively return 0.

This would result in a spurious backward jump in the reported residue back
to the full transfer size, breaking monotonic progress expectations for any
polling client.

[ ... ]
> @@ -374,13 +406,26 @@ dma_chan_tx_status(struct dma_chan *dchan, dma_cookie_t cookie,
>  
>  	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) &&
> +			 (axi_chan_is_hw_enable(chan) || chan->is_paused))
> +			/* In-flight descriptor: read progress from hardware */
> +			completed_length = axi_chan_get_xferred(chan, desc);
> +		else
> +			/* Still queued, nothing transferred yet */
> +			completed_length = 0;

[Severity: Medium]
Could this condition incorrectly zero out the completed length before IRQ
handling finishes, causing the residue to spike?

For non-cyclic transfers, the AXI DMA hardware automatically clears the
channel enable bit (DMAC_CHEN) when a transfer finishes.

If a client polls dma_chan_tx_status() during the small window after
hardware completion but before the interrupt handler acquires vc.lock to
reap the descriptor, vdesc == vchan_next_desc(&chan->vc) remains true.

However, axi_chan_is_hw_enable(chan) evaluates to false because the hardware
disabled the channel. The logic incorrectly falls through to the else branch,
setting completed_length = 0.

This causes the reported residue to instantaneously jump from nearly 0 back
to the full transfer length, breaking monotonic progress expectations.

> +
> +		bytes = length - min_t(u32, completed_length, length);
>  	}
>

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