Re: [PATCH v2 56/64] dmaengine: omap-dma: use dma_chan_kill_bh

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

Pre-existing issues:
- [High] Freeing channel memory in `omap_dma_free` without clearing `od->lch_map` leaves a dangling pointer that can be dereferenced by the interrupt handler in the probe error path.
- [High] In `omap_dma_remove`, the shared IRQ is freed before hardware interrupts are disabled, and the wrong interrupt enable register is cleared.
--

--- Patch [56]: [PATCH v2 56/64] dmaengine: omap-dma: use dma_chan_kill_bh ---
commit fe56c19ed9d7f6c471a97210acc16fdccedd520f
Author: Allen Pais <[email protected]>

dmaengine: omap-dma: use dma_chan_kill_bh

virt-dma now dispatches completion callbacks through per-channel BH
work instead of its tasklet. Cancel that work during teardown before
channel storage is released.

> diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
> --- a/drivers/dma/ti/omap-dma.c
> +++ b/drivers/dma/ti/omap-dma.c

[ ... ]

> @@ -1521,7 +1521,7 @@ static void omap_dma_free(struct omap_dmadev *od)
>  			struct omap_chan, vc.chan.device_node);
>  
>  		list_del(&c->vc.chan.device_node);
> -		tasklet_kill(&c->vc.task);
> +		dma_chan_kill_bh(&c->vc.chan);
>  		kfree(c);

[Severity: High]
This is a pre-existing issue, but does freeing the channel memory here leave
a dangling pointer in od->lch_map?

If of_dma_controller_register() fails during probe, omap_dma_free() is called
to loop through all channels and free them. If a client has already requested a
channel after dma_async_device_register() succeeded, od->irq_enable_mask will
be non-zero and od->lch_map will still contain pointers to these now-freed
channels.

Since devm_request_irq() leaves the interrupt handler active until probe
finishes and devres cleans it up, an incoming interrupt could cause
omap_dma_irq() to read the mask, index into od->lch_map, and dereference
the freed memory. 

Should od->lch_map be cleared when freeing the channel?

[ ... ]

[Severity: High]
This is also a pre-existing issue, but does omap_dma_remove() clear the wrong 
IRQ enable register and free the shared IRQ too early?

Looking at omap_dma_remove() in drivers/dma/ti/omap-dma.c:

omap_dma_remove() {
    ...
    devm_free_irq(&pdev->dev, irq, od);
    ...
    if (!omap_dma_legacy(od)) {
        /* Disable all interrupts */
        omap_dma_glbl_write(od, IRQENABLE_L0, 0);
    }
    ...
}

The shared IRQ is freed before hardware interrupts are disabled. It also
writes to IRQENABLE_L0 instead of IRQENABLE_L1, which was used during
setup to enable them. 

If the hardware asserts an interrupt after the handler is removed, could it
assert the shared line indefinitely and cause an unhandled interrupt storm
since the source is never cleared?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=56