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

[Matthew Auld]

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.

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