Re: [PATCH v2 5/6] dmaengine: arm-dma350: Add CIX Sky1 integration support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] dmaengine: arm-dma350: Tasklet Use-After-Free on teardown
- [Medium] dmaengine: arm-dma350: leak of reserved memory on probe error paths
- [Medium] dmaengine: arm-dma350: leak of reserved memory assignment on probe error
- [Medium] dmaengine: arm-dma350: reserved memory leak in probe error paths
- [Low] kconfig: ARM_DMA350 selects MFD_SYSCON without HAS_IOMEM dependency
--

commit a78806dbf20ce04511d1cbf853c6105e7756c04b
Author: Jelly Jia <Jelly.Jia@cixtech.com>
dmaengine: arm-dma350: Add CIX Sky1 integration support

The CIX Sky1 SoC integrates DMA-350 controllers with custom peripheral
address mapping and interrupt routing. This commit adds device tree matching,
clock, reset, and syscon-based interrupt routing during probe and resume.
It also introduces support for attaching to a reserved memory region.

> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index ae6a682c9f769..efa6d280bb36e 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -97,9 +97,13 @@ config ARM_DMA350
>  	depends on ARM || ARM64 || COMPILE_TEST
>  	select DMA_ENGINE
>  	select DMA_VIRTUAL_CHANNELS
> +	select MFD_SYSCON

[Severity: Low]
Is this missing a dependency on HAS_IOMEM? MFD_SYSCON inherently depends on
HAS_IOMEM in drivers/mfd/Kconfig, so this unconditional select can cause
Kconfig unmet direct dependency warnings on architectures without it (e.g.
UML) during COMPILE_TEST. Since the driver uses MMIO functions, should it
depend on HAS_IOMEM?

[ ... ]

> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index e3fbfcce66e8e..e443a9c3189b2 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c

[ ... ]

> @@ -1201,6 +1264,13 @@ static int d350_probe(struct platform_device *pdev)
>  		dmac->dma.device_prep_dma_memset = d350_prep_memset;
>  	}
>  
> +	if (of_property_present(dev->of_node, "memory-region")) {
> +		ret = of_reserved_mem_device_init(dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to attach reserved memory\n");
> +	}
> +
>  	platform_set_drvdata(pdev, dmac);
>  
>  	ret = dma_async_device_register(&dmac->dma);

[Severity: Medium]
If dma_async_device_register() or the subsequent of_dma_controller_register()
fails here, the function returns directly. Does this code leak the reserved
memory assignment from of_reserved_mem_device_init() above? It seems
of_reserved_mem_device_release() is needed on these error paths.

[ ... ]

> @@ -1223,9 +1293,30 @@ static void d350_remove(struct platform_device *pdev)
>  
>  	of_dma_controller_free(pdev->dev.of_node);
>  	dma_async_device_unregister(&dmac->dma);
> +	of_reserved_mem_device_release(&pdev->dev);
>  }

[Severity: High]
Can a pending tasklet trigger a use-after-free here?

When d350_remove() is called, dma_async_device_unregister() unregisters the
device, but clients might have freed their channels via
d350_free_chan_resources() without killing the tasklet (since
vchan_free_chan_resources() does not call tasklet_kill()).

If a tasklet is pending, and d350_remove() returns, the dmac structure
will be freed via devres, but the tasklet might still execute and dereference
the freed virtual channel. Should tasklet_kill() be called during channel
resource cleanup to prevent this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009051846.1115962-1-Jelly.Jia@cixtech.com?part=5