Re: [PATCH v2 3/5] dmaengine: ti: omap-dma: fix dma_pool_destroy before omap_dma_free in error paths

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Unconditional write to `IRQENABLE_L1` in `omap_dma_probe` error paths triggers WARN_ON splats on OMAP1 legacy platforms.

Pre-existing issues:
- [Critical] The driver teardown paths free channel structures and destroy the DMA pool without explicitly stopping actively running hardware DMA channels.
- [High] The patch attempts to fix a memory leak of channel descriptors, but misses the actively executing descriptor `c->desc`.
--

commit 5698cd1c75a26052d705aeceb286850e8b0239ca
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: ti: omap-dma: fix dma_pool_destroy before omap_dma_free in error paths

The probe error paths and remove path called omap_dma_free() after
dma_pool_destroy(), leaving the desc_pool dangling. This patch reorders
the teardown sequence, disables hardware interrupts in probe error
paths, and drains virt-dma descriptor lists in omap_dma_free().

> diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
> index 839e04f53fc2d..dde270646bb94 100644
> --- a/drivers/dma/ti/omap-dma.c
> +++ b/drivers/dma/ti/omap-dma.c
> @@ -1522,6 +1522,7 @@ static void omap_dma_free(struct omap_dmadev *od)
>  
>  		list_del(&c->vc.chan.device_node);
>  		tasklet_kill(&c->vc.task);
> +		vchan_free_chan_resources(&c->vc);

[Severity: High]
This is a pre-existing issue, but does this leak the actively executing
descriptor?

When a descriptor is issued to the hardware, omap_dma_start_desc() removes
it from the virt-dma lists and assigns it to c->desc. Since
vchan_free_chan_resources() only drains the virt-dma lists and does not
inspect c->desc, could the active descriptor and its associated hardware
pool allocations be permanently leaked before the channel is freed?

> +		kfree(c);

[Severity: Critical]
This isn't a bug introduced by this patch, but are the channels freed here
without stopping the underlying hardware DMA engine?

If omap_dma_stop() or omap_dma_terminate_all() is never called to halt the
hardware channel, the DMA hardware might continue executing transfers
asynchronously. Could this cause the hardware to scribble over memory when
fetching descriptors, especially since the DMA pool is destroyed shortly
after in omap_dma_remove() and the probe error paths?

>  	}
>  }

[ ... ]

> @@ -1808,9 +1809,14 @@ static int omap_dma_probe(struct platform_device *pdev)
>  	if (rc) {
>  		pr_warn("OMAP-DMA: failed to register slave DMA engine device: %d\n",
>  			rc);
> +		spin_lock_irq(&od->irq_lock);
> +		od->irq_enable_mask = 0;
> +		omap_dma_glbl_write(od, IRQENABLE_L1, 0);

[Severity: Medium]
Does this unconditional write to IRQENABLE_L1 trigger a WARN_ON splat on
OMAP1 legacy platforms?

On OMAP1 legacy platforms where omap_dma_legacy(od) is true, IRQENABLE_L1
is not mapped in reg_map and evaluates to a zero-initialized register with
type OMAP_DMA_REG_NONE (0). Attempting to read or write OMAP_DMA_REG_NONE
unconditionally triggers a WARN_ON(1) inside omap_dma_write() and
omap_dma_read().

Should this interrupt disable logic be guarded by a check for
!omap_dma_legacy(od) as done in omap_dma_remove()?

> +		spin_unlock_irq(&od->irq_lock);
> +		omap_dma_glbl_read(od, IRQENABLE_L1);
> +		omap_dma_free(od);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601003553.72573-1-rosenp@gmail.com?part=3