[v4,02/10] locking: introduce devm_mutex_init

  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>
---
 include/linux/mutex.h        | 23 +++++++++++++++++++++++
 kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
 2 files changed, 45 insertions(+)
  

On 12/14/23 12:36, 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>
> ---
>   include/linux/mutex.h        | 23 +++++++++++++++++++++++
>   kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
>   2 files changed, 45 insertions(+)
>
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index a33aa9eb9fc3..ebd03ff1ef66 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -21,6 +21,8 @@
>   #include <linux/debug_locks.h>
>   #include <linux/cleanup.h>
>   
> +struct device;
> +
>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
>   # define __DEP_MAP_MUTEX_INITIALIZER(lockname)			\
>   		, .dep_map = {					\
> @@ -127,6 +129,20 @@ extern void __mutex_init(struct mutex *lock, const char *name,
>    */
>   extern bool mutex_is_locked(struct mutex *lock);
>   
> +#ifdef CONFIG_DEBUG_MUTEXES
> +
> +int devm_mutex_init(struct device *dev, struct mutex *lock);
Please add "extern" to the function declaration to be consistent with 
other functional declarations in mutex.h.
> +
> +#else
> +
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +	mutex_init(lock);
> +	return 0;
> +}

I would prefer you to add a devm_mutex_init macro after the function 
declaration and put this inline function at the end of header if the 
devm_mutex_init macro isn't defined. In this way, you don't need to 
repeat this inline function twice as it has no dependency on PREEMPT_RT.

By doing this, you can also move the function declaration right after 
mutex_destroy() without the need to add another #ifdef 
CONFIG_DEBUG_MUTEXES block.

> +
> +#endif
> +
>   #else /* !CONFIG_PREEMPT_RT */
>   /*
>    * Preempt-RT variant based on rtmutexes.
> @@ -169,6 +185,13 @@ do {							\
>   							\
>   	__mutex_init((mutex), #mutex, &__key);		\
>   } while (0)
> +
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +	mutex_init(lock);
> +	return 0;
> +}
> +
>   #endif /* CONFIG_PREEMPT_RT */
>   
>   /*
> 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);

The mutex-debug.c change looks fine to me.

Cheers,
Longman
  

Le 14/12/2023 à 18:36, 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>
> ---
>   include/linux/mutex.h        | 23 +++++++++++++++++++++++
>   kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
>   2 files changed, 45 insertions(+)
> 
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index a33aa9eb9fc3..ebd03ff1ef66 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -21,6 +21,8 @@
>   #include <linux/debug_locks.h>
>   #include <linux/cleanup.h>
> 
> +struct device;
> +
>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
>   # define __DEP_MAP_MUTEX_INITIALIZER(lockname)                 \
>                  , .dep_map = {                                  \
> @@ -127,6 +129,20 @@ extern void __mutex_init(struct mutex *lock, const char *name,
>    */
>   extern bool mutex_is_locked(struct mutex *lock);
> 
> +#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)
> +{
> +       mutex_init(lock);
> +       return 0;
> +}
> +
> +#endif
> +
>   #else /* !CONFIG_PREEMPT_RT */
>   /*
>    * Preempt-RT variant based on rtmutexes.
> @@ -169,6 +185,13 @@ do {                                                       \
>                                                          \
>          __mutex_init((mutex), #mutex, &__key);          \
>   } while (0)
> +
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +       mutex_init(lock);
> +       return 0;
> +}
> +
>   #endif /* CONFIG_PREEMPT_RT */
> 
>   /*
> 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
> 

I think it would make sense to keep mutex_destroy() and 
devm_mutex_init() together, see exemple below:

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index ebd03ff1ef66..c620759ff85b 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -83,14 +83,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

  /**
@@ -131,10 +127,13 @@ extern bool mutex_is_locked(struct mutex *lock);

  #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);
@@ -169,8 +168,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)			\
@@ -186,6 +183,8 @@ do {							\
  	__mutex_init((mutex), #mutex, &__key);		\
  } while (0)

+static inline void mutex_destroy(struct mutex *lock) { }
+
  static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
  {
  	mutex_init(lock);
---

Would also be nice to have a comment explaining that when 
mutex_destroy() is a nop, devm_mutext_init() doesn't need to register a 
release function.

Either way,

Reviewed-by: christophe.leroy@csgroup.eu
  

Le 14/12/2023 à 19:48, Waiman Long a écrit :
> 
> On 12/14/23 12:36, 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>
>> ---
>>   include/linux/mutex.h        | 23 +++++++++++++++++++++++
>>   kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
>>   2 files changed, 45 insertions(+)
>>
>> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
>> index a33aa9eb9fc3..ebd03ff1ef66 100644
>> --- a/include/linux/mutex.h
>> +++ b/include/linux/mutex.h
>> @@ -21,6 +21,8 @@
>>   #include <linux/debug_locks.h>
>>   #include <linux/cleanup.h>
>> +struct device;
>> +
>>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>   # define __DEP_MAP_MUTEX_INITIALIZER(lockname)            \
>>           , .dep_map = {                    \
>> @@ -127,6 +129,20 @@ extern void __mutex_init(struct mutex *lock, 
>> const char *name,
>>    */
>>   extern bool mutex_is_locked(struct mutex *lock);
>> +#ifdef CONFIG_DEBUG_MUTEXES
>> +
>> +int devm_mutex_init(struct device *dev, struct mutex *lock);
> Please add "extern" to the function declaration to be consistent with 
> other functional declarations in mutex.h.

'extern' is pointless and deprecated on function prototypes. Already 
having some is not a good reason to add new ones, errors from the past 
should be avoided nowadays. With time they should all disappear so don't 
add new ones.

>> +
>> +#else
>> +
>> +static inline int devm_mutex_init(struct device *dev, struct mutex 
>> *lock)
>> +{
>> +    mutex_init(lock);
>> +    return 0;
>> +}
> 
> I would prefer you to add a devm_mutex_init macro after the function 
> declaration and put this inline function at the end of header if the 
> devm_mutex_init macro isn't defined. In this way, you don't need to 
> repeat this inline function twice as it has no dependency on PREEMPT_RT.

It is already done that way for other functions in that file. Should be 
kept consistant. I agree with you it is not ideal, maybe we should 
rework that file completely but I don't like the idea of a 
devm_mutex_init macro for that.

Christophe

> 
> By doing this, you can also move the function declaration right after 
> mutex_destroy() without the need to add another #ifdef 
> CONFIG_DEBUG_MUTEXES block.
> 
>> +
>> +#endif
>> +
>>   #else /* !CONFIG_PREEMPT_RT */
>>   /*
>>    * Preempt-RT variant based on rtmutexes.
>> @@ -169,6 +185,13 @@ do {                            \
>>                               \
>>       __mutex_init((mutex), #mutex, &__key);        \
>>   } while (0)
>> +
>> +static inline int devm_mutex_init(struct device *dev, struct mutex 
>> *lock)
>> +{
>> +    mutex_init(lock);
>> +    return 0;
>> +}
>> +
>>   #endif /* CONFIG_PREEMPT_RT */
>>   /*
>> 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);
> 
> The mutex-debug.c change looks fine to me.
> 
> Cheers,
> Longman
> 
>
  

On 12/14/23 14:53, Christophe Leroy wrote:
>
> Le 14/12/2023 à 19:48, Waiman Long a écrit :
>> On 12/14/23 12:36, 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>
>>> ---
>>>    include/linux/mutex.h        | 23 +++++++++++++++++++++++
>>>    kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
>>>    2 files changed, 45 insertions(+)
>>>
>>> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
>>> index a33aa9eb9fc3..ebd03ff1ef66 100644
>>> --- a/include/linux/mutex.h
>>> +++ b/include/linux/mutex.h
>>> @@ -21,6 +21,8 @@
>>>    #include <linux/debug_locks.h>
>>>    #include <linux/cleanup.h>
>>> +struct device;
>>> +
>>>    #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>>    # define __DEP_MAP_MUTEX_INITIALIZER(lockname)            \
>>>            , .dep_map = {                    \
>>> @@ -127,6 +129,20 @@ extern void __mutex_init(struct mutex *lock,
>>> const char *name,
>>>     */
>>>    extern bool mutex_is_locked(struct mutex *lock);
>>> +#ifdef CONFIG_DEBUG_MUTEXES
>>> +
>>> +int devm_mutex_init(struct device *dev, struct mutex *lock);
>> Please add "extern" to the function declaration to be consistent with
>> other functional declarations in mutex.h.
> 'extern' is pointless and deprecated on function prototypes. Already
> having some is not a good reason to add new ones, errors from the past
> should be avoided nowadays. With time they should all disappear so don't
> add new ones.
Yes, "extern" is optional. It is just a suggestion and I am going to 
argue about that.
>
>>> +
>>> +#else
>>> +
>>> +static inline int devm_mutex_init(struct device *dev, struct mutex
>>> *lock)
>>> +{
>>> +    mutex_init(lock);
>>> +    return 0;
>>> +}
>> I would prefer you to add a devm_mutex_init macro after the function
>> declaration and put this inline function at the end of header if the
>> devm_mutex_init macro isn't defined. In this way, you don't need to
>> repeat this inline function twice as it has no dependency on PREEMPT_RT.
> It is already done that way for other functions in that file. Should be
> kept consistant. I agree with you it is not ideal, maybe we should
> rework that file completely but I don't like the idea of a
> devm_mutex_init macro for that.

devm_mutex_init() is not an API for the core mutex code. That is why I 
want to minimize change to the existing code layout. Putting it at the 
end will reduce confusion when developers look up mutex.h header file to 
find out what mutex functions are available.

Cheers,
Longman
  

Le 14/12/2023 à 22:48, Waiman Long a écrit :
> On 12/14/23 14:53, Christophe Leroy wrote:
>>
>> Le 14/12/2023 à 19:48, Waiman Long a écrit :
>>> On 12/14/23 12:36, 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>
>>>> ---
>>>>    include/linux/mutex.h        | 23 +++++++++++++++++++++++
>>>>    kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
>>>>    2 files changed, 45 insertions(+)
>>>>
>>>> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
>>>> index a33aa9eb9fc3..ebd03ff1ef66 100644
>>>> --- a/include/linux/mutex.h
>>>> +++ b/include/linux/mutex.h
>>>> @@ -21,6 +21,8 @@
>>>>    #include <linux/debug_locks.h>
>>>>    #include <linux/cleanup.h>
>>>> +struct device;
>>>> +
>>>>    #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>>>    # define __DEP_MAP_MUTEX_INITIALIZER(lockname)            \
>>>>            , .dep_map = {                    \
>>>> @@ -127,6 +129,20 @@ extern void __mutex_init(struct mutex *lock,
>>>> const char *name,
>>>>     */
>>>>    extern bool mutex_is_locked(struct mutex *lock);
>>>> +#ifdef CONFIG_DEBUG_MUTEXES
>>>> +
>>>> +int devm_mutex_init(struct device *dev, struct mutex *lock);
>>> Please add "extern" to the function declaration to be consistent with
>>> other functional declarations in mutex.h.
>> 'extern' is pointless and deprecated on function prototypes. Already
>> having some is not a good reason to add new ones, errors from the past
>> should be avoided nowadays. With time they should all disappear so don't
>> add new ones.
> Yes, "extern" is optional. It is just a suggestion and I am going to 
> argue about that.

FWIW, note that when you perform a strict check with checkpatch.pl, you 
get a warning for that:

$ ./scripts/checkpatch.pl --strict -g HEAD
CHECK: extern prototypes should be avoided in .h files
#56: FILE: include/linux/mutex.h:131:
+extern int devm_mutex_init(struct device *dev, struct mutex *lock);

total: 0 errors, 0 warnings, 1 checks, 99 lines checked

>>
>>>> +
>>>> +#else
>>>> +
>>>> +static inline int devm_mutex_init(struct device *dev, struct mutex
>>>> *lock)
>>>> +{
>>>> +    mutex_init(lock);
>>>> +    return 0;
>>>> +}
>>> I would prefer you to add a devm_mutex_init macro after the function
>>> declaration and put this inline function at the end of header if the
>>> devm_mutex_init macro isn't defined. In this way, you don't need to
>>> repeat this inline function twice as it has no dependency on PREEMPT_RT.
>> It is already done that way for other functions in that file. Should be
>> kept consistant. I agree with you it is not ideal, maybe we should
>> rework that file completely but I don't like the idea of a
>> devm_mutex_init macro for that.
> 
> devm_mutex_init() is not an API for the core mutex code. That is why I 
> want to minimize change to the existing code layout. Putting it at the 
> end will reduce confusion when developers look up mutex.h header file to 
> find out what mutex functions are available.

If I look at linux/gpio.h we are more or less in the same situation I think.

devm_mutex_init() is not an API for the core mutex code, but developers 
need to know the managed functions for mutex exist, and having them at 
the same place as non managed functions looks better to me. Now I agree 
with you that this duplication of functions is not the best, and it also 
applies to existing content of mutex.h so maybe we can do something 
about it later and improve the situation.

Christophe
  

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
  

Hello Christophe

On 12/15/23 08:46, Christophe Leroy wrote:
> 
> 
> Le 14/12/2023 à 22:48, Waiman Long a écrit :
>> On 12/14/23 14:53, Christophe Leroy wrote:
>>>
>>> Le 14/12/2023 à 19:48, Waiman Long a écrit :
>>>> On 12/14/23 12:36, 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>
>>>>> ---
>>>>>     include/linux/mutex.h        | 23 +++++++++++++++++++++++
>>>>>     kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
>>>>>     2 files changed, 45 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
>>>>> index a33aa9eb9fc3..ebd03ff1ef66 100644
>>>>> --- a/include/linux/mutex.h
>>>>> +++ b/include/linux/mutex.h
>>>>> @@ -21,6 +21,8 @@
>>>>>     #include <linux/debug_locks.h>
>>>>>     #include <linux/cleanup.h>
>>>>> +struct device;
>>>>> +
>>>>>     #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>>>>     # define __DEP_MAP_MUTEX_INITIALIZER(lockname)            \
>>>>>             , .dep_map = {                    \
>>>>> @@ -127,6 +129,20 @@ extern void __mutex_init(struct mutex *lock,
>>>>> const char *name,
>>>>>      */
>>>>>     extern bool mutex_is_locked(struct mutex *lock);
>>>>> +#ifdef CONFIG_DEBUG_MUTEXES
>>>>> +
>>>>> +int devm_mutex_init(struct device *dev, struct mutex *lock);
>>>> Please add "extern" to the function declaration to be consistent with
>>>> other functional declarations in mutex.h.
>>> 'extern' is pointless and deprecated on function prototypes. Already
>>> having some is not a good reason to add new ones, errors from the past
>>> should be avoided nowadays. With time they should all disappear so don't
>>> add new ones.
>> Yes, "extern" is optional. It is just a suggestion and I am going to
>> argue about that.
> 
> FWIW, note that when you perform a strict check with checkpatch.pl, you
> get a warning for that:
> 
> $ ./scripts/checkpatch.pl --strict -g HEAD
> CHECK: extern prototypes should be avoided in .h files
> #56: FILE: include/linux/mutex.h:131:
> +extern int devm_mutex_init(struct device *dev, struct mutex *lock);
> 
> total: 0 errors, 0 warnings, 1 checks, 99 lines checked

This is ambiguous situation about extern. It's deprecated and useless on 
one hand but harmless. And those externs will not disappear by themself 
- it'll be one patch that clean them all at once (in one header at 
least) so one more extern will not alter the overall picture.

On the other hand if we manage to place devm_mutex_init near 
mutex_destroy then we'll have:

int devm_mutex_init(struct device *dev, struct mutex *lock);
extern void mutex_destroy(struct mutex *lock);

and it raises questions and does not look very nice.

>>>
>>>>> +
>>>>> +#else
>>>>> +
>>>>> +static inline int devm_mutex_init(struct device *dev, struct mutex
>>>>> *lock)
>>>>> +{
>>>>> +    mutex_init(lock);
>>>>> +    return 0;
>>>>> +}
>>>> I would prefer you to add a devm_mutex_init macro after the function
>>>> declaration and put this inline function at the end of header if the
>>>> devm_mutex_init macro isn't defined. In this way, you don't need to
>>>> repeat this inline function twice as it has no dependency on PREEMPT_RT.
>>> It is already done that way for other functions in that file. Should be
>>> kept consistant. I agree with you it is not ideal, maybe we should
>>> rework that file completely but I don't like the idea of a
>>> devm_mutex_init macro for that.
>>
>> devm_mutex_init() is not an API for the core mutex code. That is why I
>> want to minimize change to the existing code layout. Putting it at the
>> end will reduce confusion when developers look up mutex.h header file to
>> find out what mutex functions are available.
> 
> If I look at linux/gpio.h we are more or less in the same situation I think.
> 
> devm_mutex_init() is not an API for the core mutex code, but developers
> need to know the managed functions for mutex exist, and having them at
> the same place as non managed functions looks better to me. Now I agree
> with you that this duplication of functions is not the best, and it also
> applies to existing content of mutex.h so maybe we can do something
> about it later and improve the situation.
> 
> Christophe
  

Le 17/12/2023 à 02:05, 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 12/15/23 08:46, Christophe Leroy wrote:
>>
>>
>> Le 14/12/2023 à 22:48, Waiman Long a écrit :
>>> On 12/14/23 14:53, Christophe Leroy wrote:
>>>>
>>>> Le 14/12/2023 à 19:48, Waiman Long a écrit :
>>>>> On 12/14/23 12:36, 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>
>>>>>> ---
>>>>>>     include/linux/mutex.h        | 23 +++++++++++++++++++++++
>>>>>>     kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
>>>>>>     2 files changed, 45 insertions(+)
>>>>>>
>>>>>> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
>>>>>> index a33aa9eb9fc3..ebd03ff1ef66 100644
>>>>>> --- a/include/linux/mutex.h
>>>>>> +++ b/include/linux/mutex.h
>>>>>> @@ -21,6 +21,8 @@
>>>>>>     #include <linux/debug_locks.h>
>>>>>>     #include <linux/cleanup.h>
>>>>>> +struct device;
>>>>>> +
>>>>>>     #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>>>>>     # define __DEP_MAP_MUTEX_INITIALIZER(lockname)            \
>>>>>>             , .dep_map = {                    \
>>>>>> @@ -127,6 +129,20 @@ extern void __mutex_init(struct mutex *lock,
>>>>>> const char *name,
>>>>>>      */
>>>>>>     extern bool mutex_is_locked(struct mutex *lock);
>>>>>> +#ifdef CONFIG_DEBUG_MUTEXES
>>>>>> +
>>>>>> +int devm_mutex_init(struct device *dev, struct mutex *lock);
>>>>> Please add "extern" to the function declaration to be consistent with
>>>>> other functional declarations in mutex.h.
>>>> 'extern' is pointless and deprecated on function prototypes. Already
>>>> having some is not a good reason to add new ones, errors from the past
>>>> should be avoided nowadays. With time they should all disappear so 
>>>> don't
>>>> add new ones.
>>> Yes, "extern" is optional. It is just a suggestion and I am going to
>>> argue about that.
>>
>> FWIW, note that when you perform a strict check with checkpatch.pl, you
>> get a warning for that:
>>
>> $ ./scripts/checkpatch.pl --strict -g HEAD
>> CHECK: extern prototypes should be avoided in .h files
>> #56: FILE: include/linux/mutex.h:131:
>> +extern int devm_mutex_init(struct device *dev, struct mutex *lock);
>>
>> total: 0 errors, 0 warnings, 1 checks, 99 lines checked
> 
> This is ambiguous situation about extern. It's deprecated and useless on
> one hand but harmless. And those externs will not disappear by themself
> - it'll be one patch that clean them all at once (in one header at
> least) so one more extern will not alter the overall picture.

That kind of cleanup patch bomb is a nightmare for backporting, so if it 
happens one day it should be as light as possible, hence the importance 
to not add new ones and remove existing one everytime you modify or move 
a line including it for whatever reason.

> 
> On the other hand if we manage to place devm_mutex_init near
> mutex_destroy then we'll have:
> 
> int devm_mutex_init(struct device *dev, struct mutex *lock);
> extern void mutex_destroy(struct mutex *lock);

I sent you an alternative proposal that avoids duplication of the static 
inline version of devm_mutex_init(). If you agree with it just take it 
into your series and that question will vanish.

> 
> and it raises questions and does not look very nice.

If you look at linux/mm.h there are plenty of them anyway, so why do 
different ? For an exemple look at 
https://elixir.bootlin.com/linux/v6.7-rc4/source/include/linux/mm.h#L2372


Christophe