Re: [PATCH v23 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 6 potential issue(s) to consider:

Pre-existing issues:
- [High] Unclocked Hardware Access in `bam_dma_remove()`
- [High] Unclocked Register Access in IRQ Handler
- [High] Tasklets are killed too late in `bam_dma_remove()`, causing a race condition with device teardown
- [High] Requesting the IRQ before PM runtime is fully enabled can trigger a system-hanging IRQ storm
- [Medium] Unbalanced `clk_disable_unprepare()` in `bam_dma_remove()` triggers clock framework warning
- [Medium] PM Runtime usage counter leak on `pm_runtime_get_sync()` failures
--

--- Patch [2]: [PATCH v23 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path ---
commit 9cb6e1d095c67c46c54a187984f1dac9efb8fda4
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

dmaengine: qcom: bam_dma: free interrupt before the clock in error path
    
This commit stops using devres for the BAM interrupt, freeing it manually
in the probe error path and remove functions. This prevents the interrupt
from firing after the clock is disabled.

Link: 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 isn't a bug introduced by this patch, but does requesting this
interrupt before PM runtime is fully enabled risk a system-hanging IRQ
storm?

In bam_dma_probe(), the global BAM interrupts are unmasked before this call,
but pm_runtime_enable() is not called until the end of the probe function.

If an interrupt fires immediately, bam_dma_irq() will call
pm_runtime_get_sync(). Since PM runtime isn't enabled yet, it returns
-EACCES, and the handler returns IRQ_NONE without clearing the interrupt.

Because the interrupt is level-sensitive (IRQF_TRIGGER_HIGH), could the
hardware instantly re-assert it and trap the CPU in an infinite loop?

[ ... ]

> @@ -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);
> +
>  	pm_runtime_force_suspend(&pdev->dev);
>  
>  	of_dma_controller_free(pdev->dev.of_node);
> @@ -1409,8 +1412,6 @@ static void bam_dma_remove(struct platform_device *pdev)
>  	/* mask all interrupts for this execution environment */
>  	writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));

[Severity: High]
This is a pre-existing issue, but does this write result in unclocked
hardware access?

When pm_runtime_force_suspend() is called just above, it invokes
bam_dma_runtime_suspend() which explicitly disables the device clock via
clk_disable().

Immediately afterward, writel_relaxed() attempts to mask interrupts on the
now-unclocked hardware. Could this access trigger an external bus abort and
crash the kernel during device removal?

> -	devm_free_irq(bdev->dev, bdev->irq, bdev);
> -
>  	for (i = 0; i < bdev->num_channels; i++) {
>  		bam_dma_terminate_all(&bdev->channels[i].vc.chan);

[Severity: High]
This is a pre-existing issue, but does bam_dma_terminate_all() also access
hardware registers while the clock is disabled?

Similar to the interrupt masking above, this invokes channel initialization
and reset logic. Could accessing unclocked registers here cause a synchronous
bus abort?

>  		tasklet_kill(&bdev->channels[i].vc.task);

[Severity: High]
This isn't a bug introduced by this patch, but are these channel tasklets
and the global DMA tasklet terminated too late?

Since pm_runtime_disable() is never called in this remove function, if a
tasklet was scheduled prior to free_irq(), it might execute concurrently
with this channel teardown.

The tasklet calls pm_runtime_get_sync(), which could successfully wake the
device and initiate DMA transfers while the main thread is unregistering
channels and freeing FIFO memory. Could this lead to a use-after-free and
hardware state corruption?


I also noticed a few other pre-existing issues in this driver outside the
immediate diff context that might need attention:

drivers/dma/qcom/bam_dma.c:bam_dma_remove() {
    ...
    clk_disable_unprepare(bdev->bamclk);
}

[Severity: Medium]
This is a pre-existing issue, but does calling clk_disable_unprepare() here
cause a clock framework warning?

In probe, the clock is prepared and enabled exactly once. During removal,
pm_runtime_force_suspend() guarantees the device is in a suspended state,
meaning bam_dma_runtime_suspend() has already decremented the clock enable
count to 0.

Calling clk_disable_unprepare() here attempts to disable the clock a second
time, which triggers a WARN splat in clk_core_disable() and corrupts the
state. Should only clk_unprepare() be used here instead?

drivers/dma/qcom/bam_dma.c:bam_dma_irq() {
    ...
    srcs |= process_channel_irqs(bdev);
    ...
}

[Severity: High]
This is a pre-existing issue, but can this lead to unclocked register access
in the IRQ handler?

In bam_dma_irq(), process_channel_irqs() is called before
pm_runtime_get_sync(). process_channel_irqs() immediately reads the
BAM_IRQ_SRCS_EE hardware register.

If a spurious or delayed interrupt is delivered while the device is in a
runtime-suspended state (and the clock is disabled), will this unclocked
read trigger an external bus abort and crash the system?

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

[Severity: Medium]
This is a pre-existing issue, but does this leak the PM runtime usage
counter when pm_runtime_get_sync() fails?

pm_runtime_get_sync() increments the runtime PM usage counter
unconditionally. If it returns an error, the caller must decrement it via
pm_runtime_put_noidle().

Checking for a negative return value and returning immediately leaves the
counter incremented. This same pattern appears to exist in bam_pause(),
bam_resume(), bam_free_chan(), and bam_dma_irq(). Will this prevent the BAM
DMA controller from ever entering runtime suspend again?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-qcom-qce-cmd-descr-v23-0-1be02deffda9@oss.qualcomm.com?part=2