[PATCH] drm/pagemap: Unlock cache->lock before freeing it

[PATCH] drm/pagemap: Unlock cache->lock before freeing it

[Matthew Brost]

From: Bart Van Assche <bvanassche@acm.org>

Although freeing a spinlock without unlocking it is fine, this confuses
static analyzers. Hence this patch.

Fixes: 77f14f2f2d73 ("drm/pagemap: Add a drm_pagemap cache and shrinker")
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/drm_pagemap_util.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_pagemap_util.c b/drivers/gpu/drm/drm_pagemap_util.c
index 14ddb948a32e..50cb5f9cdac5 100644
--- a/drivers/gpu/drm/drm_pagemap_util.c
+++ b/drivers/gpu/drm/drm_pagemap_util.c
@@ -74,6 +74,8 @@ static void drm_pagemap_cache_fini(void *arg)
 		cache->dpagemap = NULL;
 		spin_unlock(&cache->lock);
 		drm_pagemap_destroy(dpagemap, false);
+	} else {
+		spin_unlock(&cache->lock);
 	}
 
 out:
-- 
2.34.1

Claude review: drm/pagemap: Unlock cache->lock before freeing it

[Claude Code Review Bot]

Overall Series Review

Subject: drm/pagemap: Unlock cache->lock before freeing it
Author: Matthew Brost <matthew.brost@intel.com>
Patches: 1
Reviewed: 2026-02-24T09:36:36.206852

---

This is a single patch that adds a missing `spin_unlock(&cache->lock)` call in the `else` branch of `drm_pagemap_cache_fini()`. When `drm_pagemap_shrinker_cancel()` returns false (meaning the shrinker has already dequeued the pagemap), the code falls through to the `out:` label with the spinlock still held, and then proceeds to `kfree(cache)`, freeing the lock while still held.

The fix is straightforward and correct for what it claims to do. The commit message describes this as satisfying static analyzers, though it's worth noting that `CONFIG_DEBUG_SPINLOCK` / lockdep builds will also warn about freeing a held spinlock, so this is arguably a real (debug) bug, not just a static analysis false positive.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/pagemap: Unlock cache->lock before freeing it

[Claude Code Review Bot]

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