Re: [PATCH 7/7] dmaengine: dw-edma: Defer channel IRQ handling to workqueue

[sashiko-bot@kernel.org]

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

New issues:
- [High] chan->irq_pending is not cleared when deferred IRQ work is cancelled, leaving stale events that corrupt subsequent transfers.

Pre-existing issues:
- [High] The driver requests shared interrupts before initializing the per-channel structures, causing a guaranteed NULL pointer dereference if an interrupt fires early.
- [High] The `dma_device` and `dw_edma_chan` structures are freed by devres while DMA clients may still hold references to them.
--

--- Patch [7]: [PATCH 7/7] dmaengine: dw-edma: Defer channel IRQ handling to workqueue ---
commit 47745d1148d964a79c090cf3e2315c193e04da4d
Author: Koichiro Den <den@valinux.co.jp>
Subject: dmaengine: dw-edma: Defer channel IRQ handling to workqueue

This commit defers channel IRQ handling to a workqueue to reduce
contention on the hard IRQ handler. It introduces work items to process
per-channel completion and abort events asynchronously.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 5664421c6f15b..704d8f9746e8b 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c

[ ... ]

> @@ -930,6 +973,7 @@ static void dw_edma_device_synchronize(struct dma_chan *dchan)
>  	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
>  
>  	dw_edma_wait_termination(dchan);
> +	cancel_work_sync(&chan->irq_work);

[Severity: High]
Does this code leave stale events in chan->irq_pending?

When the work is cancelled here, the work item won't execute, but it looks
like chan->irq_pending is never cleared.

If a hardware interrupt had set DW_EDMA_IRQ_ABORT in irq_pending just before
the channel was stopped, won't that stale event remain in the atomic
variable?

When the client later configures the channel and starts a new transfer,
wouldn't the next legitimate hardware interrupt cause dw_edma_irq_work() to
see both events and erroneously abort the new active transfer?

>  	vchan_synchronize(&chan->vc);
>  }

[ ... ]

> @@ -1185,10 +1231,21 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  	/* Disable eDMA, only to establish the ideal initial conditions */
>  	dw_edma_core_off(dw);
>  
> +	/*
> +	 * Deferred IRQ works are queued from the hard IRQ handlers, so the
> +	 * workqueue must exist before any IRQ is requested.
> +	 */
> +	dw->wq = alloc_workqueue("dw-edma:%s", WQ_UNBOUND | WQ_HIGHPRI, 0,
> +				 dev_name(chip->dev));
> +	if (!dw->wq)
> +		return -ENOMEM;
> +
>  	/* Request IRQs */
>  	err = dw_edma_irq_request(dw, &wr_alloc, &rd_alloc);

[Severity: High]
This is a pre-existing issue, but could this sequence cause a NULL pointer
dereference if the shared interrupt fires early?

During probe, dw_edma_irq_request() requests the shared interrupt before
dw_edma_channel_setup() completes:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_irq_request() {
    ...
    err = request_irq(irq, dw_edma_interrupt_common,
                      IRQF_SHARED, dw->name, &dw->irq[0]);
    ...
}

At this point, dw->irq[i].dw is still NULL because it's only initialized
later in dw_edma_channel_setup():

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_channel_setup() {
    ...
    irq->dw = dw;
    ...
}

If the interrupt fires immediately after request_irq(), wouldn't the handler
crash here when dereferencing dw_irq->dw?

drivers/dma/dw-edma/dw-edma-core.h:dw_edma_core_handle_int() {
    return dw_irq->dw->core->handle_int(dw_irq, dir, done, abort);
}

> -	if (err)
> +	if (err) {
> +		destroy_workqueue(dw->wq);
>  		return err;
> +	}
>  
>  	/* Allocate a dedicated virtual IRQ for interrupt-emulation doorbells */
>  	err = dw_edma_emul_irq_alloc(dw);

[ ... ]

> @@ -1235,6 +1293,11 @@ int dw_edma_remove(struct dw_edma_chip *chip)
>  		free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]);
>  	dw_edma_emul_irq_free(dw);
>  
> +	for (i = 0; i < dw->wr_ch_cnt + dw->rd_ch_cnt; i++)
> +		cancel_work_sync(&dw->chan[i].irq_work);
> +
> +	destroy_workqueue(dw->wq);
> +
>  	/* Deregister eDMA device */
>  	dma_async_device_unregister(&dw->dma);

[Severity: High]
This is a pre-existing issue, but is there a risk of a Use-After-Free here
during driver unbind?

The dw->chan array and dw context are allocated using devres (devm_kcalloc
and devm_kzalloc).

When dma_async_device_unregister() is called, it removes the device from the
DMA engine subsystem but doesn't block waiting for active clients to drop
their channel references.

Once remove() returns, the devres mechanism will automatically free the
memory. If a client still holds a reference to a channel, won't subsequent
use or a later call to dma_release_channel() dereference freed memory?

>  	list_for_each_entry_safe(chan, _chan, &dw->dma.channels,
>  				 vc.chan.device_node) {

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