| Message ID | 20230517152244.348171-1-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/7] drm: fix drmm_mutex_init() | expand |
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
Hi Am 17.05.23 um 17:22 schrieb Matthew Auld: > 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. If that's what is required, then OK. But even with your commit mesage, I find it entirely non-obvious what the problem is. Isn't there a way to annotate drmm_mutex_init() so that lockdep treats it like a regular mutex_init()? Best regards Thomas > > Reported-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()") > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Jocelyn Falempe <jfalempe@redhat.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > --- > drivers/gpu/drm/drm_managed.c | 26 -------------------------- > include/drm/drm_managed.h | 23 ++++++++++++++++++++++- > 2 files changed, 22 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c > index 4cf214de50c4..71c49819a7a2 100644 > --- a/drivers/gpu/drm/drm_managed.c > +++ b/drivers/gpu/drm/drm_managed.c > @@ -263,29 +263,3 @@ void drmm_kfree(struct drm_device *dev, void *data) > free_dr(dr_match); > } > EXPORT_SYMBOL(drmm_kfree); > - > -static 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); > diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h > index 359883942612..01f977e91933 100644 > --- a/include/drm/drm_managed.h > +++ b/include/drm/drm_managed.h > @@ -105,6 +105,27 @@ 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); > +static inline 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(). > + */ > +#define drmm_mutex_init(dev, lock) ({ \ > + mutex_init(lock); \ > + drmm_add_action_or_reset(dev, __drmm_mutex_release, lock); \ > +}) \ > > #endif
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 > >
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 > > > >
On 17/05/2023 17:21, Thomas Zimmermann wrote: > Hi > > Am 17.05.23 um 17:22 schrieb Matthew Auld: >> 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. > > If that's what is required, then OK. But even with your commit mesage, I > find it entirely non-obvious what the problem is. Isn't there a way to > annotate drmm_mutex_init() so that lockdep treats it like a regular > mutex_init()? AFAICT the issue is that with the existing drmm_mutex_init() we basically 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); .... } I think the special static __key is what lockdep uses to identify a lock class, so every time we call drmm_mutex_init() we should expect a different key. However since this is just a normal function the key will be created once and then all callers use the same key. For example, if you print mutex->depmap.key you will get the same pointer underneath for different drmm_mutex_init callers. And then ofc lockdep gets really confused. Turning it into a macro ensures that each drmm_mutex_init() generates a different "static struct lock_class_key __key" for each invocation, which looks to be inline with what mutex_init() wants. I'm not sure if there a better way to solve this... > > Best regards > Thomas > >> >> Reported-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()") >> Cc: Thomas Zimmermann <tzimmermann@suse.de> >> Cc: Jocelyn Falempe <jfalempe@redhat.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: dri-devel@lists.freedesktop.org >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> --- >> drivers/gpu/drm/drm_managed.c | 26 -------------------------- >> include/drm/drm_managed.h | 23 ++++++++++++++++++++++- >> 2 files changed, 22 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_managed.c >> b/drivers/gpu/drm/drm_managed.c >> index 4cf214de50c4..71c49819a7a2 100644 >> --- a/drivers/gpu/drm/drm_managed.c >> +++ b/drivers/gpu/drm/drm_managed.c >> @@ -263,29 +263,3 @@ void drmm_kfree(struct drm_device *dev, void *data) >> free_dr(dr_match); >> } >> EXPORT_SYMBOL(drmm_kfree); >> - >> -static 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); >> diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h >> index 359883942612..01f977e91933 100644 >> --- a/include/drm/drm_managed.h >> +++ b/include/drm/drm_managed.h >> @@ -105,6 +105,27 @@ 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); >> +static inline 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(). >> + */ >> +#define drmm_mutex_init(dev, lock) ({ \ >> + mutex_init(lock); \ >> + drmm_add_action_or_reset(dev, __drmm_mutex_release, lock); \ >> +}) \ >> #endif >
Hi Am 17.05.23 um 19:04 schrieb Matthew Auld: > On 17/05/2023 17:21, Thomas Zimmermann wrote: >> Hi >> >> Am 17.05.23 um 17:22 schrieb Matthew Auld: >>> 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. >> >> If that's what is required, then OK. But even with your commit mesage, >> I find it entirely non-obvious what the problem is. Isn't there a way >> to annotate drmm_mutex_init() so that lockdep treats it like a regular >> mutex_init()? > > AFAICT the issue is that with the existing drmm_mutex_init() we > basically 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); > .... > } > > I think the special static __key is what lockdep uses to identify a lock > class, so every time we call drmm_mutex_init() we should expect a > different key. However since this is just a normal function the key will > be created once and then all callers use the same key. For example, if > you print mutex->depmap.key you will get the same pointer underneath for > different drmm_mutex_init callers. And then ofc lockdep gets really > confused. > > Turning it into a macro ensures that each drmm_mutex_init() generates a > different "static struct lock_class_key __key" for each invocation, > which looks to be inline with what mutex_init() wants. OK, thanks a lot for explaining. Best regards Thomas > > I'm not sure if there a better way to solve this... > >> >> Best regards >> Thomas >> >>> >>> Reported-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>> Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()") >>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>> Cc: Jocelyn Falempe <jfalempe@redhat.com> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Cc: dri-devel@lists.freedesktop.org >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>> --- >>> drivers/gpu/drm/drm_managed.c | 26 -------------------------- >>> include/drm/drm_managed.h | 23 ++++++++++++++++++++++- >>> 2 files changed, 22 insertions(+), 27 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_managed.c >>> b/drivers/gpu/drm/drm_managed.c >>> index 4cf214de50c4..71c49819a7a2 100644 >>> --- a/drivers/gpu/drm/drm_managed.c >>> +++ b/drivers/gpu/drm/drm_managed.c >>> @@ -263,29 +263,3 @@ void drmm_kfree(struct drm_device *dev, void *data) >>> free_dr(dr_match); >>> } >>> EXPORT_SYMBOL(drmm_kfree); >>> - >>> -static 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); >>> diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h >>> index 359883942612..01f977e91933 100644 >>> --- a/include/drm/drm_managed.h >>> +++ b/include/drm/drm_managed.h >>> @@ -105,6 +105,27 @@ 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); >>> +static inline 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(). >>> + */ >>> +#define drmm_mutex_init(dev, lock) ({ \ >>> + mutex_init(lock); \ >>> + drmm_add_action_or_reset(dev, __drmm_mutex_release, >>> lock); \ >>> +}) \ >>> #endif >>
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 >>> >>> >
diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c index 4cf214de50c4..71c49819a7a2 100644 --- a/drivers/gpu/drm/drm_managed.c +++ b/drivers/gpu/drm/drm_managed.c @@ -263,29 +263,3 @@ void drmm_kfree(struct drm_device *dev, void *data) free_dr(dr_match); } EXPORT_SYMBOL(drmm_kfree); - -static 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); diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h index 359883942612..01f977e91933 100644 --- a/include/drm/drm_managed.h +++ b/include/drm/drm_managed.h @@ -105,6 +105,27 @@ 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); +static inline 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(). + */ +#define drmm_mutex_init(dev, lock) ({ \ + mutex_init(lock); \ + drmm_add_action_or_reset(dev, __drmm_mutex_release, lock); \ +}) \ #endif
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. Reported-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()") Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Jocelyn Falempe <jfalempe@redhat.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: dri-devel@lists.freedesktop.org Signed-off-by: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/drm_managed.c | 26 -------------------------- include/drm/drm_managed.h | 23 ++++++++++++++++++++++- 2 files changed, 22 insertions(+), 27 deletions(-)