Re: [PATCH v2 60/64] dmaengine: bam-dma: use dma_chan_kill_bh

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] In the probe error path, the BAM interrupts are not masked and the IRQ is not freed before disabling clocks and cleaning up state.
- [High] Hardware registers are accessed while the peripheral clock is disabled in `bam_dma_remove`, leading to bus faults (Data Abort).
- [High] Failing to drop the PM runtime usage counter on error paths prevents the device from ever entering runtime suspend.
- [High] The driver fails to implement the `device_synchronize` DMA engine callback, rendering `dmaengine_synchronize()` a no-op.
- [Medium] Calling `clk_disable_unprepare` in `bam_dma_remove` underflows the clock enable count.
--

--- Patch [60]: [PATCH v2 60/64] dmaengine: bam-dma: use dma_chan_kill_bh ---
commit 66f1d34a1a1de31180ac528ea6a897680d6ee0d9
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: bam-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/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 1bb26af0405f3..74bc2fe445b32 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -1387,7 +1387,7 @@ static int bam_dma_probe(struct platform_device *pdev)
>  	dma_async_device_unregister(&bdev->common);
>  err_bam_channel_exit:
>  	for (i = 0; i < bdev->num_channels; i++)
> -		tasklet_kill(&bdev->channels[i].vc.task);
> +		dma_chan_kill_bh(&bdev->channels[i].vc.chan);

[Severity: High]
This isn't a bug introduced by this patch, but the driver fails to implement
the device_synchronize DMA engine callback here in bam_dma_probe().

Because bdev->common.device_synchronize is never assigned,
dmaengine_synchronize() returns immediately. This could allow DMA clients
to free memory while the completion bottom-half work is still executing
concurrently. 

Should the driver provide a wrapper that calls vchan_synchronize()?

>  err_tasklet_kill:
>  	tasklet_kill(&bdev->task);
>  err_disable_clk:
>  	clk_disable_unprepare(bdev->bamclk);

[Severity: High]
This is a pre-existing issue, but does this error path leave the BAM
interrupts active before disabling the clock?

Because devm_request_irq() is used, the interrupt is not freed until after
probe() returns. If an interrupt fires during this window, bam_dma_irq()
will access BAM hardware registers while the clock is disabled via
clk_disable_unprepare(), which can trigger a synchronous external abort on
QCOM platforms.

Could we mask interrupts and free the IRQ explicitly before disabling clocks?

> @@ -1413,7 +1413,7 @@ static void bam_dma_remove(struct platform_device *pdev)
>  
>  	for (i = 0; i < bdev->num_channels; i++) {
>  		bam_dma_terminate_all(&bdev->channels[i].vc.chan);
> -		tasklet_kill(&bdev->channels[i].vc.task);
> +		dma_chan_kill_bh(&bdev->channels[i].vc.chan);

[Severity: High]
This isn't a bug introduced by this patch, but are hardware registers
accessed after the clock is disabled?

Looking at bam_dma_remove(), pm_runtime_force_suspend() is called early,
which disables the clock. Immediately after, writel_relaxed() is called to
mask interrupts, and bam_dma_terminate_all() accesses hardware. Accessing
unclocked peripheral registers on QCOM platforms triggers a fatal
synchronous external abort.

Should the hardware shutdown logic occur before forcing PM suspend?

[Severity: Medium]
This is a pre-existing issue, but is there a double-disable of the clock in
bam_dma_remove()?

Since pm_runtime_force_suspend() has already suspended the device and
disabled the clock, the later call to clk_disable_unprepare() at the end
of this function will disable it again, causing the clock enable count to
drop below zero.

[Severity: High]
This isn't a bug introduced by this patch, but does the
pm_runtime_get_sync() failure path in bam_dma_irq() leak the PM usage
counter?

If pm_runtime_get_sync() returns a negative error:

drivers/dma/qcom/bam_dma.c:bam_dma_irq() {
	...
	ret = pm_runtime_get_sync(bdev->dev);
	if (ret < 0)
		return IRQ_NONE;
	...
}

Returning IRQ_NONE immediately without calling pm_runtime_put_noidle()
permanently leaks the PM reference count.

Additionally, returning IRQ_NONE without clearing the hardware interrupt
status leaves the IRQ asserted, potentially causing an IRQ storm that leads
to a CPU soft-lockup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785183549.git.allen.lkml@gmail.com?part=60