Re: [PATCH v2 03/11] dmaengine: switchtec-dma: halt channel on alloc_chan_resources error

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Early return in `switchtec_dma_chan_stop()` on hardware timeout fails to clear DMA base registers, leading to a Use-After-Free of DMA memory by the active hardware when descriptors are unconditionally freed.
- [High] The completion handler `switchtec_dma_cleanup_completed()` synchronously invokes client callbacks while holding the `complete_lock` spinlock, creating a severe deadlock hazard.
- [High] The driver intentionally leaves the `submit_lock` spinlock locked when returning from `switchtec_dma_prep_desc()`, requiring the client to submit the descriptor to release it. This breaks DMA API semantics and risks permanent deadlock.
- [High] Holding a spinlock while busy-waiting for hardware status for up to 100ms severely degrades system scheduling latency.
--

--- Patch [3]: [PATCH v2 03/11] dmaengine: switchtec-dma: halt channel on alloc_chan_resources error ---
commit 53cba0d04c9fe827ca20ae65df9b3ba987573e8e
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: halt channel on alloc_chan_resources error

This commit modifies the error unwind path in
switchtec_dma_alloc_chan_resources() to call switchtec_dma_chan_stop()
before freeing descriptors, aiming to halt the channel and clear DMA base
registers.

Link: https://lore.kernel.org/dmaengine/20260717223647.F0A051F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index f77da31aeb65e..107769cca772f 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1040,6 +1040,7 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
>  	swdma_chan->comp_ring_active = false;
>  	spin_unlock_bh(&swdma_chan->complete_lock);
>  err_disable_channel:
> +	switchtec_dma_chan_stop(swdma_chan);

[Severity: Critical]
This isn't a bug introduced by this patch, but does this error path risk a
DMA use-after-free if the hardware becomes unresponsive?

Looking at switchtec_dma_chan_stop():

drivers/dma/switchtec_dma.c:switchtec_dma_chan_stop() {
        rc = halt_channel(swdma_chan);
        if (rc)
                return;

        // ...
        writel(0, &swdma_chan->mmio_chan_fw->sq_base_lo);
        // ...
}

If halt_channel() times out and returns an error, switchtec_dma_chan_stop()
returns early and bypasses clearing the DMA base registers. The error path
then proceeds to blindly free the descriptor rings via
switchtec_dma_free_desc().

If the hardware later resumes, couldn't it overwrite the freed system memory
using the uncleared physical addresses?

>  	disable_channel(swdma_chan);
>  err_free_desc:
>  	switchtec_dma_free_desc(swdma_chan);

[ ... ]

[Severity: High]
This is a pre-existing issue, but is it safe to invoke client callbacks while
holding the completion spinlock?

drivers/dma/switchtec_dma.c:switchtec_dma_cleanup_completed() {
        spin_lock_bh(&swdma_chan->complete_lock);
        // ...
        dmaengine_desc_get_callback_invoke(&desc->txd, &res);
        // ...
        spin_unlock_bh(&swdma_chan->complete_lock);
}

If a client callback calls standard DMA engine APIs like
dmaengine_terminate_all() to handle errors, those driver functions will
attempt to acquire the exact same non-recursive complete_lock. Does this
create a deadlock hazard?


[Severity: High]
This isn't a bug introduced by this patch, but does switchtec_dma_prep_desc()
leak a spinlock to the caller?

drivers/dma/switchtec_dma.c:switchtec_dma_prep_desc() {
        spin_lock_bh(&swdma_chan->submit_lock);
        // ...
        /* return with the lock held, it will be released in tx_submit */
        return &desc->txd;
}

The DMA Engine API does not guarantee that a prepared descriptor will be
submitted. If a client aborts and discards the descriptor before submission,
does this leave the CPU permanently deadlocked with bottom-halves disabled?

Also, if a client attempts to prepare multiple segments in sequence before
submitting them, wouldn't the second call to switchtec_dma_prep_desc()
attempt to acquire the already-held submit_lock?


[Severity: High]
This is a pre-existing issue, but does the channel halting logic introduce
massive scheduling latency spikes?

drivers/dma/switchtec_dma.c:halt_channel() {
        spin_lock(&swdma_chan->hw_ctrl_lock);
        writeb(SWITCHTEC_CHAN_CTRL_HALT, &chan_hw->ctrl);
        ret = wait_for_chan_status(chan_hw, SWITCHTEC_CHAN_STS_HALTED, true);
        spin_unlock(&swdma_chan->hw_ctrl_lock);
}

The wait_for_chan_status() function uses readl_poll_timeout_atomic() with a
100ms timeout.

If the hardware is slow or unresponsive, will the CPU be trapped in a
busy-wait loop with preemption disabled for up to 100 milliseconds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721155739.62120-1-logang@deltatee.com?part=3