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