Re: [PATCH 4/4] dmaengine: dw-edma: Add spinlock to protect DONE_INT_MASK and ABORT_INT_MASK

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The teardown sequence in `dw_edma_remove` incorrectly powers off the hardware and frees interrupts before unregistering the DMA device, exposing a race window.
--

commit 3dac310797da8c464a27d4656c26826f3d2a7401
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: dw-edma: Add spinlock to protect DONE_INT_MASK and ABORT_INT_MASK

This commit adds a spinlock to dw_edma_v0_core_start() to serialize access
to the shared DONE_INT_MASK and ABORT_INT_MASK registers. This prevents
race conditions from concurrent updates across multiple channels.

Link: https://lore.kernel.org/dmaengine/20260109-edma_ll-v2-1-5c0b27b2c664@nxp.com/

> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> index 69e8279adec81..cfdd6463252e6 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
[ ... ]
> @@ -408,6 +409,8 @@ static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  			}
>  		}
>  		/* Interrupt unmask - done, abort */
> +		raw_spin_lock_irqsave(&dw->lock, flags);
> +

[Severity: High]
This is a pre-existing issue, but I noticed a potential race window related to
how dw_edma_remove() interacts with this code.

In dw_edma_remove(), the driver teardown sequence calls dw_edma_core_off() to
disable the engine and mask interrupts, then frees the IRQs, and only
subsequently calls dma_async_device_unregister().

Since the DMA device remains registered in the dmaengine framework during this
window, can concurrent DMA clients still issue new transfers?

If a client calls dmaengine_submit() and dma_async_issue_pending(), the
execution traces down to dw_edma_v0_core_start().

Could this write to the engine_en register and unmask interrupts here,
unexpectedly re-enabling the hardware? Since the IRQs have already been freed,
would this result in the transfer completion never being signaled, leading to
a hung task?

Should the teardown sequence call dma_async_device_unregister() first to
safely quiesce the subsystem and prevent new requests before stopping the
hardware and freeing IRQs?

>  		tmp = GET_RW_32(dw, chan->dir, int_mask);
>  		tmp &= ~FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id));
>  		tmp &= ~FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id));
> @@ -416,6 +419,9 @@ static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  		tmp = GET_RW_32(dw, chan->dir, linked_list_err_en);
>  		tmp |= FIELD_PREP(EDMA_V0_LINKED_LIST_ERR_MASK, BIT(chan->id));
>  		SET_RW_32(dw, chan->dir, linked_list_err_en, tmp);
> +
> +		raw_spin_unlock_irqrestore(&dw->lock, flags);
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521142153.2957432-1-den@valinux.co.jp?part=4