Re: [PATCH v3] drm/pagemap_util: Ensure proper cache lock management on free

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Fri, 2026-03-06 at 15:43 +0000, Jonathan Cavitt wrote:
> Static analysis issue:
> 
> Though probably unnecessary given the cache is being freed at this
> step,
> for the sake of consistency, ensure that the cache lock is always
> unlocked after drm_pagemap_cache_fini.
> 

Typically you don't start the commit message with the tool that found
it, like "fixing an error found by checkpatch.pl" or something similar,
but rather mention it at the bottom after explaining the fix and its
implications.

Also I'd skip the "Though probably unnecessary..." sentence. Spinlocks
typically disable preemption and if the code-path missing the unlock is
hit, preemption will remain disabled even if the lock is subsequently
freed.

Fix looks good, so with the commit message fixed,

Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>


> v2:
> - Use requested code flow (Maarten)
> 
> v3:
> - Clear cache->dpagemap (Matt Brost, Maarten)
> 
> Fixes: 77f14f2f2d73f ("drm/pagemap: Add a drm_pagemap cache and
> shrinker")
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Cc: Thomas Hellstrom <thomas.hellstrom@linux.intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_pagemap_util.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_pagemap_util.c
> b/drivers/gpu/drm/drm_pagemap_util.c
> index 14ddb948a32e..6111d90a38e2 100644
> --- a/drivers/gpu/drm/drm_pagemap_util.c
> +++ b/drivers/gpu/drm/drm_pagemap_util.c
> @@ -65,18 +65,14 @@ static void drm_pagemap_cache_fini(void *arg)
>  	drm_dbg(cache->shrinker->drm, "Destroying dpagemap
> cache.\n");
>  	spin_lock(&cache->lock);
>  	dpagemap = cache->dpagemap;
> -	if (!dpagemap) {
> -		spin_unlock(&cache->lock);
> -		goto out;
> -	}
> +	cache->dpagemap = NULL;
> +	if (dpagemap && !drm_pagemap_shrinker_cancel(dpagemap))
> +		dpagemap = NULL;
> +	spin_unlock(&cache->lock);
>  
> -	if (drm_pagemap_shrinker_cancel(dpagemap)) {
> -		cache->dpagemap = NULL;
> -		spin_unlock(&cache->lock);
> +	if (dpagemap)
>  		drm_pagemap_destroy(dpagemap, false);
> -	}
>  
> -out:
>  	mutex_destroy(&cache->lookup_mutex);
>  	kfree(cache);
>  }