Re: [PATCH v2 9/11] drm: xlnx: zynqmp_dpsub: Use devm_of_reserved_mem_device_init()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] CMA area is released while DRM GEM objects might still be alive, leading to memory corruption when freeing DMA memory.
- [High] The APB clock (`apb_clk`) prepare and enable counts are leaked on multiple error paths during probe and clock initialization.
- [High] The allocated `dpsub` structure is leaked if `dma_set_mask` fails.
--

commit 608bb00e624e20e20a7867fa3c7bb9e1c899263d
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

drm: xlnx: zynqmp_dpsub: Use devm_of_reserved_mem_device_init()

This commit replaces the manual of_reserved_mem_device_init and
of_reserved_mem_device_release pair with the devres-managed
devm_of_reserved_mem_device_init, letting the device resource manager
handle cleanup automatically.

> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> index 53ab1a2a5aafa..e93a7a299b525 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> @@ -203,7 +203,7 @@ static int zynqmp_dpsub_probe(struct platform_device *pdev)
>  	dma_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
>  
>  	/* Try the reserved memory. Proceed if there's none. */
> -	of_reserved_mem_device_init(&pdev->dev);
> +	devm_of_reserved_mem_device_init(&pdev->dev);

[Severity: Critical]
This is a pre-existing issue, but does tying the CMA area release to the
platform device's devres cleanup happen too early for DRM GEM objects?

When a DRM device is unbound while userspace holds an open file descriptor,
the GEM objects can outlive the platform device. When userspace eventually
closes the file descriptor, drm_gem_dma_free() calls into the DMA API.
Because the CMA area is already cleaned up upon platform device removal,
can the DMA API fail to recognize the memory as CMA and incorrectly free it
into the buddy allocator, causing memory corruption?

[Severity: High]
This isn't a bug introduced by this patch, but looking at the earlier
sections of zynqmp_dpsub_probe(), we have this error path:

    ret = dma_set_mask(dpsub->dev, DMA_BIT_MASK(ZYNQMP_DISP_MAX_DMA_BIT));
    if (ret)
        return ret;

Does this early return leak the dynamically allocated dpsub structure?
It returns directly instead of jumping to an error handling label where
zynqmp_dpsub_release() or a free operation would be called.

[ ... ]

> @@ -255,7 +255,6 @@ static int zynqmp_dpsub_probe(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	clk_disable_unprepare(dpsub->apb_clk);
>  err_mem:
> -	of_reserved_mem_device_release(&pdev->dev);
>  	if (!dpsub->drm)
>  		zynqmp_dpsub_release(dpsub);
>  	return ret;

[Severity: High]
This is a pre-existing issue, but does this error handling correctly clean up
the apb_clk resource?

If zynqmp_dpsub_parse_dt() fails earlier in the probe function, the code
jumps to the err_mem label. Jumping here skips the err_pm label where
clk_disable_unprepare() is called, which seems to leave the clock enabled
permanently.

Similarly, in zynqmp_dpsub_init_clocks(), if it successfully enables apb_clk
but then fails to acquire vid_clk with -EPROBE_DEFER:

    else if (PTR_ERR(dpsub->vid_clk) == -EPROBE_DEFER)
        return PTR_ERR(dpsub->vid_clk);

Does this return the error immediately without unwinding and disabling the
clock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902201640.2024648-1-mukesh.ojha@oss.qualcomm.com?part=9