Re: [PATCH v2] drm: fix drmm_mutex_init()

[Matthew Auld <matthew.auld@xxxxxxxxx>]

On 22/05/2023 10:43, Thomas Zimmermann wrote:
> Hi
>
> Am 19.05.23 um 11:07 schrieb Matthew Auld:
> > In mutex_init() lockdep identifies a lock by defining a special static
> > key for each lock class. However if we wrap the macro in a function,
> > like in drmm_mutex_init(), we 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);
> >        ....
> > }
> >
> > The static __key here is what lockdep uses to identify the lock class,
> > however since this is just a normal function the key here will be
> > created once, where all callers then use the same key. In effect the
> > mutex->depmap.key will be the same pointer for different
> > drmm_mutex_init() callers. This then results in impossible lockdep
> > splats since lockdep thinks completely unrelated locks are the same lock
> > class.
> >
> > To fix this turn drmm_mutex_init() into a macro such that it generates a
> > different "static struct lock_class_key __key" for each invocation,
> > which looks to be inline with what mutex_init() wants.
> >
> > v2:
> >    - Revamp the commit message with clearer explanation of the issue.
> >    - Rather export __drmm_mutex_release() than static inline.
> >
> > Reported-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> > Reported-by: Sarah Walker <sarah.walker@xxxxxxxxxx>
> > Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()")
> > Cc: Stanislaw Gruszka <stanislaw.gruszka@xxxxxxxxxxxxxxx>
> > Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > 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>
>
> Acked-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
>
> Shall I add the patch to drm-misc-fixes?

Yes, please do. Thanks.

> Best regards
> Thomas
>
> > ---
> >   drivers/gpu/drm/drm_managed.c | 22 ++--------------------
> >   include/drm/drm_managed.h     | 18 +++++++++++++++++-
> >   2 files changed, 19 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_managed.c 
> > b/drivers/gpu/drm/drm_managed.c
> > index 4cf214de50c4..c21c3f623033 100644
> > --- a/drivers/gpu/drm/drm_managed.c
> > +++ b/drivers/gpu/drm/drm_managed.c
> > @@ -264,28 +264,10 @@ void drmm_kfree(struct drm_device *dev, void *data)
> >   }
> >   EXPORT_SYMBOL(drmm_kfree);
> > -static void drmm_mutex_release(struct drm_device *dev, void *res)
> > +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);
> > +EXPORT_SYMBOL(__drmm_mutex_release);
> > diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
> > index 359883942612..ad08f834af40 100644
> > --- a/include/drm/drm_managed.h
> > +++ b/include/drm/drm_managed.h
> > @@ -105,6 +105,22 @@ 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);
> > +void __drmm_mutex_release(struct drm_device *dev, void *res);
> > +
> > +/**
> > + * 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