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

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

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

[Claude Code Review Bot]

Patch Review

**Coding style issue — declaration after statement:**

```c
+	new_state = drm_atomic_get_new_plane_state(state, plane);
+	if (!new_state)
+		return;
+	old_state = drm_atomic_get_old_plane_state(state, plane);
+	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);
```

The `struct drm_crtc *crtc` and `struct vmw_private *dev_priv` declarations are interspersed with code. While C99/GNU extensions allow this, Linux kernel style strongly prefers all declarations at the top of the block. Move `crtc` and `dev_priv` declarations to the top (alongside `new_state`, `old_state`) and assign them after the NULL checks. Also there's a double space (`=  new_state`) on the crtc assignment line.

**Questionable NULL check on new_state:** In the atomic commit path, `drm_atomic_get_new_plane_state()` should always return a valid state for a plane that is part of the commit — this function is called from `atomic_update`, which only runs for planes included in the commit. If `new_state` is NULL here, something has gone seriously wrong at a higher layer. A `WARN_ON(!new_state)` plus return might be more appropriate to flag the unexpected condition rather than silently returning.

**Logic correctness:** The ternary for `crtc` is correct — it safely handles `old_state` being NULL. The original code `new_state->crtc ?: old_state->crtc` would crash if `new_state->crtc` was NULL and `old_state` was also NULL.

---
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

**C99 mixed declarations issue.** The patch adds early-return guards for null `new_state` and `old_state`, which is correct defensively, but introduces a variable declaration after a statement, which is problematic for kernel coding style:

```c
	new_state = drm_atomic_get_new_plane_state(state, plane);
	if (!new_state)
		return;
	old_state = drm_atomic_get_old_plane_state(state, plane);
	struct drm_crtc *crtc = new_state->crtc ? new_state->crtc :
		(old_state ? old_state->crtc : NULL);
```

The `struct drm_crtc *crtc` declaration appears after the `if (!new_state) return;` statement. While C99/C11 allows this, mixing declarations and statements like this after control flow is poor style. `crtc` should be declared at the top of the function alongside the other variables, and assigned later.

**Questionable whether `new_state` can actually be NULL.** In the atomic commit path, `drm_atomic_get_new_plane_state` should always return a valid state for a plane that is part of the atomic update. If this function is being called, the plane was part of the commit. If the null dereference in the IGT test is real, it might indicate a deeper issue worth investigating rather than just papering over with null checks. That said, defensive programming here is not unreasonable as a crash fix.

**The `old_state` null handling is good.** Guarding `old_state->crtc` with `old_state ? old_state->crtc : NULL` is the right fix for the originally reported null dereference — on first enable there may be no old state, and the `?:` operator unconditionally dereferences `old_state`.

---
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

**Purpose:** Fix a null dereference when `old_state` is NULL in `vmw_cursor_plane_atomic_update`, found by IGT test `kms_cursor_legacy@torture-bo`.

**Issues:**

1. **Declaration-after-statement / mixed declarations and code (style):** The patch introduces variable declarations after statements, which violates the kernel coding style (and strict C89 rules still enforced by some kernel configs):

```c
new_state = drm_atomic_get_new_plane_state(state, plane);
if (!new_state)
    return;
old_state = drm_atomic_get_old_plane_state(state, plane);
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);
```

The declarations of `crtc` and `dev_priv` appear after executable statements (the `if` guards). While C99/C11 allows this and the kernel has moved to C11, placing declarations like `struct drm_crtc *crtc` and `struct vmw_private *dev_priv` after `if` statements with `return` is unusual in kernel code. It would be cleaner to declare `crtc` at the top with `new_state`/`old_state` and assign it after the null checks.

2. **Double space:** Minor nit — there's a double space before `new_state->crtc`:
```c
struct drm_crtc *crtc =  new_state->crtc ? new_state->crtc :
```

3. **Can `new_state` actually be NULL here?** In the atomic commit path, `drm_atomic_get_new_plane_state` should always return non-NULL for a plane that is in the commit. If the plane is not in the atomic state, it shouldn't be called at all. The commit message says this fixes `kms_cursor_legacy@torture-bo`, but it would be helpful to understand the exact call path that leads to `new_state` being NULL — this might indicate the real bug is elsewhere (the plane shouldn't be in an update callback if it's not part of the state).

4. **Missing Fixes tag specificity:** The Fixes tag references the refactor commit but no explanation is given as to why the prior code (which used the same `drm_atomic_get_old/new_plane_state` calls) didn't trigger this. A sentence about the calling context would help reviewers.

---
Generated by Claude Code Patch Reviewer

[PATCH v5 0/4] Fix some issues from igt runs.

[Maaz Mombasawala]

I ran igt-gpu-tools with vmwgfx and fixed some issues I found.

Changes in v5:
- Get display unit from cursor plane, so there is no need for
old_crtc.
- Cast ALIGN to u64.
- Check return value of ttm_bo_reserve().

Changes in v4:
- For ttm ref change, changed to incrementing the refcount on
the ttm base object of the dumb surface instead of holding a
ttm ref object so that it is not exposed to tfile.
- Added Fixes messages.
- Removed BO cleanup patch.

Changes in v3:
- Minor changes so series can apply cleanly on drm-misc-next.

Changes in v2:
- Changes based on Zack's comments.
- For vrefresh changes, removed stdu specific checks so that test
kms_invalid_mode@overflow-vrefresh passes with sou and ldu displays.
For ttm ref changes, fixed a ref leak in ttm_prime_fd_to_handle()
which fixes a memleak in test vmw_prime@tri-map-dmabuf and removed
release callback for dumb buffer surface since the gem buffer will
handle release of the surface.
Added commit to remove duplicate functions in vmwgfx_bo.h.

Maaz Mombasawala (4):
  drm/vmwgfx: Add some checks to vmw_cursor_plane_atomic_update
  drm/vmwgfx: Check vrefresh in drm_mode_setcrtc.
  drm/vmwgfx: Reserve ttm object before resv usage
  drm/vmwgfx: Change ttm refs for dumb buffers.

 drivers/gpu/drm/vmwgfx/ttm_object.c          |  7 +++--
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c           |  8 +++++-
 drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c | 16 +++++++----
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h          |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c          | 30 ++++++++++++++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h          |  9 ++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c         | 12 ++------
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c      | 14 ++++++++-
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c         |  8 ++++++
 9 files changed, 86 insertions(+), 19 deletions(-)

-- 
2.54.0

[PATCH v5 1/4] 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.
Get display unit from cursor plane instead to fix this and also do an
early return if there is no new_state since there is nothing to do in
that case.
This fixes igt test kms_cursor_legacy@torture-bo.

Fixes: 965544150d1c ("drm/vmwgfx: Refactor cursor handling")
Cc: Zack Rusin <zack.rusin@broadcom.com>
Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
index b010fc7ca68e..35e3fd2bbdad 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
@@ -22,6 +22,9 @@
 #define VMW_CURSOR_SNOOP_WIDTH 64
 #define VMW_CURSOR_SNOOP_HEIGHT 64
 
+#define vmw_cursor_to_du(cursor_plane) \
+	container_of(cursor_plane, struct vmw_display_unit, cursor.base)
+
 struct vmw_svga_fifo_cmd_define_cursor {
 	u32 cmd;
 	SVGAFifoCmdDefineAlphaCursor cursor;
@@ -742,13 +745,14 @@ vmw_cursor_plane_atomic_update(struct drm_plane *plane,
 			       struct drm_atomic_commit *state)
 {
 	struct vmw_bo *bo;
-	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 = new_state->crtc ?: old_state->crtc;
+	struct drm_plane_state *new_state;
+
+	new_state = drm_atomic_get_new_plane_state(state, plane);
+	if (!new_state)
+		return;
+
 	struct vmw_private *dev_priv = vmw_priv(plane->dev);
-	struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+	struct vmw_display_unit *du = vmw_cursor_to_du(plane);
 	struct vmw_plane_state *vps = vmw_plane_state_to_vps(new_state);
 	s32 hotspot_x, hotspot_y, cursor_x, cursor_y;
 
-- 
2.54.0

[PATCH v5 2/4] drm/vmwgfx: Check vrefresh in drm_mode_setcrtc.

[Maaz Mombasawala]

Add the mode_valid() callback in drm_mode_config_funcs to check for valid
mode during drm_mode_setcrtc.
Both the mode_valid() callbacks also check if the vrefresh value is
correct.
This fixes igt test kms_invalid_mode@overflow-vrefresh.

Signed-off-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c  | 30 ++++++++++++++++++++++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  |  9 +++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 12 +++--------
 3 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index a170e38c5223..11edb422d2ba 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1063,8 +1063,38 @@ vmw_kms_atomic_check_modeset(struct drm_device *dev,
 	return ret;
 }
 
+static enum drm_mode_status
+vmw_kms_config_mode_valid(struct drm_device *dev,
+			  const struct drm_display_mode *mode)
+{
+	enum drm_mode_status ret;
+	struct vmw_private *dev_priv = vmw_priv(dev);
+	u64 assumed_cpp = dev_priv->assume_16bpp ? 2 : 4;
+	/* Align width and height to account for GPU tile over-alignment */
+	u64 required_mem = 0;
+
+	ret = drm_mode_validate_size(mode, dev_priv->texture_max_width,
+				     dev_priv->texture_max_height);
+	if (ret != MODE_OK)
+		return ret;
+
+	required_mem = (u64)ALIGN(mode->hdisplay, GPU_TILE_SIZE) *
+		       ALIGN(mode->vdisplay, GPU_TILE_SIZE) *
+		       assumed_cpp;
+	required_mem = ALIGN(required_mem, PAGE_SIZE);
+
+	if (required_mem > dev_priv->max_primary_mem)
+		return MODE_MEM;
+
+	if (!drm_mode_vrefresh(mode))
+		return MODE_BAD;
+
+	return MODE_OK;
+}
+
 static const struct drm_mode_config_funcs vmw_kms_funcs = {
 	.fb_create = vmw_kms_fb_create,
+	.mode_valid = vmw_kms_config_mode_valid,
 	.atomic_check = vmw_kms_atomic_check_modeset,
 	.atomic_commit = drm_atomic_helper_commit,
 };
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
index 2224d7d91d1b..1b717caecc78 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
@@ -16,6 +16,15 @@
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_probe_helper.h>
 
+/*
+ * Some renderers such as llvmpipe will align the width and height of their
+ * buffers to match their tile size. We need to keep this in mind when exposing
+ * modes to userspace so that this possible over-allocation will not exceed
+ * graphics memory. 64x64 pixels seems to be a reasonable upper bound for the
+ * tile size of current renderers.
+ */
+#define GPU_TILE_SIZE 64
+
 /**
  * struct vmw_du_update_plane - Closure structure for vmw_du_helper_plane_update
  * @plane: Plane which is being updated.
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index 4139837f4caf..04c5c20588d8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -45,15 +45,6 @@
 #define vmw_connector_to_stdu(x) \
 	container_of(x, struct vmw_screen_target_display_unit, base.connector)
 
-/*
- * Some renderers such as llvmpipe will align the width and height of their
- * buffers to match their tile size. We need to keep this in mind when exposing
- * modes to userspace so that this possible over-allocation will not exceed
- * graphics memory. 64x64 pixels seems to be a reasonable upper bound for the
- * tile size of current renderers.
- */
-#define GPU_TILE_SIZE 64
-
 enum stdu_content_type {
 	SAME_AS_DISPLAY = 0,
 	SEPARATE_SURFACE,
@@ -870,6 +861,9 @@ vmw_stdu_connector_mode_valid(struct drm_connector *connector,
 	if (required_mem > dev_priv->max_mob_size)
 		return MODE_MEM;
 
+	if (!drm_mode_vrefresh(mode))
+		return MODE_BAD;
+
 	return MODE_OK;
 }
 
-- 
2.54.0

[PATCH v5 3/4] drm/vmwgfx: Reserve ttm object before resv usage

[Maaz Mombasawala]

We hit dma_resv_assert_held warnings in several places, reserve the ttm
base object when it's being used to avoid them.

Signed-off-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c   | 6 ++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 8 ++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index 1264a9ac4241..a69a6764ead2 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -52,10 +52,16 @@ static void vmw_bo_free(struct ttm_buffer_object *bo)
 		WARN_ON(vbo != res->guest_memory_bo);
 		WARN_ON(!res->guest_memory_bo);
 		if (res->guest_memory_bo) {
+			int ret = 0;
 			/* Reserve and switch the backing mob. */
 			mutex_lock(&res->dev_priv->cmdbuf_mutex);
 			(void)vmw_resource_reserve(res, false, true);
+			ret = ttm_bo_reserve(bo, false, false, NULL);
+			if (ret != 0)
+				drm_warn(&res->dev_priv->drm,
+					 "%s: failed reserve\n", __func__);
 			vmw_resource_mob_detach(res);
+			ttm_bo_unreserve(bo);
 			if (res->dirty)
 				res->func->dirty_free(res);
 			if (res->coherent)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
index 7b8163b5e501..d0c371df7e4c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
@@ -131,6 +131,7 @@ crc_generate_worker(struct work_struct *work)
 	spin_unlock_irq(&du->vkms.crc_state_lock);
 
 	if (surf) {
+		int ret;
 		if (vmw_surface_sync(vmw, surf)) {
 			drm_warn(
 				crtc->dev,
@@ -138,7 +139,14 @@ crc_generate_worker(struct work_struct *work)
 			return;
 		}
 
+		ret = ttm_bo_reserve(&surf->res.guest_memory_bo->tbo, false, false, NULL);
+		if (ret != 0) {
+			drm_warn(&vmw->drm, "%s: failed reserve\n", __func__);
+			goto done;
+		}
 		compute_crc(crtc, surf, &crc32);
+		ttm_bo_unreserve(&surf->res.guest_memory_bo->tbo);
+done:
 		vmw_surface_unreference(&surf);
 	}
 
-- 
2.54.0

[PATCH v5 4/4] drm/vmwgfx: Change ttm refs for dumb buffers.

[Maaz Mombasawala]

Preserve a ref to surface during dumb buffer creation. This keeps the dumb
buffer valid for framebuffer usage and fixes all igt tests that use dumb
buffers.
Also fix ttm_prime_fd_to_handle(), which in the error case was leaking a
dma_buf reference. During vmw_prime_fd_to_handle() this function
is expected to fail for dumb buffers since the fd is for a gem object,
the dma_buf would in turn hold a reference to the dumb buffer gem object
and cause a memory leak.

Fixes: f42c09e614f1 ("drm/vmwgfx: Fix dumb buffer leak")
Cc: Ian Forbes <ian.forbes@broadcom.com>
Cc: Zack Rusin <zack.rusin@broadcom.com>
Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/ttm_object.c     |  7 +++++--
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c      |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h     |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 14 +++++++++++++-
 4 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/ttm_object.c b/drivers/gpu/drm/vmwgfx/ttm_object.c
index 2421b0dd057c..f9042bafdc93 100644
--- a/drivers/gpu/drm/vmwgfx/ttm_object.c
+++ b/drivers/gpu/drm/vmwgfx/ttm_object.c
@@ -547,14 +547,17 @@ int ttm_prime_fd_to_handle(struct ttm_object_file *tfile,
 	if (IS_ERR(dma_buf))
 		return PTR_ERR(dma_buf);
 
-	if (dma_buf->ops != &tdev->ops)
-		return -ENOSYS;
+	if (dma_buf->ops != &tdev->ops) {
+		ret = -ENOSYS;
+		goto out;
+	}
 
 	prime = (struct ttm_prime_object *) dma_buf->priv;
 	base = &prime->base;
 	*handle = base->handle;
 	ret = ttm_ref_object_add(tfile, base, NULL, false);
 
+out:
 	dma_buf_put(dma_buf);
 
 	return ret;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index a69a6764ead2..3de255a619a8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -72,7 +72,7 @@ static void vmw_bo_free(struct ttm_buffer_object *bo)
 					       0);
 			mutex_unlock(&res->dev_priv->cmdbuf_mutex);
 		}
-		vmw_surface_unreference(&vbo->dumb_surface);
+		vmw_dumb_surface_unref(&vbo->dumb_surface);
 	}
 	WARN_ON(!RB_EMPTY_ROOT(&vbo->res_tree));
 	drm_gem_object_release(&vbo->tbo.base);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 812f1224f83e..cd46d3995ade 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -1185,6 +1185,7 @@ u32 vmw_lookup_surface_handle_for_buffer(struct vmw_private *vmw,
 int vmw_dumb_create(struct drm_file *file_priv,
 		    struct drm_device *dev,
 		    struct drm_mode_create_dumb *args);
+void vmw_dumb_surface_unref(struct vmw_surface **dumb_surface);
 
 /*
  * Shader management - vmwgfx_shader.c
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index ab611943d774..5a9c953eb73c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -2328,11 +2328,23 @@ int vmw_dumb_create(struct drm_file *file_priv,
 	struct vmw_user_surface *usurf = container_of(vbo->dumb_surface,
 						struct vmw_user_surface, srf);
 	usurf->prime.base.refcount_release = NULL;
+
+	ttm_base_object_lookup_for_ref(dev_priv->tdev, arg.rep.handle);
+
 err:
 	if (res)
 		vmw_resource_unreference(&res);
-
 	ttm_ref_object_base_unref(tfile, arg.rep.handle);
 
 	return ret;
 }
+
+void vmw_dumb_surface_unref(struct vmw_surface **dumb_surface)
+{
+	struct vmw_user_surface *usurf = container_of(*dumb_surface,
+						struct vmw_user_surface, srf);
+	struct ttm_base_object *base = &usurf->prime.base;
+
+	ttm_base_object_unref(&base);
+	vmw_surface_unreference(dumb_surface);
+}
-- 
2.54.0

Claude review: Fix some issues from igt runs.

[Claude Code Review Bot]

Overall Series Review

Subject: Fix some issues from igt runs.
Author: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
Patches: 5
Reviewed: 2026-06-04T13:30:06.941577

---

This is a 4-patch series from Maaz Mombasawala (Broadcom/VMware) fixing issues found by running igt-gpu-tools against the vmwgfx driver, now at v5. The series addresses a null deref in cursor updates, missing vrefresh validation, missing TTM reservation locks, and dumb buffer lifetime management.

**Patches 1 and 2** are clean and correct. **Patch 3** has a problematic error handling path in `vmw_bo_free` where a failed `ttm_bo_reserve` will still proceed to call `vmw_resource_mob_detach` (which asserts the lock is held) and `ttm_bo_unreserve` (on an unreserved BO). **Patch 4** has the most significant issue: the return value of `ttm_base_object_lookup_for_ref()` is not checked, which can lead to a refcount underflow in `vmw_dumb_surface_unref()` at cleanup time if the lookup fails. Additionally, Patch 3 is missing a Fixes tag even though it addresses specific dma_resv_assert_held warnings.

---

---
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

**Status: Looks good, minor style nit**

The fix correctly addresses a null dereference when `old_state` is not present. The root cause is that the old code did:
```c
struct drm_crtc *crtc = new_state->crtc ?: old_state->crtc;
struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
```
...which crashes if both are NULL or `old_state` itself is NULL.

The new approach of getting `du` via `container_of(plane, struct vmw_display_unit, cursor.base)` is correct — the cursor plane *is* `du->cursor.base`, so `container_of` is valid and avoids the dependency on any plane state. The early return on `!new_state` is also appropriate since there's nothing to update.

**Minor style nit**: The declarations of `dev_priv` and `du` appear after the `if (!new_state) return;` statement:
```c
new_state = drm_atomic_get_new_plane_state(state, plane);
if (!new_state)
    return;

struct vmw_private *dev_priv = vmw_priv(plane->dev);
struct vmw_display_unit *du = vmw_cursor_to_du(plane);
```
While this is valid C11 (which the kernel uses), it's a coding style preference issue. Some reviewers prefer all declarations before statements within a block. Not a correctness issue.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vmwgfx: Check vrefresh in drm_mode_setcrtc.

[Claude Code Review Bot]

Patch Review

**Status: Looks good, but missing Fixes/Cc stable tags**

The patch adds `drm_mode_config_funcs.mode_valid` to validate modes set via the legacy `drm_mode_setcrtc` path, which the existing per-connector `mode_valid` doesn't cover. The vrefresh check (`if (!drm_mode_vrefresh(mode)) return MODE_BAD`) is also added to the STDU connector's `vmw_stdu_connector_mode_valid`.

Good decisions in this patch:
- Moving `GPU_TILE_SIZE` from `vmwgfx_stdu.c` to `vmwgfx_kms.h` for reuse.
- Casting the `ALIGN` result to `u64` to avoid 32-bit overflow in `required_mem` calculation:
  ```c
  required_mem = (u64)ALIGN(mode->hdisplay, GPU_TILE_SIZE) *
                 ALIGN(mode->vdisplay, GPU_TILE_SIZE) *
                 assumed_cpp;
  ```

**Missing tags**: This patch fixes `kms_invalid_mode@overflow-vrefresh` but has no `Fixes:` tag or `Cc: stable@` annotation. If this is a regression from a specific commit, a Fixes tag would help with backporting. Even without a clear single culprit, a Cc: stable would be appropriate if this matters for older kernels.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vmwgfx: Reserve ttm object before resv usage

[Claude Code Review Bot]

Patch Review

**Status: Has issues in error handling**

The intent is correct — `vmw_resource_mob_detach()` calls `dma_resv_assert_held()`, so the buffer object's reservation must be held.

**Issue in `vmw_bo_free` (vmwgfx_bo.c)**: If `ttm_bo_reserve()` fails, the code warns but continues to call both `vmw_resource_mob_detach(res)` and `ttm_bo_unreserve(bo)`:
```c
ret = ttm_bo_reserve(bo, false, false, NULL);
if (ret != 0)
    drm_warn(&res->dev_priv->drm,
             "%s: failed reserve\n", __func__);
vmw_resource_mob_detach(res);   /* asserts lock held — will fire */
ttm_bo_unreserve(bo);           /* unreserves without holding reservation */
```

If the reserve fails:
1. `vmw_resource_mob_detach` will hit the `dma_resv_assert_held` assertion — the very thing this patch is meant to fix.
2. `ttm_bo_unreserve` on a non-reserved BO is undefined behavior (likely corrupts the ww_mutex state).

In practice, `ttm_bo_reserve(bo, false, false, NULL)` (non-interruptible, blocking) should essentially never fail, but since v5 explicitly added this check per review feedback, the error path should be correct. A simple approach would be to gate the unreserve:
```c
ret = ttm_bo_reserve(bo, false, false, NULL);
if (ret != 0)
    drm_warn(...);
vmw_resource_mob_detach(res);
if (ret == 0)
    ttm_bo_unreserve(bo);
```
Or skip the detach on failure if that's safe for the destroy path.

**The `crc_generate_worker` change (vmwgfx_vkms.c)** looks correct — the `goto done` properly skips `compute_crc` and falls through to `vmw_surface_unreference(&surf)`.

**Missing tags**: This patch is also missing `Fixes:` and `Cc: stable` tags despite fixing lockdep/dma_resv assertion warnings.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vmwgfx: Change ttm refs for dumb buffers.

[Claude Code Review Bot]

Patch Review

**Status: Has a significant unchecked return value**

**Critical issue — unchecked return value (vmwgfx_surface.c:2332)**:
```c
ttm_base_object_lookup_for_ref(dev_priv->tdev, arg.rep.handle);
```
`ttm_base_object_lookup_for_ref()` returns `struct ttm_base_object *` (or NULL on failure), but the return value is completely discarded. This function is called to acquire an extra reference on the base object that will later be dropped by `vmw_dumb_surface_unref → ttm_base_object_unref`. If the lookup fails and no reference is acquired, the subsequent `ttm_base_object_unref` in `vmw_dumb_surface_unref` will decrement a refcount that was never incremented, causing a refcount underflow and potential use-after-free.

The fix should check the return value:
```c
if (!ttm_base_object_lookup_for_ref(dev_priv->tdev, arg.rep.handle)) {
    ret = -EINVAL;
    goto err;
}
```

**The dma_buf leak fix in `ttm_prime_fd_to_handle` (ttm_object.c)** is correct and straightforward — the early return was missing a `dma_buf_put()`:
```c
// Before: leaked dma_buf ref on !tdev->ops
if (dma_buf->ops != &tdev->ops)
    return -ENOSYS;

// After: properly releases via goto
if (dma_buf->ops != &tdev->ops) {
    ret = -ENOSYS;
    goto out;
}
```

**The `vmw_dumb_surface_unref` function**: Setting `refcount_release = NULL` is safe because `ttm_release_base()` (ttm_object.c:241) checks for NULL before calling it. The function correctly pairs `ttm_base_object_unref` (drops the base object ref from `lookup_for_ref`) with `vmw_surface_unreference` (drops the resource ref). The ordering is safe since the base object destruction (with NULL release callback) only does IDR cleanup and tfile unref, not memory deallocation.

---
Generated by Claude Code Patch Reviewer