Re: [PATCH v20 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path

[sashiko-bot@kernel.org]

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

New issues:
- [High] Moving `free_irq()` to the start of `bam_dma_remove()` creates an interrupt storm window.

Pre-existing issues:
- [Critical] `bam_dma_remove()` performs unconditional unclocked hardware accesses after `pm_runtime_force_suspend()`, causing a system crash.
- [High] Concurrent execution of the pending async workqueue with `pm_runtime_force_suspend()` causes unclocked hardware accesses.
- [High] The interrupt handler `bam_dma_irq()` unconditionally accesses hardware registers, causing a crash on spurious interrupts.
--

commit 8349aa20615406eee52bb3c2ccef1b8f57571551
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

dmaengine: qcom: bam_dma: free interrupt before the clock in error path

The BAM interrupt is requested with a devres helper and so on error it's
freed after probe() returns. We disable the clock before freeing or
masking it so it may still fire and we may end up reading BAM registers
with clock disabled.

Stop using devres for interrupts as we free it in remove() manually
anyway. Add an appropriate label and free the interrupt before disabling
the clock in error path and in remove().

Closes: https://sashiko.dev/#/patchset/20260427-qcom-qce-cmd-descr-v16-0-945fd1cafbbc%40oss.qualcomm.com?part=2

> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 1bb26af0405f3..fc155e0d1870c 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -1332,8 +1332,7 @@ static int bam_dma_probe(struct platform_device *pdev)
>  	for (i = 0; i < bdev->num_channels; i++)
>  		bam_channel_init(bdev, &bdev->channels[i], i);
>  
> -	ret = devm_request_irq(bdev->dev, bdev->irq, bam_dma_irq,
> -			IRQF_TRIGGER_HIGH, "bam_dma", bdev);
> +	ret = request_irq(bdev->irq, bam_dma_irq, IRQF_TRIGGER_HIGH, "bam_dma", bdev);

[Severity: High]
This is a pre-existing issue, but does bam_dma_irq() unconditionally access
hardware registers before enabling the clock? 

If a spurious interrupt triggers while the device is runtime-suspended,
process_channel_irqs() might read BAM_IRQ_SRCS_EE before pm_runtime_get_sync()
is called to turn the clock on. 

Could this lead to a synchronous external abort?

[ ... ]

> @@ -1401,6 +1402,8 @@ static void bam_dma_remove(struct platform_device *pdev)
>  	struct bam_device *bdev = platform_get_drvdata(pdev);
>  	u32 i;
>  
> +	free_irq(bdev->irq, bdev);
> +

[Severity: High]
By unregistering the handler here, before hardware interrupts are masked,
could this create a window for an unhandled interrupt storm? 

If a DMA transaction completes and asserts the level-triggered
IRQF_TRIGGER_HIGH line, there is no software handler to clear it until the
mask is written below. This might force the kernel to permanently disable
the IRQ line.

>  	pm_runtime_force_suspend(&pdev->dev);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does forcing the device to
suspend here in bam_dma_remove() race with pending asynchronous work? 

If a DMA interrupt schedules the workqueue, the async work might execute
concurrently and acquire a runtime PM reference. Because
pm_runtime_force_suspend() ignores active usage counters, it could forcefully
disable the clock while the workqueue is still writing to BAM registers.

>  	of_dma_controller_free(pdev->dev.of_node);
>  	dma_async_device_unregister(&bdev->common);
>  
>  	/* mask all interrupts for this execution environment */
>  	writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));

[Severity: Critical]
This is a pre-existing issue, but does writing to BAM hardware registers here
in bam_dma_remove() cause a crash? 

The preceding call to pm_runtime_force_suspend() disables the bamclk, so this
writel_relaxed() and subsequent termination calls appear to perform unclocked
hardware accesses, which could trigger a synchronous external abort.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-qcom-qce-cmd-descr-v20-0-56f67da84c05@oss.qualcomm.com?part=2