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/pagemap: Unlock cache->lock before freeing it Date: Tue, 24 Feb 2026 09:36:36 +1000 Message-ID: In-Reply-To: <20260223222937.2008492-1-matthew.brost@intel.com> References: <20260223222937.2008492-1-matthew.brost@intel.com> <20260223222937.2008492-1-matthew.brost@intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review The function `drm_pagemap_cache_fini()` acquires `cache->lock`, checks whether `cache->dpagemap` is non-NULL, and if so, attempts `drm_pagemap_shrinker_cancel()`. The current code only unlocks in the true branch: > if (drm_pagemap_shrinker_cancel(dpagemap)) { > cache->dpagemap = NULL; > spin_unlock(&cache->lock); > drm_pagemap_destroy(dpagemap, false); >+ } else { >+ spin_unlock(&cache->lock); > } The fix is correct. Without the `else` branch, when `drm_pagemap_shrinker_cancel()` returns false, execution falls through to `kfree(cache)` with the spinlock still held. One minor observation: in the `else` case, `cache->dpagemap` is left pointing at the pagemap that the shrinker is concurrently destroying, while the `true` branch NULLs it out. Since `kfree(cache)` immediately follows, this doesn't matter in practice -- the field becomes inaccessible once the cache is freed. But there is a narrow window between `spin_unlock(&cache->lock)` and `kfree(cache)` where the shrinker scan path (`drm_pagemap_shrinker_scan`) could take `cache->lock` and access `cache->dpagemap`. This is a pre-existing design concern independent of this patch, so not something to block on here. The commit message statement "freeing a spinlock without unlocking it is fine" is slightly misleading -- it's fine in non-debug builds where spinlocks are no-ops or simple flags, but `CONFIG_DEBUG_SPINLOCK` and lockdep will complain. Still, the patch does what's needed. No other issues found. --- Generated by Claude Code Patch Reviewer