Re: [PATCH v5 1/7] drm: fix drmm_mutex_init()

[Matthew Auld <matthew.auld@xxxxxxxxx>]

On 17/05/2023 17:21, Thomas Zimmermann wrote:
> Hi
>
> Am 17.05.23 um 17:22 schrieb Matthew Auld:
> > In mutex_init() lockdep seems to identify a lock by defining a static
> > key for each lock class. However if we wrap the whole thing in a
> > function the static key will be the same for everything calling that
> > function, which looks to be the case for drmm_mutex_init(). This then
> > results in impossible lockdep splats since lockdep thinks completely
> > unrelated locks are the same lock class. The other issue is that when
> > looking at splats we lose the actual lock name, where instead of seeing
> > something like xe->mem_access.lock for the name, we just see something
> > generic like lock#8.
> >
> > Attempt to fix this by converting drmm_mutex_init() into a macro, which
> > should ensure that mutex_init() behaves as expected.
>
> If that's what is required, then OK. But even with your commit mesage, I 
> find it entirely non-obvious what the problem is. Isn't there a way to 
> annotate drmm_mutex_init() so that lockdep treats it like a regular 
> mutex_init()?

AFAICT the issue is that with the existing drmm_mutex_init() we 
basically end up generating:

int drmm_mutex_init(struct drm_device *dev, struct mutex *lock)
{
     static struct lock_class_key __key;

     __mutex_init((lock), "lock", &__key);
     ....
}

I think the special static __key is what lockdep uses to identify a lock 
class, so every time we call drmm_mutex_init() we should expect a 
different key. However since this is just a normal function the key will 
be created once and then all callers use the same key. For example, if 
you print mutex->depmap.key you will get the same pointer underneath for 
different drmm_mutex_init callers. And then ofc lockdep gets really 
confused.

Turning it into a macro ensures that each drmm_mutex_init() generates a 
different "static struct lock_class_key __key" for each invocation, 
which looks to be inline with what mutex_init() wants.

I'm not sure if there a better way to solve this...

> Best regards
> Thomas
>
> > Reported-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> > Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()")
> > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > Cc: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/drm_managed.c | 26 --------------------------
> >   include/drm/drm_managed.h     | 23 ++++++++++++++++++++++-
> >   2 files changed, 22 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_managed.c 
> > b/drivers/gpu/drm/drm_managed.c
> > index 4cf214de50c4..71c49819a7a2 100644
> > --- a/drivers/gpu/drm/drm_managed.c
> > +++ b/drivers/gpu/drm/drm_managed.c
> > @@ -263,29 +263,3 @@ void drmm_kfree(struct drm_device *dev, void *data)
> >       free_dr(dr_match);
> >   }
> >   EXPORT_SYMBOL(drmm_kfree);
> > -
> > -static void drmm_mutex_release(struct drm_device *dev, void *res)
> > -{
> > -    struct mutex *lock = res;
> > -
> > -    mutex_destroy(lock);
> > -}
> > -
> > -/**
> > - * drmm_mutex_init - &drm_device-managed mutex_init()
> > - * @dev: DRM device
> > - * @lock: lock to be initialized
> > - *
> > - * Returns:
> > - * 0 on success, or a negative errno code otherwise.
> > - *
> > - * This is a &drm_device-managed version of mutex_init(). The 
> > initialized
> > - * lock is automatically destroyed on the final drm_dev_put().
> > - */
> > -int drmm_mutex_init(struct drm_device *dev, struct mutex *lock)
> > -{
> > -    mutex_init(lock);
> > -
> > -    return drmm_add_action_or_reset(dev, drmm_mutex_release, lock);
> > -}
> > -EXPORT_SYMBOL(drmm_mutex_init);
> > diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
> > index 359883942612..01f977e91933 100644
> > --- a/include/drm/drm_managed.h
> > +++ b/include/drm/drm_managed.h
> > @@ -105,6 +105,27 @@ char *drmm_kstrdup(struct drm_device *dev, const 
> > char *s, gfp_t gfp);
> >   void drmm_kfree(struct drm_device *dev, void *data);
> > -int drmm_mutex_init(struct drm_device *dev, struct mutex *lock);
> > +static inline void __drmm_mutex_release(struct drm_device *dev, void 
> > *res)
> > +{
> > +    struct mutex *lock = res;
> > +
> > +    mutex_destroy(lock);
> > +}
> > +
> > +/**
> > + * drmm_mutex_init - &drm_device-managed mutex_init()
> > + * @dev: DRM device
> > + * @lock: lock to be initialized
> > + *
> > + * Returns:
> > + * 0 on success, or a negative errno code otherwise.
> > + *
> > + * This is a &drm_device-managed version of mutex_init(). The 
> > initialized
> > + * lock is automatically destroyed on the final drm_dev_put().
> > + */
> > +#define drmm_mutex_init(dev, lock) ({                         \
> > +    mutex_init(lock);                             \
> > +    drmm_add_action_or_reset(dev, __drmm_mutex_release, lock);         \
> > +})                                         \
> >   #endif