Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] AXI reset is performed after detaching the IOMMU domain, risking memory corruption. Pre-existing issues: - [High] Unpowered hardware access and unbalanced PM runtime usage due to ignored error in vop2_enable(). - [High] Clock enable count underflow and resource tracking mismatch on early return in vop2_crtc_atomic_enable(). -- commit 13864800def8231541cc3847f090a3ab6ab365c2 Author: Cristian Ciocaltea <[email protected]> Subject: drm/rockchip: vop2: Reset AXI and DCLK to improve robustness Assert the AXI reset in the CRTC disable path, and the VP DCLK reset in the enable path. These resets are intended to leave the hardware in a clean state for the next use, helping recover from exceptions such as IOMMU page faults. > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > index 4cce3e336f5b3..2833fb49ad81a 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c [ ... ] > @@ -938,6 +959,8 @@ static void vop2_disable(struct vop2 *vop2) > { > rockchip_drm_dma_detach_device(vop2->drm, vop2->dev); > > + vop2_clk_reset(vop2, vop2->axi_rst); > + [Severity: High] Does asserting the AXI reset here risk memory corruption? If the AXI master is recovering from an exception and still issuing DMA requests, detaching the IOMMU domain first leaves a window where the master could access physical memory without translation. Could this lead to memory corruption or spurious page faults? Should the AXI master be explicitly reset before the IOMMU is detached? > pm_runtime_put_sync(vop2->dev); > > regcache_drop_region(vop2->map, 0, vop2_regmap_config.max_register); [ ... ] > @@ -1948,6 +1971,8 @@ static void vop2_crtc_atomic_enable(struct drm_crtc > *crtc, > > vop2_crtc_atomic_try_set_gamma(vop2, vp, crtc, crtc_state); > > + vop2_clk_reset(vop2, vp->dclk_rst); > + [Severity: High] This isn't a bug introduced by this patch, but is it possible for unpowered hardware access to cause a crash if vop2_enable() fails? In vop2_enable(), if pm_runtime_resume_and_get() or clock preparation fails, it logs an error and returns void. However, vop2_crtc_atomic_enable() unconditionally increments vop2->enable_count and continues configuring hardware: if (!vop2->enable_count) vop2_enable(vop2); vop2->enable_count++; Since the power domain and core clocks would remain off on failure, could the subsequent writes to VOP2 hardware registers trigger a synchronous external abort and crash the system? [Severity: High] This is a pre-existing issue, but can an early return in vop2_crtc_atomic_enable() cause severe state mismatches? If clk_prepare_enable(vp->dclk) fails earlier in this function: ret = clk_prepare_enable(vp->dclk); if (ret < 0) { drm_err(vop2->drm, "failed to enable dclk for video port%d - %d\n", vp->id, ret); vop2_unlock(vop2); return; } The function returns early without updating state properly. During disable, vop2_crtc_atomic_disable() blindly decrements the tracking state: clk_disable_unprepare(vp->dclk); vop2->enable_count--; if (!vop2->enable_count) vop2_disable(vop2); If another CRTC is active, couldn't this underflow or prematurely drop the count to 0, unintentionally shutting off core clocks and PM runtime for the actively running CRTC? > drm_crtc_vblank_on(crtc); > > vop2_unlock(vop2); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
