Re: [PATCH] dmaengine: switchtec-dma: preserve results for out-of-order completions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition on the first ring traversal can cause premature completion of in-flight DMA descriptors.
- [High] Descriptor hardware control flags accumulate across reuse.
- [High] Invoking client callbacks while holding `complete_lock` causes a deadlock if the client calls `dmaengine_tx_status`.
- [Medium] Memory leak of descriptor rings on error path during channel resource allocation.
--

--- Patch [1]: [PATCH] dmaengine: switchtec-dma: preserve results for out-of-order completions ---
commit 4a9da15cea6ee54c832c41e3ac56d3a47db7138d
Author: Hao-Qun Huang <alvinhuang0603@gmail.com>

dmaengine: switchtec-dma: preserve results for out-of-order completions

This patch modifies the driver to store the transfer result within each
descriptor rather than on the stack. Passing the descriptor's own saved
result prevents out-of-order completions from assigning incorrect status
and residue values when retiring a batch of descriptors.

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 3ef9286406159..681540836e73d 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c

[ ... ]

> @@ -488,7 +488,7 @@ switchtec_dma_cleanup_completed(struct switchtec_dma_chan *swdma_chan)
>  		do {
>  			dma_cookie_complete(&desc->txd);
>  			dma_descriptor_unmap(&desc->txd);
> -			dmaengine_desc_get_callback_invoke(&desc->txd, &res);
> +			dmaengine_desc_get_callback_invoke(&desc->txd, &desc->result);

[Severity: High]
This is a pre-existing issue, but does invoking the client callback here
while holding complete_lock cause a deadlock if the client queries status?

Looking at switchtec_dma_tx_status(), it unconditionally calls
switchtec_dma_cleanup_completed(), which attempts to acquire
complete_lock. If a client callback invokes dmaengine_tx_status(), it
would trigger a self-deadlock since the non-recursive lock is already
held.

>  			desc->txd.callback = NULL;
>  			desc->txd.callback_result = NULL;
>  			desc->completed = false;

[Severity: High]
This is a pre-existing issue, but could this loop prematurely complete
in-flight DMA descriptors due to a race condition?

In switchtec_dma_alloc_desc(), descriptors are initialized with
desc->completed = true.

Concurrently, if the submission thread calls switchtec_dma_prep_desc()
(setting desc->completed = false) and updates swdma_chan->head, could the
consumer thread evaluate the loop-exit condition without a read memory
barrier:

switchtec_dma_cleanup_completed() {
    ...
        if (!desc->completed)
            break;
    } while (CIRC_CNT(READ_ONCE(swdma_chan->head), swdma_chan->tail,
                      SWITCHTEC_DMA_SQ_SIZE));
}

and see a stale true state for desc->completed alongside an updated head
state, erroneously completing a newly submitted descriptor?

[ ... ]

[Severity: Medium]
This is a pre-existing issue, but does this error path leak the allocated
descriptor rings?

If device_alloc_chan_resources() fails, the dmaengine core does not invoke
device_free_chan_resources(). In switchtec_dma_alloc_chan_resources():

switchtec_dma_alloc_chan_resources() {
    ...
    rc = switchtec_dma_alloc_desc(swdma_chan);
    if (rc)
        return rc;

    rc = enable_channel(swdma_chan);
    if (rc)
        return rc;
    ...
}

The function returns directly without freeing the memory allocated by
switchtec_dma_alloc_desc().


[Severity: High]
This is a pre-existing issue, but do the hardware control flags accumulate
across descriptor reuse?

In switchtec_dma_prep_desc(), flags are conditionally added:

switchtec_dma_prep_desc() {
    ...
    if (flags & DMA_PREP_INTERRUPT)
        desc->hw->ctrl |= SWITCHTEC_SE_LIOF;

    if (flags & DMA_PREP_FENCE)
        desc->hw->ctrl |= SWITCHTEC_SE_BRR;
    ...
}

Because the descriptor slots are recycled and the flags are never cleared,
a new transfer prepared without DMA_PREP_INTERRUPT could inappropriately
trigger an interrupt if the recycled descriptor slot previously had it set.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718223257.5662.alvinhuang0603@gmail.com?part=1