| Message ID | c16599b23afa853a44d13b906af5683027959a26.1702621174.git.christophe.leroy@csgroup.eu |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v4-bis] locking: introduce devm_mutex_init | expand |
On Fri, Dec 15, 2023 at 8:23 AM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > From: George Stark <gnstark@salutedevices.com> > > 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() Missing period. ... > } while (0) > #endif /* CONFIG_PREEMPT_RT */ ^^^ (1) > +struct device; > + > +/* > + * devm_mutex_init() registers a function that calls mutex_destroy() > + * when the ressource is released. > + * > + * When mutex_destroy() is a not, there is no need to register that > + * function. > + */ > +#ifdef CONFIG_DEBUG_MUTEXES Shouldn't this be #if defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_PREEMPT_RT) (see (1) as well)? > +void mutex_destroy(struct mutex *lock); > +int devm_mutex_init(struct device *dev, struct mutex *lock); > +#else > +static inline void mutex_destroy(struct mutex *lock) {} > + > +static inline int devm_mutex_init(struct device *dev, struct mutex *lock) > +{ > + mutex_init(lock); > + return 0; > +} > +#endif
Le 15/12/2023 à 16:58, Andy Shevchenko a écrit : > On Fri, Dec 15, 2023 at 8:23 AM Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: >> >> From: George Stark <gnstark@salutedevices.com> >> >> 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() > > Missing period. > > ... > >> } while (0) >> #endif /* CONFIG_PREEMPT_RT */ > > ^^^ (1) > >> +struct device; >> + >> +/* >> + * devm_mutex_init() registers a function that calls mutex_destroy() >> + * when the ressource is released. >> + * >> + * When mutex_destroy() is a not, there is no need to register that >> + * function. >> + */ >> +#ifdef CONFIG_DEBUG_MUTEXES > > Shouldn't this be > > #if defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_PREEMPT_RT) > > (see (1) as well)? Isn't needed, handled by Kconfig: config DEBUG_MUTEXES bool "Mutex debugging: basic checks" depends on DEBUG_KERNEL && !PREEMPT_RT > >> +void mutex_destroy(struct mutex *lock); >> +int devm_mutex_init(struct device *dev, struct mutex *lock); >> +#else >> +static inline void mutex_destroy(struct mutex *lock) {} >> + >> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock) >> +{ >> + mutex_init(lock); >> + return 0; >> +} >> +#endif >
On 12/15/23 10:58, Andy Shevchenko wrote: > On Fri, Dec 15, 2023 at 8:23 AM Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: >> From: George Stark <gnstark@salutedevices.com> >> >> 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() > Missing period. > > ... > >> } while (0) >> #endif /* CONFIG_PREEMPT_RT */ > ^^^ (1) > >> +struct device; >> + >> +/* >> + * devm_mutex_init() registers a function that calls mutex_destroy() >> + * when the ressource is released. >> + * >> + * When mutex_destroy() is a not, there is no need to register that >> + * function. >> + */ >> +#ifdef CONFIG_DEBUG_MUTEXES > Shouldn't this be > > #if defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_PREEMPT_RT) > > (see (1) as well)? CONFIG_DEBUG_MUTEXES and CONFIG_PREEMPT_RT are mutually exclusive. At most one of them can be set. Cheers, Longman
diff --git a/include/linux/mutex.h b/include/linux/mutex.h index a33aa9eb9fc3..db847220ef44 100644 --- a/include/linux/mutex.h +++ b/include/linux/mutex.h @@ -81,14 +81,10 @@ struct mutex { #define __DEBUG_MUTEX_INITIALIZER(lockname) \ , .magic = &lockname -extern void mutex_destroy(struct mutex *lock); - #else # define __DEBUG_MUTEX_INITIALIZER(lockname) -static inline void mutex_destroy(struct mutex *lock) {} - #endif /** @@ -153,8 +149,6 @@ extern void __mutex_rt_init(struct mutex *lock, const char *name, struct lock_class_key *key); extern int mutex_trylock(struct mutex *lock); -static inline void mutex_destroy(struct mutex *lock) { } - #define mutex_is_locked(l) rt_mutex_base_is_locked(&(l)->rtmutex) #define __mutex_init(mutex, name, key) \ @@ -171,6 +165,28 @@ do { \ } while (0) #endif /* CONFIG_PREEMPT_RT */ +struct device; + +/* + * devm_mutex_init() registers a function that calls mutex_destroy() + * when the ressource is released. + * + * When mutex_destroy() is a not, there is no need to register that + * function. + */ +#ifdef CONFIG_DEBUG_MUTEXES +void mutex_destroy(struct mutex *lock); +int devm_mutex_init(struct device *dev, struct mutex *lock); +#else +static inline void mutex_destroy(struct mutex *lock) {} + +static inline int devm_mutex_init(struct device *dev, struct mutex *lock) +{ + mutex_init(lock); + return 0; +} +#endif + /* * 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..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);