Re: [PATCH v4 06/14] drm/rockchip: vop2: Fix VOP2_MAX_DCLK_RATE overflow on 32-bit

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the DRM subsystem guidelines explicitly designate CRTC atomic enable and disable callbacks as executing in atomic context where sleeping is forbidden. However, `vop2_crtc_atomic_enable` and `vop2_crtc_atomic_disable` invoke multiple sleeping functions. For example, both callbacks invoke `vop2_lock()` (which acquires a mutex). `vop2_crtc_atomic_enable` calls `clk_prepare_enable()`, and `vop2_crtc_atomic_disable` calls `wait_for_completion_timeout()`.
--

commit 42f6f5bd762f8191ecc03127da51288c0ad3ea76
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/rockchip: vop2: Fix VOP2_MAX_DCLK_RATE overflow on 32-bit

This commit modifies the VOP2_MAX_DCLK_RATE macro to use an unsigned long 
long suffix. This ensures 64-bit arithmetic is used during clock rate 
calculations, preventing integer overflows on architectures where unsigned 
long is 32-bit.

Link: https://lore.kernel.org/all/20260623203310.9F8C81F000E9@smtp.kernel.org/

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 12a6341e3c988..c3f72cac63690 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -102,7 +102,7 @@ enum vop2_afbc_format {
>  	VOP2_AFBC_FMT_INVALID = -1,
>  };
>  
> -#define VOP2_MAX_DCLK_RATE		600000000UL
> +#define VOP2_MAX_DCLK_RATE		600000000ULL

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this file, I
noticed that the CRTC atomic callbacks appear to invoke sleeping functions.

In vop2_crtc_atomic_enable():

    vop2_lock(vop2);

    ret = clk_prepare_enable(vp->dclk);

And in vop2_crtc_atomic_disable():

    ret = wait_for_completion_timeout(&vp->dsp_hold_completion,
                                      msecs_to_jiffies(50));

Does this code risk system instability or deadlocks? The DRM subsystem 
guidelines designate CRTC atomic enable and disable callbacks as executing 
in an atomic context where sleeping is forbidden, yet these paths acquire a
mutex, enable clocks, and wait for completion timeouts.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-dw-hdmi-qp-yuv-v4-0-fb45bf4147eb@collabora.com?part=6