Claude review: drm/pagemap_util: Ensure proper cache lock management on free

[Claude Code Review Bot <claude-review@example.com>]

Overall Series Review

Subject: drm/pagemap_util: Ensure proper cache lock management on free
Author: Jonathan Cavitt <jonathan.cavitt@intel.com>
Patches: 1
Reviewed: 2026-02-21T05:51:04.385592

---

This is a single-patch submission fixing a spinlock leak in `drm_pagemap_cache_fini()` in the new `drm_pagemap_util.c` infrastructure. The bug is real: when `dpagemap` is non-NULL but `drm_pagemap_shrinker_cancel()` returns false (meaning the shrinker scan already dequeued the dpagemap), the original code falls through the `if` block without ever calling `spin_unlock(&cache->lock)`. This would cause a spinlock deadlock on any subsequent lock attempt and trigger lockdep warnings.

The fix is correct but uses an unusual control flow pattern — placing the `out:` label inside an `else` block and jumping into it with `goto`. While this is legal C, it results in code that is harder to read than necessary for a two-path function. A straightforward alternative would be to simply add `spin_unlock(&cache->lock)` in the else branch without reusing the `out:` label, or to restructure the function to unlock unconditionally before the conditional destroy.

The commit message undersells the issue. It says the lock is "probably unnecessary given the cache is being freed" and frames it as a consistency fix for static analysis. But the spinlock leak is a genuine bug: if the path is reached, `spin_unlock` is never called, and the subsequent `kfree(cache)` frees a locked spinlock, which will trigger `CONFIG_DEBUG_LOCK_ALLOC` warnings. The path *is* reachable — the shrinker scan can race with cache teardown, removing the dpagemap from the shrink list and making `drm_pagemap_shrinker_cancel()` return false while `cache->dpagemap` is still non-NULL.

---
Generated by Claude Code Patch Reviewer