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

[Thomas Zimmermann]

Hi

Am 17.05.23 um 19:04 schrieb 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.

OK, thanks a lot for explaining.

Best regards
Thomas

> 
> 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
>>

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/intel-xe/attachments/20230517/cd5c88de/attachment.sig>