Re: [PATCH 3/3] dmaengine: sprd: Remove obsolete 32-bit DMA mask fallback

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Premature IRQ request leading to potential NULL pointer dereference and uninitialized spinlock access.
- [Medium] PM runtime usage counter and enabled state leak on the `pm_runtime_get_sync()` error path.
--

commit 323a1a8d4f9b18f09fa8dd3e9872da5d0d05321b
Author: Ruizhe Zhou <zhouruizhe@resnics.com>

dmaengine: sprd: Remove obsolete 32-bit DMA mask fallback

This commit removes the obsolete 32-bit DMA mask fallback from
sprd_dma_probe(), relying solely on the 36-bit DMA mask request, while
retaining the error check to report genuine DMA setup failures.

> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 087fea3af2e41..b9f705dd80e52 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -1117,11 +1117,8 @@ static int sprd_dma_probe(struct platform_device *pdev)
>  
>  	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(36));
>  	if (ret) {
> -		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> -		if (ret) {
> -			dev_err(&pdev->dev, "unable to set coherent mask to 32\n");
> -			return ret;
> -		}
> +		dev_err(&pdev->dev, "unable to set 36-bit DMA mask\n");
> +		return ret;
>  	}
>  
>  	/* Parse new and deprecated dma-channels properties */

[Severity: High]
This isn't a bug introduced by this patch, but looking further down in
sprd_dma_probe(), could the premature IRQ request lead to a NULL pointer
dereference or uninitialized spinlock access if an interrupt fires immediately?

If an interrupt is pending at the controller level, the handler dma_irq_handle()
will execute as soon as it is requested:

	ret = devm_request_irq(&pdev->dev, sdev->irq, dma_irq_handle,
			       0, "sprd_dma", (void *)sdev);

The interrupt handler will then dereference sdev->glb_base, which is mapped
later:

	sdev->glb_base = devm_platform_ioremap_resource(pdev, 0);

And it will attempt to acquire the spinlock, which is initialized even later:

	vchan_init(&dma_chn->vc, &sdev->dma_dev);

[Severity: Medium]
This is a pre-existing issue, but does the pm_runtime_get_sync() error path
leak the PM runtime usage counter and leave PM runtime enabled?

	ret = pm_runtime_get_sync(&pdev->dev);
	if (ret < 0)
		goto err_rpm;

If pm_runtime_get_sync() fails and returns a negative error code, it jumps
to the err_rpm label:

err_rpm:
	sprd_dma_disable(sdev);
	return ret;

Since pm_runtime_get_sync() increments the usage counter even on failure,
shouldn't this path call pm_runtime_put_noidle() to drop the usage counter,
and balance the earlier pm_runtime_enable() call with pm_runtime_disable()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903115441.912500-1-zhouruizhe@resnics.com?part=3