Re: [PATCH v2 7/11] drm: kmb: 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:
- [High] The return value of kmb_initialize_clocks() is ignored in kmb_hw_init(), leading to potential hardware hangs.
- [High] Calling drm_atomic_helper_shutdown() after disabling clocks and PM runtime leads to hardware access on a powered-down device and a system hang.
- [High] Dereferencing an ERR_PTR during error handling of DSI initialization causes a kernel panic.
--

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

drm: kmb: Use devm_of_reserved_mem_device_init()

This commit updates the driver to use the devres-managed variant of
of_reserved_mem_device_init(), simplifying the error and removal paths by
removing manual cleanup calls.

> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
> index 7c2eb1152fc2f..88fe377940f30 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -140,13 +140,12 @@ static int kmb_hw_init(struct drm_device *drm, unsigned long flags)
>  	/* Allocate LCD interrupt resources */
>  	irq_lcd = platform_get_irq(pdev, 0);

[Severity: High]
This is a pre-existing issue, but I noticed that just before this hunk in
kmb_hw_init(), the return value of kmb_initialize_clocks() is discarded:

	/* Enable display clocks */
	kmb_initialize_clocks(kmb, &pdev->dev);

	/* Register irqs here - section 17.3 in databook

If kmb_initialize_clocks() fails, the driver proceeds with probe and will
eventually access unclocked MMIO registers during modeset. Can this lead to
synchronous external aborts and system hangs?

[ ... ]

> @@ -465,8 +459,6 @@ static void kmb_remove(struct platform_device *pdev)
>  	pm_runtime_put_sync(drm->dev);
>  	pm_runtime_disable(drm->dev);
>  
> -	of_reserved_mem_device_release(drm->dev);
> -
>  	/* Release clks */
>  	kmb_display_clk_disable(kmb);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does the shutdown sequence
in kmb_remove() cause hardware access on a powered-down device?

Right after this hunk, the function completes with:

	/* Unregister DSI host */
	kmb_dsi_host_unregister(kmb->kmb_dsi);
	drm_atomic_helper_shutdown(drm);
}

Since pm_runtime_disable() and kmb_display_clk_disable() are called prior to
drm_atomic_helper_shutdown(), atomic_disable callbacks will read and write
hardware registers while clocks are disabled. Will this cause a bus fault or
system hang during driver unbind?

[Severity: High]
This is also a pre-existing issue, but outside the diff in kmb_probe(), does
the error handling for kmb_dsi_init() cause an invalid pointer dereference?

If kmb_dsi_init() fails, it returns an ERR_PTR:

	kmb->kmb_dsi = kmb_dsi_init(dsi_pdev);
	if (IS_ERR(kmb->kmb_dsi)) {
		drm_err(&kmb->drm, "failed to initialize DSI\n");
		ret = PTR_ERR(kmb->kmb_dsi);
		goto err_free1;
	}

The error handling path unconditionally passes this ERR_PTR to
kmb_dsi_host_unregister(), which passes it to kmb_dsi_clk_disable(). This
dereferences the pointer when attempting to access kmb_dsi->clk_mipi:

drivers/gpu/drm/kmb/kmb_dsi.c:kmb_dsi_clk_disable() {
	clk_disable_unprepare(kmb_dsi->clk_mipi);

Will this result in a kernel panic due to dereferencing the ERR_PTR on memory
allocation failure during initialization?

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