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

Pre-existing issues:
- [High] Hardware registers are accessed unconditionally in `bam_dma_remove` after the device clock is disabled, triggering a synchronous external abort.
- [High] The `bam_dma_irq` handler reads hardware registers before ensuring the device is active, causing a system crash if an interrupt fires while suspended.
- [High] Requesting the IRQ before enabling runtime PM in `bam_dma_probe` creates a window for an unresolvable interrupt storm.
--

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

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

This commit replaces devm_request_irq() with manual request_irq() and
free_irq() to ensure the interrupt is freed before the clock is disabled
in the error path and remove(). This prevents spurious interrupts from
reading registers while the clock is powered off.

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 requesting the IRQ before enabling runtime PM
in bam_dma_probe() creates a window for an unresolvable interrupt storm.

If an interrupt is pending when request_irq() is called, bam_dma_irq() fires
and calls pm_runtime_get_sync(). Because pm_runtime_enable() hasn't been called
yet, pm_runtime_get_sync() fails, causing the handler to return IRQ_NONE
without clearing the global BAM interrupt:

bam_dma_irq()
    ...
    ret = pm_runtime_get_sync(bdev->dev);
    if (ret < 0)
        return IRQ_NONE;

    if (srcs & BAM_IRQ) {
        clr_mask = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_STTS));
        ...

Because the IRQ is requested with IRQF_TRIGGER_HIGH, the controller will
immediately re-fire, locking up the CPU. Could the IRQ request be moved after
runtime PM is enabled?

[Severity: High]
This is another pre-existing issue, but the bam_dma_irq() handler reads
hardware registers before ensuring the device is active. 

If an interrupt fires while suspended (e.g., from a shared line or a race),
the handler calls process_channel_irqs(), which immediately executes a
readl_relaxed():

bam_dma_irq()
    srcs |= process_channel_irqs(bdev);
        ...
        process_channel_irqs()
            srcs = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_SRCS_EE));

This happens before pm_runtime_get_sync() is called to wake the device, which
will cause a synchronous external abort (kernel panic) on ARM platforms if the
clock is disabled. Should the runtime PM wake happen before accessing any
registers?

>  	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));
>  
> -	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 isn't a bug introduced by this patch, but hardware registers are accessed
unconditionally in bam_dma_remove() after the device clock is disabled.

When pm_runtime_force_suspend() is called, it invokes the runtime suspend
callback to disable the BAM clock. Immediately after this, the function calls
writel_relaxed() to mask interrupts:

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

It also calls bam_dma_terminate_all(), which resets the channel via
bam_chan_init_hw() and performs multiple unclocked hardware accesses.

Accessing hardware registers while the peripheral clock is disabled causes a
synchronous external abort on Qualcomm ARM platforms. Should the register
accesses and termination happen before pm_runtime_force_suspend()?

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