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

[Thomas Zimmermann]

Hi

Am 17.05.23 um 19:03 schrieb Thomas Hellström:
> On Wed, 2023-05-17 at 17:29 +0100, Matthew Auld wrote:
>> On 17/05/2023 17:05, Stanislaw Gruszka wrote:
>>> On Wed, May 17, 2023 at 04:22:38PM +0100, Matthew Auld wrote:
>>>> 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.
>>>
>>> Nice catch :-) we observed lockdep deadlock false alarms too, but I
>>> could
>>> not spot it out and were adding lockdep_set_class(key) to avoid
>>> those.
>>>
>>>
>>>> +static inline void __drmm_mutex_release(struct drm_device *dev,
>>>> void *res)
>>>
>>> Can this be inline if used in drmm_add_action_or_reset() ?
>>
>> I think so. Did I missing something here? It at least builds for me.
> 
> I think in each file that contains a drmm_mutex_init(), the code will
> need a pointer to the function __drmm_mutex_release() and the compiler
> will therefore need to emit a non-inlined static version of the
> function in that file. Not sure if that's a problem, though. If so
> could make it extern?

I'd prefer to export __drmm_mutex_release() from the module over the 
current static inline.

Best regards
Thomas

> 
> /Thomas
> 
> 
> 
>>
>>>
>>>
>>>> +{
>>>> +       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);           \
>>>> +})
>>>>              \
>>>
>>> Regards
>>> Stanislaw
>>>
>>>
> 

-- 
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/457c4983/attachment.sig>