From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/pagemap_util: Ensure proper cache lock management on free Date: Sat, 21 Feb 2026 05:51:04 +1000 Message-ID: In-Reply-To: <20260220190943.66961-2-jonathan.cavitt@intel.com> References: <20260220190943.66961-2-jonathan.cavitt@intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Overall Series Review Subject: drm/pagemap_util: Ensure proper cache lock management on free Author: Jonathan Cavitt Patches: 1 Reviewed: 2026-02-21T05:51:04.385592 --- This is a single-patch submission fixing a spinlock leak in `drm_pagemap_ca= che_fini()` in the new `drm_pagemap_util.c` infrastructure. The bug is real= : when `dpagemap` is non-NULL but `drm_pagemap_shrinker_cancel()` returns f= alse (meaning the shrinker scan already dequeued the dpagemap), the origina= l code falls through the `if` block without ever calling `spin_unlock(&cach= e->lock)`. This would cause a spinlock deadlock on any subsequent lock atte= mpt and trigger lockdep warnings. The fix is correct but uses an unusual control flow pattern =E2=80=94 placi= ng 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 nece= ssary for a two-path function. A straightforward alternative would be to si= mply add `spin_unlock(&cache->lock)` in the else branch without reusing the= `out:` label, or to restructure the function to unlock unconditionally bef= ore the conditional destroy. The commit message undersells the issue. It says the lock is "probably unne= cessary 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` warni= ngs. The path *is* reachable =E2=80=94 the shrinker scan can race with cach= e teardown, removing the dpagemap from the shrink list and making `drm_page= map_shrinker_cancel()` return false while `cache->dpagemap` is still non-NU= LL. --- Generated by Claude Code Patch Reviewer