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

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

Pre-existing issues:
- [Critical] Hardware register accesses occur on a suspended, unclocked device during module removal and channel termination.
- [High] Unprotected hardware access in the interrupt handler before acquiring a PM runtime reference.
- [High] PM usage counter leak on pm_runtime_get_sync() error paths permanently breaks runtime PM.
--

--- Patch [3]: [PATCH v24 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue ---
commit 08e5256f7c45114b9d7ecdee4105f52f7321782a
Author: Bartosz Golaszewski <[email protected]>
Subject: dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue

This patch converts the BAM DMA driver from using legacy tasklets to using
the high-priority variant of the BH workqueue for starting DMA transactions.

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

If a spurious interrupt or hardware glitch causes the IRQ handler to fire
while the device is in a runtime-suspended state, this unprotected read
could cause a fatal NoC error.

> -	/* 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);
>  	if (ret < 0)

[Severity: High]
This is also a pre-existing issue, but if pm_runtime_get_sync() fails here
and returns a negative value, the function returns IRQ_NONE without dropping
the PM usage counter.

Since pm_runtime_get_sync() increments the usage counter even on failure,
should there be a call to pm_runtime_put_noidle() on this error path to
prevent a permanent PM usage counter leak? (A similar pattern also exists in
bam_start_dma(), bam_pause(), and bam_resume()).

[ ... ]

> @@ -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);
>  
>  	clk_disable_unprepare(bdev->bamclk);
>  }

[Severity: Critical]
This is a pre-existing issue, but in bam_dma_remove(), is it possible for an
unclocked hardware write to BAM_IRQ_SRCS_MSK_EE to occur immediately after
the clock is disabled by pm_runtime_force_suspend() higher up in this
function?

Additionally, bam_dma_terminate_all() is called nearby, which may also
perform unclocked hardware writes via bam_chan_init_hw() if there are
pending descriptors.

Could this lead to a synchronous external abort or NoC error during module
removal or unbinding?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3