RE: [PATCH v2] drm/pagemap_util: Ensure proper cache lock management on free

["Cavitt, Jonathan" <jonathan.cavitt@intel.com>]

-----Original Message-----
From: Brost, Matthew <matthew.brost@intel.com> 
Sent: Tuesday, February 24, 2026 3:12 PM
To: Cavitt, Jonathan <jonathan.cavitt@intel.com>
Cc: dri-devel@lists.freedesktop.org; Gupta, Saurabhg <saurabhg.gupta@intel.com>; Zuo, Alex <alex.zuo@intel.com>; thomas.hellstrom@linux.intel.com; maarten.lankhorst@linux.intel.com
Subject: Re: [PATCH v2] drm/pagemap_util: Ensure proper cache lock management on free
> 
> On Tue, Feb 24, 2026 at 03:48:33PM +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.
> > 
> > v2:
> > - Use requested code flow (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 | 13 ++++---------
> >  1 file changed, 4 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_pagemap_util.c b/drivers/gpu/drm/drm_pagemap_util.c
> > index 14ddb948a32e..66203a19f8f6 100644
> > --- a/drivers/gpu/drm/drm_pagemap_util.c
> > +++ b/drivers/gpu/drm/drm_pagemap_util.c
> > @@ -65,18 +65,13 @@ 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;
> > -	}
> > +	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);
> 
> The logic is different here as 'cache->dpagemap' never gets set to NULL
> under cache->lock when drm_pagemap_shrinker_cancel returns true.

I forgot to add this back in as a part of the v2 request, although in this case,
cache->dpagemap = NULL can just be assigned as such without checking the
return of drm_pagemap_shrinker_cancel.
I'll fix this for V3 and add the intel-xe list at that time.  Thank you.

-Jonathan Cavitt

> 
> Also be sure to include intel-xe list for CI too.
> 
> Matt
> 
> > -	}
> >  
> > -out:
> >  	mutex_destroy(&cache->lookup_mutex);
> >  	kfree(cache);
> >  }
> > -- 
> > 2.43.0
> > 
>