Re: [PATCH 2/9] drm/rockchip: vop2: Reset AXI and DCLK to improve robustness

[sashiko-bot@kernel.org]

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 <cristian.ciocaltea@collabora.com>
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/20260617-dw-hdmi-qp-yuv-v1-0-a665cfd06d7d@collabora.com?part=2