Re: [PATCH v20 05/14] dmaengine: qcom: bam_dma: Add pipe_lock_supported flag support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Prematurely freeing the IRQ in `bam_dma_remove()` before unregistering the DMA device.

Pre-existing issues:
- [High] Accessing hardware registers after the clock has been disabled in `bam_dma_remove()`.
--

commit c3d67a708644d5b3039f73321dfd2a2034b5a7b2
Author: Bartosz Golaszewski <brgl@kernel.org>

dmaengine: qcom: bam_dma: Add pipe_lock_supported flag support

Extend the device match data with a flag indicating whether the IP
supports the BAM lock/unlock feature. Set it to true on BAM IP versions
1.4.0 and above.

> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 8ce0fe085c5fe..f3e713a5259c2 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -115,6 +115,7 @@ struct reg_offset_data {
>  
>  struct bam_device_data {
>  	const struct reg_offset_data *reg_info;
> +	bool pipe_lock_supported;
>  };
>  
>  static const struct reg_offset_data bam_v1_3_reg_info[] = {

[ ... ]

Looking at bam_dma_remove() in drivers/dma/qcom/bam_dma.c:

drivers/dma/qcom/bam_dma.c:bam_dma_remove() {
...
	free_irq(bdev->irq, bdev);

	pm_runtime_force_suspend(&pdev->dev);

	of_dma_controller_free(pdev->dev.of_node);
	dma_async_device_unregister(&bdev->common);
...
}

[Severity: High]
If a client has an active DMA transaction when the DMA engine is removed (e.g.,
via sysfs unbind or module unload), does freeing the interrupt early mean the
completion interrupt will never be serviced?

This could cause the client driver to wait indefinitely for the DMA transaction
to finish, leading to unkillable tasks or system hangs. Should the DMA engine
be unregistered and channels properly quiesced before the IRQ is released?


drivers/dma/qcom/bam_dma.c:bam_dma_remove() {
...
	pm_runtime_force_suspend(&pdev->dev);

	of_dma_controller_free(pdev->dev.of_node);
	dma_async_device_unregister(&bdev->common);

	/* mask all interrupts for this execution environment */
	writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));

	for (i = 0; i < bdev->num_channels; i++) {
		bam_dma_terminate_all(&bdev->channels[i].vc.chan);
...
}

[Severity: High]
This is a pre-existing issue, but can accessing hardware registers after the
clock has been disabled here cause a system crash?

The clock is disabled by pm_runtime_force_suspend(), making the subsequent
writel_relaxed() and bam_dma_terminate_all() register accesses fatal. On
Qualcomm SoCs, accessing BAM registers while the clock is gated triggers a
synchronous external abort (NOC error), resulting in an immediate kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-qcom-qce-cmd-descr-v20-0-56f67da84c05@oss.qualcomm.com?part=5