Re: [PATCH v3 10/10] drm/vmwgfx: Remove unused field struct vmwgfx_du_update_plane.old_state

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Plane atomic_update callbacks indirectly call sleeping functions (mutex_lock/mutex_lock_interruptible) from an atomic context.
--

commit deccb03fba43ed6006d88ae9933240eb52b52fee
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/vmwgfx: Remove unused field struct vmwgfx_du_update_plane.old_state

This commit removes the unused old_state field from the
vmw_du_update_plane structure and updates all caller functions to
accommodate this change.

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
[ ... ]
> @@ -721,7 +717,6 @@ static void
>  vmw_sou_primary_plane_atomic_update(struct drm_plane *plane,
>  				    struct drm_atomic_commit *state)
>  {
> -	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state, plane);
>  	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, plane);
>  	struct drm_crtc *crtc = new_state->crtc;
>  	struct vmw_fence_obj *fence = NULL;
> @@ -734,12 +729,9 @@ vmw_sou_primary_plane_atomic_update(struct drm_plane *plane,
>  			vmw_framebuffer_to_vfb(new_state->fb);
>  
>  		if (vfb->bo)
> -			ret = vmw_sou_plane_update_bo(dev_priv, plane,
> -						      old_state, vfb, &fence);
> +			ret = vmw_sou_plane_update_bo(dev_priv, plane, vfb, &fence);
>  		else
> -			ret = vmw_sou_plane_update_surface(dev_priv, plane,
> -							   old_state, vfb,
> -							   &fence);
> +			ret = vmw_sou_plane_update_surface(dev_priv, plane, vfb, &fence);

[Severity: High]
This is a pre-existing issue, but does this code path end up calling sleeping
functions from an atomic context?

Looking at the call chain, vmw_sou_primary_plane_atomic_update() is a plane
atomic update callback, which runs in atomic context where sleeping is
forbidden. However, it calls vmw_sou_plane_update_surface(), which assigns a
mutex pointer to the update structure:

vmw_sou_plane_update_surface()
  srf_update.base.mutex = &dev_priv->cmdbuf_mutex;
  vmw_du_helper_plane_update(&srf_update.base);

This closure is then passed down to vmw_du_helper_plane_update(), which calls
vmw_validation_prepare(). That function then takes the lock using
mutex_lock() or mutex_lock_interruptible().

Could this lead to system instability or deadlocks if the atomic commit runs
in a non-blocking path where sleeping is not allowed?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527145113.241595-1-tzimmermann@suse.de?part=10