Re: [PATCH v3 05/13] 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:

New issues:
- [High] In partial ownership mode, `dw_edma_probe()` skips the global core reset but fails to quiesce the DMA channels it owns, exposing the driver to fatal race conditions if the hardware is active from a prior state.

Pre-existing issues:
- [High] Teardown order in `dw_edma_remove()` disables hardware and frees IRQs before unregistering the DMA device.
--

commit b3810423aa75ab232c8bec54552ca8cb72acd420
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Add partial channel ownership mode

This commit adds a partial ownership flag to allow a dw-edma instance to
represent a subset of a controller shared with another OS instance. It skips
the initial core reset during probe and uses a limited quiesce path during
remove, while enforcing ownership granularity for shared registers.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index c782eaa12021e..d87791205837b 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -1128,8 +1156,10 @@ 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)) {
> +		/* Disable eDMA only when this instance owns the controller. */
> +		dw_edma_core_off(dw);
> +	}

[Severity: High]
If the instance is in partial ownership mode, could the hardware be
active from a prior state (e.g. kexec or an endpoint driver)?

By skipping the global core reset without calling dw_edma_core_quiesce()
in its place, the hardware remains active. The driver requests IRQs via
dw_edma_irq_request() just below this, before calling
dw_edma_channel_setup().

If a pending interrupt fires during this window, the handler will invoke
dw_edma_done_interrupt(), which locks and dereferences uninitialized
vchan structures:

dw_edma_interrupt_write()
  dw_edma_done_interrupt()
    spin_lock_irqsave(&chan->vc.lock, flags)
    vchan_next_desc()

Since vchan_init() has not been called yet during dw_edma_channel_setup(),
does this sequence lead to a kernel panic or NULL pointer dereference?

>  
>  	/* Request IRQs */
>  	err = dw_edma_irq_request(dw, &wr_alloc, &rd_alloc);
[ ... ]
> @@ -1173,8 +1203,10 @@ int dw_edma_remove(struct dw_edma_chip *chip)
>  	if (!dw)
>  		return -ENODEV;
>  
> -	/* Disable eDMA */
> -	dw_edma_core_off(dw);
> +	if (chip->flags & DW_EDMA_CHIP_PARTIAL)
> +		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 the teardown order in
dw_edma_remove() expose the driver to a race condition?

The hardware is disabled or quiesced and the IRQs are freed before
dma_async_device_unregister() is called. By leaving the dmaengine device
registered during this teardown, clients can still issue new transfers via
vchan_issue_pending().

This would invoke dw_edma_v0_core_start(), writing to registers of a
disabled DMA engine, potentially racing with dw_edma_core_quiesce().
Additionally, transfers submitted after free_irq() will hang indefinitely
since completion interrupts are no longer available.

Should dma_async_device_unregister() be called before tearing down the
underlying hardware and interrupts?

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