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 <[email protected]> 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/[email protected]/ > 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/[email protected]?part=3
