Re: [PATCH v3 02/14] drm/rockchip: vop2: Fix resource leak on vop2_enable() error path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch modifies `vop2_enable()` to return an error code and cleanly unwind resources on failure, but fails to update its caller `vop2_crtc_atomic_enable()` to check this return value. This converts a resource leak into a critical double-free and system crash.
--

--- Patch [2]: [PATCH v3 02/14] drm/rockchip: vop2: Fix resource leak on vop2_enable() error path ---
commit d8725e1d1e43785d7a93679c9bc6634b3ebd7fab
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/rockchip: vop2: Fix resource leak on vop2_enable() error path

vop2_enable() returns void, so callers cannot tell whether powering up
the VOP2 succeeded.

Additionally, if rockchip_drm_dma_attach_device() fails, vop2_enable()
returns without disabling the core clocks or dropping the runtime PM
reference taken earlier, leaking both.

Make vop2_enable() return int and unwind all resources on every failure
path.  As disabling the core clocks is also needed in vop2_disable(),
factor it out into a local helper used by both.

Link: https://lore.kernel.org/all/20260623202043.8BFE21F000E9@smtp.kernel.org/

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 4cce3e336f5b3..e11dc8c7f78ba 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c

[ ... ]

> @@ -871,7 +878,7 @@ static void rk3588_vop2_power_domain_enable_all(struct vop2 *vop2)
>  	vop2_writel(vop2, RK3588_SYS_PD_CTRL, pd);
>  }
>  
> -static void vop2_enable(struct vop2 *vop2)
> +static int vop2_enable(struct vop2 *vop2)
>  {
>  	int ret;
>  	u32 version;

[ ... ]

> @@ -932,6 +938,17 @@ static void vop2_enable(struct vop2 *vop2)
>  		    VOP2_INT_BUS_ERRPR << 16 | VOP2_INT_BUS_ERRPR);
>  	vop2_writel(vop2, RK3568_SYS1_INT_EN,
>  		    VOP2_INT_BUS_ERRPR << 16 | VOP2_INT_BUS_ERRPR);
> +
> +	return 0;
> +
> +err_detach_dma:
> +	rockchip_drm_dma_detach_device(vop2->drm, vop2->dev);
> +err_disable_clks:
> +	vop2_core_clks_disable_unprepare(vop2);
> +err_put_pm:
> +	pm_runtime_put_sync(vop2->dev);
> +
> +	return ret;
>  }

[Severity: High]
Does this code leave vop2_crtc_atomic_enable() vulnerable to a crash if
vop2_enable() fails?

The newly added error path in vop2_enable() unwinds resources (power and
clocks), invalidating the assumption that the hardware is active.

However, the caller vop2_crtc_atomic_enable() still ignores the return
value:

	if (!vop2->enable_count)
		vop2_enable(vop2);

	vop2->enable_count++;

If vop2_enable() fails (e.g., due to DMA attach failure), it disables the
core clocks and puts the PM runtime.

Because vop2_crtc_atomic_enable() ignores the error, it unconditionally
increments enable_count and proceeds to access VOP2 MMIO registers.

Can this result in a system crash (Synchronous External Abort) due to MMIO
access to unpowered/unclocked hardware, followed by a double-free of PM
runtime and clock resources on teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-dw-hdmi-qp-yuv-v3-0-a4a982a9f2e7@collabora.com?part=2