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: Thu, 04 Jun 2026 13:30:07 +1000
Message-ID: <review-patch3-20260602004203.102901-4-maaz.mombasawala@broadcom.com>
In-Reply-To: <20260602004203.102901-4-maaz.mombasawala@broadcom.com>
References: <20260602004203.102901-1-maaz.mombasawala@broadcom.com> <20260602004203.102901-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

**Status: Has issues in error handling**

The intent is correct =E2=80=94 `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 `t=
tm_bo_unreserve(bo)`:
```c
ret =3D ttm_bo_reserve(bo, false, false, NULL);
if (ret !=3D 0)
    drm_warn(&res->dev_priv->drm,
             "%s: failed reserve\n", __func__);
vmw_resource_mob_detach(res);   /* asserts lock held =E2=80=94 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 =
=E2=80=94 the very thing this patch is meant to fix.
2. `ttm_bo_unreserve` on a non-reserved BO is undefined behavior (likely co=
rrupts the ww_mutex state).

In practice, `ttm_bo_reserve(bo, false, false, NULL)` (non-interruptible, b=
locking) should essentially never fail, but since v5 explicitly added this =
check per review feedback, the error path should be correct. A simple appro=
ach would be to gate the unreserve:
```c
ret =3D ttm_bo_reserve(bo, false, false, NULL);
if (ret !=3D 0)
    drm_warn(...);
vmw_resource_mob_detach(res);
if (ret =3D=3D 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 =E2=80=
=94 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