Re: [PATCH v2 40/64] dmaengine: dw-axi-dmac: kill vchan BH on teardown

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

Pre-existing issues:
- [High] Manual deletion of DMA channels from the device list in dw_remove() sabotages the DMA engine core's unregistration process, leading to memory, IDA, and sysfs leaks.
- [High] Flushing the completion bottom half (dma_chan_kill_bh) after disabling hardware clocks can cause a system hang if a client callback issues a new transfer.
--

--- Patch [40]: [PATCH v2 40/64] dmaengine: dw-axi-dmac: kill vchan BH on teardown ---
commit d93e02ecaf0e9550e4241cd3c6ed8d45227cb4a9
Author: Allen Pais <[email protected]>

dmaengine: dw-axi-dmac: kill vchan BH on teardown

This commit updates virt-dma cleanup during teardown to use
dma_chan_kill_bh() instead of tasklet_kill().

> diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> index bcefaff03b5c6..5a5d569b188d3 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> @@ -1663,7 +1663,7 @@ static void dw_remove(struct platform_device *pdev)
>  	list_for_each_entry_safe(chan, _chan, &dw->dma.channels,
>  				 vc.chan.device_node) {
>  		list_del(&chan->vc.chan.device_node);

[Severity: High]
This isn't a bug introduced by this patch, but does calling list_del()
on the channel here sabotage the DMA engine core's unregistration process?

The DMA core's dma_async_device_unregister() uses the device->channels list
to tear down each channel. By manually removing the channels from this list
in dw_remove(), the core teardown logic sees an empty list and skips channel
unregistration.

Does this leak the channel's IDA, its sysfs device node (dmaXchanY), and
its dynamically allocated per-CPU memory (chan->local)?

> -		tasklet_kill(&chan->vc.task);
> +		dma_chan_kill_bh(&chan->vc.chan);

[Severity: High]
This is a pre-existing issue, but does flushing the bottom half here risk
a system hang?

Earlier in dw_remove(), the hardware clocks are disabled via
axi_dma_suspend(). If a client callback (invoked by the bottom half flush)
submits a new DMA transfer, the driver will attempt to read or write DMA
controller registers to start the transfer.

Can accessing these registers without clocks enabled cause a synchronous
external abort?

>  	}
>  }
>

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