Claude review: Fix some issues from igt runs.

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: 7
Reviewed: 2026-05-16T10:17:48.595176

---

This is a v3 series of 5 patches fixing issues discovered by running igt-gpu-tools against vmwgfx. The fixes address: a NULL dereference in cursor plane update, missing vrefresh validation, missing TTM reservation assertions, a dma_buf reference leak, dumb buffer TTM reference lifetime issues, and cleanup of duplicate helper functions.

The series is reasonable in scope and addresses real bugs. However, there are some coding style issues and one concern about the safety of the reservation approach in the bo destructor path.

---
Generated by Claude Code Patch Reviewer

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: 6
Reviewed: 2026-05-16T14:26:30.462768

---

This is a 5-patch series fixing issues found via igt-gpu-tools testing on vmwgfx. The fixes address: a null dereference in cursor plane updates, missing vrefresh validation, missing TTM BO reservation lock acquisition, dumb buffer reference count leaks, and removal of duplicate helper functions. The fixes are generally reasonable and address real bugs, but there are some coding style issues (patch 1) and a locking concern (patch 3) worth discussing.

---

---
Generated by Claude Code Patch Reviewer

[PATCH v4 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 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           |  4 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c | 16 +++++++----
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h          |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c          | 28 ++++++++++++++++++++
 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         | 12 +++++++++
 9 files changed, 85 insertions(+), 18 deletions(-)

-- 
2.54.0

[PATCH v4 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.
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.

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, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
index b010fc7ca68e..583115c94800 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
@@ -742,11 +742,17 @@ 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;
+	struct drm_plane_state *old_state;
+
+	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);
 	struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
 	struct vmw_plane_state *vps = vmw_plane_state_to_vps(new_state);
-- 
2.54.0

[PATCH v4 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  | 28 ++++++++++++++++++++++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  |  9 +++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 12 +++---------
 3 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 1b407b61f683..4db29b645a38 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1063,8 +1063,36 @@ 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 = ALIGN(mode->hdisplay, GPU_TILE_SIZE) *
+			   ALIGN(mode->vdisplay, GPU_TILE_SIZE) *
+			   assumed_cpp;
+	required_mem = ALIGN(required_mem, PAGE_SIZE);
+
+	ret = drm_mode_validate_size(mode, dev_priv->texture_max_width,
+				     dev_priv->texture_max_height);
+	if (ret != MODE_OK)
+		return ret;
+
+	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 v4 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   |  2 ++
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 12 ++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index 9c7a73c0b0dc..6817e0b8ce99 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -55,7 +55,9 @@ static void vmw_bo_free(struct ttm_buffer_object *bo)
 			/* Reserve and switch the backing mob. */
 			mutex_lock(&res->dev_priv->cmdbuf_mutex);
 			(void)vmw_resource_reserve(res, false, true);
+			ttm_bo_reserve(bo, false, false, NULL);
 			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..da6ccd5e318e 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,18 @@ 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 v4 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 6817e0b8ce99..d1ce1a58e134 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -68,7 +68,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 38bea8abab84..cda55ee009d2 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -1186,6 +1186,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 b2d3927b5567..34e55ad07ca9 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-05-25T19:32:32.822170

---

This is a 4-patch series from Maaz Mombasawala (Broadcom/VMware) fixing issues discovered while running IGT GPU test suite against vmwgfx. The patches address: a null-pointer dereference in cursor plane updates, missing vrefresh validation, missing TTM reservation assertions, and dumb buffer refcounting fixes.

The series is reasonable in scope and each patch addresses a real bug. However, there are notable code quality issues: **patch 1** has a C declaration-after-statement style violation, **patch 3** adds a TTM reservation in `vmw_bo_free` that may be called from a context where the reservation could deadlock, and **patch 4** has a critical issue with the return value of `ttm_base_object_lookup_for_ref()` being discarded (silently ignoring errors on the success path). The patches are generally on the right track but need refinement in several areas.

---

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

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

[Claude Code Review Bot]

Patch Review

**Purpose:** Add `mode_valid` callback to `drm_mode_config_funcs` to reject modes with zero vrefresh during `drm_mode_setcrtc`, and add vrefresh checks to STDU connector mode validation. Fixes IGT test `kms_invalid_mode@overflow-vrefresh`.

**Issues:**

1. **Config `mode_valid` checks `max_primary_mem` but connector checks different limits:** The new `vmw_kms_config_mode_valid()` validates against `dev_priv->max_primary_mem`, while the STDU connector's `vmw_stdu_connector_mode_valid()` also checks `max_mob_pages` and `max_mob_size` plus STDU-specific width/height limits. This is correct — the config-level check is necessarily more permissive since it's display-unit agnostic — but it's worth noting that the config-level check could pass modes that the connector level rejects.

2. **`GPU_TILE_SIZE` moved to header — good refactoring:** Moving the `GPU_TILE_SIZE` define from `vmwgfx_stdu.c` to `vmwgfx_kms.h` is the right thing since it's now used in both files.

3. **Missing Fixes tag:** Unlike patches 1 and 4, this patch has no `Fixes:` tag. The cover letter mentions adding Fixes messages, but this patch doesn't have one. If this is a regression fix, it should have one.

4. **Overall approach looks correct:** Using `drm_mode_config_funcs.mode_valid` is the correct hook for validating modes during `drm_mode_setcrtc` for all display types (SOU, LDU, STDU). The vrefresh check with `drm_mode_vrefresh()` returning 0 is a sensible rejection criterion.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Purpose:** Fix `dma_resv_assert_held` warnings by reserving the TTM buffer object before operations that require the reservation to be held.

**Issues:**

1. **`ttm_bo_reserve` in `vmw_bo_free` — potential deadlock concern:** In `vmw_bo_free`, the buffer object is being freed (refcount is 0, per the `WARN_ON` at line 45). Calling `ttm_bo_reserve(bo, false, false, NULL)` on a BO whose refcount is already 0 is unusual. The reservation is typically held by something with a live reference. Since this is a teardown path and no other thread should hold a reference, the reservation should succeed immediately, but this pattern is fragile. If anything else tries to acquire the reservation concurrently (which shouldn't happen given refcount==0), it could deadlock with `interruptible=false, no_wait=false`.

```c
ttm_bo_reserve(bo, false, false, NULL);
vmw_resource_mob_detach(res);
ttm_bo_unreserve(bo);
```

Consider whether `dma_resv_lock` directly (without going through `ttm_bo_reserve`) would be more appropriate here, or whether the callers already guarantee the reservation is held and the real fix should be elsewhere.

2. **Missing Fixes tag:** No `Fixes:` tag on this patch, despite addressing concrete warnings.

3. **VKMS CRC worker — `goto done` skips `compute_crc` but still unreferences:** The error handling in `crc_generate_worker` looks correct — on reserve failure, it jumps to `done:` which calls `vmw_surface_unreference`, which is the right cleanup. However, the blank line after `done:` is unnecessary:

```c
done:

	vmw_surface_unreference(&surf);
```

4. **VKMS reserve — is `guest_memory_bo` guaranteed non-NULL?** The code does:
```c
ret = ttm_bo_reserve(&surf->res.guest_memory_bo->tbo, false, false, NULL);
```
If `surf->res.guest_memory_bo` could be NULL, this would oops. Looking at the flow, `vmw_surface_sync` succeeded so the surface should have a valid backing BO, but a NULL check or assertion might be prudent.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Purpose:** Fix dumb buffer lifetime by taking an extra ttm_base_object reference during creation and releasing it properly on cleanup. Also fixes a dma_buf reference leak in `ttm_prime_fd_to_handle`.

**Issues:**

1. **Critical: Return value of `ttm_base_object_lookup_for_ref` is discarded on success path:**

```c
ttm_base_object_lookup_for_ref(dev_priv->tdev, arg.rep.handle);

err:
```

`ttm_base_object_lookup_for_ref()` returns a `struct ttm_base_object *` which is the refcount-incremented pointer, and this return value is completely discarded. The intent is clearly to increment the base object's refcount, but ignoring the return value means:
- If the lookup fails (returns NULL), the code silently proceeds without error handling.
- The code is relying on the side-effect of `kref_get_unless_zero` inside the function, but by convention the caller is expected to use the returned pointer and eventually call `ttm_base_object_unref` — ignoring the return value makes the intent unclear.

This should at minimum check the return value and handle the error case. Something like:
```c
if (!ttm_base_object_lookup_for_ref(dev_priv->tdev, arg.rep.handle)) {
    ret = -EINVAL;
    goto err;
}
```

2. **`ttm_prime_fd_to_handle` dma_buf leak fix — correct and straightforward:** The fix to add `dma_buf_put()` on the non-matching ops path is clearly correct. The old code returned `-ENOSYS` without calling `dma_buf_put(dma_buf)`, leaking the reference acquired by `dma_buf_get(fd)`.

```c
if (dma_buf->ops != &tdev->ops) {
    ret = -ENOSYS;
    goto out;
}
```

This is a clean and correct fix.

3. **`vmw_dumb_surface_unref` — double-free risk?** The function does:
```c
void vmw_dumb_surface_unref(struct vmw_surface **dumb_surface)
{
    struct ttm_base_object *base = &usurf->prime.base;
    ttm_base_object_unref(&base);
    vmw_surface_unreference(dumb_surface);
}
```

This relies on both the base object ref and the surface ref being in sync. If `refcount_release` is NULL (set earlier in `vmw_dumb_create`), then `ttm_base_object_unref` dropping to zero won't trigger any release callback. The `vmw_surface_unreference` then handles the surface cleanup. The ordering matters — `ttm_base_object_unref` must go first since `vmw_surface_unreference` may free the containing structure. This looks correct as written, since `vmw_surface_unreference` will only free when its own refcount drops to zero.

4. **Commit message says "fixes all igt tests that use dumb buffers" — that's a broad claim.** It would be better to list specific test names that were verified.

5. **The error-path `ttm_ref_object_base_unref` call ordering:** After the patch, on the success path the code does:
```c
ttm_base_object_lookup_for_ref(...)  // take base ref
err:
    vmw_resource_unreference(&res);   // drop resource ref
    ttm_ref_object_base_unref(tfile, arg.rep.handle);  // drop tfile ref
```

The `ttm_ref_object_base_unref` still runs on the success path. This means after `vmw_dumb_create` returns successfully, the only thing keeping the surface alive is the extra base object reference taken by `ttm_base_object_lookup_for_ref`. This is the design intent but should be documented with a comment.

---
Generated by Claude Code Patch Reviewer