| Message ID | 20230519090733.489019-1-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm: fix drmm_mutex_init() | expand |
On Fri, 19 May 2023 10:07:33 +0100 Matthew Auld <matthew.auld@intel.com> wrote: > In mutex_init() lockdep identifies a lock by defining a special static > key for each lock class. However if we wrap the macro in a function, > like in drmm_mutex_init(), we 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); > .... > } > > The static __key here is what lockdep uses to identify the lock class, > however since this is just a normal function the key here will be > created once, where all callers then use the same key. In effect the > mutex->depmap.key will be the same pointer for different > drmm_mutex_init() callers. This then results in impossible lockdep > splats since lockdep thinks completely unrelated locks are the same lock > class. > > To fix this turn drmm_mutex_init() into a macro such that it generates a > different "static struct lock_class_key __key" for each invocation, > which looks to be inline with what mutex_init() wants. > > v2: > - Revamp the commit message with clearer explanation of the issue. > - Rather export __drmm_mutex_release() than static inline. > > Reported-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Reported-by: Sarah Walker <sarah.walker@imgtec.com> > Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()") > Cc: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > Cc: Boris Brezillon <boris.brezillon@collabora.com> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > 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 | 22 ++-------------------- > include/drm/drm_managed.h | 18 +++++++++++++++++- > 2 files changed, 19 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c > index 4cf214de50c4..c21c3f623033 100644 > --- a/drivers/gpu/drm/drm_managed.c > +++ b/drivers/gpu/drm/drm_managed.c > @@ -264,28 +264,10 @@ void drmm_kfree(struct drm_device *dev, void *data) > } > EXPORT_SYMBOL(drmm_kfree); > > -static void drmm_mutex_release(struct drm_device *dev, void *res) > +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); > +EXPORT_SYMBOL(__drmm_mutex_release); > diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h > index 359883942612..ad08f834af40 100644 > --- a/include/drm/drm_managed.h > +++ b/include/drm/drm_managed.h > @@ -105,6 +105,22 @@ 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); > +void __drmm_mutex_release(struct drm_device *dev, void *res); > + > +/** > + * 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 Fri, May 19, 2023 at 10:07:33AM +0100, Matthew Auld wrote: > In mutex_init() lockdep identifies a lock by defining a special static > key for each lock class. However if we wrap the macro in a function, > like in drmm_mutex_init(), we 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); > .... > } > > The static __key here is what lockdep uses to identify the lock class, > however since this is just a normal function the key here will be > created once, where all callers then use the same key. In effect the > mutex->depmap.key will be the same pointer for different > drmm_mutex_init() callers. This then results in impossible lockdep > splats since lockdep thinks completely unrelated locks are the same lock > class. > > To fix this turn drmm_mutex_init() into a macro such that it generates a > different "static struct lock_class_key __key" for each invocation, > which looks to be inline with what mutex_init() wants. > > v2: > - Revamp the commit message with clearer explanation of the issue. > - Rather export __drmm_mutex_release() than static inline. > > Reported-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Reported-by: Sarah Walker <sarah.walker@imgtec.com> > Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()") > Cc: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > Cc: Boris Brezillon <boris.brezillon@collabora.com> > 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> Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> Regards Stanislaw
On Fri, May 19, 2023 at 10:07:33AM +0100, Matthew Auld wrote: >In mutex_init() lockdep identifies a lock by defining a special static >key for each lock class. However if we wrap the macro in a function, >like in drmm_mutex_init(), we 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); > .... >} > >The static __key here is what lockdep uses to identify the lock class, >however since this is just a normal function the key here will be >created once, where all callers then use the same key. In effect the >mutex->depmap.key will be the same pointer for different >drmm_mutex_init() callers. This then results in impossible lockdep >splats since lockdep thinks completely unrelated locks are the same lock >class. > >To fix this turn drmm_mutex_init() into a macro such that it generates a >different "static struct lock_class_key __key" for each invocation, >which looks to be inline with what mutex_init() wants. > >v2: > - Revamp the commit message with clearer explanation of the issue. > - Rather export __drmm_mutex_release() than static inline. > >Reported-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >Reported-by: Sarah Walker <sarah.walker@imgtec.com> >Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()") >Cc: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> >Cc: Boris Brezillon <boris.brezillon@collabora.com> >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> that explains well some splats we've been seeing on xe. Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> thanks Lucas De Marchi >--- > drivers/gpu/drm/drm_managed.c | 22 ++-------------------- > include/drm/drm_managed.h | 18 +++++++++++++++++- > 2 files changed, 19 insertions(+), 21 deletions(-) > >diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c >index 4cf214de50c4..c21c3f623033 100644 >--- a/drivers/gpu/drm/drm_managed.c >+++ b/drivers/gpu/drm/drm_managed.c >@@ -264,28 +264,10 @@ void drmm_kfree(struct drm_device *dev, void *data) > } > EXPORT_SYMBOL(drmm_kfree); > >-static void drmm_mutex_release(struct drm_device *dev, void *res) >+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); >+EXPORT_SYMBOL(__drmm_mutex_release); >diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h >index 359883942612..ad08f834af40 100644 >--- a/include/drm/drm_managed.h >+++ b/include/drm/drm_managed.h >@@ -105,6 +105,22 @@ 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); >+void __drmm_mutex_release(struct drm_device *dev, void *res); >+ >+/** >+ * 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 >-- >2.40.1 >
Hi Am 19.05.23 um 11:07 schrieb Matthew Auld: > In mutex_init() lockdep identifies a lock by defining a special static > key for each lock class. However if we wrap the macro in a function, > like in drmm_mutex_init(), we 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); > .... > } > > The static __key here is what lockdep uses to identify the lock class, > however since this is just a normal function the key here will be > created once, where all callers then use the same key. In effect the > mutex->depmap.key will be the same pointer for different > drmm_mutex_init() callers. This then results in impossible lockdep > splats since lockdep thinks completely unrelated locks are the same lock > class. > > To fix this turn drmm_mutex_init() into a macro such that it generates a > different "static struct lock_class_key __key" for each invocation, > which looks to be inline with what mutex_init() wants. > > v2: > - Revamp the commit message with clearer explanation of the issue. > - Rather export __drmm_mutex_release() than static inline. > > Reported-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Reported-by: Sarah Walker <sarah.walker@imgtec.com> > Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()") > Cc: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > Cc: Boris Brezillon <boris.brezillon@collabora.com> > 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> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> Shall I add the patch to drm-misc-fixes? Best regards Thomas > --- > drivers/gpu/drm/drm_managed.c | 22 ++-------------------- > include/drm/drm_managed.h | 18 +++++++++++++++++- > 2 files changed, 19 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c > index 4cf214de50c4..c21c3f623033 100644 > --- a/drivers/gpu/drm/drm_managed.c > +++ b/drivers/gpu/drm/drm_managed.c > @@ -264,28 +264,10 @@ void drmm_kfree(struct drm_device *dev, void *data) > } > EXPORT_SYMBOL(drmm_kfree); > > -static void drmm_mutex_release(struct drm_device *dev, void *res) > +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); > +EXPORT_SYMBOL(__drmm_mutex_release); > diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h > index 359883942612..ad08f834af40 100644 > --- a/include/drm/drm_managed.h > +++ b/include/drm/drm_managed.h > @@ -105,6 +105,22 @@ 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); > +void __drmm_mutex_release(struct drm_device *dev, void *res); > + > +/** > + * 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 22/05/2023 10:43, Thomas Zimmermann wrote: > Hi > > Am 19.05.23 um 11:07 schrieb Matthew Auld: >> In mutex_init() lockdep identifies a lock by defining a special static >> key for each lock class. However if we wrap the macro in a function, >> like in drmm_mutex_init(), we 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); >> .... >> } >> >> The static __key here is what lockdep uses to identify the lock class, >> however since this is just a normal function the key here will be >> created once, where all callers then use the same key. In effect the >> mutex->depmap.key will be the same pointer for different >> drmm_mutex_init() callers. This then results in impossible lockdep >> splats since lockdep thinks completely unrelated locks are the same lock >> class. >> >> To fix this turn drmm_mutex_init() into a macro such that it generates a >> different "static struct lock_class_key __key" for each invocation, >> which looks to be inline with what mutex_init() wants. >> >> v2: >> - Revamp the commit message with clearer explanation of the issue. >> - Rather export __drmm_mutex_release() than static inline. >> >> Reported-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> Reported-by: Sarah Walker <sarah.walker@imgtec.com> >> Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()") >> Cc: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> >> Cc: Boris Brezillon <boris.brezillon@collabora.com> >> 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> > > Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > > Shall I add the patch to drm-misc-fixes? Yes, please do. Thanks. > > Best regards > Thomas > >> --- >> drivers/gpu/drm/drm_managed.c | 22 ++-------------------- >> include/drm/drm_managed.h | 18 +++++++++++++++++- >> 2 files changed, 19 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_managed.c >> b/drivers/gpu/drm/drm_managed.c >> index 4cf214de50c4..c21c3f623033 100644 >> --- a/drivers/gpu/drm/drm_managed.c >> +++ b/drivers/gpu/drm/drm_managed.c >> @@ -264,28 +264,10 @@ void drmm_kfree(struct drm_device *dev, void *data) >> } >> EXPORT_SYMBOL(drmm_kfree); >> -static void drmm_mutex_release(struct drm_device *dev, void *res) >> +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); >> +EXPORT_SYMBOL(__drmm_mutex_release); >> diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h >> index 359883942612..ad08f834af40 100644 >> --- a/include/drm/drm_managed.h >> +++ b/include/drm/drm_managed.h >> @@ -105,6 +105,22 @@ 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); >> +void __drmm_mutex_release(struct drm_device *dev, void *res); >> + >> +/** >> + * 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 >
Am 22.05.23 um 11:50 schrieb Matthew Auld: > On 22/05/2023 10:43, Thomas Zimmermann wrote: >> Hi >> >> Am 19.05.23 um 11:07 schrieb Matthew Auld: >>> In mutex_init() lockdep identifies a lock by defining a special static >>> key for each lock class. However if we wrap the macro in a function, >>> like in drmm_mutex_init(), we 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); >>> .... >>> } >>> >>> The static __key here is what lockdep uses to identify the lock class, >>> however since this is just a normal function the key here will be >>> created once, where all callers then use the same key. In effect the >>> mutex->depmap.key will be the same pointer for different >>> drmm_mutex_init() callers. This then results in impossible lockdep >>> splats since lockdep thinks completely unrelated locks are the same lock >>> class. >>> >>> To fix this turn drmm_mutex_init() into a macro such that it generates a >>> different "static struct lock_class_key __key" for each invocation, >>> which looks to be inline with what mutex_init() wants. >>> >>> v2: >>> - Revamp the commit message with clearer explanation of the issue. >>> - Rather export __drmm_mutex_release() than static inline. >>> >>> Reported-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>> Reported-by: Sarah Walker <sarah.walker@imgtec.com> >>> Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()") >>> Cc: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> >>> Cc: Boris Brezillon <boris.brezillon@collabora.com> >>> 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> >> >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> >> >> Shall I add the patch to drm-misc-fixes? > > Yes, please do. Thanks. Merged. Thanks for the patch. > >> >> Best regards >> Thomas >> >>> --- >>> drivers/gpu/drm/drm_managed.c | 22 ++-------------------- >>> include/drm/drm_managed.h | 18 +++++++++++++++++- >>> 2 files changed, 19 insertions(+), 21 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_managed.c >>> b/drivers/gpu/drm/drm_managed.c >>> index 4cf214de50c4..c21c3f623033 100644 >>> --- a/drivers/gpu/drm/drm_managed.c >>> +++ b/drivers/gpu/drm/drm_managed.c >>> @@ -264,28 +264,10 @@ void drmm_kfree(struct drm_device *dev, void >>> *data) >>> } >>> EXPORT_SYMBOL(drmm_kfree); >>> -static void drmm_mutex_release(struct drm_device *dev, void *res) >>> +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); >>> +EXPORT_SYMBOL(__drmm_mutex_release); >>> diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h >>> index 359883942612..ad08f834af40 100644 >>> --- a/include/drm/drm_managed.h >>> +++ b/include/drm/drm_managed.h >>> @@ -105,6 +105,22 @@ 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); >>> +void __drmm_mutex_release(struct drm_device *dev, void *res); >>> + >>> +/** >>> + * 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 >>
diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c index 4cf214de50c4..c21c3f623033 100644 --- a/drivers/gpu/drm/drm_managed.c +++ b/drivers/gpu/drm/drm_managed.c @@ -264,28 +264,10 @@ void drmm_kfree(struct drm_device *dev, void *data) } EXPORT_SYMBOL(drmm_kfree); -static void drmm_mutex_release(struct drm_device *dev, void *res) +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); +EXPORT_SYMBOL(__drmm_mutex_release); diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h index 359883942612..ad08f834af40 100644 --- a/include/drm/drm_managed.h +++ b/include/drm/drm_managed.h @@ -105,6 +105,22 @@ 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); +void __drmm_mutex_release(struct drm_device *dev, void *res); + +/** + * 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 identifies a lock by defining a special static key for each lock class. However if we wrap the macro in a function, like in drmm_mutex_init(), we 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); .... } The static __key here is what lockdep uses to identify the lock class, however since this is just a normal function the key here will be created once, where all callers then use the same key. In effect the mutex->depmap.key will be the same pointer for different drmm_mutex_init() callers. This then results in impossible lockdep splats since lockdep thinks completely unrelated locks are the same lock class. To fix this turn drmm_mutex_init() into a macro such that it generates a different "static struct lock_class_key __key" for each invocation, which looks to be inline with what mutex_init() wants. v2: - Revamp the commit message with clearer explanation of the issue. - Rather export __drmm_mutex_release() than static inline. Reported-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> Reported-by: Sarah Walker <sarah.walker@imgtec.com> Fixes: e13f13e039dc ("drm: Add DRM-managed mutex_init()") Cc: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> Cc: Boris Brezillon <boris.brezillon@collabora.com> 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 | 22 ++-------------------- include/drm/drm_managed.h | 18 +++++++++++++++++- 2 files changed, 19 insertions(+), 21 deletions(-)