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

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

Patch Review

**The Bug — missing `cache->dpagemap = NULL`:**

The original code sets `cache->dpagemap = NULL` inside the `drm_pagemap_shrinker_cancel` success path (line 74):

```c
if (drm_pagemap_shrinker_cancel(dpagemap)) {
    cache->dpagemap = NULL;   // <-- this is present in the original
    spin_unlock(&cache->lock);
    drm_pagemap_destroy(dpagemap, false);
}
```

The new code drops this assignment entirely:

```c
if (dpagemap && !drm_pagemap_shrinker_cancel(dpagemap))
    dpagemap = NULL;
spin_unlock(&cache->lock);

if (dpagemap)
    drm_pagemap_destroy(dpagemap, false);
```

The local variable `dpagemap` is set to NULL, but `cache->dpagemap` is never cleared. While the cache is about to be freed (the function proceeds to `kfree(cache)`), the original code was explicitly NULLing the field while still holding the spinlock, which prevents any concurrent accessor from seeing a stale pointer between the `spin_unlock` and the `kfree`. Since `drm_pagemap_get_from_cache_if_active()` does `spin_lock(&cache->lock); dpagemap = drm_pagemap_get_unless_zero(cache->dpagemap); spin_unlock(&cache->lock);`, there is a small window where a concurrent reader could grab `cache->dpagemap` after the lock is dropped but before `kfree(cache)`. The original code avoided this by setting `cache->dpagemap = NULL` under the lock. The new code should do:

```c
if (dpagemap && !drm_pagemap_shrinker_cancel(dpagemap))
    dpagemap = NULL;
cache->dpagemap = NULL;  /* clear before unlock */
spin_unlock(&cache->lock);
```

That said, this is a `devm` fini callback — at device teardown time, concurrent cache accesses would themselves be a bug. So in practice this may be benign, but for correctness under the stated goal of "consistency," the `cache->dpagemap = NULL` assignment should be preserved.

**The original bug being fixed:**

In the existing code, when `drm_pagemap_shrinker_cancel()` returns `false`, the lock is **never released** — control falls through without calling `spin_unlock`, then jumps to `out:` where `mutex_destroy` and `kfree` happen with the spinlock still held. This is a genuine bug (lock leak), so the patch is fixing a real issue, not just a static analysis aesthetic.

```c
// Original: if shrinker_cancel returns false, no unlock!
if (drm_pagemap_shrinker_cancel(dpagemap)) {
    cache->dpagemap = NULL;
    spin_unlock(&cache->lock);     // only unlocked here
    drm_pagemap_destroy(dpagemap, false);
}
// falls through to out: without unlock
```

The new code correctly ensures the lock is always released on a single path, which is good.

**Minor style note:** The negated condition `!drm_pagemap_shrinker_cancel(dpagemap)` makes the logic slightly harder to follow — "if dpagemap exists and cancel *failed*, then null out dpagemap" — but this matches the flow requested by Maarten per the v2 changelog, so it's a reasonable style trade-off.

**Summary:** The patch fixes a real spin_unlock omission bug but introduces a minor regression by dropping the `cache->dpagemap = NULL` assignment. That assignment should be added back (unconditionally, before the unlock) to maintain the original defensive clearing behavior.

---
Generated by Claude Code Patch Reviewer