Re: [PATCH] dmaengine: xilinx_dma: Fix hardware buffer descriptor chain after cyclic DMA

On Mon, Aug 17, 2026 at 03:36:52PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [Low] Fragile 64-bit division on `dma_addr_t` using `sizeof`.
>
> Pre-existing issues:
> - [Critical] Hardware descriptor ring desynchronization due to relying on `free_seg_list` ordering mapping exactly to physical continuity.
> - [High] Unlocked modification of `chan->free_seg_list` in error paths and submission rejection paths causes list corruption.
> --
>
> --- Patch [1]: [PATCH] dmaengine: xilinx_dma: Fix hardware buffer descriptor chain after cyclic DMA ---
> commit df3c4c6392659223fe0433932cd3afd45fdb012e
> Author: Alex Bereza <[email protected]>
>
> dmaengine: xilinx_dma: Fix hardware buffer descriptor chain after cyclic DMA
>
> Using the DMA in cyclic mode modifies the hardware buffer descriptor chain
> so that the last descriptor points back to the first. This patch ensures
> non-cyclic mode works after cyclic mode by restoring the original buffer
> descriptor ring setup during segment cleanup.
>
> > diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> > index bef2b031dba19..6d00aa97c8fc9 100644
> > --- a/drivers/dma/xilinx/xilinx_dma.c
> > +++ b/drivers/dma/xilinx/xilinx_dma.c
> > @@ -756,15 +756,25 @@ xilinx_aximcdma_alloc_tx_segment(struct xilinx_dma_chan *chan)
>
> [ ... ]
>
> > -static void xilinx_dma_clean_hw_desc(struct xilinx_axidma_desc_hw *hw)
> > +static void xilinx_dma_clean_hw_desc(struct xilinx_dma_chan *chan,
> > +				     struct xilinx_axidma_tx_segment *segment)
> >  {
> > -	u32 next_desc = hw->next_desc;
> > -	u32 next_desc_msb = hw->next_desc_msb;
> > +	dma_addr_t next;
> > +	u32 i;
> >
> > -	memset(hw, 0, sizeof(struct xilinx_axidma_desc_hw));
> > +	/*
> > +	 * Restore the buffer descriptor's next descriptor pointer to the value
> > +	 * set up in xilinx_dma_alloc_chan_resources(). Otherwise using the DMA
> > +	 * in cyclic mode leaves the next descriptor pointer altered and
> > +	 * prevents subsequent non-cyclic transfers.
> > +	 */
> > +	i = (segment->phys - chan->seg_p) / sizeof(*chan->seg_v);
>
> [Severity: Low]
> Is this calculation relying on compiler optimization to avoid an unresolved
> __udivdi3 build failure on 32-bit platforms?
>
> Because segment->phys and chan->seg_p are dma_addr_t, which can be 64-bit
> on 32-bit platforms with LPAE, this creates a 64-bit division. It currently
> works because sizeof(*chan->seg_v) happens to be exactly 128 bytes due to
> __aligned(64), allowing the compiler to optimize it into a bitwise right
> shift.
>
> If a field is added that pushes the alignment boundary to a non-power-of-2,
> would this break 32-bit builds? Could we use native pointer arithmetic like
> i = segment - chan->seg_v instead?

look good by this way.

Frank

>
> > +	next = chan->seg_p +
> > +	       sizeof(*chan->seg_v) * ((i + 1) % XILINX_DMA_NUM_DESCS);
> >
> > -	hw->next_desc = next_desc;
> > -	hw->next_desc_msb = next_desc_msb;
> > +	memset(&segment->hw, 0, sizeof(segment->hw));
> > +	segment->hw.next_desc = lower_32_bits(next);
> > +	segment->hw.next_desc_msb = upper_32_bits(next);
> >  }
> >
> >  static void xilinx_mcdma_clean_hw_desc(struct xilinx_aximcdma_desc_hw *hw)
>
> [ ... ]
>
> > @@ -786,7 +796,7 @@ static void xilinx_mcdma_clean_hw_desc(struct xilinx_aximcdma_desc_hw *hw)
> >  static void xilinx_dma_free_tx_segment(struct xilinx_dma_chan *chan,
> >  				struct xilinx_axidma_tx_segment *segment)
> >  {
> > -	xilinx_dma_clean_hw_desc(&segment->hw);
> > +	xilinx_dma_clean_hw_desc(chan, segment);
> >
> >  	list_add_tail(&segment->node, &chan->free_seg_list);
>
> [Severity: High]
> This is a pre-existing issue, but could this unlocked modification of
> chan->free_seg_list lead to list corruption?
>
> In error paths like xilinx_dma_prep_slave_sg() on allocation failure, or
> when xilinx_dma_tx_submit() rejects a submission,
> xilinx_dma_free_tx_descriptor() is called locklessly. This invokes
> xilinx_dma_free_tx_segment() which calls list_add_tail() on
> chan->free_seg_list here.
>
> If the interrupt tasklet concurrently calls xilinx_axidma_alloc_tx_segment()
> and modifies the list while holding chan->lock, couldn't this cause
> linked-list corruption and kernel panics?
>
> [Severity: Critical]
> This is a pre-existing issue, but will relying on free_seg_list ordering
> cause hardware descriptor ring desynchronization?
>
> In xilinx_dma_prep_slave_sg(), segments are allocated and queued:
>
> drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_prep_slave_sg() {
>     ...
>     segment = xilinx_axidma_alloc_tx_segment(chan);
>     ...
>     hw = &segment->hw;
>     ...
>     list_add_tail(&segment->node, &desc->segments);
>     ...
> }
>
> This never dynamically updates hw->next_desc to link the transaction's
> segments together. If the free_seg_list becomes out of order (for example,
> due to xilinx_dma_tx_submit() rejecting and freeing segments), the segments
> popped will not be physically contiguous.
>
> Since hw->next_desc isn't updated, won't the hardware blindly follow the
> statically-initialized next_desc pointers into unrelated physical segments,
> potentially corrupting other active transactions?
>
> >  }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260817-fix-hw-buf-desc-after-cyclic-mode-v1-1-1fe47e701d6c@bereza.email?part=1