Re: [PATCH 2/5] dmaengine: sf-pdma: fix race between done and error interrupts

Frank Li
2026-02-20T20:26:52Z

On Sat, Feb 21, 2026 at 03:43:54AM +0800, Max Hsu wrote:
> According to the FU540-C000 v1p5 [1] and FU740-C000 v1p7 [2] specs,
> when a DMA transaction error occurs, the hardware sets both the
> DONE and ERROR interrupt bits simultaneously.

Nit: need extra empty line between two paragraph.

> On SMP systems, this can cause the done_isr and err_isr to execute
> concurrently on different CPUs, leading to race conditions and
> NULL pointer dereferences.

On SMP systems, the race conditons and NULL pointer dereferences happen if
the done_isr and err_isr to execute concurrently on different CPUs

>
> Fix by:
> - In done_isr: abort if ERROR bit is set or DONE bit was already cleared
> - In err_isr: clear both DONE and ERROR bits to prevent done_isr from
> processing the same transaction
>
> Link: https://www.sifive.com/document-file/freedom-u540-c000-manual [1]
> Link: https://www.sifive.com/document-file/freedom-u740-c000-manual [2]
> Fixes: 6973886ad58e ("dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00")
> Cc: stable@vger.kernel.org
> Signed-off-by: Max Hsu <max.hsu@sifive.com>
> ---
> drivers/dma/sf-pdma/sf-pdma.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
> index 7ad3c29be146..ac7d3b127a24 100644
> --- a/drivers/dma/sf-pdma/sf-pdma.c
> +++ b/drivers/dma/sf-pdma/sf-pdma.c
> @@ -346,9 +346,25 @@ static irqreturn_t sf_pdma_done_isr(int irq, void *dev_id)
> struct sf_pdma_chan *chan = dev_id;
> struct pdma_regs *regs = &chan->regs;
> u64 residue;
> + u32 control_reg;
>
> spin_lock(&chan->vchan.lock);
> - writel((readl(regs->ctrl)) & ~PDMA_DONE_STATUS_MASK, regs->ctrl);
> + control_reg = readl(regs->ctrl);
> + if (control_reg & PDMA_ERR_STATUS_MASK) {
> + spin_unlock(&chan->vchan.lock);
> + return IRQ_HANDLED;
> + }
> +
> + /*
> + * Check if DONE bit is still set. If not, the error ISR on another
> + * CPU has already cleared it, so abort to avoid double-processing.
> + */
> + if (!(control_reg & PDMA_DONE_STATUS_MASK)) {
> + spin_unlock(&chan->vchan.lock);
> + return IRQ_HANDLED;
> + }
> +
> + writel((control_reg & ~PDMA_DONE_STATUS_MASK), regs->ctrl);
> residue = readq(regs->residue);
>
> if (!residue) {
> @@ -375,7 +391,7 @@ static irqreturn_t sf_pdma_err_isr(int irq, void *dev_id)
> struct pdma_regs *regs = &chan->regs;
>
> spin_lock(&chan->lock);
> - writel((readl(regs->ctrl)) & ~PDMA_ERR_STATUS_MASK, regs->ctrl);
> + writel((readl(regs->ctrl)) & ~(PDMA_DONE_STATUS_MASK | PDMA_ERR_STATUS_MASK), regs->ctrl);
> spin_unlock(&chan->lock);
>
> tasklet_schedule(&chan->err_tasklet);

ideally, it'd better handle by one function

sf_pdma_isr()
{
stat = readl(regs->ctrl);
writel(stat & ~(PDMA_DONE_STATUS_MASK | PDMA_ERR_STATUS_MASK), regs->ctrl);

if (err)
return err_handle();

if (done)
return done_handle();

return IRQ_NONE;
}

Anyways, this also work

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> --
> 2.43.0
>