| Message ID | 20240314084531.1935545-2-gnstark@salutedevices.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | devm_led_classdev_register() usage problem | expand |
On Thu, Mar 14, 2024 at 10:46 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() Missing period at the end. .... > Suggested by-by: Christophe Leroy <christophe.leroy@csgroup.eu> Needs properly spelled tag. ... > +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 mutex_destroy() > + * no really need to register it in devm subsystem. in the devm > + */ > + return 0; > +} ... > @@ -19,6 +19,7 @@ > #include <linux/kallsyms.h> > #include <linux/interrupt.h> > #include <linux/debug_locks.h> > +#include <linux/device.h> Without seeing much context can't say if there is a better (more ordered) place to squeeze a new header to. Please, check. ... After addressing the above comments Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
On 3/14/24 04:45, 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> > Suggested by-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > include/linux/mutex.h | 27 +++++++++++++++++++++++++++ > kernel/locking/mutex-debug.c | 11 +++++++++++ > 2 files changed, 38 insertions(+) > > diff --git a/include/linux/mutex.h b/include/linux/mutex.h > index 67edc4ca2bee..f57e005ded24 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,31 @@ 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) \ > +({ \ > + typeof(mutex) mutex_ = (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 Acked-by: Waiman Long <longman@redhat.com>
diff --git a/include/linux/mutex.h b/include/linux/mutex.h index 67edc4ca2bee..f57e005ded24 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,31 @@ 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) \ +({ \ + typeof(mutex) mutex_ = (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
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> Suggested by-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- include/linux/mutex.h | 27 +++++++++++++++++++++++++++ kernel/locking/mutex-debug.c | 11 +++++++++++ 2 files changed, 38 insertions(+)