From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/vmwgfx: Reserve ttm object before resv usage Date: Sat, 16 May 2026 14:26:31 +1000 Message-ID: In-Reply-To: <20260512002752.2826685-4-maaz.mombasawala@broadcom.com> References: <20260512002752.2826685-1-maaz.mombasawala@broadcom.com> <20260512002752.2826685-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 fix is correct.** Looking at `vmw_resource_mob_detach`, it ca= lls `dma_resv_assert_held(gbo->tbo.base.resv)`, which requires the TTM BO r= eservation lock to be held. `vmw_resource_reserve` does NOT acquire the BO = reservation lock =E2=80=94 it only takes the resource off the LRU list and = optionally allocates a guest memory buffer. So the added `ttm_bo_reserve`/`= ttm_bo_unreserve` around `vmw_resource_mob_detach` in `vmw_bo_free` is nece= ssary: ```c (void)vmw_resource_reserve(res, false, true); + ttm_bo_reserve(bo, false, false, NULL); vmw_resource_mob_detach(res); + ttm_bo_unreserve(bo); ``` However, the return value of `ttm_bo_reserve` is not checked. It's called w= ith `interruptible=3Dfalse` and `no_wait=3Dfalse`, so it should only fail i= f the BO is being destroyed =E2=80=94 but since we're *in* the BO's free ca= llback, this is the BO being destroyed. This is a potential concern: can yo= u successfully reserve a BO that is in the process of being freed (refcount= =3D=3D 0, as asserted at line 45)? The `vmw_resource_release` path does `B= UG_ON(ret)` for the same call, so at minimum the error should not be silent= ly ignored. **vkms crc_generate_worker fix =E2=80=94 goto skips vmw_surface_unreference= .** The error path jumps to `done:` which falls through to `vmw_surface_unr= eference(&surf)`, so the surface reference is properly released. However, t= he blank line after `done:` is unnecessary style-wise: ```c +done: + vmw_surface_unreference(&surf); ``` **The reservation scope is wider than necessary in vkms.** The lock is held= across the entire `compute_crc` call. This is functionally correct (the BO= must be reserved during access), but worth noting if `compute_crc` is expe= nsive. --- Generated by Claude Code Patch Reviewer