Re: [PATCH v2 03/13] drm/rockchip: vop2: Balance state on atomic_enable error paths

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Powering down VOP2 hardware on `atomic_enable` error causes a kernel panic when DRM core unconditionally proceeds to `atomic_update` and `atomic_flush`.

Pre-existing issues:
- [High] VBLANK events are leaked if `atomic_enable` fails, permanently hanging userspace waiting for a page flip completion.
--

commit 635b59397bdbb9526ef2f8c21f8fd4904f6e98d9
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/rockchip: vop2: Balance state on atomic_enable error paths

vop2_crtc_atomic_enable() prepares vp->dclk and calls vop2_enable(),
incrementing the shared vop2->enable_count before programming registers.
This commit attempts to fix a resource leak by reverting the setup on
failure, and tracking the per-VP bring-up state so atomic_disable() skips
teardown when atomic_enable() fails.

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
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -1033,6 +1033,9 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
>  
>  	vop2_lock(vop2);
>  
> +	if (!vp->enabled)
> +		goto out_unlock;
> +

[Severity: High]
This is a pre-existing issue, but does this early return cause VBLANK
events to be leaked if atomic_enable() fails?

If atomic_enable() fails, drm_crtc_vblank_on() is skipped. The DRM core
still continues to atomic_flush(), which consumes the event and stores it:

vop2_crtc_atomic_flush() {
    ...
    if (crtc->state->event) {
        WARN_ON(drm_crtc_vblank_get(crtc));
        vp->event = crtc->state->event;
        crtc->state->event = NULL;
    }
    ...
}

Because the VBLANK interrupt wasn't enabled, it never fires to process
vp->event. When the CRTC is eventually disabled, this new early return
bypasses the teardown and ignores the trapped vp->event. Could this cause
the userspace process waiting for the page flip completion to hang
indefinitely?

[ ... ]

> @@ -1965,6 +1971,18 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>  
>  	drm_crtc_vblank_on(crtc);
>  
> +	vp->enabled = true;
> +
> +	vop2_unlock(vop2);
> +
> +	return;
> +
> +err_put_enable_count:
> +	if (!--vop2->enable_count)
> +		vop2_disable(vop2);
> +

[Severity: Critical]
Does powering down the VOP2 hardware on an atomic_enable() error cause a
synchronous external abort (bus hang)?

Since atomic_enable() returns void, the DRM core will unconditionally
proceed to call the atomic_update() and atomic_flush() callbacks even if
setup fails here. If vop2_disable() gates the core clocks, subsequent
unconditional MMIO accesses like this one in vop2_plane_atomic_update()
will access unclocked memory:

vop2_plane_atomic_update() {
    ...
    vop2_win_write(win, VOP2_WIN_AXI_BUS_ID, win->data->axi_bus_id);
    ...
}

Similar unconditional accesses happen in vop2_crtc_atomic_flush() when it
calls vop2_post_config(). Can this result in an immediate kernel panic on
transient clock setup failures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-dw-hdmi-qp-yuv-v2-0-b91beb33dedf@collabora.com?part=3