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

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

Patch Review

The original code has three paths through `drm_pagemap_cache_fini`:

1. `dpagemap` is NULL → unlock, goto out
2. `drm_pagemap_shrinker_cancel()` returns true → set NULL, unlock, destroy
3. `drm_pagemap_shrinker_cancel()` returns false → **no unlock** (bug)

The fix correctly ensures all three paths call `spin_unlock`. However:

> +	if (!dpagemap)
> 		goto out;
>
> 	if (drm_pagemap_shrinker_cancel(dpagemap)) {
> 		cache->dpagemap = NULL;
> 		spin_unlock(&cache->lock);
> 		drm_pagemap_destroy(dpagemap, false);
> +	} else {
> +out:
> +		spin_unlock(&cache->lock);
> 	}

Placing a goto label inside an `else` block is valid C but unusual enough to make readers do a double-take. The `out:` label is jumped to when `dpagemap` is NULL (skipping the `if/else` condition entirely), while the `else` clause handles the cancel-failure case — these are logically different situations that happen to share the same cleanup action. A slightly cleaner alternative would be:

```c
if (!dpagemap)
    goto out;

if (drm_pagemap_shrinker_cancel(dpagemap)) {
    cache->dpagemap = NULL;
    spin_unlock(&cache->lock);
    drm_pagemap_destroy(dpagemap, false);
    goto done;
}
out:
spin_unlock(&cache->lock);
done:
mutex_destroy(&cache->lookup_mutex);
kfree(cache);
```

That said, the current approach does produce the correct behavior for all three paths, so this is a readability preference, not a correctness issue.

There is also a question about the cancel-failure path: when `drm_pagemap_shrinker_cancel()` returns false, the code unlocks but does not set `cache->dpagemap = NULL` and does not call `drm_pagemap_destroy()`. This means the dpagemap is left dangling as the cache is immediately freed afterward. Looking at `drm_pagemap_shrinker_scan()`, when the shrinker races and wins, it sets `cache->dpagemap = NULL` and calls `drm_pagemap_destroy()` itself, so the dpagemap is handled by the shrinker. This is fine — the race is benign because by the time `drm_pagemap_cache_fini` sees `shrink_link` as empty, the shrinker has already taken ownership. However, there's also a window where `drm_pagemap_shrinker_scan` has called `list_del_init` on `shrink_link` but has not yet reached `cache->dpagemap = NULL`. In that narrow window, `drm_pagemap_cache_fini` would see `dpagemap` as non-NULL and `shrink_link` as empty, take the else/out path, and then `kfree(cache)`. Then the shrinker would proceed to `spin_lock(&cache->lock)` on freed memory. This race existed before this patch and is not introduced by it, but it may be worth noting to the author since the Fixes tag suggests this is meant to be a complete fix for the function.

The commit message says "Static analysis issue" and "Though probably unnecessary given the cache is being freed at this step." This is inaccurate — the spinlock leak is a real bug, not a cosmetic static analysis finding. Even though the cache is about to be freed, calling `kfree()` on a locked spinlock is wrong and will trigger `CONFIG_DEBUG_LOCK_ALLOC` warnings. The commit message should state the actual consequence more clearly.

---
Generated by Claude Code Patch Reviewer