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

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

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