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

[Matthew Auld]

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 at linux.intel.com>
>> Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()")
>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>> Cc: Jocelyn Falempe <jfalempe at redhat.com>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Cc: dri-devel at lists.freedesktop.org
>> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>> ---
>>   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
>