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

[PATCH v5 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>

v4->v5:
leds: aw2013: unlock mutex before destroying it
	merged separately and removed from the series

locking/mutex: move mutex_destroy() definition lower
	introduce optional refactoring patch

locking/mutex: introduce devm_mutex_init
	leave only one devm_mutex_init definition
	add tag Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

leds* patches
	remove #include <linux/devm-helpers.h> due to devm_mutext_init in mutex.h now

[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/#m63126025f5d1bdcef69bcad50f2e58274d42e2d

George Stark (10):
  locking/mutex: move mutex_destroy() definition lower
  locking/mutex: 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 | 14 ++++----------
 drivers/leds/leds-aw200xx.c  | 32 +++++++++++++++++++++-----------
 drivers/leds/leds-aw2013.c   | 25 +++++++++++++------------
 drivers/leds/leds-lm3532.c   | 29 +++++++++++++++++------------
 drivers/leds/leds-lp3952.c   | 21 +++++++++++----------
 drivers/leds/leds-mlxreg.c   | 14 +++++---------
 drivers/leds/leds-nic78bx.c  | 23 +++++++++++++----------
 drivers/leds/leds-powernv.c  | 23 ++++++++---------------
 include/linux/mutex.h        | 27 +++++++++++++++++++++++----
 kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
 10 files changed, 137 insertions(+), 93 deletions(-)

--
2.25.1

[PATCH v5 01/10] locking/mutex: move mutex_destroy() definition lower

[George Stark]

mutex_destroy() definition is located in the middle of the code related
purely to mutex initialization so place it separately after mutex_init()

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
Hello Waiman. This is helper patch to make resulting mutex.h look like we discussed
it in December. It was in you cleanup patch at first but slept away somehow

 include/linux/mutex.h | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 67edc4ca2bee..f7611c092db7 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -37,14 +37,10 @@
 # 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

 #ifndef CONFIG_PREEMPT_RT
@@ -117,6 +113,16 @@ do {							\
 } while (0)
 #endif /* CONFIG_PREEMPT_RT */

+#ifdef CONFIG_DEBUG_MUTEXES
+
+void mutex_destroy(struct mutex *lock);
+
+#else
+
+static inline void mutex_destroy(struct mutex *lock) {}
+
+#endif
+
 /*
  * See kernel/locking/mutex.c for detailed documentation of these APIs.
  * Also see Documentation/locking/mutex-design.rst.
--
2.25.1

[PATCH v5 02/10] locking/mutex: 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>
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot
 to make this patch happen.

 include/linux/mutex.h        | 13 +++++++++++++
 kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index f7611c092db7..9bcf72cb941a 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -22,6 +22,8 @@
 #include <linux/cleanup.h>
 #include <linux/mutex_types.h>

+struct device;
+
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 # define __DEP_MAP_MUTEX_INITIALIZER(lockname)			\
 		, .dep_map = {					\
@@ -115,10 +117,21 @@ do {							\

 #ifdef CONFIG_DEBUG_MUTEXES

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

 #else

+static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+	/*
+	 * since mutex_destroy is nop actually there's no need to register it
+	 * in devm subsystem.
+	 */
+	mutex_init(lock);
+	return 0;
+}
+
 static inline void mutex_destroy(struct mutex *lock) {}

 #endif
diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
index bc8abb8549d2..c9efab1a8026 100644
--- a/kernel/locking/mutex-debug.c
+++ b/kernel/locking/mutex-debug.c
@@ -19,6 +19,7 @@
 #include <linux/kallsyms.h>
 #include <linux/interrupt.h>
 #include <linux/debug_locks.h>
+#include <linux/device.h>

 #include "mutex.h"

@@ -104,3 +105,24 @@ void mutex_destroy(struct mutex *lock)
 }

 EXPORT_SYMBOL_GPL(mutex_destroy);
+
+static void devm_mutex_release(void *res)
+{
+	mutex_destroy(res);
+}
+
+/**
+ * devm_mutex_init - Resource-managed mutex initialization
+ * @dev:	Device which lifetime mutex is bound to
+ * @lock:	Pointer to a mutex
+ *
+ * Initialize mutex which is automatically destroyed when the driver is detached.
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ */
+int devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+	mutex_init(lock);
+	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
+}
+EXPORT_SYMBOL_GPL(devm_mutex_init);
--
2.25.1

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[Marek Behún]

On Thu, Mar 07, 2024 at 05:40:26AM +0300, George Stark wrote:
> Using of devm API leads to a certain order of releasing resources.
> So all dependent resources which are not devm-wrapped should be deleted
> with respect to devm-release order. Mutex is one of such objects that
> often is bound to other resources and has no own devm wrapping.
> Since mutex_destroy() actually does nothing in non-debug builds
> frequently calling mutex_destroy() is just ignored which is safe for now
> but wrong formally and can lead to a problem if mutex_destroy() will be
> extended so introduce devm_mutex_init()
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot
>  to make this patch happen.
> 
>  include/linux/mutex.h        | 13 +++++++++++++
>  kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index f7611c092db7..9bcf72cb941a 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -22,6 +22,8 @@
>  #include <linux/cleanup.h>
>  #include <linux/mutex_types.h>
> 
> +struct device;
> +
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
>  # define __DEP_MAP_MUTEX_INITIALIZER(lockname)			\
>  		, .dep_map = {					\
> @@ -115,10 +117,21 @@ do {							\
> 
>  #ifdef CONFIG_DEBUG_MUTEXES
> 
> +int devm_mutex_init(struct device *dev, struct mutex *lock);
>  void mutex_destroy(struct mutex *lock);
> 
>  #else
> 
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +	/*
> +	 * since mutex_destroy is nop actually there's no need to register it
> +	 * in devm subsystem.
> +	 */
> +	mutex_init(lock);
> +	return 0;
> +}
> +
>  static inline void mutex_destroy(struct mutex *lock) {}
> 
>  #endif
> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
> index bc8abb8549d2..c9efab1a8026 100644
> --- a/kernel/locking/mutex-debug.c
> +++ b/kernel/locking/mutex-debug.c
> @@ -19,6 +19,7 @@
>  #include <linux/kallsyms.h>
>  #include <linux/interrupt.h>
>  #include <linux/debug_locks.h>
> +#include <linux/device.h>
> 
>  #include "mutex.h"
> 
> @@ -104,3 +105,24 @@ void mutex_destroy(struct mutex *lock)
>  }
> 
>  EXPORT_SYMBOL_GPL(mutex_destroy);
> +
> +static void devm_mutex_release(void *res)
> +{
> +	mutex_destroy(res);
> +}
> +
> +/**
> + * devm_mutex_init - Resource-managed mutex initialization
> + * @dev:	Device which lifetime mutex is bound to
> + * @lock:	Pointer to a mutex
> + *
> + * Initialize mutex which is automatically destroyed when the driver is detached.
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +	mutex_init(lock);
> +	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> +}
> +EXPORT_SYMBOL_GPL(devm_mutex_init);

Hi George,

look at
https://lore.kernel.org/lkml/7013bf9e-2663-4613-ae61-61872e81355b@redhat.com/
where Matthew and Hans explain that devm_mutex_init needs to be a macro
because of the static lockdep key. 

so this should be something like:

static inline int __devm_mutex_init(struct device *dev, struct mutex *mutex,
				    const char *name,
				    struct lock_class_key *key)
{
	__mutex_init(mutex, name, key);
	return devm_add_action_or_reset(dev, devm_mutex_release, mutex);
}

#define devm_mutex_init(dev, mutex)				\
do {								\
	static struct lock_class_key __key;			\
								\
	__devm_mutex_init(dev, (mutex), #mutex, &__key);	\
} while (0);


Marek

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[Waiman Long]

On 3/7/24 04:56, Marek Behún wrote:
> On Thu, Mar 07, 2024 at 05:40:26AM +0300, George Stark wrote:
>> Using of devm API leads to a certain order of releasing resources.
>> So all dependent resources which are not devm-wrapped should be deleted
>> with respect to devm-release order. Mutex is one of such objects that
>> often is bound to other resources and has no own devm wrapping.
>> Since mutex_destroy() actually does nothing in non-debug builds
>> frequently calling mutex_destroy() is just ignored which is safe for now
>> but wrong formally and can lead to a problem if mutex_destroy() will be
>> extended so introduce devm_mutex_init()
>>
>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot
>>   to make this patch happen.
>>
>>   include/linux/mutex.h        | 13 +++++++++++++
>>   kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
>>   2 files changed, 35 insertions(+)
>>
>> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
>> index f7611c092db7..9bcf72cb941a 100644
>> --- a/include/linux/mutex.h
>> +++ b/include/linux/mutex.h
>> @@ -22,6 +22,8 @@
>>   #include <linux/cleanup.h>
>>   #include <linux/mutex_types.h>
>>
>> +struct device;
>> +
>>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>   # define __DEP_MAP_MUTEX_INITIALIZER(lockname)			\
>>   		, .dep_map = {					\
>> @@ -115,10 +117,21 @@ do {							\
>>
>>   #ifdef CONFIG_DEBUG_MUTEXES
>>
>> +int devm_mutex_init(struct device *dev, struct mutex *lock);
>>   void mutex_destroy(struct mutex *lock);
>>
>>   #else
>>
>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>> +{
>> +	/*
>> +	 * since mutex_destroy is nop actually there's no need to register it
>> +	 * in devm subsystem.
>> +	 */
>> +	mutex_init(lock);
>> +	return 0;
>> +}
>> +
>>   static inline void mutex_destroy(struct mutex *lock) {}
>>
>>   #endif
>> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
>> index bc8abb8549d2..c9efab1a8026 100644
>> --- a/kernel/locking/mutex-debug.c
>> +++ b/kernel/locking/mutex-debug.c
>> @@ -19,6 +19,7 @@
>>   #include <linux/kallsyms.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/debug_locks.h>
>> +#include <linux/device.h>
>>
>>   #include "mutex.h"
>>
>> @@ -104,3 +105,24 @@ void mutex_destroy(struct mutex *lock)
>>   }
>>
>>   EXPORT_SYMBOL_GPL(mutex_destroy);
>> +
>> +static void devm_mutex_release(void *res)
>> +{
>> +	mutex_destroy(res);
>> +}
>> +
>> +/**
>> + * devm_mutex_init - Resource-managed mutex initialization
>> + * @dev:	Device which lifetime mutex is bound to
>> + * @lock:	Pointer to a mutex
>> + *
>> + * Initialize mutex which is automatically destroyed when the driver is detached.
>> + *
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +int devm_mutex_init(struct device *dev, struct mutex *lock)
>> +{
>> +	mutex_init(lock);
>> +	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mutex_init);
> Hi George,
>
> look at
> https://lore.kernel.org/lkml/7013bf9e-2663-4613-ae61-61872e81355b@redhat.com/
> where Matthew and Hans explain that devm_mutex_init needs to be a macro
> because of the static lockdep key.
>
> so this should be something like:
>
> static inline int __devm_mutex_init(struct device *dev, struct mutex *mutex,
> 				    const char *name,
> 				    struct lock_class_key *key)
> {
> 	__mutex_init(mutex, name, key);
> 	return devm_add_action_or_reset(dev, devm_mutex_release, mutex);
> }
>
> #define devm_mutex_init(dev, mutex)				\
> do {								\
> 	static struct lock_class_key __key;			\
> 								\
> 	__devm_mutex_init(dev, (mutex), #mutex, &__key);	\
> } while (0);
>
>
> Marek

Making devm_mutex_init() a function will make all the devm_mutex share 
the same lockdep key. Making it a macro will make each caller of 
devm_mutex_init() have a distinct lockdep key. It all depends on whether 
all the devm_mutexes have the same lock usage pattern or not and whether 
it is possible for one devm_mutex to be nested inside another. So either 
way can be fine depending on the mutex usage pattern. My suggestion is 
to use a function, if possible, unless it will cause a false positive 
lockdep splat as there is a limit on the maximum # of lockdep keys that 
can be used.

Cheers,
Longman

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[Marek Behún]

On Thu, 7 Mar 2024 08:39:46 -0500
Waiman Long <longman@redhat.com> wrote:

> On 3/7/24 04:56, Marek Behún wrote:
> > On Thu, Mar 07, 2024 at 05:40:26AM +0300, George Stark wrote:  
> >> Using of devm API leads to a certain order of releasing resources.
> >> So all dependent resources which are not devm-wrapped should be deleted
> >> with respect to devm-release order. Mutex is one of such objects that
> >> often is bound to other resources and has no own devm wrapping.
> >> Since mutex_destroy() actually does nothing in non-debug builds
> >> frequently calling mutex_destroy() is just ignored which is safe for now
> >> but wrong formally and can lead to a problem if mutex_destroy() will be
> >> extended so introduce devm_mutex_init()
> >>
> >> Signed-off-by: George Stark <gnstark@salutedevices.com>
> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> >> ---
> >>   Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot
> >>   to make this patch happen.
> >>
> >>   include/linux/mutex.h        | 13 +++++++++++++
> >>   kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
> >>   2 files changed, 35 insertions(+)
> >>
> >> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> >> index f7611c092db7..9bcf72cb941a 100644
> >> --- a/include/linux/mutex.h
> >> +++ b/include/linux/mutex.h
> >> @@ -22,6 +22,8 @@
> >>   #include <linux/cleanup.h>
> >>   #include <linux/mutex_types.h>
> >>
> >> +struct device;
> >> +
> >>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >>   # define __DEP_MAP_MUTEX_INITIALIZER(lockname)			\
> >>   		, .dep_map = {					\
> >> @@ -115,10 +117,21 @@ do {							\
> >>
> >>   #ifdef CONFIG_DEBUG_MUTEXES
> >>
> >> +int devm_mutex_init(struct device *dev, struct mutex *lock);
> >>   void mutex_destroy(struct mutex *lock);
> >>
> >>   #else
> >>
> >> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> >> +{
> >> +	/*
> >> +	 * since mutex_destroy is nop actually there's no need to register it
> >> +	 * in devm subsystem.
> >> +	 */
> >> +	mutex_init(lock);
> >> +	return 0;
> >> +}
> >> +
> >>   static inline void mutex_destroy(struct mutex *lock) {}
> >>
> >>   #endif
> >> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
> >> index bc8abb8549d2..c9efab1a8026 100644
> >> --- a/kernel/locking/mutex-debug.c
> >> +++ b/kernel/locking/mutex-debug.c
> >> @@ -19,6 +19,7 @@
> >>   #include <linux/kallsyms.h>
> >>   #include <linux/interrupt.h>
> >>   #include <linux/debug_locks.h>
> >> +#include <linux/device.h>
> >>
> >>   #include "mutex.h"
> >>
> >> @@ -104,3 +105,24 @@ void mutex_destroy(struct mutex *lock)
> >>   }
> >>
> >>   EXPORT_SYMBOL_GPL(mutex_destroy);
> >> +
> >> +static void devm_mutex_release(void *res)
> >> +{
> >> +	mutex_destroy(res);
> >> +}
> >> +
> >> +/**
> >> + * devm_mutex_init - Resource-managed mutex initialization
> >> + * @dev:	Device which lifetime mutex is bound to
> >> + * @lock:	Pointer to a mutex
> >> + *
> >> + * Initialize mutex which is automatically destroyed when the driver is detached.
> >> + *
> >> + * Returns: 0 on success or a negative error code on failure.
> >> + */
> >> +int devm_mutex_init(struct device *dev, struct mutex *lock)
> >> +{
> >> +	mutex_init(lock);
> >> +	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> >> +}
> >> +EXPORT_SYMBOL_GPL(devm_mutex_init);  
> > Hi George,
> >
> > look at
> > https://lore.kernel.org/lkml/7013bf9e-2663-4613-ae61-61872e81355b@redhat.com/
> > where Matthew and Hans explain that devm_mutex_init needs to be a macro
> > because of the static lockdep key.
> >
> > so this should be something like:
> >
> > static inline int __devm_mutex_init(struct device *dev, struct mutex *mutex,
> > 				    const char *name,
> > 				    struct lock_class_key *key)
> > {
> > 	__mutex_init(mutex, name, key);
> > 	return devm_add_action_or_reset(dev, devm_mutex_release, mutex);
> > }
> >
> > #define devm_mutex_init(dev, mutex)				\
> > do {								\
> > 	static struct lock_class_key __key;			\
> > 								\
> > 	__devm_mutex_init(dev, (mutex), #mutex, &__key);	\
> > } while (0);
> >
> >
> > Marek  
> 
> Making devm_mutex_init() a function will make all the devm_mutex share 
> the same lockdep key. Making it a macro will make each caller of 
> devm_mutex_init() have a distinct lockdep key. It all depends on whether 
> all the devm_mutexes have the same lock usage pattern or not and whether 
> it is possible for one devm_mutex to be nested inside another. So either 
> way can be fine depending on the mutex usage pattern. My suggestion is 
> to use a function, if possible, unless it will cause a false positive 
> lockdep splat as there is a limit on the maximum # of lockdep keys that 
> can be used.

devm_mutex_init() should behave like other similar function
initializing stuff with resource management. I.e. it should behave like
mutex_init(), but with resource management.

mutex_init() is a macro generating static lockdep key for each instance,
so devm_mutex_init() should also generate static lockdep key for each
instance.

Marek

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[George Stark]

Hello Waiman, Marek

Thanks for the review.

I've never used lockdep for debug but it seems preferable to
keep that feature working. It could be look like this:


diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index f7611c092db7..574f6de6084d 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -22,6 +22,8 @@
  #include <linux/cleanup.h>
  #include <linux/mutex_types.h>

+struct device;
+
  #ifdef CONFIG_DEBUG_LOCK_ALLOC
  # define __DEP_MAP_MUTEX_INITIALIZER(lockname)			\
  		, .dep_map = {					\
@@ -115,10 +117,31 @@ do {							\

  #ifdef CONFIG_DEBUG_MUTEXES

+int debug_devm_mutex_init(struct device *dev, struct mutex *lock);
+
+#define devm_mutex_init(dev, mutex)			\
+({							\
+	int ret;					\
+	mutex_init(mutex);				\
+	ret = debug_devm_mutex_init(dev, mutex);	\
+	ret;						\
+})
+
  void mutex_destroy(struct mutex *lock);

  #else

+/*
+* When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so
+* there's no really need to register it in devm subsystem.
+*/
+#define devm_mutex_init(dev, mutex)			\
+({							\
+	typecheck(struct device *, dev);		\
+	mutex_init(mutex);				\
+	0;						\
+})
+
  static inline void mutex_destroy(struct mutex *lock) {}

  #endif
diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
index bc8abb8549d2..967a5367c79a 100644
--- a/kernel/locking/mutex-debug.c
+++ b/kernel/locking/mutex-debug.c
@@ -19,6 +19,7 @@
  #include <linux/kallsyms.h>
  #include <linux/interrupt.h>
  #include <linux/debug_locks.h>
+#include <linux/device.h>

  #include "mutex.h"

@@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const char 
*name,
  	lock->magic = lock;
  }

+static void devm_mutex_release(void *res)
+{
+	mutex_destroy(res);
+}
+
+int debug_devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
+}
+
  /***
   * mutex_destroy - mark a mutex unusable
   * @lock: the mutex to be destroyed
-- 
2.25.1



And now I would drop the the refactoring patch with moving down 
mutex_destroy. devm block is big enough to be declared standalone.


On 3/7/24 19:44, Marek Behún wrote:
> On Thu, 7 Mar 2024 08:39:46 -0500
> Waiman Long <longman@redhat.com> wrote:
> 
>> On 3/7/24 04:56, Marek Behún wrote:
>>> On Thu, Mar 07, 2024 at 05:40:26AM +0300, George Stark wrote:
>>>> Using of devm API leads to a certain order of releasing resources.
>>>> So all dependent resources which are not devm-wrapped should be deleted
>>>> with respect to devm-release order. Mutex is one of such objects that
>>>> often is bound to other resources and has no own devm wrapping.
>>>> Since mutex_destroy() actually does nothing in non-debug builds
>>>> frequently calling mutex_destroy() is just ignored which is safe for now
>>>> but wrong formally and can lead to a problem if mutex_destroy() will be
>>>> extended so introduce devm_mutex_init()
>>>>
>>>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>> ---
>>>>    Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot
>>>>    to make this patch happen.
>>>>
>>>>    include/linux/mutex.h        | 13 +++++++++++++
>>>>    kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
>>>>    2 files changed, 35 insertions(+)
>>>>
>>>> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
>>>> index f7611c092db7..9bcf72cb941a 100644
>>>> --- a/include/linux/mutex.h
>>>> +++ b/include/linux/mutex.h
>>>> @@ -22,6 +22,8 @@
>>>>    #include <linux/cleanup.h>
>>>>    #include <linux/mutex_types.h>
>>>>
>>>> +struct device;
>>>> +
>>>>    #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>>>    # define __DEP_MAP_MUTEX_INITIALIZER(lockname)			\
>>>>    		, .dep_map = {					\
>>>> @@ -115,10 +117,21 @@ do {							\
>>>>
>>>>    #ifdef CONFIG_DEBUG_MUTEXES
>>>>
>>>> +int devm_mutex_init(struct device *dev, struct mutex *lock);
>>>>    void mutex_destroy(struct mutex *lock);
>>>>
>>>>    #else
>>>>
>>>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>>>> +{
>>>> +	/*
>>>> +	 * since mutex_destroy is nop actually there's no need to register it
>>>> +	 * in devm subsystem.
>>>> +	 */
>>>> +	mutex_init(lock);
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    static inline void mutex_destroy(struct mutex *lock) {}
>>>>
>>>>    #endif
>>>> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
>>>> index bc8abb8549d2..c9efab1a8026 100644
>>>> --- a/kernel/locking/mutex-debug.c
>>>> +++ b/kernel/locking/mutex-debug.c
>>>> @@ -19,6 +19,7 @@
>>>>    #include <linux/kallsyms.h>
>>>>    #include <linux/interrupt.h>
>>>>    #include <linux/debug_locks.h>
>>>> +#include <linux/device.h>
>>>>
>>>>    #include "mutex.h"
>>>>
>>>> @@ -104,3 +105,24 @@ void mutex_destroy(struct mutex *lock)
>>>>    }
>>>>
>>>>    EXPORT_SYMBOL_GPL(mutex_destroy);
>>>> +
>>>> +static void devm_mutex_release(void *res)
>>>> +{
>>>> +	mutex_destroy(res);
>>>> +}
>>>> +
>>>> +/**
>>>> + * devm_mutex_init - Resource-managed mutex initialization
>>>> + * @dev:	Device which lifetime mutex is bound to
>>>> + * @lock:	Pointer to a mutex
>>>> + *
>>>> + * Initialize mutex which is automatically destroyed when the driver is detached.
>>>> + *
>>>> + * Returns: 0 on success or a negative error code on failure.
>>>> + */
>>>> +int devm_mutex_init(struct device *dev, struct mutex *lock)
>>>> +{
>>>> +	mutex_init(lock);
>>>> +	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(devm_mutex_init);
>>> Hi George,
>>>
>>> look at
>>> https://lore.kernel.org/lkml/7013bf9e-2663-4613-ae61-61872e81355b@redhat.com/
>>> where Matthew and Hans explain that devm_mutex_init needs to be a macro
>>> because of the static lockdep key.
>>>
>>> so this should be something like:
>>>
>>> static inline int __devm_mutex_init(struct device *dev, struct mutex *mutex,
>>> 				    const char *name,
>>> 				    struct lock_class_key *key)
>>> {
>>> 	__mutex_init(mutex, name, key);
>>> 	return devm_add_action_or_reset(dev, devm_mutex_release, mutex);
>>> }
>>>
>>> #define devm_mutex_init(dev, mutex)				\
>>> do {								\
>>> 	static struct lock_class_key __key;			\
>>> 								\
>>> 	__devm_mutex_init(dev, (mutex), #mutex, &__key);	\
>>> } while (0);
>>>
>>>
>>> Marek
>>
>> Making devm_mutex_init() a function will make all the devm_mutex share
>> the same lockdep key. Making it a macro will make each caller of
>> devm_mutex_init() have a distinct lockdep key. It all depends on whether
>> all the devm_mutexes have the same lock usage pattern or not and whether
>> it is possible for one devm_mutex to be nested inside another. So either
>> way can be fine depending on the mutex usage pattern. My suggestion is
>> to use a function, if possible, unless it will cause a false positive
>> lockdep splat as there is a limit on the maximum # of lockdep keys that
>> can be used.
> 
> devm_mutex_init() should behave like other similar function
> initializing stuff with resource management. I.e. it should behave like
> mutex_init(), but with resource management.
> 
> mutex_init() is a macro generating static lockdep key for each instance,
> so devm_mutex_init() should also generate static lockdep key for each
> instance.
> 
> Marek

-- 
Best regards
George

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[Waiman Long]

On 3/11/24 19:47, George Stark wrote:
> Hello Waiman, Marek
>
> Thanks for the review.
>
> I've never used lockdep for debug but it seems preferable to
> keep that feature working. It could be look like this:
>
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index f7611c092db7..574f6de6084d 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -22,6 +22,8 @@
>  #include <linux/cleanup.h>
>  #include <linux/mutex_types.h>
>
> +struct device;
> +
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
>  # define __DEP_MAP_MUTEX_INITIALIZER(lockname)            \
>          , .dep_map = {                    \
> @@ -115,10 +117,31 @@ do {                            \
>
>  #ifdef CONFIG_DEBUG_MUTEXES
>
> +int debug_devm_mutex_init(struct device *dev, struct mutex *lock);
> +
> +#define devm_mutex_init(dev, mutex)            \
> +({                            \
> +    int ret;                    \
> +    mutex_init(mutex);                \
> +    ret = debug_devm_mutex_init(dev, mutex);    \
> +    ret;                        \
> +})

The int ret variable is not needed. The macro can just end with 
debug_devm_mutex_init().


> +
>  void mutex_destroy(struct mutex *lock);
>
>  #else
>
> +/*
> +* When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so
> +* there's no really need to register it in devm subsystem.
"no really need"?
> +*/
> +#define devm_mutex_init(dev, mutex)            \
> +({                            \
> +    typecheck(struct device *, dev);        \
> +    mutex_init(mutex);                \
> +    0;                        \
> +})

Do we need a typecheck() here? Compilation will fail with 
CONFIG_DEBUG_MUTEXES if dev is not a device pointer.


> +
>  static inline void mutex_destroy(struct mutex *lock) {}
>
>  #endif
> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
> index bc8abb8549d2..967a5367c79a 100644
> --- a/kernel/locking/mutex-debug.c
> +++ b/kernel/locking/mutex-debug.c
> @@ -19,6 +19,7 @@
>  #include <linux/kallsyms.h>
>  #include <linux/interrupt.h>
>  #include <linux/debug_locks.h>
> +#include <linux/device.h>
>
>  #include "mutex.h"
>
> @@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const 
> char *name,
>      lock->magic = lock;
>  }
>
> +static void devm_mutex_release(void *res)
> +{
> +    mutex_destroy(res);
> +}
> +
> +int debug_devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +    return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> +}
> +
>  /***
>   * mutex_destroy - mark a mutex unusable
>   * @lock: the mutex to be destroyed

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[Christophe Leroy]

Le 12/03/2024 à 02:10, Waiman Long a écrit :
> On 3/11/24 19:47, George Stark wrote:
>> Hello Waiman, Marek
>>
>> Thanks for the review.
>>
>> I've never used lockdep for debug but it seems preferable to
>> keep that feature working. It could be look like this:
>>
>> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
>> index f7611c092db7..574f6de6084d 100644
>> --- a/include/linux/mutex.h
>> +++ b/include/linux/mutex.h
>> @@ -22,6 +22,8 @@
>>  #include <linux/cleanup.h>
>>  #include <linux/mutex_types.h>
>>
>> +struct device;
>> +
>>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>  # define __DEP_MAP_MUTEX_INITIALIZER(lockname)            \
>>          , .dep_map = {                    \
>> @@ -115,10 +117,31 @@ do {                            \
>>
>>  #ifdef CONFIG_DEBUG_MUTEXES
>>
>> +int debug_devm_mutex_init(struct device *dev, struct mutex *lock);
>> +
>> +#define devm_mutex_init(dev, mutex)            \
>> +({                            \
>> +    int ret;                    \
>> +    mutex_init(mutex);                \
>> +    ret = debug_devm_mutex_init(dev, mutex);    \
>> +    ret;                        \
>> +})
> 
> The int ret variable is not needed. The macro can just end with 
> debug_devm_mutex_init().
> 
> 
>> +
>>  void mutex_destroy(struct mutex *lock);
>>
>>  #else
>>
>> +/*
>> +* When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so
>> +* there's no really need to register it in devm subsystem.
> "no really need"?
>> +*/
>> +#define devm_mutex_init(dev, mutex)            \
>> +({                            \
>> +    typecheck(struct device *, dev);        \
>> +    mutex_init(mutex);                \
>> +    0;                        \
>> +})
> 
> Do we need a typecheck() here? Compilation will fail with 
> CONFIG_DEBUG_MUTEXES if dev is not a device pointer.

I guess the idea is to have it fail _also_ when CONFIG_DEBUG_MUTEXES is 
not selected, in order to discover errors as soon as possible.

> 
> 
>> +
>>  static inline void mutex_destroy(struct mutex *lock) {}
>>
>>  #endif
>> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
>> index bc8abb8549d2..967a5367c79a 100644
>> --- a/kernel/locking/mutex-debug.c
>> +++ b/kernel/locking/mutex-debug.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/kallsyms.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/debug_locks.h>
>> +#include <linux/device.h>
>>
>>  #include "mutex.h"
>>
>> @@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const 
>> char *name,
>>      lock->magic = lock;
>>  }
>>
>> +static void devm_mutex_release(void *res)
>> +{
>> +    mutex_destroy(res);
>> +}
>> +
>> +int debug_devm_mutex_init(struct device *dev, struct mutex *lock)
>> +{
>> +    return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>> +}
>> +
>>  /***
>>   * mutex_destroy - mark a mutex unusable
>>   * @lock: the mutex to be destroyed
> 

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[Christophe Leroy]

Le 12/03/2024 à 00:47, George Stark a écrit :
> [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. 
> Découvrez pourquoi ceci est important à 
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hello Waiman, Marek
> 
> Thanks for the review.
> 
> I've never used lockdep for debug but it seems preferable to
> keep that feature working. It could be look like this:

For sure it is a must. I'm not used to it either hence my overlook.

> 
> 
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index f7611c092db7..574f6de6084d 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -22,6 +22,8 @@
>   #include <linux/cleanup.h>
>   #include <linux/mutex_types.h>
> 
> +struct device;
> +
>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
>   # define __DEP_MAP_MUTEX_INITIALIZER(lockname)                        \
>                 , .dep_map = {                                  \
> @@ -115,10 +117,31 @@ do 
> {                                                      \
> 
>   #ifdef CONFIG_DEBUG_MUTEXES
> 
> +int debug_devm_mutex_init(struct device *dev, struct mutex *lock);
> +
> +#define devm_mutex_init(dev, mutex)                    \
> +({                                                     \
> +       int ret;                                        \
> +       mutex_init(mutex);                              \
> +       ret = debug_devm_mutex_init(dev, mutex);        \
> +       ret;                                            \
> +})
> +

I think it would be preferable to minimise the number of macros.

If I were you I would keep your devm_mutex_init() as is but rename it 
__devm_mutex_init() and just remove the mutex_init() from it, then add 
only one macro that works independant of CONFIG_DEBUG_MUTEXES:

#define devm_mutex_init(dev, mutex)	\
({					\
	mutex_init(mutex);		\
	__devm_mutex_init(dev, mutex);	\
})

With that, no need of a second version of the macro and no need for the 
typecheck either.

Note the __ which is a clear indication that allthough that function is 
declared in public mutex.h, it is not meant to be used outside of it.



>   void mutex_destroy(struct mutex *lock);
> 
>   #else
> 
> +/*
> +* When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so
> +* there's no really need to register it in devm subsystem.
> +*/
> +#define devm_mutex_init(dev, mutex)                    \
> +({                                                     \
> +       typecheck(struct device *, dev);                \
> +       mutex_init(mutex);                              \
> +       0;                                              \
> +})
> +
>   static inline void mutex_destroy(struct mutex *lock) {}
> 
>   #endif
> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
> index bc8abb8549d2..967a5367c79a 100644
> --- a/kernel/locking/mutex-debug.c
> +++ b/kernel/locking/mutex-debug.c
> @@ -19,6 +19,7 @@
>   #include <linux/kallsyms.h>
>   #include <linux/interrupt.h>
>   #include <linux/debug_locks.h>
> +#include <linux/device.h>
> 
>   #include "mutex.h"
> 
> @@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const char
> *name,
>         lock->magic = lock;
>   }
> 
> +static void devm_mutex_release(void *res)
> +{
> +       mutex_destroy(res);
> +}
> +
> +int debug_devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +       return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> +}
> +
>   /***
>    * mutex_destroy - mark a mutex unusable
>    * @lock: the mutex to be destroyed
> -- 
> 2.25.1
> 
> 
> 
> And now I would drop the the refactoring patch with moving down
> mutex_destroy. devm block is big enough to be declared standalone.
> 
> 
> On 3/7/24 19:44, Marek Behún wrote:
>> On Thu, 7 Mar 2024 08:39:46 -0500
>> Waiman Long <longman@redhat.com> wrote:
>>
>>> On 3/7/24 04:56, Marek Behún wrote:
>>>> On Thu, Mar 07, 2024 at 05:40:26AM +0300, George Stark wrote:
>>>>> Using of devm API leads to a certain order of releasing resources.
>>>>> So all dependent resources which are not devm-wrapped should be 
>>>>> deleted
>>>>> with respect to devm-release order. Mutex is one of such objects that
>>>>> often is bound to other resources and has no own devm wrapping.
>>>>> Since mutex_destroy() actually does nothing in non-debug builds
>>>>> frequently calling mutex_destroy() is just ignored which is safe 
>>>>> for now
>>>>> but wrong formally and can lead to a problem if mutex_destroy() 
>>>>> will be
>>>>> extended so introduce devm_mutex_init()
>>>>>
>>>>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>>> ---
>>>>>    Hello Christophe. Hope you don't mind I put you SoB tag because 
>>>>> you helped alot
>>>>>    to make this patch happen.
>>>>>
>>>>>    include/linux/mutex.h        | 13 +++++++++++++
>>>>>    kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
>>>>>    2 files changed, 35 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
>>>>> index f7611c092db7..9bcf72cb941a 100644
>>>>> --- a/include/linux/mutex.h
>>>>> +++ b/include/linux/mutex.h
>>>>> @@ -22,6 +22,8 @@
>>>>>    #include <linux/cleanup.h>
>>>>>    #include <linux/mutex_types.h>
>>>>>
>>>>> +struct device;
>>>>> +
>>>>>    #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>>>>    # define __DEP_MAP_MUTEX_INITIALIZER(lockname)                  \
>>>>>                    , .dep_map = {                                  \
>>>>> @@ -115,10 +117,21 @@ do 
>>>>> {                                                 \
>>>>>
>>>>>    #ifdef CONFIG_DEBUG_MUTEXES
>>>>>
>>>>> +int devm_mutex_init(struct device *dev, struct mutex *lock);
>>>>>    void mutex_destroy(struct mutex *lock);
>>>>>
>>>>>    #else
>>>>>
>>>>> +static inline int devm_mutex_init(struct device *dev, struct mutex 
>>>>> *lock)
>>>>> +{
>>>>> +  /*
>>>>> +   * since mutex_destroy is nop actually there's no need to 
>>>>> register it
>>>>> +   * in devm subsystem.
>>>>> +   */
>>>>> +  mutex_init(lock);
>>>>> +  return 0;
>>>>> +}
>>>>> +
>>>>>    static inline void mutex_destroy(struct mutex *lock) {}
>>>>>
>>>>>    #endif
>>>>> diff --git a/kernel/locking/mutex-debug.c 
>>>>> b/kernel/locking/mutex-debug.c
>>>>> index bc8abb8549d2..c9efab1a8026 100644
>>>>> --- a/kernel/locking/mutex-debug.c
>>>>> +++ b/kernel/locking/mutex-debug.c
>>>>> @@ -19,6 +19,7 @@
>>>>>    #include <linux/kallsyms.h>
>>>>>    #include <linux/interrupt.h>
>>>>>    #include <linux/debug_locks.h>
>>>>> +#include <linux/device.h>
>>>>>
>>>>>    #include "mutex.h"
>>>>>
>>>>> @@ -104,3 +105,24 @@ void mutex_destroy(struct mutex *lock)
>>>>>    }
>>>>>
>>>>>    EXPORT_SYMBOL_GPL(mutex_destroy);
>>>>> +
>>>>> +static void devm_mutex_release(void *res)
>>>>> +{
>>>>> +  mutex_destroy(res);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * devm_mutex_init - Resource-managed mutex initialization
>>>>> + * @dev:  Device which lifetime mutex is bound to
>>>>> + * @lock: Pointer to a mutex
>>>>> + *
>>>>> + * Initialize mutex which is automatically destroyed when the 
>>>>> driver is detached.
>>>>> + *
>>>>> + * Returns: 0 on success or a negative error code on failure.
>>>>> + */
>>>>> +int devm_mutex_init(struct device *dev, struct mutex *lock)
>>>>> +{
>>>>> +  mutex_init(lock);
>>>>> +  return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(devm_mutex_init);
>>>> Hi George,
>>>>
>>>> look at
>>>> https://lore.kernel.org/lkml/7013bf9e-2663-4613-ae61-61872e81355b@redhat.com/
>>>> where Matthew and Hans explain that devm_mutex_init needs to be a macro
>>>> because of the static lockdep key.
>>>>
>>>> so this should be something like:
>>>>
>>>> static inline int __devm_mutex_init(struct device *dev, struct mutex 
>>>> *mutex,
>>>>                                 const char *name,
>>>>                                 struct lock_class_key *key)
>>>> {
>>>>     __mutex_init(mutex, name, key);
>>>>     return devm_add_action_or_reset(dev, devm_mutex_release, mutex);
>>>> }
>>>>
>>>> #define devm_mutex_init(dev, mutex)                         \
>>>> do {                                                                \
>>>>     static struct lock_class_key __key;                     \
>>>>                                                             \
>>>>     __devm_mutex_init(dev, (mutex), #mutex, &__key);        \
>>>> } while (0);
>>>>
>>>>
>>>> Marek
>>>
>>> Making devm_mutex_init() a function will make all the devm_mutex share
>>> the same lockdep key. Making it a macro will make each caller of
>>> devm_mutex_init() have a distinct lockdep key. It all depends on whether
>>> all the devm_mutexes have the same lock usage pattern or not and whether
>>> it is possible for one devm_mutex to be nested inside another. So either
>>> way can be fine depending on the mutex usage pattern. My suggestion is
>>> to use a function, if possible, unless it will cause a false positive
>>> lockdep splat as there is a limit on the maximum # of lockdep keys that
>>> can be used.
>>
>> devm_mutex_init() should behave like other similar function
>> initializing stuff with resource management. I.e. it should behave like
>> mutex_init(), but with resource management.
>>
>> mutex_init() is a macro generating static lockdep key for each instance,
>> so devm_mutex_init() should also generate static lockdep key for each
>> instance.
>>
>> Marek
> 
> -- 
> Best regards
> George

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[George Stark]

Hello Christophe

Thanks for the review
You were right about typecheck - it was meant to check errors even if 
CONFIG_DEBUG_MUTEXES was off.

Here's new version based on the comments:

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 67edc4ca2bee..9193b163038f 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -22,6 +22,8 @@
  #include <linux/cleanup.h>
  #include <linux/mutex_types.h>

+struct device;
+
  #ifdef CONFIG_DEBUG_LOCK_ALLOC
  # define __DEP_MAP_MUTEX_INITIALIZER(lockname)			\
  		, .dep_map = {					\
@@ -117,6 +119,34 @@ do {							\
  } while (0)
  #endif /* CONFIG_PREEMPT_RT */

+#ifdef CONFIG_DEBUG_MUTEXES
+
+int debug_devm_mutex_init(struct device *dev, struct mutex *lock);
+
+static inline int __devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+	return debug_devm_mutex_init(dev, lock);
+}
+
+#else
+
+static inline int __devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+	/*
+	* When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so
+	* no really need to register it in devm subsystem.
+	*/
+	return 0;
+}
+
+#endif
+
+#define devm_mutex_init(dev, mutex)			\
+({							\
+	mutex_init(mutex);				\
+	__devm_mutex_init(dev, mutex);			\
+})
+
  /*
   * See kernel/locking/mutex.c for detailed documentation of these APIs.
   * Also see Documentation/locking/mutex-design.rst.
diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
index bc8abb8549d2..967a5367c79a 100644
--- a/kernel/locking/mutex-debug.c
+++ b/kernel/locking/mutex-debug.c
@@ -19,6 +19,7 @@
  #include <linux/kallsyms.h>
  #include <linux/interrupt.h>
  #include <linux/debug_locks.h>
+#include <linux/device.h>

  #include "mutex.h"

@@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const char 
*name,
  	lock->magic = lock;
  }

+static void devm_mutex_release(void *res)
+{
+	mutex_destroy(res);
+}
+
+int debug_devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
+}
+
  /***
   * mutex_destroy - mark a mutex unusable
   * @lock: the mutex to be destroyed
-- 
2.25.1



On 3/12/24 09:04, Christophe Leroy wrote:

...

> 
> I think it would be preferable to minimise the number of macros.
> 
> If I were you I would keep your devm_mutex_init() as is but rename it
> __devm_mutex_init() and just remove the mutex_init() from it, then add
> only one macro that works independant of CONFIG_DEBUG_MUTEXES:
> 
> #define devm_mutex_init(dev, mutex)	\
> ({					\
> 	mutex_init(mutex);		\
> 	__devm_mutex_init(dev, mutex);	\
> })
> 
> With that, no need of a second version of the macro and no need for the
> typecheck either.
> 
> Note the __ which is a clear indication that allthough that function is
> declared in public mutex.h, it is not meant to be used outside of it.
> 
> 
> 


-- 
Best regards
George

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[Christophe Leroy]

Le 12/03/2024 à 12:39, George Stark a écrit :
> [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. 
> Découvrez pourquoi ceci est important à 
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hello Christophe
> 
> Thanks for the review
> You were right about typecheck - it was meant to check errors even if
> CONFIG_DEBUG_MUTEXES was off.

Yes that's current practice in order to catch problems as soon as possible.

> 
> Here's new version based on the comments:
> 
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index 67edc4ca2bee..9193b163038f 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -22,6 +22,8 @@
>   #include <linux/cleanup.h>
>   #include <linux/mutex_types.h>
> 
> +struct device;
> +
>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
>   # define __DEP_MAP_MUTEX_INITIALIZER(lockname)                        \
>                 , .dep_map = {                                  \
> @@ -117,6 +119,34 @@ do 
> {                                                       \
>   } while (0)
>   #endif /* CONFIG_PREEMPT_RT */
> 
> +#ifdef CONFIG_DEBUG_MUTEXES
> +
> +int debug_devm_mutex_init(struct device *dev, struct mutex *lock);
> +
> +static inline int __devm_mutex_init(struct device *dev, struct mutex 
> *lock)
> +{
> +       return debug_devm_mutex_init(dev, lock);
> +}

You don't need that inline function, just change debug_devm_mutex_init() 
to __devm_mutex_init().

> +
> +#else
> +
> +static inline int __devm_mutex_init(struct device *dev, struct mutex 
> *lock)
> +{
> +       /*
> +       * When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so
> +       * no really need to register it in devm subsystem.
> +       */

Don't know if it is because tabs are replaced by blanks in you email, 
but the stars should be aligned

/* ...
  * ...
  */

> +       return 0;
> +}
> +
> +#endif
> +
> +#define devm_mutex_init(dev, mutex)                    \
> +({                                                     \
> +       mutex_init(mutex);                              \
> +       __devm_mutex_init(dev, mutex);                  \
> +})
> +
>   /*
>    * See kernel/locking/mutex.c for detailed documentation of these APIs.
>    * Also see Documentation/locking/mutex-design.rst.
> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
> index bc8abb8549d2..967a5367c79a 100644
> --- a/kernel/locking/mutex-debug.c
> +++ b/kernel/locking/mutex-debug.c
> @@ -19,6 +19,7 @@
>   #include <linux/kallsyms.h>
>   #include <linux/interrupt.h>
>   #include <linux/debug_locks.h>
> +#include <linux/device.h>
> 
>   #include "mutex.h"
> 
> @@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const char
> *name,
>         lock->magic = lock;
>   }
> 
> +static void devm_mutex_release(void *res)
> +{
> +       mutex_destroy(res);
> +}
> +
> +int debug_devm_mutex_init(struct device *dev, struct mutex *lock)

Rename __devm_mutex_init();

It makes it more clear that nobody is expected to call it directly.

> +{
> +       return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> +}
> +
>   /***
>    * mutex_destroy - mark a mutex unusable
>    * @lock: the mutex to be destroyed
> -- 
> 2.25.1
> 
> 

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[George Stark]

Hello Christophe

On 3/12/24 14:51, Christophe Leroy wrote:
> 
> 
> Le 12/03/2024 à 12:39, George Stark a écrit :
>> [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com.
>> Découvrez pourquoi ceci est important à
>> https://aka.ms/LearnAboutSenderIdentification ]

...

> You don't need that inline function, just change debug_devm_mutex_init()
> to __devm_mutex_init().

I stuck to debug_* name because mutex-debug.c already exports a set
of debug_ calls so...
Well it's not essential anyway. Here's the next try:

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 67edc4ca2bee..537b5ea18ceb 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -22,6 +22,8 @@
  #include <linux/cleanup.h>
  #include <linux/mutex_types.h>

+struct device;
+
  #ifdef CONFIG_DEBUG_LOCK_ALLOC
  # define __DEP_MAP_MUTEX_INITIALIZER(lockname)			\
  		, .dep_map = {					\
@@ -117,6 +119,29 @@ do {							\
  } while (0)
  #endif /* CONFIG_PREEMPT_RT */

+#ifdef CONFIG_DEBUG_MUTEXES
+
+int __devm_mutex_init(struct device *dev, struct mutex *lock);
+
+#else
+
+static inline int __devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+	/*
+	 * When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so
+	 * no really need to register it in devm subsystem.
+	 */
+	return 0;
+}
+
+#endif
+
+#define devm_mutex_init(dev, mutex)			\
+({							\
+	mutex_init(mutex);				\
+	__devm_mutex_init(dev, mutex);			\
+})
+
  /*
   * See kernel/locking/mutex.c for detailed documentation of these APIs.
   * Also see Documentation/locking/mutex-design.rst.
diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
index bc8abb8549d2..6aa77e3dc82e 100644
--- a/kernel/locking/mutex-debug.c
+++ b/kernel/locking/mutex-debug.c
@@ -19,6 +19,7 @@
  #include <linux/kallsyms.h>
  #include <linux/interrupt.h>
  #include <linux/debug_locks.h>
+#include <linux/device.h>

  #include "mutex.h"

@@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const char 
*name,
  	lock->magic = lock;
  }

+static void devm_mutex_release(void *res)
+{
+	mutex_destroy(res);
+}
+
+int __devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
+}
+
  /***
   * mutex_destroy - mark a mutex unusable
   * @lock: the mutex to be destroyed
-- 
2.25.1



>> +
>> +#else
>> +
>> +static inline int __devm_mutex_init(struct device *dev, struct mutex
>> *lock)
>> +{
>> +       /*
>> +       * When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so
>> +       * no really need to register it in devm subsystem.
>> +       */
> 
> Don't know if it is because tabs are replaced by blanks in you email,
> but the stars should be aligned

Ack


-- 
Best regards
George

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[Christophe Leroy]

Le 12/03/2024 à 16:30, George Stark a écrit :
> [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. 
> Découvrez pourquoi ceci est important à 
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hello Christophe
> 
> On 3/12/24 14:51, Christophe Leroy wrote:
>>
>>
>> Le 12/03/2024 à 12:39, George Stark a écrit :
>>> [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com.
>>> Découvrez pourquoi ceci est important à
>>> https://aka.ms/LearnAboutSenderIdentification ]
> 
> ...
> 
>> You don't need that inline function, just change debug_devm_mutex_init()
>> to __devm_mutex_init().
> 
> I stuck to debug_* name because mutex-debug.c already exports a set
> of debug_ calls so...

Ah yes you are right I didn't see that. On the other hand all those 
debug_mutex_* are used by kernel/locking/mutex.c.
Here we really don't want our new function to be called by anything else 
than devm_mutex_init so by calling it __devm_mutex_init() you kind of 
tie them together.

> Well it's not essential anyway. Here's the next try:

Looks good to me.

> 
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index 67edc4ca2bee..537b5ea18ceb 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -22,6 +22,8 @@
>   #include <linux/cleanup.h>
>   #include <linux/mutex_types.h>
> 
> +struct device;
> +
>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
>   # define __DEP_MAP_MUTEX_INITIALIZER(lockname)                        \
>                 , .dep_map = {                                  \
> @@ -117,6 +119,29 @@ do 
> {                                                       \
>   } while (0)
>   #endif /* CONFIG_PREEMPT_RT */
> 
> +#ifdef CONFIG_DEBUG_MUTEXES
> +
> +int __devm_mutex_init(struct device *dev, struct mutex *lock);
> +
> +#else
> +
> +static inline int __devm_mutex_init(struct device *dev, struct mutex 
> *lock)
> +{
> +       /*
> +        * When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so
> +        * no really need to register it in devm subsystem.
> +        */
> +       return 0;
> +}
> +
> +#endif
> +
> +#define devm_mutex_init(dev, mutex)                    \
> +({                                                     \
> +       mutex_init(mutex);                              \
> +       __devm_mutex_init(dev, mutex);                  \
> +})
> +
>   /*
>    * See kernel/locking/mutex.c for detailed documentation of these APIs.
>    * Also see Documentation/locking/mutex-design.rst.
> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
> index bc8abb8549d2..6aa77e3dc82e 100644
> --- a/kernel/locking/mutex-debug.c
> +++ b/kernel/locking/mutex-debug.c
> @@ -19,6 +19,7 @@
>   #include <linux/kallsyms.h>
>   #include <linux/interrupt.h>
>   #include <linux/debug_locks.h>
> +#include <linux/device.h>
> 
>   #include "mutex.h"
> 
> @@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const char
> *name,
>         lock->magic = lock;
>   }
> 
> +static void devm_mutex_release(void *res)
> +{
> +       mutex_destroy(res);
> +}
> +
> +int __devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +       return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> +}
> +
>   /***
>    * mutex_destroy - mark a mutex unusable
>    * @lock: the mutex to be destroyed
> -- 
> 2.25.1
> 
> 
> 
>>> +
>>> +#else
>>> +
>>> +static inline int __devm_mutex_init(struct device *dev, struct mutex
>>> *lock)
>>> +{
>>> +       /*
>>> +       * When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a 
>>> nop so
>>> +       * no really need to register it in devm subsystem.
>>> +       */
>>
>> Don't know if it is because tabs are replaced by blanks in you email,
>> but the stars should be aligned
> 
> Ack
> 
> 
> -- 
> Best regards
> George

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[Andy Shevchenko]

On Thu, Mar 7, 2024 at 4:40 AM George Stark <gnstark@salutedevices.com> wrote:
>
> Using of devm API leads to a certain order of releasing resources.
> So all dependent resources which are not devm-wrapped should be deleted
> with respect to devm-release order. Mutex is one of such objects that
> often is bound to other resources and has no own devm wrapping.
> Since mutex_destroy() actually does nothing in non-debug builds
> frequently calling mutex_destroy() is just ignored which is safe for now
> but wrong formally and can lead to a problem if mutex_destroy() will be
> extended so introduce devm_mutex_init()
>
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

>  Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot
>  to make this patch happen.

You also need to figure out who should be the author of the patch and
probably add a (missing) Co-developed-by. After all you should also
follow the correct order of SoBs.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[George Stark]

Hello Andy

On 3/7/24 13:34, Andy Shevchenko wrote:
> On Thu, Mar 7, 2024 at 4:40 AM George Stark <gnstark@salutedevices.com> wrote:
>>
>> Using of devm API leads to a certain order of releasing resources.
>> So all dependent resources which are not devm-wrapped should be deleted
>> with respect to devm-release order. Mutex is one of such objects that
>> often is bound to other resources and has no own devm wrapping.
>> Since mutex_destroy() actually does nothing in non-debug builds
>> frequently calling mutex_destroy() is just ignored which is safe for now
>> but wrong formally and can lead to a problem if mutex_destroy() will be
>> extended so introduce devm_mutex_init()
>>
>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> 
>>   Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot
>>   to make this patch happen.
> 
> You also need to figure out who should be the author of the patch and
> probably add a (missing) Co-developed-by. After all you should also
> follow the correct order of SoBs.
> 

Thanks for the review.
I explained in the other letter as I see it. So I'd leave myself
as author and add appropriate tag with Christophe's name.
BTW what do you mean by correct SoB order?
Is it alphabetical order or order of importance?

-- 
Best regards
George

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[Christophe Leroy]

Le 12/03/2024 à 01:01, George Stark a écrit :
> [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. 
> Découvrez pourquoi ceci est important à 
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hello Andy
> 
> On 3/7/24 13:34, Andy Shevchenko wrote:
>> On Thu, Mar 7, 2024 at 4:40 AM George Stark 
>> <gnstark@salutedevices.com> wrote:
>>>
>>> Using of devm API leads to a certain order of releasing resources.
>>> So all dependent resources which are not devm-wrapped should be deleted
>>> with respect to devm-release order. Mutex is one of such objects that
>>> often is bound to other resources and has no own devm wrapping.
>>> Since mutex_destroy() actually does nothing in non-debug builds
>>> frequently calling mutex_destroy() is just ignored which is safe for now
>>> but wrong formally and can lead to a problem if mutex_destroy() will be
>>> extended so introduce devm_mutex_init()
>>>
>>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>
>>>   Hello Christophe. Hope you don't mind I put you SoB tag because you 
>>> helped alot
>>>   to make this patch happen.
>>
>> You also need to figure out who should be the author of the patch and
>> probably add a (missing) Co-developed-by. After all you should also
>> follow the correct order of SoBs.
>>
> 
> Thanks for the review.
> I explained in the other letter as I see it. So I'd leave myself
> as author and add appropriate tag with Christophe's name.
> BTW what do you mean by correct SoB order?
> Is it alphabetical order or order of importance?
> 

The correct order is to first have the Author's SoB.

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[Andy Shevchenko]

On Tue, Mar 12, 2024 at 7:41 AM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
> Le 12/03/2024 à 01:01, George Stark a écrit :
> > [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com.
> > Découvrez pourquoi ceci est important à
> > https://aka.ms/LearnAboutSenderIdentification ]
> > On 3/7/24 13:34, Andy Shevchenko wrote:
> >> On Thu, Mar 7, 2024 at 4:40 AM George Stark
> >> <gnstark@salutedevices.com> wrote:

...

> >>> Signed-off-by: George Stark <gnstark@salutedevices.com>
> >>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> >>
> >>>   Hello Christophe. Hope you don't mind I put you SoB tag because you
> >>> helped alot
> >>>   to make this patch happen.
> >>
> >> You also need to figure out who should be the author of the patch and
> >> probably add a (missing) Co-developed-by. After all you should also
> >> follow the correct order of SoBs.
> >
> > Thanks for the review.
> > I explained in the other letter as I see it. So I'd leave myself
> > as author and add appropriate tag with Christophe's name.
> > BTW what do you mean by correct SoB order?
> > Is it alphabetical order or order of importance?

> The correct order is to first have the Author's SoB.

At the last one is submitters. So, if it's the same person, which one
should go first?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[Christophe Leroy]

Le 07/03/2024 à 03:40, George Stark a écrit :
> [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> Using of devm API leads to a certain order of releasing resources.
> So all dependent resources which are not devm-wrapped should be deleted
> with respect to devm-release order. Mutex is one of such objects that
> often is bound to other resources and has no own devm wrapping.
> Since mutex_destroy() actually does nothing in non-debug builds
> frequently calling mutex_destroy() is just ignored which is safe for now
> but wrong formally and can lead to a problem if mutex_destroy() will be
> extended so introduce devm_mutex_init()
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>   Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot
>   to make this patch happen.

Up to you, I sent a RFC patch based on yours with my ideas included 
because an exemple is easier than a lot of words for understanding, and 
my scripts automatically sets the Signed-off-by: but feel free to change 
it to Suggested-by:

Christophe

> 
>   include/linux/mutex.h        | 13 +++++++++++++
>   kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
>   2 files changed, 35 insertions(+)
> 
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index f7611c092db7..9bcf72cb941a 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -22,6 +22,8 @@
>   #include <linux/cleanup.h>
>   #include <linux/mutex_types.h>
> 
> +struct device;
> +
>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
>   # define __DEP_MAP_MUTEX_INITIALIZER(lockname)                 \
>                  , .dep_map = {                                  \
> @@ -115,10 +117,21 @@ do {                                                      \
> 
>   #ifdef CONFIG_DEBUG_MUTEXES
> 
> +int devm_mutex_init(struct device *dev, struct mutex *lock);
>   void mutex_destroy(struct mutex *lock);
> 
>   #else
> 
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +       /*
> +        * since mutex_destroy is nop actually there's no need to register it
> +        * in devm subsystem.
> +        */
> +       mutex_init(lock);
> +       return 0;
> +}
> +
>   static inline void mutex_destroy(struct mutex *lock) {}
> 
>   #endif
> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
> index bc8abb8549d2..c9efab1a8026 100644
> --- a/kernel/locking/mutex-debug.c
> +++ b/kernel/locking/mutex-debug.c
> @@ -19,6 +19,7 @@
>   #include <linux/kallsyms.h>
>   #include <linux/interrupt.h>
>   #include <linux/debug_locks.h>
> +#include <linux/device.h>
> 
>   #include "mutex.h"
> 
> @@ -104,3 +105,24 @@ void mutex_destroy(struct mutex *lock)
>   }
> 
>   EXPORT_SYMBOL_GPL(mutex_destroy);
> +
> +static void devm_mutex_release(void *res)
> +{
> +       mutex_destroy(res);
> +}
> +
> +/**
> + * devm_mutex_init - Resource-managed mutex initialization
> + * @dev:       Device which lifetime mutex is bound to
> + * @lock:      Pointer to a mutex
> + *
> + * Initialize mutex which is automatically destroyed when the driver is detached.
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +       mutex_init(lock);
> +       return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> +}
> +EXPORT_SYMBOL_GPL(devm_mutex_init);
> --
> 2.25.1
> 

Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[George Stark]

Hello Christophe

On 3/7/24 16:50, Christophe Leroy wrote:
> 
> 
> Le 07/03/2024 à 03:40, George Stark a écrit :
>> [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>>
>> Using of devm API leads to a certain order of releasing resources.
>> So all dependent resources which are not devm-wrapped should be deleted
>> with respect to devm-release order. Mutex is one of such objects that
>> often is bound to other resources and has no own devm wrapping.
>> Since mutex_destroy() actually does nothing in non-debug builds
>> frequently calling mutex_destroy() is just ignored which is safe for now
>> but wrong formally and can lead to a problem if mutex_destroy() will be
>> extended so introduce devm_mutex_init()
>>
>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>    Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot
>>    to make this patch happen.
> 
> Up to you, I sent a RFC patch based on yours with my ideas included
> because an exemple is easier than a lot of words for understanding, and
> my scripts automatically sets the Signed-off-by: but feel free to change
> it to Suggested-by:

Although we had close ideas for the final patch in v4
you encouraged me to do it in the right (=effective) way and go back
from devm-helpers.h to mutex.h in the first place, reinforced the 
concept with appropriate examples from existing code, reviewed a lot. 
Thanks. Probably Suggested-by: is more suited here

-- 
Best regards
George

[PATCH v5 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 | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 17235a5e576a..6475eadcb0df 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -320,6 +320,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,
@@ -336,7 +341,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;
@@ -384,6 +392,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;
@@ -406,19 +418,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 */ },
@@ -432,7 +434,6 @@ static struct i2c_driver aw2013_driver = {
 		.of_match_table = aw2013_match_table,
 	},
 	.probe = aw2013_probe,
-	.remove = aw2013_remove,
 };
 
 module_i2c_driver(aw2013_driver);
-- 
2.25.1

[PATCH v5 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 | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index f584a7f98fc5..5cba52d07b38 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -530,6 +530,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 +578,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 +597,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 +615,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 +663,6 @@ static struct i2c_driver aw200xx_driver = {
 		.of_match_table = aw200xx_match_table,
 	},
 	.probe = aw200xx_probe,
-	.remove = aw200xx_remove,
 	.id_table = aw200xx_id,
 };
 module_i2c_driver(aw200xx_driver);
-- 
2.25.1

[PATCH v5 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 5d18bbfd1f23..e24bfe686312 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_MAPLE,
 };
 
+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 v5 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 | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index 13662a4aa1f2..aa7966eb506f 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -542,6 +542,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 +563,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 +704,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 +719,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 +733,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 v5 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 | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-nic78bx.c b/drivers/leds/leds-nic78bx.c
index a86b43dd995e..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,15 +180,6 @@ static int nic78bx_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static void 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);
-}
-
 static const struct acpi_device_id led_device_ids[] = {
 	{"NIC78B3", 0},
 	{"", 0},
@@ -184,7 +188,6 @@ MODULE_DEVICE_TABLE(acpi, led_device_ids);
 
 static struct platform_driver led_driver = {
 	.probe = nic78bx_probe,
-	.remove_new = nic78bx_remove,
 	.driver = {
 		.name = KBUILD_MODNAME,
 		.acpi_match_table = ACPI_PTR(led_device_ids),
-- 
2.25.1

[PATCH v5 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 | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/leds-mlxreg.c b/drivers/leds/leds-mlxreg.c
index d8e3d5d8d2d0..b1510cd32e47 100644
--- a/drivers/leds/leds-mlxreg.c
+++ b/drivers/leds/leds-mlxreg.c
@@ -257,6 +257,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) {
@@ -268,26 +269,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 void mlxreg_led_remove(struct platform_device *pdev)
-{
-	struct mlxreg_led_priv_data *priv = dev_get_drvdata(&pdev->dev);
-
-	mutex_destroy(&priv->access_lock);
-}
-
 static struct platform_driver mlxreg_led_driver = {
 	.driver = {
 	    .name = "leds-mlxreg",
 	},
 	.probe = mlxreg_led_probe,
-	.remove_new = mlxreg_led_remove,
 };
 
 module_platform_driver(mlxreg_led_driver);
-- 
2.25.1

[PATCH v5 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 | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-an30259a.c b/drivers/leds/leds-an30259a.c
index 0216afed3b6e..decfca447d8a 100644
--- a/drivers/leds/leds-an30259a.c
+++ b/drivers/leds/leds-an30259a.c
@@ -283,7 +283,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 +320,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 +342,6 @@ static struct i2c_driver an30259a_driver = {
 		.of_match_table = an30259a_match_table,
 	},
 	.probe = an30259a_probe,
-	.remove = an30259a_remove,
 	.id_table = an30259a_id,
 };
 
-- 
2.25.1

[PATCH v5 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 4f01acb75727..9c6fb7d6e0e7 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 void 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