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

[sashiko-bot@kernel.org]

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 <alex@bereza.email>

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?

> +	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