Re: [PATCH V6 2/4] dmaengine: xilinx_dma: Move descriptors to done list based on completion bit

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional transition to `chan->idle = true` while `active_list` is not empty causes a permanent DMA stall and unsafe writes to active hardware registers.
- [High] Missing `dma_rmb()` after reading the hardware descriptor completion bit allows speculative reads of stale descriptor fields.
--

--- Patch [2]: [PATCH V6 2/4] dmaengine: xilinx_dma: Move descriptors to done list based on completion bit ---
commit f3afd1d99816a230e330fe49141913c537fd8bc9
Author: Srinivas Neeli <[email protected]>

dmaengine: xilinx_dma: Move descriptors to done list based on completion bit

This commit updates the descriptor completion logic to check the completion
bit before moving MCDMA descriptors from the active list to the done list.
This addresses an issue where interrupt coalescing could cause uncompleted
descriptors to be erroneously reported as complete.

> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 965fb69a2a3ad..9b8b6f1acd4ea 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -1773,6 +1773,17 @@ static void xilinx_dma_complete_descriptor(struct xilinx_dma_chan *chan)
>  					      struct xilinx_axidma_tx_segment, node);
>  			if (!(seg->hw.status & XILINX_DMA_BD_COMP_MASK) && chan->has_sg)
>  				break;
> +		} else if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIMCDMA) {
> +			struct xilinx_aximcdma_tx_segment *seg;
> +			u32 status;
> +
> +			seg = list_last_entry(&desc->segments,
> +					      struct xilinx_aximcdma_tx_segment,
> +					      node);
> +			status = (chan->direction == DMA_DEV_TO_MEM) ?
> +				seg->hw.s2mm_status : seg->hw.mm2s_status;
> +			if (!(status & XILINX_DMA_BD_COMP_MASK))
> +				break;

[Severity: High]
If this breaks early and leaves uncompleted descriptors in the active list, does
it cause issues with the interrupt handler?

Looking at xilinx_mcdma_irq_handler(), it unconditionally calls
start_transfer():

	xilinx_dma_complete_descriptor(chan);
	chan->idle = true;
	chan->start_transfer(chan);

If xilinx_mcdma_start_transfer() is called while the DMA is still actively
processing the old chain, it will write the new descriptor chain to the CDESC
register:

	/* Program current descriptor */
	xilinx_write(chan, XILINX_MCDMA_CHAN_CDESC_OFFSET(chan->tdest),
		     head_desc->async_tx.phys);

Could writing to the CDESC register while the channel is still running cause a
permanent DMA stall or corrupt the hardware state?

[Severity: High]
Is a read memory barrier needed after reading the hardware descriptor
completion bit?

If the completion mask is set, the code may proceed to calculate the transfer
residue in xilinx_dma_get_residue():

	residue -= aximcdma_hw->s2mm_status & chan->xdev->max_buffer_len;

Without a dma_rmb() between checking the completion bit and reading the
preceding segments' status fields, is it possible for weakly ordered
architectures to speculatively read stale values, leading to incorrect
residue calculations?

>  		}
>  		if (chan->has_sg && chan->xdev->dma_config->dmatype !=
>  		    XDMA_TYPE_VDMA)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2