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() ?
> +{
> + 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
