From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/vmwgfx: Reserve ttm object before resv usage
Date: Sat, 16 May 2026 10:17:49 +1000
Message-ID: <review-patch3-20260514224819.3631763-4-maaz.mombasawala@broadcom.com>
In-Reply-To: <20260514224819.3631763-4-maaz.mombasawala@broadcom.com>
References: <20260514224819.3631763-1-maaz.mombasawala@broadcom.com> <20260514224819.3631763-4-maaz.mombasawala@broadcom.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**vmw_bo_free reservation concern:** The added `ttm_bo_reserve`/`ttm_bo_unr=
eserve` pair wraps `vmw_resource_mob_detach` in the buffer object destructo=
r:

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

This is inside `vmw_bo_free`, which is the TTM buffer object destruction ca=
llback. At this point the buffer object refcount is already 0 (confirmed by=
 the `WARN_ON(kref_read(&vbo->tbo.base.refcount) !=3D 0)` at the top of `vm=
w_bo_free`). Calling `ttm_bo_reserve` on an object with refcount 0 is poten=
tially problematic =E2=80=94 `ttm_bo_reserve` calls `__ttm_bo_reserve` whic=
h does `ww_mutex_lock` on the reservation object. If another thread is conc=
urrently trying to lock this (e.g., during eviction), the object could be i=
n an inconsistent state. The return value of `ttm_bo_reserve` is also not c=
hecked here (the `(void)` cast on `vmw_resource_reserve` above suggests ign=
oring errors is intentional for this path, but the reserve call itself coul=
d fail).

Compare with `vmw_resource_release` (vmwgfx_resource.c:129) which does `ttm=
_bo_reserve` but has `BUG_ON(ret)` =E2=80=94 at least there the BO has a va=
lid refcount.

Consider whether `dma_resv_assert_held` is the right thing to assert in `vm=
w_resource_mob_detach`, or whether the assertion should be conditionally re=
laxed for the destructor path.

**vkms CRC worker reservation =E2=80=94 reasonable fix:**

```c
+		ret =3D ttm_bo_reserve(&surf->res.guest_memory_bo->tbo, false, false, NU=
LL);
+		if (ret !=3D 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);
```

This is a valid fix since `compute_crc` calls `vmw_bo_map_and_cache`, which=
 likely needs the reservation held. However, looking at the `goto done` pat=
h:

```c
+done:
+
 		vmw_surface_unreference(&surf);
```

There's a blank line after the `done:` label =E2=80=94 minor style nit. Mor=
e importantly, when the reservation fails the CRC is silently skipped with =
only a warning. This is acceptable for a CRC worker (best-effort), but the =
CRC subsystem will get stale/missing CRC entries for the affected frames.

---
Generated by Claude Code Patch Reviewer