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/gem: Stop exposing the racy/unsafe drm_gem_lru_remove() helper Date: Thu, 07 May 2026 13:31:56 +1000 Message-ID: In-Reply-To: <20260506-panthor-shrinker-fixes-v1-3-e7721526de96@collabora.com> References: <20260506-panthor-shrinker-fixes-v1-0-e7721526de96@collabora.com> <20260506-panthor-shrinker-fixes-v1-3-e7721526de96@collabora.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 This patch removes the public `drm_gem_lru_remove()` function entirely and = inlines a safe version in `drm_gem_object_release()`. The key insight: at `= drm_gem_object_release()` time, refcount is zero, meaning no other code pat= h can concurrently change `obj->lru`, so it's safe to read `obj->lru` witho= ut the LRU lock, then acquire the lock to remove. ```c + if (obj->lru) { + guard(mutex)(obj->lru->lock); + drm_gem_lru_remove_locked(obj); + } ``` The `guard(mutex)` usage is appropriate =E2=80=94 the scope-based locking w= ill release the mutex at the end of the enclosing block (the `if` block). The `drm_gem_lru_remove_locked()` static function is moved from its origina= l location (near the other LRU helpers) up to just before `drm_gem_object_r= elease()` so it's available at the call site. This is a code motion, not a = change. The removal of the declaration from `drm_gem.h`: ```c -void drm_gem_lru_remove(struct drm_gem_object *obj); ``` This is correct. After patch 1 removes the only panthor call site, and this= patch removes the function definition and the call in `drm_gem_object_rele= ase()`, there are no remaining callers. The cover letter notes MSM is the o= nly other user of `drm_gem_lru`, but MSM doesn't use `drm_gem_lru_remove()`= =E2=80=94 it uses `drm_gem_lru_move_tail()` to move objects between LRUs, = which is the safe pattern. **One observation about EXPORT_SYMBOL:** The patch removes the `EXPORT_SYMB= OL(drm_gem_lru_remove)` line along with the function. This is correct =E2= =80=94 no out-of-tree users should depend on this racy API. No issues found. Patch is correct and a good cleanup. **Summary:** All three patches are well-reasoned, correctly fix real races,= and the series ordering is logical. The Reviewed-by tags from the reporter= are present. This series looks good to merge. The only thing I'd flag is e= nsuring MSM is tested or at least audited that it doesn't call `drm_gem_lru= _remove()` anywhere =E2=80=94 which the cover letter already addresses by C= C'ing MSM maintainers. --- Generated by Claude Code Patch Reviewer