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

Pre-existing issues:
- [Critical] Driver removal forcefully suspends the device before hardware teardown and workqueue cancellation, causing system crashes and Use-After-Free.
- [High] The IRQ handler accesses hardware registers before ensuring the device is active, leading to a potential system crash on spurious interrupts.
- [High] Error paths for `pm_runtime_get_sync()` fail to decrement the PM usage counter, permanently preventing the device from runtime suspending.
- [High] DMA controller is exposed to clients before runtime PM is enabled, dropping transactions.
- [Medium] Missing pm_runtime_disable() in driver removal.
--

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

This patch replaces the legacy tasklet implementation in the BAM DMA driver
with a high-priority BH workqueue. It updates the probe, remove, irq, and
execution paths to use workqueue APIs.

> 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 isn't a bug introduced by this patch, but does process_channel_irqs()
read hardware registers before pm_runtime_get_sync() ensures the clock is
enabled?

If a spurious interrupt arrives while the controller is in runtime suspend
(clocks disabled), could reading the register cause a synchronous external
abort and crash the system?

>  	if (ret < 0)
> -		return IRQ_NONE;

[Severity: High]
This is a pre-existing issue, but if pm_runtime_get_sync() fails, it still
increments the usage counter.

Does this early return fail to drop the PM usage counter (e.g., via
pm_runtime_put_noidle()), permanently skewing the reference count and
preventing the hardware from entering low-power states?

It looks like this same error handling pattern is also present in
bam_start_dma() and bam_resume().

[ ... ]

> @@ -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);
>  
>  	bdev->channels = devm_kcalloc(bdev->dev, bdev->num_channels,
>  				sizeof(*bdev->channels), GFP_KERNEL);
>  
>  	if (!bdev->channels) {
>  		ret = -ENOMEM;
> -		goto err_tasklet_kill;
> +		goto err_workqueue_cancel;
>  	}

[Severity: High]
This is a pre-existing issue, but it looks like bam_dma_probe() registers
the DMA controller with the subsystem before runtime PM is enabled.

If a consumer device instantly requests a channel and submits a transaction,
could pm_runtime_get_sync() return -EACCES and silently abort the transfer
because PM runtime is not yet active?

[ ... ]

> @@ -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: Critical]
This is a pre-existing issue, but does workqueue cancellation happen after
channel FIFOs have been freed by dma_free_wc() earlier in this function?

Could a concurrently running bam_dma_work execute bam_start_dma() and
access the freed fifo_virt memory, leading to a use-after-free?

[Severity: Critical]
This is also a pre-existing issue, but bam_dma_remove() calls
pm_runtime_force_suspend() early on, which disables the hardware clocks.

Does the driver then perform unclocked register accesses (e.g., by writing
to the BAM_IRQ_SRCS_MSK_EE register and calling bam_dma_terminate_all())
before reaching this point, potentially causing a kernel panic?

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

[Severity: Medium]
This is a pre-existing issue, but the function exits without calling
pm_runtime_disable().

Does this leave the PM core's runtime PM state active for this device after
the driver is unbound?

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