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: Mon, 25 May 2026 19:32:33 +1000
Message-ID: <review-patch3-20260521223756.627311-4-maaz.mombasawala@broadcom.com>
In-Reply-To: <20260521223756.627311-4-maaz.mombasawala@broadcom.com>
References: <20260521223756.627311-1-maaz.mombasawala@broadcom.com> <20260521223756.627311-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

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

**Issues:**

1. **`ttm_bo_reserve` in `vmw_bo_free` =E2=80=94 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 typical=
ly held by something with a live reference. Since this is a teardown path a=
nd 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=3D=3D0=
), it could deadlock with `interruptible=3Dfalse, no_wait=3Dfalse`.

```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_re=
serve`) would be more appropriate here, or whether the callers already guar=
antee 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 =E2=80=94 `goto done` skips `compute_crc` but still un=
references:** The error handling in `crc_generate_worker` looks correct =E2=
=80=94 on reserve failure, it jumps to `done:` which calls `vmw_surface_unr=
eference`, which is the right cleanup. However, the blank line after `done:=
` is unnecessary:

```c
done:

	vmw_surface_unreference(&surf);
```

4. **VKMS reserve =E2=80=94 is `guest_memory_bo` guaranteed non-NULL?** The=
 code does:
```c
ret =3D 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 t=
he flow, `vmw_surface_sync` succeeded so the surface should have a valid ba=
cking BO, but a NULL check or assertion might be prudent.

---
Generated by Claude Code Patch Reviewer