Re: [PATCH v5 05/14] dmaengine: dw-edma: Add partial channel ownership mode

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect teardown order in dw_edma_remove() causes a race condition by freeing IRQs and disabling hardware before unregistering the DMA device.
- [High] The driver requests shared IRQs before initializing its channel data structures, creating a race window where an early interrupt could access uninitialized memory.
--

--- Patch [5]: [PATCH v5 05/14] dmaengine: dw-edma: Add partial channel ownership mode ---
commit 430bec8da60538800159c6651e2aa6c04e375a54
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Add partial channel ownership mode

This commit introduces a partial ownership flag for DesignWare eDMA instances
to preserve controller-wide state owned by another OS instance. It skips the
initial core reset, using a limited quiesce path instead of the full core-off
path during probe and remove.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 392981f3b7bb5..cef75da3f2f6b 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c

[ ... ]

> @@ -1039,8 +1084,18 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  	snprintf(dw->name, sizeof(dw->name), "dw-edma-core:%s",
>  		 dev_name(chip->dev));
>  
> -	/* Disable eDMA, only to establish the ideal initial conditions */
> -	dw_edma_core_off(dw);
> +	if (chip->flags & DW_EDMA_CHIP_PARTIAL) {
> +		/*
> +		 * Do not reset the shared controller, but drain stale state
> +		 * from resources represented by this instance.
> +		 */
> +		err = dw_edma_core_quiesce(dw);
> +		if (err)
> +			return err;
> +	} else {
> +		/* Disable eDMA only when this instance owns the controller. */
> +		dw_edma_core_off(dw);
> +	}
>  
>  	/* Request IRQs */
>  	err = dw_edma_irq_request(dw, &wr_alloc, &rd_alloc);

[Severity: High]
This is a pre-existing issue, but does requesting shared IRQs before
initializing channel data structures open a race window?

In dw_edma_probe(), dw_edma_irq_request() installs an IRQF_SHARED handler.
Following this, dw_edma_channel_setup() begins initializing channels and
exposes them to the interrupt handler by setting the mask bit:

irq->wr_mask |= BIT(chan->id);

If a peer device on the shared IRQ line triggers an interrupt before
vchan_init() has initialized the channel's spinlocks and list heads, could
the handler execute dw_edma_done_interrupt() and attempt to acquire
uninitialized locks or dereference uninitialized lists?

[ ... ]

> @@ -1078,14 +1133,16 @@ int dw_edma_remove(struct dw_edma_chip *chip)
>  	struct dw_edma_chan *chan, *_chan;
>  	struct device *dev = chip->dev;
>  	struct dw_edma *dw = chip->dw;
> -	int i;
> +	int i, err = 0;
>  
>  	/* Skip removal if no private data found */
>  	if (!dw)
>  		return -ENODEV;
>  
> -	/* Disable eDMA */
> -	dw_edma_core_off(dw);
> +	if (chip->flags & DW_EDMA_CHIP_PARTIAL)
> +		err = dw_edma_core_quiesce(dw);
> +	else
> +		dw_edma_core_off(dw);
>  
>  	/* Free irqs */
>  	for (i = (dw->nr_irqs - 1); i >= 0; i--)
>  		free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]);
>  	dw_edma_emul_irq_free(dw);
>  
>  	/* Deregister eDMA device */
>  	dma_async_device_unregister(&dw->dma);

[Severity: High]
This is a pre-existing issue, but does disabling the hardware and freeing
IRQs before unregistering the DMA device create a race condition?

If a client DMA driver holds a reference to the eDMA channel and submits a
new DMA request after dw_edma_remove() powers down the hardware, but before
dma_async_device_unregister() removes the device from the subsystem, could
the client access powered-down hardware registers and cause a bus fault?

Would it be safer to reverse this teardown order so the public interface is
unregistered before freeing the underlying resources?

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