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

Pre-existing issues:
- [Critical] Unclocked hardware register access in `bam_dma_remove()` leading to a synchronous external abort (system crash).
- [High] Unsafe hardware access in the IRQ handler before acquiring a Runtime PM reference.
- [High] Concurrency race allowing tasklets to execute unclocked hardware accesses during device removal.
- [High] Clock enable count underflow during device removal.
- [High] Runtime PM usage counter leak in the IRQ handler on failure path.
--

--- Patch [2]: [PATCH v21 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path ---
commit 9ca369a6aae9bb2fa96d12a2fd87eaad7821b59b
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.

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
> --- 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 I have a question about the interrupt
handler bam_dma_irq() being requested here.

Does the interrupt handler read hardware registers before guaranteeing
the clock is enabled?

In drivers/dma/qcom/bam_dma.c:bam_dma_irq():
    srcs |= process_channel_irqs(bdev);
    ...
    ret = pm_runtime_get_sync(bdev->dev);

process_channel_irqs() executes a readl_relaxed() on BAM_IRQ_SRCS_EE.
If a spurious interrupt fires or races with an auto-suspend transition,
could this read occur while the device is suspended and its clock is
off, causing a synchronous external abort?

[Severity: High]
This is also a pre-existing issue, but looking further into the error
path of bam_dma_irq():

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

If pm_runtime_get_sync() fails, it returns without dropping the reference.
Does this leak the usage counter and permanently prevent the device from
entering lower power states?

>  	if (ret)
>  		goto err_bam_channel_exit;

[ ... ]

> @@ -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: Critical]
This isn't a bug introduced by this patch, but does this sequence cause
unclocked hardware register accesses?

In drivers/dma/qcom/bam_dma.c:bam_dma_remove(), the clock is forced off
unconditionally by pm_runtime_force_suspend(&pdev->dev). Immediately
afterward, the code writes to BAM registers:

    pm_runtime_force_suspend(&pdev->dev);
    ...
    writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));

It then calls bam_dma_terminate_all(), which performs more register
accesses in bam_chan_init_hw(). Can accessing these registers with the
clock off lead to a system crash (SError)?

> -	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);
>  		tasklet_kill(&bdev->channels[i].vc.task);

[Severity: High]
This is a pre-existing issue, but does killing the tasklets after
pm_runtime_force_suspend() leave a race window?

If a scheduled tasklet is concurrently executing bam_start_dma() (having
already incremented the usage count via pm_runtime_get_sync()),
pm_runtime_force_suspend() will ignore the usage counter and disable
the clock anyway.

Could the racing tasklet then access BAM registers with the clock off,
causing a system crash?

[Severity: High]
This isn't a bug introduced by this patch, but does bam_dma_remove()
underflow the clock enable count?

pm_runtime_force_suspend() invokes the runtime suspend callback which
calls clk_disable(). Later in bam_dma_remove(), the driver calls
clk_disable_unprepare(bdev->bamclk) unconditionally.

Does this result in an unbalanced second call to clk_disable() that
underflows the enable count?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-qce-cmd-descr-v21-0-bc2583e18475@oss.qualcomm.com?part=2