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

AMD General

Hi,

> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: Wednesday, August 19, 2026 11:17 PM
> To: Neeli, Srinivas <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]
> Subject: 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)
>

The analysis seems to have been performed on an older code base. The current linux-next tree already contains fixes that address these concerns,
so the reported issues do not apply to this patch. I am ignoring these comments for this revision.

Thanks
Srinivas Neeli

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