[PATCH v4 00/10] devm_led_classdev_register() usage problem

[PATCH v4 00/10] devm_led_classdev_register() usage problem

[George Stark]

This patch series fixes the problem of devm_led_classdev_register misusing.

The basic problem is described in [1]. Shortly when devm_led_classdev_register()
is used then led_classdev_unregister() called after driver's remove() callback.
led_classdev_unregister() calls driver's brightness_set callback and that callback
may use resources which were destroyed already in driver's remove().

After discussion with maintainers [2] [3] we decided:
1) don't touch led subsytem core code and don't remove led_set_brightness() from it
but fix drivers
2) don't use devm_led_classdev_unregister

So the solution is to use devm wrappers for all resources
driver's brightness_set() depends on. And introduce dedicated devm wrapper
for mutex as it's often used resource.

[1] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/
[2] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mc132b9b350fa51931b4fcfe14705d9f06e91421f
[3] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mdbf572a85c33f869a553caf986b6228bb65c8383

Changelog:
v1->v2:
	revise patch series completely

v2->v3:
locking: add define if mutex_destroy() is not an empty function
	new patch, discussed here [8]

devm-helpers: introduce devm_mutex_init
	previous version [4]
	- revise code based on mutex_destroy define
	- update commit message
	- update devm_mutex_init()'s description

leds: aw2013: unlock mutex before destroying it
	previous version [5]
	- make this patch first in the series
	- add tags Fixes and RvB by Andy 

leds: aw2013: use devm API to cleanup module's resources
	previous version [6]
	- make aw2013_chip_disable_action()'s body oneline
	- don't shadow devm_mutex_init() return code

leds: aw200xx: use devm API to cleanup module's resources
	previous version [7]
	- make aw200xx_*_action()'s bodies oneline
	- don't shadow devm_mutex_init() return code

leds: lm3532: use devm API to cleanup module's resources
leds: nic78bx: use devm API to cleanup module's resources
leds: mlxreg: use devm_mutex_init for mutex initializtion
leds: an30259a: use devm_mutext_init for mutext initialization
leds: powernv: add LED_RETAIN_AT_SHUTDOWN flag for leds
	- those pathes were planned but not sent in the series #2 due to mail server
	problem on my side. I revised them according to the comments.

v3->v4:
locking: introduce devm_mutex_init
	new patch
	- move devm_mutex_init implementation completely from devm-helpers.h to mutex.h

locking: add define if mutex_destroy() is not an empty function
	drop the patch [9]

devm-helpers: introduce devm_mutex_init
	drop the patch [10]

leds: aw2013: use devm API to cleanup module's resources
	- add tag Tested-by: Nikita Travkin <nikita@trvn.ru>

[4] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#mf500af0eda2a9ffc95594607dbe4cb64f2e3c9a8
[5] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#mc92df4fb4f7d4187fb01cc1144acfa5fb5230dd2
[6] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#m300df89710c43cc2ab598baa16c68dd0a0d7d681
[7] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#m8e5c65e0c6b137c91fa00bb9320ad581164d1d0b
[8] https://lore.kernel.org/lkml/377e4437-7051-4d88-ae68-1460bcd692e1@redhat.com/T/#m5f84a4a2f387d49678783e652b9e658e02c27450
[9] https://lore.kernel.org/lkml/20231213223020.2713164-1-gnstark@salutedevices.com/T/#m19ad1fc04c560012c1e27418e3156d0c9306dd84
[10] https://lore.kernel.org/lkml/20231213223020.2713164-1-gnstark@salutedevices.com/T/#m63126025f5d1bdcef69bcad50f2e58274d42e2d7

George Stark (10):
  leds: aw2013: unlock mutex before destroying it
  locking: introduce devm_mutex_init
  leds: aw2013: use devm API to cleanup module's resources
  leds: aw200xx: use devm API to cleanup module's resources
  leds: lp3952: use devm API to cleanup module's resources
  leds: lm3532: use devm API to cleanup module's resources
  leds: nic78bx: use devm API to cleanup module's resources
  leds: mlxreg: use devm_mutex_init for mutex initializtion
  leds: an30259a: use devm_mutext_init for mutext initialization
  leds: powernv: use LED_RETAIN_AT_SHUTDOWN flag for leds

 drivers/leds/leds-an30259a.c | 15 +++++----------
 drivers/leds/leds-aw200xx.c  | 33 ++++++++++++++++++++++-----------
 drivers/leds/leds-aw2013.c   | 27 +++++++++++++++------------
 drivers/leds/leds-lm3532.c   | 30 ++++++++++++++++++------------
 drivers/leds/leds-lp3952.c   | 21 +++++++++++----------
 drivers/leds/leds-mlxreg.c   | 17 ++++++-----------
 drivers/leds/leds-nic78bx.c  | 25 +++++++++++++------------
 drivers/leds/leds-powernv.c  | 23 ++++++++---------------
 include/linux/mutex.h        | 23 +++++++++++++++++++++++
 kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
 10 files changed, 143 insertions(+), 93 deletions(-)

-- 
2.25.1

[PATCH v4 01/10] leds: aw2013: unlock mutex before destroying it

[George Stark]

In the probe() callback in case of error mutex is destroyed being locked
which is not allowed so unlock the mutex before destroying.

Fixes: 59ea3c9faf32 ("leds: add aw2013 driver")
Signed-off-by: George Stark <gnstark@salutedevices.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/leds/leds-aw2013.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 59765640b70f..c2bc0782c0cd 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -397,6 +397,7 @@ static int aw2013_probe(struct i2c_client *client)
 	regulator_disable(chip->vcc_regulator);
 
 error:
+	mutex_unlock(&chip->mutex);
 	mutex_destroy(&chip->mutex);
 	return ret;
 }
-- 
2.25.1

Re: (subset) [PATCH v4 01/10] leds: aw2013: unlock mutex before destroying it

[Lee Jones]

On Thu, 14 Dec 2023 20:36:05 +0300, George Stark wrote:
> In the probe() callback in case of error mutex is destroyed being locked
> which is not allowed so unlock the mutex before destroying.
> 
> 

Applied, thanks!

[01/10] leds: aw2013: unlock mutex before destroying it
        commit: eb0f0a751c8e26b212f78fe7325fa2506c5cbb4b

--
Lee Jones [李琼斯]

[PATCH v4 02/10] locking: introduce devm_mutex_init

[George Stark]

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

Re: [PATCH v4 02/10] locking: introduce devm_mutex_init

[Waiman Long]

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

Re: [PATCH v4 02/10] locking: introduce devm_mutex_init

[Christophe Leroy]

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
> 
> 

Re: [PATCH v4 02/10] locking: introduce devm_mutex_init

[Waiman Long]

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

Re: [PATCH v4 02/10] locking: introduce devm_mutex_init

[Christophe Leroy]

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

Re: [PATCH v4 02/10] locking: introduce devm_mutex_init

[George Stark]

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

-- 
Best regards
George

Re: [PATCH v4 02/10] locking: introduce devm_mutex_init

[Christophe Leroy]

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

Re: [PATCH v4 02/10] locking: introduce devm_mutex_init

[George Stark]

Hello Christophe


On 12/17/23 12:31, Christophe Leroy wrote:

...
>>>>>>> ---
>>>>>>>      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.

Thanks for that patch by the way. The only comment is that moving 
mutex_destroy
should be done in a separate patch IMO.
Waiman Long proposed such a refactoring here:
https://lore.kernel.org/lkml/20231216013656.1382213-2-longman@redhat.com/T/

With this patch adding devm_mutex_init would be straightforward.

>>
>> 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
Oh, I see. Ok, I don't have any more arguments against removing extern.
We'll see what mutex.h maintainers decide.

> 
> Christophe

-- 
Best regards
George

Re: [PATCH v4 02/10] locking: introduce devm_mutex_init

[Christophe Leroy]

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

[PATCH RFC v4-bis] locking: introduce devm_mutex_init

[Christophe Leroy]

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()

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 include/linux/mutex.h        | 28 ++++++++++++++++++++++------
 kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
 2 files changed, 44 insertions(+), 6 deletions(-)

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);
-- 
2.41.0

Re: [PATCH RFC v4-bis] locking: introduce devm_mutex_init

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC v4-bis] locking: introduce devm_mutex_init

[Christophe Leroy]

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
> 

Re: [PATCH RFC v4-bis] locking: introduce devm_mutex_init

[Waiman Long]

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

[PATCH v4 03/10] leds: aw2013: use devm API to cleanup module's resources

[George Stark]

In this driver LEDs are registered using devm_led_classdev_register()
so they are automatically unregistered after module's remove() is done.
led_classdev_unregister() calls module's led_set_brightness() to turn off
the LEDs and that callback uses resources which were destroyed already
in module's remove() so use devm API instead of remove().

Signed-off-by: George Stark <gnstark@salutedevices.com>
Tested-by: Nikita Travkin <nikita@trvn.ru>
---
 drivers/leds/leds-aw2013.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index c2bc0782c0cd..863aeb02f278 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0+
 // Driver for Awinic AW2013 3-channel LED driver
 
+#include <linux/devm-helpers.h>
 #include <linux/i2c.h>
 #include <linux/leds.h>
 #include <linux/module.h>
@@ -318,6 +319,11 @@ static int aw2013_probe_dt(struct aw2013 *chip)
 	return 0;
 }
 
+static void aw2013_chip_disable_action(void *data)
+{
+	aw2013_chip_disable(data);
+}
+
 static const struct regmap_config aw2013_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -334,7 +340,10 @@ static int aw2013_probe(struct i2c_client *client)
 	if (!chip)
 		return -ENOMEM;
 
-	mutex_init(&chip->mutex);
+	ret = devm_mutex_init(&client->dev, &chip->mutex);
+	if (ret)
+		return ret;
+
 	mutex_lock(&chip->mutex);
 
 	chip->client = client;
@@ -378,6 +387,10 @@ static int aw2013_probe(struct i2c_client *client)
 		goto error_reg;
 	}
 
+	ret = devm_add_action(&client->dev, aw2013_chip_disable_action, chip);
+	if (ret)
+		goto error_reg;
+
 	ret = aw2013_probe_dt(chip);
 	if (ret < 0)
 		goto error_reg;
@@ -398,19 +411,9 @@ static int aw2013_probe(struct i2c_client *client)
 
 error:
 	mutex_unlock(&chip->mutex);
-	mutex_destroy(&chip->mutex);
 	return ret;
 }
 
-static void aw2013_remove(struct i2c_client *client)
-{
-	struct aw2013 *chip = i2c_get_clientdata(client);
-
-	aw2013_chip_disable(chip);
-
-	mutex_destroy(&chip->mutex);
-}
-
 static const struct of_device_id aw2013_match_table[] = {
 	{ .compatible = "awinic,aw2013", },
 	{ /* sentinel */ },
@@ -424,7 +427,6 @@ static struct i2c_driver aw2013_driver = {
 		.of_match_table = of_match_ptr(aw2013_match_table),
 	},
 	.probe = aw2013_probe,
-	.remove = aw2013_remove,
 };
 
 module_i2c_driver(aw2013_driver);
-- 
2.25.1

[PATCH v4 04/10] leds: aw200xx: use devm API to cleanup module's resources

[George Stark]

In this driver LEDs are registered using devm_led_classdev_register()
so they are automatically unregistered after module's remove() is done.
led_classdev_unregister() calls module's led_set_brightness() to turn off
the LEDs and that callback uses resources which were destroyed already
in module's remove() so use devm API instead of remove().

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 drivers/leds/leds-aw200xx.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index 1d3943f86f7f..eed1e65c1c0e 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -10,6 +10,7 @@
 #include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/container_of.h>
+#include <linux/devm-helpers.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/leds.h>
@@ -530,6 +531,16 @@ static const struct regmap_config aw200xx_regmap_config = {
 	.disable_locking = true,
 };
 
+static void aw200xx_chip_reset_action(void *data)
+{
+	aw200xx_chip_reset(data);
+}
+
+static void aw200xx_disable_action(void *data)
+{
+	aw200xx_disable(data);
+}
+
 static int aw200xx_probe(struct i2c_client *client)
 {
 	const struct aw200xx_chipdef *cdef;
@@ -568,11 +579,17 @@ static int aw200xx_probe(struct i2c_client *client)
 
 	aw200xx_enable(chip);
 
+	ret = devm_add_action(&client->dev, aw200xx_disable_action, chip);
+	if (ret)
+		return ret;
+
 	ret = aw200xx_chip_check(chip);
 	if (ret)
 		return ret;
 
-	mutex_init(&chip->mutex);
+	ret = devm_mutex_init(&client->dev, &chip->mutex);
+	if (ret)
+		return ret;
 
 	/* Need a lock now since after call aw200xx_probe_fw, sysfs nodes created */
 	mutex_lock(&chip->mutex);
@@ -581,6 +598,10 @@ static int aw200xx_probe(struct i2c_client *client)
 	if (ret)
 		goto out_unlock;
 
+	ret = devm_add_action(&client->dev, aw200xx_chip_reset_action, chip);
+	if (ret)
+		goto out_unlock;
+
 	ret = aw200xx_probe_fw(&client->dev, chip);
 	if (ret)
 		goto out_unlock;
@@ -595,15 +616,6 @@ static int aw200xx_probe(struct i2c_client *client)
 	return ret;
 }
 
-static void aw200xx_remove(struct i2c_client *client)
-{
-	struct aw200xx *chip = i2c_get_clientdata(client);
-
-	aw200xx_chip_reset(chip);
-	aw200xx_disable(chip);
-	mutex_destroy(&chip->mutex);
-}
-
 static const struct aw200xx_chipdef aw20036_cdef = {
 	.channels = 36,
 	.display_size_rows_max = 3,
@@ -652,7 +664,6 @@ static struct i2c_driver aw200xx_driver = {
 		.of_match_table = aw200xx_match_table,
 	},
 	.probe_new = aw200xx_probe,
-	.remove = aw200xx_remove,
 	.id_table = aw200xx_id,
 };
 module_i2c_driver(aw200xx_driver);
-- 
2.25.1

[PATCH v4 05/10] leds: lp3952: use devm API to cleanup module's resources

[George Stark]

In this driver LEDs are registered using devm_led_classdev_register()
so they are automatically unregistered after module's remove() is done.
led_classdev_unregister() calls module's led_set_brightness() to turn off
the LEDs and that callback uses resources which were destroyed already
in module's remove() so use devm API instead of remove().
Also drop explicit turning LEDs off from remove() due to they will be off
anyway by led_classdev_unregister().

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 drivers/leds/leds-lp3952.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-lp3952.c b/drivers/leds/leds-lp3952.c
index 3bd55652a706..fc0e02a9768f 100644
--- a/drivers/leds/leds-lp3952.c
+++ b/drivers/leds/leds-lp3952.c
@@ -207,6 +207,13 @@ static const struct regmap_config lp3952_regmap = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
+static void gpio_set_low_action(void *data)
+{
+	struct lp3952_led_array *priv = (struct lp3952_led_array *)data;
+
+	gpiod_set_value(priv->enable_gpio, 0);
+}
+
 static int lp3952_probe(struct i2c_client *client)
 {
 	int status;
@@ -226,6 +233,10 @@ static int lp3952_probe(struct i2c_client *client)
 		return status;
 	}
 
+	status = devm_add_action(&client->dev, gpio_set_low_action, priv);
+	if (status)
+		return status;
+
 	priv->regmap = devm_regmap_init_i2c(client, &lp3952_regmap);
 	if (IS_ERR(priv->regmap)) {
 		int err = PTR_ERR(priv->regmap);
@@ -254,15 +265,6 @@ static int lp3952_probe(struct i2c_client *client)
 	return 0;
 }
 
-static void lp3952_remove(struct i2c_client *client)
-{
-	struct lp3952_led_array *priv;
-
-	priv = i2c_get_clientdata(client);
-	lp3952_on_off(priv, LP3952_LED_ALL, false);
-	gpiod_set_value(priv->enable_gpio, 0);
-}
-
 static const struct i2c_device_id lp3952_id[] = {
 	{LP3952_NAME, 0},
 	{}
@@ -274,7 +276,6 @@ static struct i2c_driver lp3952_i2c_driver = {
 			.name = LP3952_NAME,
 	},
 	.probe = lp3952_probe,
-	.remove = lp3952_remove,
 	.id_table = lp3952_id,
 };
 
-- 
2.25.1

[PATCH v4 06/10] leds: lm3532: use devm API to cleanup module's resources

[George Stark]

In this driver LEDs are registered using devm_led_classdev_register()
so they are automatically unregistered after module's remove() is done.
led_classdev_unregister() calls module's led_set_brightness() to turn off
the LEDs and that callback uses resources which were destroyed already
in module's remove() so use devm API instead of remove().

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 drivers/leds/leds-lm3532.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index 13662a4aa1f2..713900947e35 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -3,6 +3,7 @@
 // Copyright (C) 2019 Texas Instruments Incorporated - https://www.ti.com/
 // https://www.ti.com/lit/ds/symlink/lm3532.pdf
 
+#include <linux/devm-helpers.h>
 #include <linux/i2c.h>
 #include <linux/leds.h>
 #include <linux/slab.h>
@@ -542,6 +543,13 @@ static int lm3532_parse_als(struct lm3532_data *priv)
 	return ret;
 }
 
+static void gpio_set_low_action(void *data)
+{
+	struct lm3532_data *priv = (struct lm3532_data *)data;
+
+	gpiod_direction_output(priv->enable_gpio, 0);
+}
+
 static int lm3532_parse_node(struct lm3532_data *priv)
 {
 	struct fwnode_handle *child = NULL;
@@ -556,6 +564,12 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 	if (IS_ERR(priv->enable_gpio))
 		priv->enable_gpio = NULL;
 
+	if (priv->enable_gpio) {
+		ret = devm_add_action(&priv->client->dev, gpio_set_low_action, priv);
+		if (ret)
+			return ret;
+	}
+
 	priv->regulator = devm_regulator_get(&priv->client->dev, "vin");
 	if (IS_ERR(priv->regulator))
 		priv->regulator = NULL;
@@ -691,7 +705,10 @@ static int lm3532_probe(struct i2c_client *client)
 		return ret;
 	}
 
-	mutex_init(&drvdata->lock);
+	ret = devm_mutex_init(&client->dev, &drvdata->lock);
+	if (ret)
+		return ret;
+
 	i2c_set_clientdata(client, drvdata);
 
 	ret = lm3532_parse_node(drvdata);
@@ -703,16 +720,6 @@ static int lm3532_probe(struct i2c_client *client)
 	return ret;
 }
 
-static void lm3532_remove(struct i2c_client *client)
-{
-	struct lm3532_data *drvdata = i2c_get_clientdata(client);
-
-	mutex_destroy(&drvdata->lock);
-
-	if (drvdata->enable_gpio)
-		gpiod_direction_output(drvdata->enable_gpio, 0);
-}
-
 static const struct of_device_id of_lm3532_leds_match[] = {
 	{ .compatible = "ti,lm3532", },
 	{},
@@ -727,7 +734,6 @@ MODULE_DEVICE_TABLE(i2c, lm3532_id);
 
 static struct i2c_driver lm3532_i2c_driver = {
 	.probe = lm3532_probe,
-	.remove = lm3532_remove,
 	.id_table = lm3532_id,
 	.driver = {
 		.name = LM3532_NAME,
-- 
2.25.1

[PATCH v4 07/10] leds: nic78bx: use devm API to cleanup module's resources

[George Stark]

In this driver LEDs are registered using devm_led_classdev_register()
so they are automatically unregistered after module's remove() is done.
led_classdev_unregister() calls module's led_set_brightness() to turn off
the LEDs and that callback uses resources which were destroyed already
in module's remove() so use devm API instead of remove().

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 drivers/leds/leds-nic78bx.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-nic78bx.c b/drivers/leds/leds-nic78bx.c
index f196f52eec1e..f3049fa14f04 100644
--- a/drivers/leds/leds-nic78bx.c
+++ b/drivers/leds/leds-nic78bx.c
@@ -118,6 +118,15 @@ static struct nic78bx_led nic78bx_leds[] = {
 	}
 };
 
+static void lock_led_reg_action(void *data)
+{
+	struct nic78bx_led_data *led_data = (struct nic78bx_led_data *)data;
+
+	/* Lock LED register */
+	outb(NIC78BX_LOCK_VALUE,
+	     led_data->io_base + NIC78BX_LOCK_REG_OFFSET);
+}
+
 static int nic78bx_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -152,6 +161,10 @@ static int nic78bx_probe(struct platform_device *pdev)
 	led_data->io_base = io_rc->start;
 	spin_lock_init(&led_data->lock);
 
+	ret = devm_add_action(dev, lock_led_reg_action, led_data);
+	if (ret)
+		return ret;
+
 	for (i = 0; i < ARRAY_SIZE(nic78bx_leds); i++) {
 		nic78bx_leds[i].data = led_data;
 
@@ -167,17 +180,6 @@ static int nic78bx_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int nic78bx_remove(struct platform_device *pdev)
-{
-	struct nic78bx_led_data *led_data = platform_get_drvdata(pdev);
-
-	/* Lock LED register */
-	outb(NIC78BX_LOCK_VALUE,
-	     led_data->io_base + NIC78BX_LOCK_REG_OFFSET);
-
-	return 0;
-}
-
 static const struct acpi_device_id led_device_ids[] = {
 	{"NIC78B3", 0},
 	{"", 0},
@@ -186,7 +188,6 @@ MODULE_DEVICE_TABLE(acpi, led_device_ids);
 
 static struct platform_driver led_driver = {
 	.probe = nic78bx_probe,
-	.remove = nic78bx_remove,
 	.driver = {
 		.name = KBUILD_MODNAME,
 		.acpi_match_table = ACPI_PTR(led_device_ids),
-- 
2.25.1

[PATCH v4 08/10] leds: mlxreg: use devm_mutex_init for mutex initializtion

[George Stark]

In this driver LEDs are registered using devm_led_classdev_register()
so they are automatically unregistered after module's remove() is done.
led_classdev_unregister() calls module's led_set_brightness() to turn off
the LEDs and that callback uses mutex which was destroyed already
in module's remove() so use devm API instead.

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 drivers/leds/leds-mlxreg.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/leds-mlxreg.c b/drivers/leds/leds-mlxreg.c
index b7855c93bd72..64a78eff05c7 100644
--- a/drivers/leds/leds-mlxreg.c
+++ b/drivers/leds/leds-mlxreg.c
@@ -5,6 +5,7 @@
 
 #include <linux/bitops.h>
 #include <linux/device.h>
+#include <linux/devm-helpers.h>
 #include <linux/io.h>
 #include <linux/leds.h>
 #include <linux/module.h>
@@ -258,6 +259,7 @@ static int mlxreg_led_probe(struct platform_device *pdev)
 {
 	struct mlxreg_core_platform_data *led_pdata;
 	struct mlxreg_led_priv_data *priv;
+	int err;
 
 	led_pdata = dev_get_platdata(&pdev->dev);
 	if (!led_pdata) {
@@ -269,28 +271,21 @@ static int mlxreg_led_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
-	mutex_init(&priv->access_lock);
+	err = devm_mutex_init(&pdev->dev, &priv->access_lock);
+	if (err)
+		return err;
+
 	priv->pdev = pdev;
 	priv->pdata = led_pdata;
 
 	return mlxreg_led_config(priv);
 }
 
-static int mlxreg_led_remove(struct platform_device *pdev)
-{
-	struct mlxreg_led_priv_data *priv = dev_get_drvdata(&pdev->dev);
-
-	mutex_destroy(&priv->access_lock);
-
-	return 0;
-}
-
 static struct platform_driver mlxreg_led_driver = {
 	.driver = {
 	    .name = "leds-mlxreg",
 	},
 	.probe = mlxreg_led_probe,
-	.remove = mlxreg_led_remove,
 };
 
 module_platform_driver(mlxreg_led_driver);
-- 
2.25.1

[PATCH v4 09/10] leds: an30259a: use devm_mutext_init for mutext initialization

[George Stark]

In this driver LEDs are registered using devm_led_classdev_register()
so they are automatically unregistered after module's remove() is done.
led_classdev_unregister() calls module's led_set_brightness() to turn off
the LEDs and that callback uses mutex which was destroyed already
in module's remove() so use devm API instead.

Signed-off-by: George Stark <gnstark@salutedevices.com>

---
 drivers/leds/leds-an30259a.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-an30259a.c b/drivers/leds/leds-an30259a.c
index 24b1041213c2..8f62c012c81a 100644
--- a/drivers/leds/leds-an30259a.c
+++ b/drivers/leds/leds-an30259a.c
@@ -7,6 +7,7 @@
 // Datasheet:
 // https://www.alliedelec.com/m/d/a9d2b3ee87c2d1a535a41dd747b1c247.pdf
 
+#include <linux/devm-helpers.h>
 #include <linux/i2c.h>
 #include <linux/leds.h>
 #include <linux/module.h>
@@ -283,7 +284,10 @@ static int an30259a_probe(struct i2c_client *client)
 	if (err < 0)
 		return err;
 
-	mutex_init(&chip->mutex);
+	err = devm_mutex_init(&client->dev, &chip->mutex);
+	if (err)
+		return err;
+
 	chip->client = client;
 	i2c_set_clientdata(client, chip);
 
@@ -317,17 +321,9 @@ static int an30259a_probe(struct i2c_client *client)
 	return 0;
 
 exit:
-	mutex_destroy(&chip->mutex);
 	return err;
 }
 
-static void an30259a_remove(struct i2c_client *client)
-{
-	struct an30259a *chip = i2c_get_clientdata(client);
-
-	mutex_destroy(&chip->mutex);
-}
-
 static const struct of_device_id an30259a_match_table[] = {
 	{ .compatible = "panasonic,an30259a", },
 	{ /* sentinel */ },
@@ -347,7 +343,6 @@ static struct i2c_driver an30259a_driver = {
 		.of_match_table = of_match_ptr(an30259a_match_table),
 	},
 	.probe = an30259a_probe,
-	.remove = an30259a_remove,
 	.id_table = an30259a_id,
 };
 
-- 
2.25.1

[PATCH v4 10/10] leds: powernv: use LED_RETAIN_AT_SHUTDOWN flag for leds

[George Stark]

This driver wants to keep its LEDs state after module is removed
and implemented it in its own way. LED subsystem supports dedicated
flag LED_RETAIN_AT_SHUTDOWN for the same purpose so use the flag
instead of custom implementation.

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 drivers/leds/leds-powernv.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
index 743e2cdd0891..018ec933ac10 100644
--- a/drivers/leds/leds-powernv.c
+++ b/drivers/leds/leds-powernv.c
@@ -30,15 +30,6 @@ static const struct led_type_map led_type_map[] = {
 };
 
 struct powernv_led_common {
-	/*
-	 * By default unload path resets all the LEDs. But on PowerNV
-	 * platform we want to retain LED state across reboot as these
-	 * are controlled by firmware. Also service processor can modify
-	 * the LEDs independent of OS. Hence avoid resetting LEDs in
-	 * unload path.
-	 */
-	bool		led_disabled;
-
 	/* Max supported LED type */
 	__be64		max_led_type;
 
@@ -178,10 +169,6 @@ static int powernv_brightness_set(struct led_classdev *led_cdev,
 	struct powernv_led_common *powernv_led_common = powernv_led->common;
 	int rc;
 
-	/* Do not modify LED in unload path */
-	if (powernv_led_common->led_disabled)
-		return 0;
-
 	mutex_lock(&powernv_led_common->lock);
 	rc = powernv_led_set(powernv_led, value);
 	mutex_unlock(&powernv_led_common->lock);
@@ -225,6 +212,14 @@ static int powernv_led_create(struct device *dev,
 
 	powernv_led->cdev.brightness_set_blocking = powernv_brightness_set;
 	powernv_led->cdev.brightness_get = powernv_brightness_get;
+	/*
+	 * By default unload path resets all the LEDs. But on PowerNV
+	 * platform we want to retain LED state across reboot as these
+	 * are controlled by firmware. Also service processor can modify
+	 * the LEDs independent of OS. Hence avoid resetting LEDs in
+	 * unload path.
+	 */
+	powernv_led->cdev.flags = LED_RETAIN_AT_SHUTDOWN;
 	powernv_led->cdev.brightness = LED_OFF;
 	powernv_led->cdev.max_brightness = LED_FULL;
 
@@ -313,9 +308,7 @@ static int powernv_led_remove(struct platform_device *pdev)
 {
 	struct powernv_led_common *powernv_led_common;
 
-	/* Disable LED operation */
 	powernv_led_common = platform_get_drvdata(pdev);
-	powernv_led_common->led_disabled = true;
 
 	/* Destroy lock */
 	mutex_destroy(&powernv_led_common->lock);
-- 
2.25.1

Re: [PATCH v4 00/10] devm_led_classdev_register() usage problem

[Lee Jones]

On Thu, 14 Dec 2023, George Stark wrote:

> This patch series fixes the problem of devm_led_classdev_register misusing.
> 
> The basic problem is described in [1]. Shortly when devm_led_classdev_register()
> is used then led_classdev_unregister() called after driver's remove() callback.
> led_classdev_unregister() calls driver's brightness_set callback and that callback
> may use resources which were destroyed already in driver's remove().
> 
> After discussion with maintainers [2] [3] we decided:
> 1) don't touch led subsytem core code and don't remove led_set_brightness() from it
> but fix drivers
> 2) don't use devm_led_classdev_unregister
> 
> So the solution is to use devm wrappers for all resources
> driver's brightness_set() depends on. And introduce dedicated devm wrapper
> for mutex as it's often used resource.
> 
> [1] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/
> [2] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mc132b9b350fa51931b4fcfe14705d9f06e91421f
> [3] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mdbf572a85c33f869a553caf986b6228bb65c8383
> 
> Changelog:
> v1->v2:
> 	revise patch series completely
> 
> v2->v3:
> locking: add define if mutex_destroy() is not an empty function
> 	new patch, discussed here [8]
> 
> devm-helpers: introduce devm_mutex_init
> 	previous version [4]
> 	- revise code based on mutex_destroy define
> 	- update commit message
> 	- update devm_mutex_init()'s description
> 
> leds: aw2013: unlock mutex before destroying it
> 	previous version [5]
> 	- make this patch first in the series
> 	- add tags Fixes and RvB by Andy 
> 
> leds: aw2013: use devm API to cleanup module's resources
> 	previous version [6]
> 	- make aw2013_chip_disable_action()'s body oneline
> 	- don't shadow devm_mutex_init() return code
> 
> leds: aw200xx: use devm API to cleanup module's resources
> 	previous version [7]
> 	- make aw200xx_*_action()'s bodies oneline
> 	- don't shadow devm_mutex_init() return code
> 
> leds: lm3532: use devm API to cleanup module's resources
> leds: nic78bx: use devm API to cleanup module's resources
> leds: mlxreg: use devm_mutex_init for mutex initializtion
> leds: an30259a: use devm_mutext_init for mutext initialization
> leds: powernv: add LED_RETAIN_AT_SHUTDOWN flag for leds
> 	- those pathes were planned but not sent in the series #2 due to mail server
> 	problem on my side. I revised them according to the comments.
> 
> v3->v4:
> locking: introduce devm_mutex_init
> 	new patch
> 	- move devm_mutex_init implementation completely from devm-helpers.h to mutex.h
> 
> locking: add define if mutex_destroy() is not an empty function
> 	drop the patch [9]
> 
> devm-helpers: introduce devm_mutex_init
> 	drop the patch [10]
> 
> leds: aw2013: use devm API to cleanup module's resources
> 	- add tag Tested-by: Nikita Travkin <nikita@trvn.ru>
> 
> [4] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#mf500af0eda2a9ffc95594607dbe4cb64f2e3c9a8
> [5] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#mc92df4fb4f7d4187fb01cc1144acfa5fb5230dd2
> [6] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#m300df89710c43cc2ab598baa16c68dd0a0d7d681
> [7] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#m8e5c65e0c6b137c91fa00bb9320ad581164d1d0b
> [8] https://lore.kernel.org/lkml/377e4437-7051-4d88-ae68-1460bcd692e1@redhat.com/T/#m5f84a4a2f387d49678783e652b9e658e02c27450
> [9] https://lore.kernel.org/lkml/20231213223020.2713164-1-gnstark@salutedevices.com/T/#m19ad1fc04c560012c1e27418e3156d0c9306dd84
> [10] https://lore.kernel.org/lkml/20231213223020.2713164-1-gnstark@salutedevices.com/T/#m63126025f5d1bdcef69bcad50f2e58274d42e2d7
> 
> George Stark (10):
>   leds: aw2013: unlock mutex before destroying it
>   locking: introduce devm_mutex_init
>   leds: aw2013: use devm API to cleanup module's resources
>   leds: aw200xx: use devm API to cleanup module's resources
>   leds: lp3952: use devm API to cleanup module's resources
>   leds: lm3532: use devm API to cleanup module's resources
>   leds: nic78bx: use devm API to cleanup module's resources
>   leds: mlxreg: use devm_mutex_init for mutex initializtion
>   leds: an30259a: use devm_mutext_init for mutext initialization
>   leds: powernv: use LED_RETAIN_AT_SHUTDOWN flag for leds
> 
>  drivers/leds/leds-an30259a.c | 15 +++++----------
>  drivers/leds/leds-aw200xx.c  | 33 ++++++++++++++++++++++-----------
>  drivers/leds/leds-aw2013.c   | 27 +++++++++++++++------------
>  drivers/leds/leds-lm3532.c   | 30 ++++++++++++++++++------------
>  drivers/leds/leds-lp3952.c   | 21 +++++++++++----------
>  drivers/leds/leds-mlxreg.c   | 17 ++++++-----------
>  drivers/leds/leds-nic78bx.c  | 25 +++++++++++++------------
>  drivers/leds/leds-powernv.c  | 23 ++++++++---------------

FYI: I'll conduct my review once the locking side is settled.

>  include/linux/mutex.h        | 23 +++++++++++++++++++++++
>  kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
>  10 files changed, 143 insertions(+), 93 deletions(-)
> 
> -- 
> 2.25.1
> 
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v4 00/10] devm_led_classdev_register() usage problem

[Andy Shevchenko]

On Thu, Dec 21, 2023 at 03:11:11PM +0000, Lee Jones wrote:
> On Thu, 14 Dec 2023, George Stark wrote:
> 
> > This patch series fixes the problem of devm_led_classdev_register misusing.
> > 
> > The basic problem is described in [1]. Shortly when devm_led_classdev_register()
> > is used then led_classdev_unregister() called after driver's remove() callback.
> > led_classdev_unregister() calls driver's brightness_set callback and that callback
> > may use resources which were destroyed already in driver's remove().
> > 
> > After discussion with maintainers [2] [3] we decided:
> > 1) don't touch led subsytem core code and don't remove led_set_brightness() from it
> > but fix drivers
> > 2) don't use devm_led_classdev_unregister
> > 
> > So the solution is to use devm wrappers for all resources
> > driver's brightness_set() depends on. And introduce dedicated devm wrapper
> > for mutex as it's often used resource.
> > 
> > [1] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/
> > [2] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mc132b9b350fa51931b4fcfe14705d9f06e91421f
> > [3] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mdbf572a85c33f869a553caf986b6228bb65c8383

...

> FYI: I'll conduct my review once the locking side is settled.

To reduce burden can you apply the first one? It's a fix.

-- 
With Best Regards,
Andy Shevchenko

Re: [DMARC error][SPF error] Re: [PATCH v4 00/10] devm_led_classdev_register() usage problem

[George Stark]

Hello Andy.


I haven't lose hope for the devm_mutex thing and keep pinging those guys 
from time to time.

Sure I can single out the fix-only patch I'll do it tomorrow.


On 2/9/24 20:11, Andy Shevchenko wrote:
> On Thu, Dec 21, 2023 at 03:11:11PM +0000, Lee Jones wrote:
>> On Thu, 14 Dec 2023, George Stark wrote:
>>
>>> This patch series fixes the problem of devm_led_classdev_register misusing.
>>>
>>> The basic problem is described in [1]. Shortly when devm_led_classdev_register()
>>> is used then led_classdev_unregister() called after driver's remove() callback.
>>> led_classdev_unregister() calls driver's brightness_set callback and that callback
>>> may use resources which were destroyed already in driver's remove().
>>>
>>> After discussion with maintainers [2] [3] we decided:
>>> 1) don't touch led subsytem core code and don't remove led_set_brightness() from it
>>> but fix drivers
>>> 2) don't use devm_led_classdev_unregister
>>>
>>> So the solution is to use devm wrappers for all resources
>>> driver's brightness_set() depends on. And introduce dedicated devm wrapper
>>> for mutex as it's often used resource.
>>>
>>> [1] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/
>>> [2] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mc132b9b350fa51931b4fcfe14705d9f06e91421f
>>> [3] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mdbf572a85c33f869a553caf986b6228bb65c8383
> 
> ...
> 
>> FYI: I'll conduct my review once the locking side is settled.
> 
> To reduce burden can you apply the first one? It's a fix.
> 

-- 
Best regards
George

Re: [DMARC error][SPF error] Re: [PATCH v4 00/10] devm_led_classdev_register() usage problem

[Andy Shevchenko]

On Mon, Feb 12, 2024 at 1:52 AM George Stark <gnstark@salutedevices.com> wrote:
> I haven't lose hope for the devm_mutex thing and keep pinging those guys
> from time to time.

I don't understand. According to v4 thread Christophe proposed on how
the patch should look like. What you need is to incorporate an updated
version into your series. Am I wrong?

> Sure I can single out the fix-only patch I'll do it tomorrow.

I believe it can be handled without issuing it separately. `b4` tool
is capable of selective choices. It was rather Q to Lee if he can/want
to apply it right away.

> On 2/9/24 20:11, Andy Shevchenko wrote:
> > On Thu, Dec 21, 2023 at 03:11:11PM +0000, Lee Jones wrote:
> >> On Thu, 14 Dec 2023, George Stark wrote:
> >>
> >>> This patch series fixes the problem of devm_led_classdev_register misusing.
> >>>
> >>> The basic problem is described in [1]. Shortly when devm_led_classdev_register()
> >>> is used then led_classdev_unregister() called after driver's remove() callback.
> >>> led_classdev_unregister() calls driver's brightness_set callback and that callback
> >>> may use resources which were destroyed already in driver's remove().
> >>>
> >>> After discussion with maintainers [2] [3] we decided:
> >>> 1) don't touch led subsytem core code and don't remove led_set_brightness() from it
> >>> but fix drivers
> >>> 2) don't use devm_led_classdev_unregister
> >>>
> >>> So the solution is to use devm wrappers for all resources
> >>> driver's brightness_set() depends on. And introduce dedicated devm wrapper
> >>> for mutex as it's often used resource.
> >>>
> >>> [1] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/
> >>> [2] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mc132b9b350fa51931b4fcfe14705d9f06e91421f
> >>> [3] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mdbf572a85c33f869a553caf986b6228bb65c8383
> >
> > ...
> >
> >> FYI: I'll conduct my review once the locking side is settled.
> >
> > To reduce burden can you apply the first one? It's a fix.

-- 
With Best Regards,
Andy Shevchenko

Re: [DMARC error][SPF error] Re: [PATCH v4 00/10] devm_led_classdev_register() usage problem

[George Stark]

Hello Andy

On 2/12/24 12:53, Andy Shevchenko wrote:
> On Mon, Feb 12, 2024 at 1:52 AM George Stark <gnstark@salutedevices.com> wrote:
>> I haven't lose hope for the devm_mutex thing and keep pinging those guys
>> from time to time.
> 
> I don't understand. According to v4 thread Christophe proposed on how
> the patch should look like. What you need is to incorporate an updated
> version into your series. Am I wrong?

We agreed that the effective way of implementing devm_mutex_init() is in 
mutex.h using forward declaration of struct device.
The only inconvenient thing is that in the mutex.h mutex_init() declared 
after mutex_destroy() so we'll have to use condition #ifdef 
CONFIG_DEBUG_MUTEXES twice. Waiman Long proposed great cleanup patch [1] 
that eliminates the need of doubling #ifdef. That patch was reviewed a 
bit but it's still unapplied (near 2 months). I'm still trying to 
contact mutex.h guys but there're no any feedback yet.

[1] 
https://lore.kernel.org/lkml/20231216013656.1382213-2-longman@redhat.com/T/#m795b230d662c1debb28463ad721ddba5b384340a


> 
>> Sure I can single out the fix-only patch I'll do it tomorrow.
> 
> I believe it can be handled without issuing it separately. `b4` tool
> is capable of selective choices. It was rather Q to Lee if he can/want
> to apply it right away.

Oh ok, that would be great.

> 
>> On 2/9/24 20:11, Andy Shevchenko wrote:
>>> On Thu, Dec 21, 2023 at 03:11:11PM +0000, Lee Jones wrote:
>>>> On Thu, 14 Dec 2023, George Stark wrote:
>>>>
>>>>> This patch series fixes the problem of devm_led_classdev_register misusing.
>>>>>
>>>>> The basic problem is described in [1]. Shortly when devm_led_classdev_register()
>>>>> is used then led_classdev_unregister() called after driver's remove() callback.
>>>>> led_classdev_unregister() calls driver's brightness_set callback and that callback
>>>>> may use resources which were destroyed already in driver's remove().
>>>>>
>>>>> After discussion with maintainers [2] [3] we decided:
>>>>> 1) don't touch led subsytem core code and don't remove led_set_brightness() from it
>>>>> but fix drivers
>>>>> 2) don't use devm_led_classdev_unregister
>>>>>
>>>>> So the solution is to use devm wrappers for all resources
>>>>> driver's brightness_set() depends on. And introduce dedicated devm wrapper
>>>>> for mutex as it's often used resource.
>>>>>
>>>>> [1] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/
>>>>> [2] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mc132b9b350fa51931b4fcfe14705d9f06e91421f
>>>>> [3] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mdbf572a85c33f869a553caf986b6228bb65c8383
>>>
>>> ...
>>>
>>>> FYI: I'll conduct my review once the locking side is settled.
>>>
>>> To reduce burden can you apply the first one? It's a fix.
> 

-- 
Best regards
George

Re: [DMARC error][SPF error] Re: [PATCH v4 00/10] devm_led_classdev_register() usage problem

[Andy Shevchenko]

On Tue, Feb 13, 2024 at 2:14 AM George Stark <gnstark@salutedevices.com> wrote:
>
> Hello Andy
>
> On 2/12/24 12:53, Andy Shevchenko wrote:
> > On Mon, Feb 12, 2024 at 1:52 AM George Stark <gnstark@salutedevices.com> wrote:
> >> I haven't lose hope for the devm_mutex thing and keep pinging those guys
> >> from time to time.
> >
> > I don't understand. According to v4 thread Christophe proposed on how
> > the patch should look like. What you need is to incorporate an updated
> > version into your series. Am I wrong?
>
> We agreed that the effective way of implementing devm_mutex_init() is in
> mutex.h using forward declaration of struct device.
> The only inconvenient thing is that in the mutex.h mutex_init() declared
> after mutex_destroy() so we'll have to use condition #ifdef
> CONFIG_DEBUG_MUTEXES twice. Waiman Long proposed great cleanup patch [1]
> that eliminates the need of doubling #ifdef. That patch was reviewed a
> bit but it's still unapplied (near 2 months). I'm still trying to
> contact mutex.h guys but there're no any feedback yet.

So the roadmap (as I see it) is:
- convince Lee to take the first patch while waiting for the others
- incorporate the above mentioned patch into your series
- make an ultimatum in case there is no reaction to get it applied on
deadline, let's say within next cycle (if Lee is okay with a such, but
this is normal practice when some maintainers are non-responsive)

P.S. In case Lee doesn't want to take the first patch separately
(let's say this week), send a new version with amended patches
included.

> [1]
> https://lore.kernel.org/lkml/20231216013656.1382213-2-longman@redhat.com/T/#m795b230d662c1debb28463ad721ddba5b384340a


-- 
With Best Regards,
Andy Shevchenko

Re: [DMARC error][SPF error] Re: [PATCH v4 00/10] devm_led_classdev_register() usage problem

[George Stark]

Hello Andy

On 2/13/24 13:55, Andy Shevchenko wrote:
> On Tue, Feb 13, 2024 at 2:14 AM George Stark <gnstark@salutedevices.com> wrote:
>>
>> Hello Andy
>>
>> On 2/12/24 12:53, Andy Shevchenko wrote:
>>> On Mon, Feb 12, 2024 at 1:52 AM George Stark <gnstark@salutedevices.com> wrote:
>>>> I haven't lose hope for the devm_mutex thing and keep pinging those guys
>>>> from time to time.
>>>
>>> I don't understand. According to v4 thread Christophe proposed on how
>>> the patch should look like. What you need is to incorporate an updated
>>> version into your series. Am I wrong?
>>
>> We agreed that the effective way of implementing devm_mutex_init() is in
>> mutex.h using forward declaration of struct device.
>> The only inconvenient thing is that in the mutex.h mutex_init() declared
>> after mutex_destroy() so we'll have to use condition #ifdef
>> CONFIG_DEBUG_MUTEXES twice. Waiman Long proposed great cleanup patch [1]
>> that eliminates the need of doubling #ifdef. That patch was reviewed a
>> bit but it's still unapplied (near 2 months). I'm still trying to
>> contact mutex.h guys but there're no any feedback yet.
> 
> So the roadmap (as I see it) is:
> - convince Lee to take the first patch while waiting for the others
> - incorporate the above mentioned patch into your series
> - make an ultimatum in case there is no reaction to get it applied on
> deadline, let's say within next cycle (if Lee is okay with a such, but
> this is normal practice when some maintainers are non-responsive)

Well, it was interesting to know that there is such a practice.

Waiman Long has just updated his patches with mutex.h cleanup [1] so I
think we can wait for that series to be merged than I'll prepare may
patchseries with or w\o the first patch.

[1] 
https://lore.kernel.org/all/20240213031656.1375951-4-longman@redhat.com/T/

> 
> P.S. In case Lee doesn't want to take the first patch separately
> (let's say this week), send a new version with amended patches
> included.

Ok

> 
>> [1]
>> https://lore.kernel.org/lkml/20231216013656.1382213-2-longman@redhat.com/T/#m795b230d662c1debb28463ad721ddba5b384340a
> 
> 

-- 
Best regards
George

Re: [PATCH v4 00/10] devm_led_classdev_register() usage problem

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 08:36:04PM +0300, George Stark wrote:
> This patch series fixes the problem of devm_led_classdev_register misusing.
> 
> The basic problem is described in [1]. Shortly when devm_led_classdev_register()
> is used then led_classdev_unregister() called after driver's remove() callback.
> led_classdev_unregister() calls driver's brightness_set callback and that callback
> may use resources which were destroyed already in driver's remove().
> 
> After discussion with maintainers [2] [3] we decided:
> 1) don't touch led subsytem core code and don't remove led_set_brightness() from it
> but fix drivers
> 2) don't use devm_led_classdev_unregister
> 
> So the solution is to use devm wrappers for all resources
> driver's brightness_set() depends on. And introduce dedicated devm wrapper
> for mutex as it's often used resource.
> 
> [1] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/
> [2] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mc132b9b350fa51931b4fcfe14705d9f06e91421f
> [3] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mdbf572a85c33f869a553caf986b6228bb65c8383

Are you going to send an updated version with the amended second patch?

-- 
With Best Regards,
Andy Shevchenko