| Message ID | 20240307024034.1548605-3-gnstark@salutedevices.com |
|---|---|
| State | New |
| Headers | show |
| Series | devm_led_classdev_register() usage problem | expand |
On Thu, Mar 07, 2024 at 05:40:26AM +0300, George Stark wrote: > Using of devm API leads to a certain order of releasing resources. > So all dependent resources which are not devm-wrapped should be deleted > with respect to devm-release order. Mutex is one of such objects that > often is bound to other resources and has no own devm wrapping. > Since mutex_destroy() actually does nothing in non-debug builds > frequently calling mutex_destroy() is just ignored which is safe for now > but wrong formally and can lead to a problem if mutex_destroy() will be > extended so introduce devm_mutex_init() > > Signed-off-by: George Stark <gnstark@salutedevices.com> > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot > to make this patch happen. > > include/linux/mutex.h | 13 +++++++++++++ > kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/include/linux/mutex.h b/include/linux/mutex.h > index f7611c092db7..9bcf72cb941a 100644 > --- a/include/linux/mutex.h > +++ b/include/linux/mutex.h > @@ -22,6 +22,8 @@ > #include <linux/cleanup.h> > #include <linux/mutex_types.h> > > +struct device; > + > #ifdef CONFIG_DEBUG_LOCK_ALLOC > # define __DEP_MAP_MUTEX_INITIALIZER(lockname) \ > , .dep_map = { \ > @@ -115,10 +117,21 @@ do { \ > > #ifdef CONFIG_DEBUG_MUTEXES > > +int devm_mutex_init(struct device *dev, struct mutex *lock); > void mutex_destroy(struct mutex *lock); > > #else > > +static inline int devm_mutex_init(struct device *dev, struct mutex *lock) > +{ > + /* > + * since mutex_destroy is nop actually there's no need to register it > + * in devm subsystem. > + */ > + mutex_init(lock); > + return 0; > +} > + > static inline void mutex_destroy(struct mutex *lock) {} > > #endif > diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c > index bc8abb8549d2..c9efab1a8026 100644 > --- a/kernel/locking/mutex-debug.c > +++ b/kernel/locking/mutex-debug.c > @@ -19,6 +19,7 @@ > #include <linux/kallsyms.h> > #include <linux/interrupt.h> > #include <linux/debug_locks.h> > +#include <linux/device.h> > > #include "mutex.h" > > @@ -104,3 +105,24 @@ void mutex_destroy(struct mutex *lock) > } > > EXPORT_SYMBOL_GPL(mutex_destroy); > + > +static void devm_mutex_release(void *res) > +{ > + mutex_destroy(res); > +} > + > +/** > + * devm_mutex_init - Resource-managed mutex initialization > + * @dev: Device which lifetime mutex is bound to > + * @lock: Pointer to a mutex > + * > + * Initialize mutex which is automatically destroyed when the driver is detached. > + * > + * Returns: 0 on success or a negative error code on failure. > + */ > +int devm_mutex_init(struct device *dev, struct mutex *lock) > +{ > + mutex_init(lock); > + return devm_add_action_or_reset(dev, devm_mutex_release, lock); > +} > +EXPORT_SYMBOL_GPL(devm_mutex_init); Hi George, look at https://lore.kernel.org/lkml/7013bf9e-2663-4613-ae61-61872e81355b@redhat.com/ where Matthew and Hans explain that devm_mutex_init needs to be a macro because of the static lockdep key. so this should be something like: static inline int __devm_mutex_init(struct device *dev, struct mutex *mutex, const char *name, struct lock_class_key *key) { __mutex_init(mutex, name, key); return devm_add_action_or_reset(dev, devm_mutex_release, mutex); } #define devm_mutex_init(dev, mutex) \ do { \ static struct lock_class_key __key; \ \ __devm_mutex_init(dev, (mutex), #mutex, &__key); \ } while (0); Marek
Le 07/03/2024 à 03:40, George Stark a écrit : > [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] > > Using of devm API leads to a certain order of releasing resources. > So all dependent resources which are not devm-wrapped should be deleted > with respect to devm-release order. Mutex is one of such objects that > often is bound to other resources and has no own devm wrapping. > Since mutex_destroy() actually does nothing in non-debug builds > frequently calling mutex_destroy() is just ignored which is safe for now > but wrong formally and can lead to a problem if mutex_destroy() will be > extended so introduce devm_mutex_init() > > Signed-off-by: George Stark <gnstark@salutedevices.com> > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot > to make this patch happen. Up to you, I sent a RFC patch based on yours with my ideas included because an exemple is easier than a lot of words for understanding, and my scripts automatically sets the Signed-off-by: but feel free to change it to Suggested-by: Christophe > > include/linux/mutex.h | 13 +++++++++++++ > kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/include/linux/mutex.h b/include/linux/mutex.h > index f7611c092db7..9bcf72cb941a 100644 > --- a/include/linux/mutex.h > +++ b/include/linux/mutex.h > @@ -22,6 +22,8 @@ > #include <linux/cleanup.h> > #include <linux/mutex_types.h> > > +struct device; > + > #ifdef CONFIG_DEBUG_LOCK_ALLOC > # define __DEP_MAP_MUTEX_INITIALIZER(lockname) \ > , .dep_map = { \ > @@ -115,10 +117,21 @@ do { \ > > #ifdef CONFIG_DEBUG_MUTEXES > > +int devm_mutex_init(struct device *dev, struct mutex *lock); > void mutex_destroy(struct mutex *lock); > > #else > > +static inline int devm_mutex_init(struct device *dev, struct mutex *lock) > +{ > + /* > + * since mutex_destroy is nop actually there's no need to register it > + * in devm subsystem. > + */ > + mutex_init(lock); > + return 0; > +} > + > static inline void mutex_destroy(struct mutex *lock) {} > > #endif > diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c > index bc8abb8549d2..c9efab1a8026 100644 > --- a/kernel/locking/mutex-debug.c > +++ b/kernel/locking/mutex-debug.c > @@ -19,6 +19,7 @@ > #include <linux/kallsyms.h> > #include <linux/interrupt.h> > #include <linux/debug_locks.h> > +#include <linux/device.h> > > #include "mutex.h" > > @@ -104,3 +105,24 @@ void mutex_destroy(struct mutex *lock) > } > > EXPORT_SYMBOL_GPL(mutex_destroy); > + > +static void devm_mutex_release(void *res) > +{ > + mutex_destroy(res); > +} > + > +/** > + * devm_mutex_init - Resource-managed mutex initialization > + * @dev: Device which lifetime mutex is bound to > + * @lock: Pointer to a mutex > + * > + * Initialize mutex which is automatically destroyed when the driver is detached. > + * > + * Returns: 0 on success or a negative error code on failure. > + */ > +int devm_mutex_init(struct device *dev, struct mutex *lock) > +{ > + mutex_init(lock); > + return devm_add_action_or_reset(dev, devm_mutex_release, lock); > +} > +EXPORT_SYMBOL_GPL(devm_mutex_init); > -- > 2.25.1 >
On Thu, 7 Mar 2024 08:39:46 -0500 Waiman Long <longman@redhat.com> wrote: > On 3/7/24 04:56, Marek Behún wrote: > > On Thu, Mar 07, 2024 at 05:40:26AM +0300, George Stark wrote: > >> Using of devm API leads to a certain order of releasing resources. > >> So all dependent resources which are not devm-wrapped should be deleted > >> with respect to devm-release order. Mutex is one of such objects that > >> often is bound to other resources and has no own devm wrapping. > >> Since mutex_destroy() actually does nothing in non-debug builds > >> frequently calling mutex_destroy() is just ignored which is safe for now > >> but wrong formally and can lead to a problem if mutex_destroy() will be > >> extended so introduce devm_mutex_init() > >> > >> Signed-off-by: George Stark <gnstark@salutedevices.com> > >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > >> --- > >> Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot > >> to make this patch happen. > >> > >> include/linux/mutex.h | 13 +++++++++++++ > >> kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++ > >> 2 files changed, 35 insertions(+) > >> > >> diff --git a/include/linux/mutex.h b/include/linux/mutex.h > >> index f7611c092db7..9bcf72cb941a 100644 > >> --- a/include/linux/mutex.h > >> +++ b/include/linux/mutex.h > >> @@ -22,6 +22,8 @@ > >> #include <linux/cleanup.h> > >> #include <linux/mutex_types.h> > >> > >> +struct device; > >> + > >> #ifdef CONFIG_DEBUG_LOCK_ALLOC > >> # define __DEP_MAP_MUTEX_INITIALIZER(lockname) \ > >> , .dep_map = { \ > >> @@ -115,10 +117,21 @@ do { \ > >> > >> #ifdef CONFIG_DEBUG_MUTEXES > >> > >> +int devm_mutex_init(struct device *dev, struct mutex *lock); > >> void mutex_destroy(struct mutex *lock); > >> > >> #else > >> > >> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock) > >> +{ > >> + /* > >> + * since mutex_destroy is nop actually there's no need to register it > >> + * in devm subsystem. > >> + */ > >> + mutex_init(lock); > >> + return 0; > >> +} > >> + > >> static inline void mutex_destroy(struct mutex *lock) {} > >> > >> #endif > >> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c > >> index bc8abb8549d2..c9efab1a8026 100644 > >> --- a/kernel/locking/mutex-debug.c > >> +++ b/kernel/locking/mutex-debug.c > >> @@ -19,6 +19,7 @@ > >> #include <linux/kallsyms.h> > >> #include <linux/interrupt.h> > >> #include <linux/debug_locks.h> > >> +#include <linux/device.h> > >> > >> #include "mutex.h" > >> > >> @@ -104,3 +105,24 @@ void mutex_destroy(struct mutex *lock) > >> } > >> > >> EXPORT_SYMBOL_GPL(mutex_destroy); > >> + > >> +static void devm_mutex_release(void *res) > >> +{ > >> + mutex_destroy(res); > >> +} > >> + > >> +/** > >> + * devm_mutex_init - Resource-managed mutex initialization > >> + * @dev: Device which lifetime mutex is bound to > >> + * @lock: Pointer to a mutex > >> + * > >> + * Initialize mutex which is automatically destroyed when the driver is detached. > >> + * > >> + * Returns: 0 on success or a negative error code on failure. > >> + */ > >> +int devm_mutex_init(struct device *dev, struct mutex *lock) > >> +{ > >> + mutex_init(lock); > >> + return devm_add_action_or_reset(dev, devm_mutex_release, lock); > >> +} > >> +EXPORT_SYMBOL_GPL(devm_mutex_init); > > Hi George, > > > > look at > > https://lore.kernel.org/lkml/7013bf9e-2663-4613-ae61-61872e81355b@redhat.com/ > > where Matthew and Hans explain that devm_mutex_init needs to be a macro > > because of the static lockdep key. > > > > so this should be something like: > > > > static inline int __devm_mutex_init(struct device *dev, struct mutex *mutex, > > const char *name, > > struct lock_class_key *key) > > { > > __mutex_init(mutex, name, key); > > return devm_add_action_or_reset(dev, devm_mutex_release, mutex); > > } > > > > #define devm_mutex_init(dev, mutex) \ > > do { \ > > static struct lock_class_key __key; \ > > \ > > __devm_mutex_init(dev, (mutex), #mutex, &__key); \ > > } while (0); > > > > > > Marek > > Making devm_mutex_init() a function will make all the devm_mutex share > the same lockdep key. Making it a macro will make each caller of > devm_mutex_init() have a distinct lockdep key. It all depends on whether > all the devm_mutexes have the same lock usage pattern or not and whether > it is possible for one devm_mutex to be nested inside another. So either > way can be fine depending on the mutex usage pattern. My suggestion is > to use a function, if possible, unless it will cause a false positive > lockdep splat as there is a limit on the maximum # of lockdep keys that > can be used. devm_mutex_init() should behave like other similar function initializing stuff with resource management. I.e. it should behave like mutex_init(), but with resource management. mutex_init() is a macro generating static lockdep key for each instance, so devm_mutex_init() should also generate static lockdep key for each instance. Marek
Hello Christophe On 3/7/24 16:50, Christophe Leroy wrote: > > > Le 07/03/2024 à 03:40, George Stark a écrit : >> [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] >> >> Using of devm API leads to a certain order of releasing resources. >> So all dependent resources which are not devm-wrapped should be deleted >> with respect to devm-release order. Mutex is one of such objects that >> often is bound to other resources and has no own devm wrapping. >> Since mutex_destroy() actually does nothing in non-debug builds >> frequently calling mutex_destroy() is just ignored which is safe for now >> but wrong formally and can lead to a problem if mutex_destroy() will be >> extended so introduce devm_mutex_init() >> >> Signed-off-by: George Stark <gnstark@salutedevices.com> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot >> to make this patch happen. > > Up to you, I sent a RFC patch based on yours with my ideas included > because an exemple is easier than a lot of words for understanding, and > my scripts automatically sets the Signed-off-by: but feel free to change > it to Suggested-by: Although we had close ideas for the final patch in v4 you encouraged me to do it in the right (=effective) way and go back from devm-helpers.h to mutex.h in the first place, reinforced the concept with appropriate examples from existing code, reviewed a lot. Thanks. Probably Suggested-by: is more suited here
Hello Waiman, Marek
Thanks for the review.
I've never used lockdep for debug but it seems preferable to
keep that feature working. It could be look like this:
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index f7611c092db7..574f6de6084d 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -22,6 +22,8 @@
#include <linux/cleanup.h>
#include <linux/mutex_types.h>
+struct device;
+
#ifdef CONFIG_DEBUG_LOCK_ALLOC
# define __DEP_MAP_MUTEX_INITIALIZER(lockname) \
, .dep_map = { \
@@ -115,10 +117,31 @@ do { \
#ifdef CONFIG_DEBUG_MUTEXES
+int debug_devm_mutex_init(struct device *dev, struct mutex *lock);
+
+#define devm_mutex_init(dev, mutex) \
+({ \
+ int ret; \
+ mutex_init(mutex); \
+ ret = debug_devm_mutex_init(dev, mutex); \
+ ret; \
+})
+
void mutex_destroy(struct mutex *lock);
#else
+/*
+* When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so
+* there's no really need to register it in devm subsystem.
+*/
+#define devm_mutex_init(dev, mutex) \
+({ \
+ typecheck(struct device *, dev); \
+ mutex_init(mutex); \
+ 0; \
+})
+
static inline void mutex_destroy(struct mutex *lock) {}
#endif
diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
index bc8abb8549d2..967a5367c79a 100644
--- a/kernel/locking/mutex-debug.c
+++ b/kernel/locking/mutex-debug.c
@@ -19,6 +19,7 @@
#include <linux/kallsyms.h>
#include <linux/interrupt.h>
#include <linux/debug_locks.h>
+#include <linux/device.h>
#include "mutex.h"
@@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const char
*name,
lock->magic = lock;
}
+static void devm_mutex_release(void *res)
+{
+ mutex_destroy(res);
+}
+
+int debug_devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+ return devm_add_action_or_reset(dev, devm_mutex_release, lock);
+}
+
/***
* mutex_destroy - mark a mutex unusable
* @lock: the mutex to be destroyed
Hello Andy On 3/7/24 13:34, Andy Shevchenko wrote: > On Thu, Mar 7, 2024 at 4:40 AM George Stark <gnstark@salutedevices.com> wrote: >> >> Using of devm API leads to a certain order of releasing resources. >> So all dependent resources which are not devm-wrapped should be deleted >> with respect to devm-release order. Mutex is one of such objects that >> often is bound to other resources and has no own devm wrapping. >> Since mutex_destroy() actually does nothing in non-debug builds >> frequently calling mutex_destroy() is just ignored which is safe for now >> but wrong formally and can lead to a problem if mutex_destroy() will be >> extended so introduce devm_mutex_init() >> >> Signed-off-by: George Stark <gnstark@salutedevices.com> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > >> Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot >> to make this patch happen. > > You also need to figure out who should be the author of the patch and > probably add a (missing) Co-developed-by. After all you should also > follow the correct order of SoBs. > Thanks for the review. I explained in the other letter as I see it. So I'd leave myself as author and add appropriate tag with Christophe's name. BTW what do you mean by correct SoB order? Is it alphabetical order or order of importance?
On 3/11/24 19:47, George Stark wrote: > Hello Waiman, Marek > > Thanks for the review. > > I've never used lockdep for debug but it seems preferable to > keep that feature working. It could be look like this: > > diff --git a/include/linux/mutex.h b/include/linux/mutex.h > index f7611c092db7..574f6de6084d 100644 > --- a/include/linux/mutex.h > +++ b/include/linux/mutex.h > @@ -22,6 +22,8 @@ > #include <linux/cleanup.h> > #include <linux/mutex_types.h> > > +struct device; > + > #ifdef CONFIG_DEBUG_LOCK_ALLOC > # define __DEP_MAP_MUTEX_INITIALIZER(lockname) \ > , .dep_map = { \ > @@ -115,10 +117,31 @@ do { \ > > #ifdef CONFIG_DEBUG_MUTEXES > > +int debug_devm_mutex_init(struct device *dev, struct mutex *lock); > + > +#define devm_mutex_init(dev, mutex) \ > +({ \ > + int ret; \ > + mutex_init(mutex); \ > + ret = debug_devm_mutex_init(dev, mutex); \ > + ret; \ > +}) The int ret variable is not needed. The macro can just end with debug_devm_mutex_init(). > + > void mutex_destroy(struct mutex *lock); > > #else > > +/* > +* When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so > +* there's no really need to register it in devm subsystem. "no really need"? > +*/ > +#define devm_mutex_init(dev, mutex) \ > +({ \ > + typecheck(struct device *, dev); \ > + mutex_init(mutex); \ > + 0; \ > +}) Do we need a typecheck() here? Compilation will fail with CONFIG_DEBUG_MUTEXES if dev is not a device pointer. > + > static inline void mutex_destroy(struct mutex *lock) {} > > #endif > diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c > index bc8abb8549d2..967a5367c79a 100644 > --- a/kernel/locking/mutex-debug.c > +++ b/kernel/locking/mutex-debug.c > @@ -19,6 +19,7 @@ > #include <linux/kallsyms.h> > #include <linux/interrupt.h> > #include <linux/debug_locks.h> > +#include <linux/device.h> > > #include "mutex.h" > > @@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const > char *name, > lock->magic = lock; > } > > +static void devm_mutex_release(void *res) > +{ > + mutex_destroy(res); > +} > + > +int debug_devm_mutex_init(struct device *dev, struct mutex *lock) > +{ > + return devm_add_action_or_reset(dev, devm_mutex_release, lock); > +} > + > /*** > * mutex_destroy - mark a mutex unusable > * @lock: the mutex to be destroyed
Le 12/03/2024 à 01:01, George Stark a écrit : > [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. > Découvrez pourquoi ceci est important à > https://aka.ms/LearnAboutSenderIdentification ] > > Hello Andy > > On 3/7/24 13:34, Andy Shevchenko wrote: >> On Thu, Mar 7, 2024 at 4:40 AM George Stark >> <gnstark@salutedevices.com> wrote: >>> >>> Using of devm API leads to a certain order of releasing resources. >>> So all dependent resources which are not devm-wrapped should be deleted >>> with respect to devm-release order. Mutex is one of such objects that >>> often is bound to other resources and has no own devm wrapping. >>> Since mutex_destroy() actually does nothing in non-debug builds >>> frequently calling mutex_destroy() is just ignored which is safe for now >>> but wrong formally and can lead to a problem if mutex_destroy() will be >>> extended so introduce devm_mutex_init() >>> >>> Signed-off-by: George Stark <gnstark@salutedevices.com> >>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> >>> Hello Christophe. Hope you don't mind I put you SoB tag because you >>> helped alot >>> to make this patch happen. >> >> You also need to figure out who should be the author of the patch and >> probably add a (missing) Co-developed-by. After all you should also >> follow the correct order of SoBs. >> > > Thanks for the review. > I explained in the other letter as I see it. So I'd leave myself > as author and add appropriate tag with Christophe's name. > BTW what do you mean by correct SoB order? > Is it alphabetical order or order of importance? > The correct order is to first have the Author's SoB.
Le 12/03/2024 à 02:10, Waiman Long a écrit : > On 3/11/24 19:47, George Stark wrote: >> Hello Waiman, Marek >> >> Thanks for the review. >> >> I've never used lockdep for debug but it seems preferable to >> keep that feature working. It could be look like this: >> >> diff --git a/include/linux/mutex.h b/include/linux/mutex.h >> index f7611c092db7..574f6de6084d 100644 >> --- a/include/linux/mutex.h >> +++ b/include/linux/mutex.h >> @@ -22,6 +22,8 @@ >> #include <linux/cleanup.h> >> #include <linux/mutex_types.h> >> >> +struct device; >> + >> #ifdef CONFIG_DEBUG_LOCK_ALLOC >> # define __DEP_MAP_MUTEX_INITIALIZER(lockname) \ >> , .dep_map = { \ >> @@ -115,10 +117,31 @@ do { \ >> >> #ifdef CONFIG_DEBUG_MUTEXES >> >> +int debug_devm_mutex_init(struct device *dev, struct mutex *lock); >> + >> +#define devm_mutex_init(dev, mutex) \ >> +({ \ >> + int ret; \ >> + mutex_init(mutex); \ >> + ret = debug_devm_mutex_init(dev, mutex); \ >> + ret; \ >> +}) > > The int ret variable is not needed. The macro can just end with > debug_devm_mutex_init(). > > >> + >> void mutex_destroy(struct mutex *lock); >> >> #else >> >> +/* >> +* When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so >> +* there's no really need to register it in devm subsystem. > "no really need"? >> +*/ >> +#define devm_mutex_init(dev, mutex) \ >> +({ \ >> + typecheck(struct device *, dev); \ >> + mutex_init(mutex); \ >> + 0; \ >> +}) > > Do we need a typecheck() here? Compilation will fail with > CONFIG_DEBUG_MUTEXES if dev is not a device pointer. I guess the idea is to have it fail _also_ when CONFIG_DEBUG_MUTEXES is not selected, in order to discover errors as soon as possible. > > >> + >> static inline void mutex_destroy(struct mutex *lock) {} >> >> #endif >> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c >> index bc8abb8549d2..967a5367c79a 100644 >> --- a/kernel/locking/mutex-debug.c >> +++ b/kernel/locking/mutex-debug.c >> @@ -19,6 +19,7 @@ >> #include <linux/kallsyms.h> >> #include <linux/interrupt.h> >> #include <linux/debug_locks.h> >> +#include <linux/device.h> >> >> #include "mutex.h" >> >> @@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const >> char *name, >> lock->magic = lock; >> } >> >> +static void devm_mutex_release(void *res) >> +{ >> + mutex_destroy(res); >> +} >> + >> +int debug_devm_mutex_init(struct device *dev, struct mutex *lock) >> +{ >> + return devm_add_action_or_reset(dev, devm_mutex_release, lock); >> +} >> + >> /*** >> * mutex_destroy - mark a mutex unusable >> * @lock: the mutex to be destroyed >
Le 12/03/2024 à 00:47, George Stark a écrit : > [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. > Découvrez pourquoi ceci est important à > https://aka.ms/LearnAboutSenderIdentification ] > > Hello Waiman, Marek > > Thanks for the review. > > I've never used lockdep for debug but it seems preferable to > keep that feature working. It could be look like this: For sure it is a must. I'm not used to it either hence my overlook. > > > diff --git a/include/linux/mutex.h b/include/linux/mutex.h > index f7611c092db7..574f6de6084d 100644 > --- a/include/linux/mutex.h > +++ b/include/linux/mutex.h > @@ -22,6 +22,8 @@ > #include <linux/cleanup.h> > #include <linux/mutex_types.h> > > +struct device; > + > #ifdef CONFIG_DEBUG_LOCK_ALLOC > # define __DEP_MAP_MUTEX_INITIALIZER(lockname) \ > , .dep_map = { \ > @@ -115,10 +117,31 @@ do > { \ > > #ifdef CONFIG_DEBUG_MUTEXES > > +int debug_devm_mutex_init(struct device *dev, struct mutex *lock); > + > +#define devm_mutex_init(dev, mutex) \ > +({ \ > + int ret; \ > + mutex_init(mutex); \ > + ret = debug_devm_mutex_init(dev, mutex); \ > + ret; \ > +}) > + I think it would be preferable to minimise the number of macros. If I were you I would keep your devm_mutex_init() as is but rename it __devm_mutex_init() and just remove the mutex_init() from it, then add only one macro that works independant of CONFIG_DEBUG_MUTEXES: #define devm_mutex_init(dev, mutex) \ ({ \ mutex_init(mutex); \ __devm_mutex_init(dev, mutex); \ }) With that, no need of a second version of the macro and no need for the typecheck either. Note the __ which is a clear indication that allthough that function is declared in public mutex.h, it is not meant to be used outside of it. > void mutex_destroy(struct mutex *lock); > > #else > > +/* > +* When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so > +* there's no really need to register it in devm subsystem. > +*/ > +#define devm_mutex_init(dev, mutex) \ > +({ \ > + typecheck(struct device *, dev); \ > + mutex_init(mutex); \ > + 0; \ > +}) > + > static inline void mutex_destroy(struct mutex *lock) {} > > #endif > diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c > index bc8abb8549d2..967a5367c79a 100644 > --- a/kernel/locking/mutex-debug.c > +++ b/kernel/locking/mutex-debug.c > @@ -19,6 +19,7 @@ > #include <linux/kallsyms.h> > #include <linux/interrupt.h> > #include <linux/debug_locks.h> > +#include <linux/device.h> > > #include "mutex.h" > > @@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const char > *name, > lock->magic = lock; > } > > +static void devm_mutex_release(void *res) > +{ > + mutex_destroy(res); > +} > + > +int debug_devm_mutex_init(struct device *dev, struct mutex *lock) > +{ > + return devm_add_action_or_reset(dev, devm_mutex_release, lock); > +} > + > /*** > * mutex_destroy - mark a mutex unusable > * @lock: the mutex to be destroyed > -- > 2.25.1 > > > > And now I would drop the the refactoring patch with moving down > mutex_destroy. devm block is big enough to be declared standalone. > > > On 3/7/24 19:44, Marek Behún wrote: >> On Thu, 7 Mar 2024 08:39:46 -0500 >> Waiman Long <longman@redhat.com> wrote: >> >>> On 3/7/24 04:56, Marek Behún wrote: >>>> On Thu, Mar 07, 2024 at 05:40:26AM +0300, George Stark wrote: >>>>> Using of devm API leads to a certain order of releasing resources. >>>>> So all dependent resources which are not devm-wrapped should be >>>>> deleted >>>>> with respect to devm-release order. Mutex is one of such objects that >>>>> often is bound to other resources and has no own devm wrapping. >>>>> Since mutex_destroy() actually does nothing in non-debug builds >>>>> frequently calling mutex_destroy() is just ignored which is safe >>>>> for now >>>>> but wrong formally and can lead to a problem if mutex_destroy() >>>>> will be >>>>> extended so introduce devm_mutex_init() >>>>> >>>>> Signed-off-by: George Stark <gnstark@salutedevices.com> >>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >>>>> --- >>>>> Hello Christophe. Hope you don't mind I put you SoB tag because >>>>> you helped alot >>>>> to make this patch happen. >>>>> >>>>> include/linux/mutex.h | 13 +++++++++++++ >>>>> kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++ >>>>> 2 files changed, 35 insertions(+) >>>>> >>>>> diff --git a/include/linux/mutex.h b/include/linux/mutex.h >>>>> index f7611c092db7..9bcf72cb941a 100644 >>>>> --- a/include/linux/mutex.h >>>>> +++ b/include/linux/mutex.h >>>>> @@ -22,6 +22,8 @@ >>>>> #include <linux/cleanup.h> >>>>> #include <linux/mutex_types.h> >>>>> >>>>> +struct device; >>>>> + >>>>> #ifdef CONFIG_DEBUG_LOCK_ALLOC >>>>> # define __DEP_MAP_MUTEX_INITIALIZER(lockname) \ >>>>> , .dep_map = { \ >>>>> @@ -115,10 +117,21 @@ do >>>>> { \ >>>>> >>>>> #ifdef CONFIG_DEBUG_MUTEXES >>>>> >>>>> +int devm_mutex_init(struct device *dev, struct mutex *lock); >>>>> void mutex_destroy(struct mutex *lock); >>>>> >>>>> #else >>>>> >>>>> +static inline int devm_mutex_init(struct device *dev, struct mutex >>>>> *lock) >>>>> +{ >>>>> + /* >>>>> + * since mutex_destroy is nop actually there's no need to >>>>> register it >>>>> + * in devm subsystem. >>>>> + */ >>>>> + mutex_init(lock); >>>>> + return 0; >>>>> +} >>>>> + >>>>> static inline void mutex_destroy(struct mutex *lock) {} >>>>> >>>>> #endif >>>>> diff --git a/kernel/locking/mutex-debug.c >>>>> b/kernel/locking/mutex-debug.c >>>>> index bc8abb8549d2..c9efab1a8026 100644 >>>>> --- a/kernel/locking/mutex-debug.c >>>>> +++ b/kernel/locking/mutex-debug.c >>>>> @@ -19,6 +19,7 @@ >>>>> #include <linux/kallsyms.h> >>>>> #include <linux/interrupt.h> >>>>> #include <linux/debug_locks.h> >>>>> +#include <linux/device.h> >>>>> >>>>> #include "mutex.h" >>>>> >>>>> @@ -104,3 +105,24 @@ void mutex_destroy(struct mutex *lock) >>>>> } >>>>> >>>>> EXPORT_SYMBOL_GPL(mutex_destroy); >>>>> + >>>>> +static void devm_mutex_release(void *res) >>>>> +{ >>>>> + mutex_destroy(res); >>>>> +} >>>>> + >>>>> +/** >>>>> + * devm_mutex_init - Resource-managed mutex initialization >>>>> + * @dev: Device which lifetime mutex is bound to >>>>> + * @lock: Pointer to a mutex >>>>> + * >>>>> + * Initialize mutex which is automatically destroyed when the >>>>> driver is detached. >>>>> + * >>>>> + * Returns: 0 on success or a negative error code on failure. >>>>> + */ >>>>> +int devm_mutex_init(struct device *dev, struct mutex *lock) >>>>> +{ >>>>> + mutex_init(lock); >>>>> + return devm_add_action_or_reset(dev, devm_mutex_release, lock); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(devm_mutex_init); >>>> Hi George, >>>> >>>> look at >>>> https://lore.kernel.org/lkml/7013bf9e-2663-4613-ae61-61872e81355b@redhat.com/ >>>> where Matthew and Hans explain that devm_mutex_init needs to be a macro >>>> because of the static lockdep key. >>>> >>>> so this should be something like: >>>> >>>> static inline int __devm_mutex_init(struct device *dev, struct mutex >>>> *mutex, >>>> const char *name, >>>> struct lock_class_key *key) >>>> { >>>> __mutex_init(mutex, name, key); >>>> return devm_add_action_or_reset(dev, devm_mutex_release, mutex); >>>> } >>>> >>>> #define devm_mutex_init(dev, mutex) \ >>>> do { \ >>>> static struct lock_class_key __key; \ >>>> \ >>>> __devm_mutex_init(dev, (mutex), #mutex, &__key); \ >>>> } while (0); >>>> >>>> >>>> Marek >>> >>> Making devm_mutex_init() a function will make all the devm_mutex share >>> the same lockdep key. Making it a macro will make each caller of >>> devm_mutex_init() have a distinct lockdep key. It all depends on whether >>> all the devm_mutexes have the same lock usage pattern or not and whether >>> it is possible for one devm_mutex to be nested inside another. So either >>> way can be fine depending on the mutex usage pattern. My suggestion is >>> to use a function, if possible, unless it will cause a false positive >>> lockdep splat as there is a limit on the maximum # of lockdep keys that >>> can be used. >> >> devm_mutex_init() should behave like other similar function >> initializing stuff with resource management. I.e. it should behave like >> mutex_init(), but with resource management. >> >> mutex_init() is a macro generating static lockdep key for each instance, >> so devm_mutex_init() should also generate static lockdep key for each >> instance. >> >> Marek > > -- > Best regards > George
On Tue, Mar 12, 2024 at 7:41 AM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > Le 12/03/2024 à 01:01, George Stark a écrit : > > [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. > > Découvrez pourquoi ceci est important à > > https://aka.ms/LearnAboutSenderIdentification ] > > On 3/7/24 13:34, Andy Shevchenko wrote: > >> On Thu, Mar 7, 2024 at 4:40 AM George Stark > >> <gnstark@salutedevices.com> wrote: ... > >>> Signed-off-by: George Stark <gnstark@salutedevices.com> > >>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > >> > >>> Hello Christophe. Hope you don't mind I put you SoB tag because you > >>> helped alot > >>> to make this patch happen. > >> > >> You also need to figure out who should be the author of the patch and > >> probably add a (missing) Co-developed-by. After all you should also > >> follow the correct order of SoBs. > > > > Thanks for the review. > > I explained in the other letter as I see it. So I'd leave myself > > as author and add appropriate tag with Christophe's name. > > BTW what do you mean by correct SoB order? > > Is it alphabetical order or order of importance? > The correct order is to first have the Author's SoB. At the last one is submitters. So, if it's the same person, which one should go first?
Hello Christophe
Thanks for the review
You were right about typecheck - it was meant to check errors even if
CONFIG_DEBUG_MUTEXES was off.
Here's new version based on the comments:
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 67edc4ca2bee..9193b163038f 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -22,6 +22,8 @@
#include <linux/cleanup.h>
#include <linux/mutex_types.h>
+struct device;
+
#ifdef CONFIG_DEBUG_LOCK_ALLOC
# define __DEP_MAP_MUTEX_INITIALIZER(lockname) \
, .dep_map = { \
@@ -117,6 +119,34 @@ do { \
} while (0)
#endif /* CONFIG_PREEMPT_RT */
+#ifdef CONFIG_DEBUG_MUTEXES
+
+int debug_devm_mutex_init(struct device *dev, struct mutex *lock);
+
+static inline int __devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+ return debug_devm_mutex_init(dev, lock);
+}
+
+#else
+
+static inline int __devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+ /*
+ * When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so
+ * no really need to register it in devm subsystem.
+ */
+ return 0;
+}
+
+#endif
+
+#define devm_mutex_init(dev, mutex) \
+({ \
+ mutex_init(mutex); \
+ __devm_mutex_init(dev, mutex); \
+})
+
/*
* See kernel/locking/mutex.c for detailed documentation of these APIs.
* Also see Documentation/locking/mutex-design.rst.
diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
index bc8abb8549d2..967a5367c79a 100644
--- a/kernel/locking/mutex-debug.c
+++ b/kernel/locking/mutex-debug.c
@@ -19,6 +19,7 @@
#include <linux/kallsyms.h>
#include <linux/interrupt.h>
#include <linux/debug_locks.h>
+#include <linux/device.h>
#include "mutex.h"
@@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const char
*name,
lock->magic = lock;
}
+static void devm_mutex_release(void *res)
+{
+ mutex_destroy(res);
+}
+
+int debug_devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+ return devm_add_action_or_reset(dev, devm_mutex_release, lock);
+}
+
/***
* mutex_destroy - mark a mutex unusable
* @lock: the mutex to be destroyed
Le 12/03/2024 à 12:39, George Stark a écrit : > [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. > Découvrez pourquoi ceci est important à > https://aka.ms/LearnAboutSenderIdentification ] > > Hello Christophe > > Thanks for the review > You were right about typecheck - it was meant to check errors even if > CONFIG_DEBUG_MUTEXES was off. Yes that's current practice in order to catch problems as soon as possible. > > Here's new version based on the comments: > > diff --git a/include/linux/mutex.h b/include/linux/mutex.h > index 67edc4ca2bee..9193b163038f 100644 > --- a/include/linux/mutex.h > +++ b/include/linux/mutex.h > @@ -22,6 +22,8 @@ > #include <linux/cleanup.h> > #include <linux/mutex_types.h> > > +struct device; > + > #ifdef CONFIG_DEBUG_LOCK_ALLOC > # define __DEP_MAP_MUTEX_INITIALIZER(lockname) \ > , .dep_map = { \ > @@ -117,6 +119,34 @@ do > { \ > } while (0) > #endif /* CONFIG_PREEMPT_RT */ > > +#ifdef CONFIG_DEBUG_MUTEXES > + > +int debug_devm_mutex_init(struct device *dev, struct mutex *lock); > + > +static inline int __devm_mutex_init(struct device *dev, struct mutex > *lock) > +{ > + return debug_devm_mutex_init(dev, lock); > +} You don't need that inline function, just change debug_devm_mutex_init() to __devm_mutex_init(). > + > +#else > + > +static inline int __devm_mutex_init(struct device *dev, struct mutex > *lock) > +{ > + /* > + * When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so > + * no really need to register it in devm subsystem. > + */ Don't know if it is because tabs are replaced by blanks in you email, but the stars should be aligned /* ... * ... */ > + return 0; > +} > + > +#endif > + > +#define devm_mutex_init(dev, mutex) \ > +({ \ > + mutex_init(mutex); \ > + __devm_mutex_init(dev, mutex); \ > +}) > + > /* > * See kernel/locking/mutex.c for detailed documentation of these APIs. > * Also see Documentation/locking/mutex-design.rst. > diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c > index bc8abb8549d2..967a5367c79a 100644 > --- a/kernel/locking/mutex-debug.c > +++ b/kernel/locking/mutex-debug.c > @@ -19,6 +19,7 @@ > #include <linux/kallsyms.h> > #include <linux/interrupt.h> > #include <linux/debug_locks.h> > +#include <linux/device.h> > > #include "mutex.h" > > @@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const char > *name, > lock->magic = lock; > } > > +static void devm_mutex_release(void *res) > +{ > + mutex_destroy(res); > +} > + > +int debug_devm_mutex_init(struct device *dev, struct mutex *lock) Rename __devm_mutex_init(); It makes it more clear that nobody is expected to call it directly. > +{ > + return devm_add_action_or_reset(dev, devm_mutex_release, lock); > +} > + > /*** > * mutex_destroy - mark a mutex unusable > * @lock: the mutex to be destroyed > -- > 2.25.1 > >
Hello Christophe On 3/12/24 14:51, Christophe Leroy wrote: > > > Le 12/03/2024 à 12:39, George Stark a écrit : >> [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. >> Découvrez pourquoi ceci est important à >> https://aka.ms/LearnAboutSenderIdentification ] ... > You don't need that inline function, just change debug_devm_mutex_init() > to __devm_mutex_init(). I stuck to debug_* name because mutex-debug.c already exports a set of debug_ calls so... Well it's not essential anyway. Here's the next try: diff --git a/include/linux/mutex.h b/include/linux/mutex.h index 67edc4ca2bee..537b5ea18ceb 100644 --- a/include/linux/mutex.h +++ b/include/linux/mutex.h @@ -22,6 +22,8 @@ #include <linux/cleanup.h> #include <linux/mutex_types.h> +struct device; + #ifdef CONFIG_DEBUG_LOCK_ALLOC # define __DEP_MAP_MUTEX_INITIALIZER(lockname) \ , .dep_map = { \ @@ -117,6 +119,29 @@ do { \ } while (0) #endif /* CONFIG_PREEMPT_RT */ +#ifdef CONFIG_DEBUG_MUTEXES + +int __devm_mutex_init(struct device *dev, struct mutex *lock); + +#else + +static inline int __devm_mutex_init(struct device *dev, struct mutex *lock) +{ + /* + * When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so + * no really need to register it in devm subsystem. + */ + return 0; +} + +#endif + +#define devm_mutex_init(dev, mutex) \ +({ \ + mutex_init(mutex); \ + __devm_mutex_init(dev, mutex); \ +}) + /* * See kernel/locking/mutex.c for detailed documentation of these APIs. * Also see Documentation/locking/mutex-design.rst. diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c index bc8abb8549d2..6aa77e3dc82e 100644 --- a/kernel/locking/mutex-debug.c +++ b/kernel/locking/mutex-debug.c @@ -19,6 +19,7 @@ #include <linux/kallsyms.h> #include <linux/interrupt.h> #include <linux/debug_locks.h> +#include <linux/device.h> #include "mutex.h" @@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const char *name, lock->magic = lock; } +static void devm_mutex_release(void *res) +{ + mutex_destroy(res); +} + +int __devm_mutex_init(struct device *dev, struct mutex *lock) +{ + return devm_add_action_or_reset(dev, devm_mutex_release, lock); +} + /*** * mutex_destroy - mark a mutex unusable * @lock: the mutex to be destroyed
Le 12/03/2024 à 16:30, George Stark a écrit : > [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. > Découvrez pourquoi ceci est important à > https://aka.ms/LearnAboutSenderIdentification ] > > Hello Christophe > > On 3/12/24 14:51, Christophe Leroy wrote: >> >> >> Le 12/03/2024 à 12:39, George Stark a écrit : >>> [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. >>> Découvrez pourquoi ceci est important à >>> https://aka.ms/LearnAboutSenderIdentification ] > > ... > >> You don't need that inline function, just change debug_devm_mutex_init() >> to __devm_mutex_init(). > > I stuck to debug_* name because mutex-debug.c already exports a set > of debug_ calls so... Ah yes you are right I didn't see that. On the other hand all those debug_mutex_* are used by kernel/locking/mutex.c. Here we really don't want our new function to be called by anything else than devm_mutex_init so by calling it __devm_mutex_init() you kind of tie them together. > Well it's not essential anyway. Here's the next try: Looks good to me. > > diff --git a/include/linux/mutex.h b/include/linux/mutex.h > index 67edc4ca2bee..537b5ea18ceb 100644 > --- a/include/linux/mutex.h > +++ b/include/linux/mutex.h > @@ -22,6 +22,8 @@ > #include <linux/cleanup.h> > #include <linux/mutex_types.h> > > +struct device; > + > #ifdef CONFIG_DEBUG_LOCK_ALLOC > # define __DEP_MAP_MUTEX_INITIALIZER(lockname) \ > , .dep_map = { \ > @@ -117,6 +119,29 @@ do > { \ > } while (0) > #endif /* CONFIG_PREEMPT_RT */ > > +#ifdef CONFIG_DEBUG_MUTEXES > + > +int __devm_mutex_init(struct device *dev, struct mutex *lock); > + > +#else > + > +static inline int __devm_mutex_init(struct device *dev, struct mutex > *lock) > +{ > + /* > + * When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so > + * no really need to register it in devm subsystem. > + */ > + return 0; > +} > + > +#endif > + > +#define devm_mutex_init(dev, mutex) \ > +({ \ > + mutex_init(mutex); \ > + __devm_mutex_init(dev, mutex); \ > +}) > + > /* > * See kernel/locking/mutex.c for detailed documentation of these APIs. > * Also see Documentation/locking/mutex-design.rst. > diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c > index bc8abb8549d2..6aa77e3dc82e 100644 > --- a/kernel/locking/mutex-debug.c > +++ b/kernel/locking/mutex-debug.c > @@ -19,6 +19,7 @@ > #include <linux/kallsyms.h> > #include <linux/interrupt.h> > #include <linux/debug_locks.h> > +#include <linux/device.h> > > #include "mutex.h" > > @@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const char > *name, > lock->magic = lock; > } > > +static void devm_mutex_release(void *res) > +{ > + mutex_destroy(res); > +} > + > +int __devm_mutex_init(struct device *dev, struct mutex *lock) > +{ > + return devm_add_action_or_reset(dev, devm_mutex_release, lock); > +} > + > /*** > * mutex_destroy - mark a mutex unusable > * @lock: the mutex to be destroyed > -- > 2.25.1 > > > >>> + >>> +#else >>> + >>> +static inline int __devm_mutex_init(struct device *dev, struct mutex >>> *lock) >>> +{ >>> + /* >>> + * When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a >>> nop so >>> + * no really need to register it in devm subsystem. >>> + */ >> >> Don't know if it is because tabs are replaced by blanks in you email, >> but the stars should be aligned > > Ack > > > -- > Best regards > George
diff --git a/include/linux/mutex.h b/include/linux/mutex.h index f7611c092db7..9bcf72cb941a 100644 --- a/include/linux/mutex.h +++ b/include/linux/mutex.h @@ -22,6 +22,8 @@ #include <linux/cleanup.h> #include <linux/mutex_types.h> +struct device; + #ifdef CONFIG_DEBUG_LOCK_ALLOC # define __DEP_MAP_MUTEX_INITIALIZER(lockname) \ , .dep_map = { \ @@ -115,10 +117,21 @@ do { \ #ifdef CONFIG_DEBUG_MUTEXES +int devm_mutex_init(struct device *dev, struct mutex *lock); void mutex_destroy(struct mutex *lock); #else +static inline int devm_mutex_init(struct device *dev, struct mutex *lock) +{ + /* + * since mutex_destroy is nop actually there's no need to register it + * in devm subsystem. + */ + mutex_init(lock); + return 0; +} + static inline void mutex_destroy(struct mutex *lock) {} #endif diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c index bc8abb8549d2..c9efab1a8026 100644 --- a/kernel/locking/mutex-debug.c +++ b/kernel/locking/mutex-debug.c @@ -19,6 +19,7 @@ #include <linux/kallsyms.h> #include <linux/interrupt.h> #include <linux/debug_locks.h> +#include <linux/device.h> #include "mutex.h" @@ -104,3 +105,24 @@ void mutex_destroy(struct mutex *lock) } EXPORT_SYMBOL_GPL(mutex_destroy); + +static void devm_mutex_release(void *res) +{ + mutex_destroy(res); +} + +/** + * devm_mutex_init - Resource-managed mutex initialization + * @dev: Device which lifetime mutex is bound to + * @lock: Pointer to a mutex + * + * Initialize mutex which is automatically destroyed when the driver is detached. + * + * Returns: 0 on success or a negative error code on failure. + */ +int devm_mutex_init(struct device *dev, struct mutex *lock) +{ + mutex_init(lock); + return devm_add_action_or_reset(dev, devm_mutex_release, lock); +} +EXPORT_SYMBOL_GPL(devm_mutex_init);