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: Make the GEM LRU lock part of drm_device Date: Sat, 16 May 2026 14:06:15 +1000 Message-ID: In-Reply-To: <20260512-panthor-shrinker-fixes-v3-1-3bf066259471@collabora.com> References: <20260512-panthor-shrinker-fixes-v3-1-3bf066259471@collabora.com> <20260512-panthor-shrinker-fixes-v3-1-3bf066259471@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 **Race fix (core change) =E2=80=94 correct:** The old `drm_gem_lru_remove()`: ```c struct drm_gem_lru *lru =3D obj->lru; if (!lru) return; mutex_lock(lru->lock); drm_gem_lru_remove_locked(obj); mutex_unlock(lru->lock); ``` reads `obj->lru` unlocked, then dereferences it for the lock pointer. The n= ew version correctly takes the device-level lock first, then checks `obj->l= ru` under the lock: ```c mutex_lock(&obj->dev->gem_lru_mutex); if (obj->lru) drm_gem_lru_remove_locked(obj); mutex_unlock(&obj->dev->gem_lru_mutex); ``` This is the right fix =E2=80=94 the lock is now always accessible without f= irst reading the mutable `obj->lru`. **`drm_gem_lru_scan()` signature change =E2=80=94 correct but note:** The new signature adds `struct drm_device *dev` as the first parameter: ```c drm_gem_lru_scan(struct drm_device *dev, struct drm_gem_lru *lru, ... ``` This is necessary because `still_in_lru` is a stack-local LRU with no assoc= iated objects, so `obj->dev` can't be used to get the lock before objects a= re dequeued. The `dev` parameter is the clean solution. All callers in MSM = correctly pass `priv->dev`. **`drm_gem_lru_init()` =E2=80=94 simplified correctly:** The lock pointer is removed from `struct drm_gem_lru` entirely, and `drm_ge= m_lru_init()` drops its `lock` parameter. All MSM callers (`msm_drv.c`) are= updated, and the `mutex_init(&priv->lru.lock)` is removed since the lock n= ow lives in `drm_device`. **`drm_device` changes =E2=80=94 correct:** The `gem_lru_mutex` is initialized in `drm_dev_init()` and destroyed in `dr= m_dev_init_release()`, following the same pattern as `filelist_mutex` and `= clientlist_mutex`. The field is added at the end of `struct drm_device` aft= er `debugfs_root`, which is fine. **MSM driver changes =E2=80=94 mechanical and correct:** All MSM files (`msm_gem.c`, `msm_gem_shrinker.c`, `msm_gem_submit.c`, `msm_= gem_vma.c`, `msm_ringbuffer.c`) switch from `priv->lru.lock` to `dev->gem_l= ru_mutex`. The `struct mutex lock` is removed from `struct msm_drm_private:= :lru`. The lockdep annotation in `msm_drv.c` correctly changes from `might_= lock(&priv->lru.lock)` to `might_lock(&ddev->gem_lru_mutex)`. **Minor observations (non-blocking):** 1. The `@lock` parameter documentation in `drm_gem_lru_init()` kdoc header = is stale after this patch =E2=80=94 the old doc says `@lock: The lock prote= cting the LRU` but the parameter is removed. The patch does remove it from = the signature but the diff context (line 180 of the mbox) still shows `@loc= k` in the doc comment context. Looking more carefully, the diff hunk starts= at the `@lock` line but doesn't delete it. However, since `@lock` is a par= ameter doc and the parameter is removed, the kernel doc build would warn. L= et me re-check... Actually, looking at the diff more carefully, the hunk at= line 179-188 shows the context line ` * @lock: The lock protecting the LRU= ` followed by the change to `drm_gem_lru_init`, so the `@lock` doc line is = retained as context. This would produce a `kernel-doc` warning since the pa= rameter no longer exists. This is minor and could be noted but is non-block= ing given the existing R-b tags. 2. The `drm_gem_lru_remove()` now unconditionally takes the mutex even if `= obj->lru` is NULL. This is a slight performance change compared to the old = early-return path, but it's necessary for correctness and the contention co= st is negligible in the NULL case (the lock is immediately released). 3. The panthor driver (`panthor_gem.c:207`) calls `drm_gem_lru_remove()` an= d will benefit from this fix without any code changes needed on its side, s= ince the API change is in the core `drm_gem_lru_init/scan` signatures, not = in `drm_gem_lru_remove`. **Overall:** Clean, well-explained patch that fixes a real race. The approa= ch of hoisting the lock to `drm_device` is the right call given that both c= urrent and upcoming users (MSM, panthor) use a single device-wide lock anyw= ay. The only nit is the stale `@lock` kdoc line for `drm_gem_lru_init`. --- Generated by Claude Code Patch Reviewer