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

["Neeli, Srinivas" <srneeli@amd.com>]

hi,

On 7/9/2026 3:41 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Software state mismatch leading to hardware corruption and channel hang in AXI MCDMA.
> - [Medium] Missing read memory barrier (`dma_rmb()`) after checking the DMA completion flag in coherent memory.
> --
>
> --- Patch [2]: [PATCH V3 2/4] dmaengine: xilinx_dma: Move descriptors to done list based on completion bit ---
> commit d59255ad1a6d4a4beb643022da1f9820716a0752
> Author: Srinivas Neeli <srinivas.neeli@amd.com>
>
> dmaengine: xilinx_dma: Move descriptors to done list based on completion bit
>
> In AXI MCDMA scatter-gather mode, xilinx_dma_complete_descriptor() previously
> moved all active descriptors to the done list unconditionally. This commit adds
> a check for the descriptor completion bit before moving entries, handling
> scenarios where interrupt coalescing causes interrupts before all queued
> descriptors are processed.
>
>> 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))
> [Severity: Medium]
> Is a read memory barrier needed after checking this completion status?
>
> The CPU polls the s2mm_status or mm2s_status field in coherent memory to
> check the completion bit. Without a dma_rmb() here, could weakly ordered
> architectures speculatively execute subsequent descriptor reads out of
> order?
>
> This might result in stale reads of the descriptor fields later on
> (for instance, during residue calculation) before the hardware has
> actually confirmed completion.

The MCDMA descriptors are allocated using dma_alloc_coherent(), which 
provides cache-coherent memory between CPU and device.

On coherent DMA memory, the CPU sees device writes without requiring 
explicit barriers, so dma_rmb() is not needed here.

>> +				break;
> [Severity: High]
> Does breaking out early here create a state mismatch with the IRQ handler?
>
> If this check breaks out leaving descriptors in the active list, the
> channel is technically still running. However, looking at the call chain,
> xilinx_mcdma_irq_handler() immediately forces the channel idle and
> starts a new transfer:
>
> xilinx_mcdma_irq_handler() {
>      ...
>      xilinx_dma_complete_descriptor(chan);
>      chan->idle = true;
>      chan->start_transfer(chan);
>      ...
> }
>
> This appears to bypass the idle safety check in
> xilinx_mcdma_start_transfer() and overwrite the CDESC hardware register
> while the DMA engine is actively processing the remaining active list.
> Could this software-splice new descriptors without establishing hardware
> links, leading to a permanent channel hang?
I believe this analysis was run against an older version of the driver. 
The list_empty() guard was added in a recent patch series:
https://lore.kernel.org/all/20260626092656.1563871-1-suraj.gupta2@amd.com/

   The current code in xilinx_mcdma_irq_handler() has this protection:

   xilinx_dma_complete_descriptor(chan);
   if (list_empty(&chan->active_list)) {
       chan->idle = true;
       chan->start_transfer(chan);
   }
When the completion check breaks early (leaving incomplete descriptors 
in active_list), the list won't be empty, so neither chan->idle = true 
nor start_transfer() will execute. The hardware state remains consistent.

Thanks
Neeli Srinivas

>
>> +		}
>>   		if (chan->has_sg && chan->xdev->dma_config->dmatype !=
>>   		    XDMA_TYPE_VDMA)
> [ ... ]
>