[PATCH v2] drm: fix drmm_mutex_init()

[Thomas Zimmermann]

Am 22.05.23 um 11:50 schrieb Matthew Auld:
> 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 at linux.intel.com>
>>> Reported-by: Sarah Walker <sarah.walker at imgtec.com>
>>> Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()")
>>> Cc: Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com>
>>> Cc: Boris Brezillon <boris.brezillon at collabora.com>
>>> 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>
>>
>> Acked-by: Thomas Zimmermann <tzimmermann at suse.de>
>>
>> Shall I add the patch to drm-misc-fixes?
> 
> Yes, please do. Thanks.

Merged. Thanks for the patch.

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

-- 
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/dri-devel/attachments/20230522/80d0122f/attachment.sig>