[PATCH] drm/vmwgfx: Add some checks to vmw_cursor_plane_atomic_update

[PATCH] drm/vmwgfx: Add some checks to vmw_cursor_plane_atomic_update

[Maaz Mombasawala]

In cases where old_state may not be present we hit a null dereference.
Add a check to fix this and also do an early return if there is no
new_state or crtc since there is nothing to do in that case.
This fixes igt test kms_cursor_legacy@torture-bo.

Signed-off-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
index c46f17ba7236..88b7185a8371 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
@@ -744,9 +744,14 @@ vmw_cursor_plane_atomic_update(struct drm_plane *plane,
 	struct vmw_bo *bo;
 	struct drm_plane_state *new_state =
 		drm_atomic_get_new_plane_state(state, plane);
+	if (!new_state)
+		return;
 	struct drm_plane_state *old_state =
 		drm_atomic_get_old_plane_state(state, plane);
-	struct drm_crtc *crtc = new_state->crtc ?: old_state->crtc;
+	struct drm_crtc *crtc = new_state->crtc ? new_state->crtc :
+		(old_state ? old_state->crtc : NULL);
+	if (!crtc)
+		return;
 	struct vmw_private *dev_priv = vmw_priv(plane->dev);
 	struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
 	struct vmw_plane_state *vps = vmw_plane_state_to_vps(new_state);
-- 
2.53.0

Claude review: drm/vmwgfx: Add some checks to vmw_cursor_plane_atomic_update

[Claude Code Review Bot]

Overall Series Review

Subject: drm/vmwgfx: Add some checks to vmw_cursor_plane_atomic_update
Author: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
Patches: 1
Reviewed: 2026-03-22T03:57:47.499306

---

This is a single-patch series adding null-pointer checks to `vmw_cursor_plane_atomic_update()` in the vmwgfx driver to fix a crash seen in the `kms_cursor_legacy@torture-bo` IGT test. The intent is reasonable — guarding against null dereferences when `old_state` or `new_state` could be absent. However, the patch has coding style issues that would prevent it from compiling cleanly under strict C standards and violate kernel coding conventions.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vmwgfx: Add some checks to vmw_cursor_plane_atomic_update

[Claude Code Review Bot]

Patch Review

**Coding style: Declarations after statements (build error / warning)**

The patch inserts a `return` statement between variable declarations:

```c
struct drm_plane_state *new_state =
    drm_atomic_get_new_plane_state(state, plane);
if (!new_state)
    return;
struct drm_plane_state *old_state =
    drm_atomic_get_old_plane_state(state, plane);
```

This interleaves statements (`if (!new_state) return;`) with declarations (`struct drm_plane_state *old_state = ...`). While C99/C11 technically allows this, the Linux kernel coding style strongly discourages mixing declarations and statements, and `-Wdeclaration-after-statement` (which is enabled in kernel builds) will produce a warning. The same issue occurs with the `if (!crtc) return;` followed by `struct vmw_private *dev_priv = ...`.

The fix should move all declarations to the top of the function and perform the null checks afterward. Something like:

```c
struct drm_plane_state *new_state =
    drm_atomic_get_new_plane_state(state, plane);
struct drm_plane_state *old_state =
    drm_atomic_get_old_plane_state(state, plane);
struct drm_crtc *crtc;
struct vmw_private *dev_priv;
...

if (!new_state)
    return;

crtc = new_state->crtc ? new_state->crtc :
    (old_state ? old_state->crtc : NULL);
if (!crtc)
    return;

dev_priv = vmw_priv(plane->dev);
```

**Questionable whether `new_state` can actually be NULL**

The `atomic_update` callback is invoked by the atomic commit path only for planes that are part of the commit. `drm_atomic_get_new_plane_state()` should always return a valid state for such a plane. If `new_state` is truly NULL here, it would indicate a deeper framework bug. The commit message says this fixes `kms_cursor_legacy@torture-bo` but doesn't explain the actual root cause — was the plane being committed without state, or was the issue specifically with `old_state` being NULL? A more detailed analysis in the commit message would help reviewers understand if this is papering over a real bug elsewhere.

**The `old_state` NULL check is the more plausible fix**

The original `old_state->crtc` dereference without a NULL check is the more likely actual bug. If a cursor plane is being enabled for the first time (no previous state), `old_state` could legitimately be NULL. Protecting against that case specifically seems correct.

**Minor: ternary replacement**

The original `?:` (GNU extension) was replaced with a fully spelled-out ternary `? new_state->crtc : ...`. This is fine and arguably more portable, though the GNU extension is widely used in the kernel.

**Summary**: The intent is good but the implementation needs rework to comply with kernel coding style (no declarations after statements). The commit message should also clarify the actual root cause more precisely — specifically which pointer was NULL when the crash occurred.

---
Generated by Claude Code Patch Reviewer