Re: [PATCH v23 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] Teardown race condition between background work and device removal
- [High] Hardware registers accessed before verifying Runtime PM state in IRQ handler
- [High] Resource leak on pm_runtime_get_sync() failure
- [High] Spurious IRQ storm and permanent device disablement due to early IRQ request
--

--- Patch [3]: [PATCH v23 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue ---
commit b043c7508eb0576f9b0b8a168ece5940655bca10
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);
>  
> -	/* 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);

[Severity: High]
This is a pre-existing issue, but does process_channel_irqs() read hardware 
registers before pm_runtime_get_sync() verifies the device is active?

If the device is runtime-suspended (clocks disabled) when a spurious or
unhandled interrupt occurs, could this unclocked read cause a bus fault?

[ ... ]

> @@ -1120,14 +1121,14 @@ static void bam_start_dma(struct bam_chan *bchan)
>  }
>  
>  /**
> - * dma_tasklet - DMA IRQ tasklet
> - * @t: tasklet argument (bam controller structure)
> + * bam_dma_work() - DMA interrupt work queue callback
> + * @work: work queue struct embedded in the BAM controller device struct
>   *
>   * Sets up next DMA operation and then processes all completed transactions
>   */
> -static void dma_tasklet(struct tasklet_struct *t)
> +static void bam_dma_work(struct work_struct *work)

[Severity: High]
This is a pre-existing issue, but looking at bam_start_dma() which is called
by this work queue:

drivers/dma/qcom/bam_dma.c:bam_start_dma() {
    ...
	vd = vchan_next_desc(&bchan->vc);
	if (!vd)
		return;

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

If pm_runtime_get_sync() fails, it returns without dropping the usage counter 
and without cleaning up the popped virt_dma descriptor.

Does this permanently leak the descriptor, hang the client waiting for 
completion, and permanently prevent device suspend?

The same early return pattern appears in bam_free_chan().

[ ... ]

> @@ -1316,14 +1316,14 @@ static int bam_dma_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_disable_clk;
>  
> -	tasklet_setup(&bdev->task, dma_tasklet);
> +	INIT_WORK(&bdev->work, bam_dma_work);

[Severity: High]
This is a pre-existing issue, but in bam_dma_probe(), the interrupt is 
registered before pm_runtime_enable() is called:

drivers/dma/qcom/bam_dma.c:bam_dma_probe() {
    ...
	ret = request_irq(bdev->irq, bam_dma_irq, ...);
    ...
	pm_runtime_enable(&pdev->dev);
    ...
}

If an error interrupt fires in this window, will pm_runtime_get_sync() in the 
IRQ handler return -EACCES because runtime PM is not yet enabled?

If so, could this cause the handler to return IRQ_NONE without clearing the 
hardware interrupt (skipping BAM_IRQ_CLR), leading to an infinite IRQ loop?

[ ... ]

> @@ -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 is a pre-existing issue, but is the work queue canceled after the device 
is forcefully suspended?

Looking at the code in bam_dma_remove():

    pm_runtime_force_suspend(&pdev->dev);
    ...
    cancel_work_sync(&bdev->work);

If bam_dma_work() executes concurrently with bam_dma_remove(), it calls
bam_start_dma(), which assumes the hardware is active. 

If pm_runtime_force_suspend() forcefully disables the device clock before the
work queue is canceled, could the background work attempt unclocked hardware 
register access and cause a synchronous external abort?

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