Re: [PATCH v22 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The interrupt handler accesses hardware registers before verifying the device's runtime PM state, risking an unclocked access crash.
- [High] Race condition between asynchronous workqueue and `pm_runtime_force_suspend` causes unclocked register access.
- [High] `bam_dma_remove` accesses hardware registers after explicitly disabling device clocks.
- [High] `bam_dma_terminate_all` accesses hardware registers without acquiring a runtime PM reference.
--

--- Patch [3]: [PATCH v22 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue ---
commit 38bf51a7c8174c93f29b343f8403a75a6af1a76e
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue

BH workqueues are a modern mechanism, aiming to replace legacy tasklets.
Let's convert the BAM DMA driver to using the high-priority variant of
the BH workqueue.

> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index fc155e0d1870c..ea3df28e777f9 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c

[ ... ]

> @@ -922,9 +923,9 @@ static irqreturn_t bam_dma_irq(int irq, void *data)
>  
>  	srcs |= process_channel_irqs(bdev);
>  

[Severity: High]
This is a pre-existing issue, but does process_channel_irqs() access
hardware registers before verifying the device's runtime PM state?

Looking at process_channel_irqs(), it performs a register read right away:

    srcs = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_SRCS_EE));

However, the interrupt handler only takes a PM reference via
pm_runtime_get_sync() after this call.

If a spurious interrupt arrives, or if handler execution is delayed until
after the device has entered runtime suspend and disabled the bamclk, would
this read hit unclocked hardware and crash the system?

> -	/* kick off tasklet to start next dma transfer */
> +	/* kick off the work queue to start next dma transfer */
>  	if (srcs & P_IRQ)
> -		tasklet_schedule(&bdev->task);
> +		queue_work(system_bh_highpri_wq, &bdev->work);
>  
>  	ret = pm_runtime_get_sync(bdev->dev);

[ ... ]

> @@ -1424,7 +1424,7 @@ static void bam_dma_remove(struct platform_device *pdev)
>  			    bdev->channels[i].fifo_phys);
>  	}
>  
> -	tasklet_kill(&bdev->task);
> +	cancel_work_sync(&bdev->work);

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
between the work queue and pm_runtime_force_suspend() in the removal path?

In bam_dma_remove(), pm_runtime_force_suspend() is called to disable the
clock before the work queue is cancelled.

If the asynchronous work queue is executing concurrently, it could pass its
internal pm_runtime_get_sync() check before force_suspend overrides the PM
state.

Would the work queue then execute bam_start_dma() and write to unclocked
hardware registers?

[Severity: High]
This is also a pre-existing issue, but does bam_dma_remove() access
hardware registers after explicitly disabling device clocks?

In bam_dma_remove(), immediately following the pm_runtime_force_suspend()
call which disables the bamclk, it issues a writel_relaxed() to
BAM_IRQ_SRCS_MSK_EE and calls bam_dma_terminate_all() which modifies
hardware registers.

Since the clock is disabled at this point, could these accesses crash the
system during driver unbind?

[Severity: High]
While looking at bam_dma_terminate_all(), this is another pre-existing
issue, but does it access hardware registers without acquiring a runtime
PM reference?

When called by DMA clients to abort a transfer, if the channel is active,
it calls bam_chan_init_hw(), which writes to the BAM_P_RST register:

bam_dma_terminate_all()
    if (!list_empty(&bchan->desc_list)) {
        ...
        bam_chan_init_hw(bchan, async_desc->dir);
    }

However, it does not call pm_runtime_get_sync() first.

Since the driver drops its PM reference immediately after submitting a
transaction (relying on a 100ms autosuspend delay), could a client calling
terminate_all hit unclocked hardware if the device has already auto-suspended?

>  
>  	clk_disable_unprepare(bdev->bamclk);
>  }

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