[PATCH v3 00/11] devm_led_classdev_register() usage problem

[PATCH v3 00/11] 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.

[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

George Stark (11):
  leds: aw2013: unlock mutex before destroying it
  locking: add define if mutex_destroy() is not an empty function
  devm-helpers: 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/devm-helpers.h | 27 +++++++++++++++++++++++++++
 include/linux/mutex.h        |  3 +++
 10 files changed, 128 insertions(+), 93 deletions(-)

-- 
2.25.1

[PATCH v3 01/11] 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

[PATCH v3 02/11] locking: add define if mutex_destroy() is not an empty function

[George Stark]

mutex_destroy() is only a debug helper and an empty function on non-debug
configurations still we can't legally ignore it because it's the
established API call and it can be extended theoretically in the future.
Sometimes it could be useful to know e.g. in the higher-level API if
mutex_destroy() really does something in the current configuration
and it's should be called or skipped otherwise for the sake of
optimization so add dedicated define to recognize these cases.

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 include/linux/mutex.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index a33aa9eb9fc3..2395ce4fcaf6 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -83,6 +83,9 @@ struct mutex {
 
 extern void mutex_destroy(struct mutex *lock);
 
+/* mutex_destroy() is a real function, not a NOP */
+#define mutex_destroy  mutex_destroy
+
 #else
 
 # define __DEBUG_MUTEX_INITIALIZER(lockname)
-- 
2.25.1

[PATCH v3 03/11] devm-helpers: 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 wrapper.
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() is
extended so introduce devm_mutex_init().

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 include/linux/devm-helpers.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
index 74891802200d..4043c3481d2e 100644
--- a/include/linux/devm-helpers.h
+++ b/include/linux/devm-helpers.h
@@ -24,6 +24,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/mutex.h>
 #include <linux/workqueue.h>
 
 static inline void devm_delayed_work_drop(void *res)
@@ -76,4 +77,30 @@ static inline int devm_work_autocancel(struct device *dev,
 	return devm_add_action(dev, devm_work_drop, w);
 }
 
+#ifdef mutex_destroy
+static inline void devm_mutex_release(void *res)
+{
+	mutex_destroy(res);
+}
+#endif
+
+/**
+ * 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.
+ */
+static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+	mutex_init(lock);
+#ifdef mutex_destroy
+	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
+#else
+	return 0;
+#endif
+}
+
 #endif
-- 
2.25.1

[PATCH v3 04/11] 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>
---
 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

Re: [PATCH v3 03/11] devm-helpers: introduce devm_mutex_init

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 12:30 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 wrapper.
> 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() is
> extended so introduce devm_mutex_init().

...

> +#ifdef mutex_destroy
> +static inline void devm_mutex_release(void *res)
> +{
> +       mutex_destroy(res);
> +}
> +#endif
> +
> +/**
> + * 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.
> + */
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +       mutex_init(lock);
> +#ifdef mutex_destroy
> +       return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> +#else
> +       return 0;
> +#endif
> +}

If this is going to be accepted, you may decrease the amount of ifdeffery.

#ifdef ...
#else
#define devm_mutex_init(dev, lock)  mutex_init(lock)
#endif


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 03/11] devm-helpers: introduce devm_mutex_init

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 12:36 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Dec 14, 2023 at 12:30 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 wrapper.
> > 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() is
> > extended so introduce devm_mutex_init().

...

> > +#ifdef mutex_destroy
> > +static inline void devm_mutex_release(void *res)
> > +{
> > +       mutex_destroy(res);
> > +}
> > +#endif
> > +
> > +/**
> > + * 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.
> > + */
> > +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> > +{
> > +       mutex_init(lock);
> > +#ifdef mutex_destroy
> > +       return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> > +#else
> > +       return 0;
> > +#endif
> > +}
>
> If this is going to be accepted, you may decrease the amount of ifdeffery.
>
> #ifdef ...
> #else
> #define devm_mutex_init(dev, lock)  mutex_init(lock)

More precisely ({ mutex_init(lock); 0; }) or as a static inline...

> #endif

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 02/11] locking: add define if mutex_destroy() is not an empty function

[Waiman Long]

On 12/13/23 17:30, George Stark wrote:
> mutex_destroy() is only a debug helper and an empty function on non-debug
> configurations still we can't legally ignore it because it's the
> established API call and it can be extended theoretically in the future.
> Sometimes it could be useful to know e.g. in the higher-level API if
> mutex_destroy() really does something in the current configuration
> and it's should be called or skipped otherwise for the sake of
> optimization so add dedicated define to recognize these cases.
>
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> ---
>   include/linux/mutex.h | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index a33aa9eb9fc3..2395ce4fcaf6 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -83,6 +83,9 @@ struct mutex {
>   
>   extern void mutex_destroy(struct mutex *lock);
>   
> +/* mutex_destroy() is a real function, not a NOP */
> +#define mutex_destroy  mutex_destroy
> +
>   #else
>   
>   # define __DEBUG_MUTEX_INITIALIZER(lockname)
Acked-by: Waiman Long <longman@redhat.com>

Re: [PATCH v3 04/11] leds: aw2013: use devm API to cleanup module's resources

[Nikita Travkin]

George Stark писал(а) 14.12.2023 03:30:
> 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>

Thanks for noticing and fixing this!
Perhaps this patch needs a Fixes tag too, like 1/11?

Tested-by: Nikita Travkin <nikita@trvn.ru>

Btw, seems like (5..11)/11 never arrived to the lists...

Nikita

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

Re: [PATCH v3 03/11] devm-helpers: introduce devm_mutex_init

[Hans de Goede]

Hi,

On 12/13/23 23:38, Andy Shevchenko wrote:
> On Thu, Dec 14, 2023 at 12:36 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Thu, Dec 14, 2023 at 12:30 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 wrapper.
>>> 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() is
>>> extended so introduce devm_mutex_init().
> 
> ...
> 
>>> +#ifdef mutex_destroy
>>> +static inline void devm_mutex_release(void *res)
>>> +{
>>> +       mutex_destroy(res);
>>> +}
>>> +#endif
>>> +
>>> +/**
>>> + * 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.
>>> + */
>>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>>> +{
>>> +       mutex_init(lock);
>>> +#ifdef mutex_destroy
>>> +       return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>>> +#else
>>> +       return 0;
>>> +#endif
>>> +}
>>
>> If this is going to be accepted, you may decrease the amount of ifdeffery.
>>
>> #ifdef ...
>> #else
>> #define devm_mutex_init(dev, lock)  mutex_init(lock)
> 
> More precisely ({ mutex_init(lock); 0; }) or as a static inline...

With a static inline we are pretty much back to the original
v3 patch.

I believe the best way to reduce the ifdef-ery is to remove
the #ifdef around devm_mutex_release() having unused
static inline ... functions in .h files is quite common,
so this one does not need a #ifdef around it and with
that removed we are down to just one #ifdef so just
removing the #ifdef around devm_mutex_release() seems
the best fix.

With that fixed you may add my:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

to the patch and I'm fine with this being routed
upstream through whatever tree is convenient.

Regards,

Hans

Re: [PATCH v3 03/11] devm-helpers: introduce devm_mutex_init

[Christophe Leroy]

Le 13/12/2023 à 23: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 ]
> 
> 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 wrapper.
> 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() is
> extended so introduce devm_mutex_init().

So you abandonned the idea of using mutex.h ?

I can't see the point to spread mutex functions into devm-helpers.h

Adding a mutex_destroy macro for this purpose looks odd. And if someone 
defines a new version of mutex_destroy() and forget the macro, it will 
go undetected.

Usually macros of that type serve the purpose of defining a fallback 
when the macro is not defined. In that case, when someone adds a new 
version without defining the macro, it gets detected because if 
conflicts with the fallback.
But in your case it works the other way round, so I will just go undetected.

For me the best solution remains to use mutex.h and have 
devm_mutex_init() defined or declared at the same place as mutex_destroy().


> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> ---
>   include/linux/devm-helpers.h | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
> 
> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
> index 74891802200d..4043c3481d2e 100644
> --- a/include/linux/devm-helpers.h
> +++ b/include/linux/devm-helpers.h
> @@ -24,6 +24,7 @@
>    */
> 
>   #include <linux/device.h>
> +#include <linux/mutex.h>
>   #include <linux/workqueue.h>
> 
>   static inline void devm_delayed_work_drop(void *res)
> @@ -76,4 +77,30 @@ static inline int devm_work_autocancel(struct device *dev,
>          return devm_add_action(dev, devm_work_drop, w);
>   }
> 
> +#ifdef mutex_destroy
> +static inline void devm_mutex_release(void *res)
> +{
> +       mutex_destroy(res);
> +}
> +#endif
> +
> +/**
> + * 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.
> + */
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +       mutex_init(lock);
> +#ifdef mutex_destroy
> +       return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> +#else
> +       return 0;
> +#endif
> +}
> +
>   #endif
> --
> 2.25.1
> 

Re: [PATCH v3 03/11] devm-helpers: introduce devm_mutex_init

[Hans de Goede]

Hi,

On 12/14/23 11:06, Christophe Leroy wrote:
> 
> 
> Le 13/12/2023 à 23: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 ]
>>
>> 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 wrapper.
>> 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() is
>> extended so introduce devm_mutex_init().
> 
> So you abandonned the idea of using mutex.h ?
> 
> I can't see the point to spread mutex functions into devm-helpers.h
> 
> Adding a mutex_destroy macro for this purpose looks odd. And if someone 
> defines a new version of mutex_destroy() and forget the macro, it will 
> go undetected.
> 
> Usually macros of that type serve the purpose of defining a fallback 
> when the macro is not defined. In that case, when someone adds a new 
> version without defining the macro, it gets detected because if 
> conflicts with the fallback.
> But in your case it works the other way round, so I will just go undetected.
> 
> For me the best solution remains to use mutex.h and have 
> devm_mutex_init() defined or declared at the same place as mutex_destroy().

FWIW defining devm_mutex_init() in mutex.h is fine
with me and makes sense to me. I also agree that putting
it there would be better if that is acceptable for
the mutex maintainers.

devm-helpers.h is there for helpers which don't fit
in another place.

Regards,

Hans




> 
> 
>>
>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>> ---
>>   include/linux/devm-helpers.h | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
>> index 74891802200d..4043c3481d2e 100644
>> --- a/include/linux/devm-helpers.h
>> +++ b/include/linux/devm-helpers.h
>> @@ -24,6 +24,7 @@
>>    */
>>
>>   #include <linux/device.h>
>> +#include <linux/mutex.h>
>>   #include <linux/workqueue.h>
>>
>>   static inline void devm_delayed_work_drop(void *res)
>> @@ -76,4 +77,30 @@ static inline int devm_work_autocancel(struct device *dev,
>>          return devm_add_action(dev, devm_work_drop, w);
>>   }
>>
>> +#ifdef mutex_destroy
>> +static inline void devm_mutex_release(void *res)
>> +{
>> +       mutex_destroy(res);
>> +}
>> +#endif
>> +
>> +/**
>> + * 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.
>> + */
>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>> +{
>> +       mutex_init(lock);
>> +#ifdef mutex_destroy
>> +       return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>> +#else
>> +       return 0;
>> +#endif
>> +}
>> +
>>   #endif
>> --
>> 2.25.1
>>

Re: [PATCH v3 03/11] devm-helpers: introduce devm_mutex_init

[George Stark]

Hello Christophe

On 12/14/23 13:06, Christophe Leroy wrote:
> 
> 
...
> 
> So you abandonned the idea of using mutex.h ?

I'm not the one who make a choice here. The patch [1] you're talking 
about was seen by everyone but it seems like no one had shown interest.
For me personally approach with #define mutex_destroy is not very usual
but if even slight mixing device with mutex.h is unacceptable what else
can we do? Avoiding the need to allocate devm slot for empty 
mutex_destroy is more important.

Should I make series #4 with the patch [1] to give it a last chance?

[1] 
https://lore.kernel.org/lkml/377e4437-7051-4d88-ae68-1460bcd692e1@redhat.com/T/#m3f6df30ffccaccb1df4669a327f349164f572931


> I can't see the point to spread mutex functions into devm-helpers.h
> 
> Adding a mutex_destroy macro for this purpose looks odd. And if someone
> defines a new version of mutex_destroy() and forget the macro, it will
> go undetected.
> 
> Usually macros of that type serve the purpose of defining a fallback
> when the macro is not defined. In that case, when someone adds a new
> version without defining the macro, it gets detected because if
> conflicts with the fallback.
> But in your case it works the other way round, so I will just go undetected.
> 
> For me the best solution remains to use mutex.h and have
> devm_mutex_init() defined or declared at the same place as mutex_destroy().
> 


> 
>>
>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>> ---
>>    include/linux/devm-helpers.h | 27 +++++++++++++++++++++++++++
>>    1 file changed, 27 insertions(+)
>>
>> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
>> index 74891802200d..4043c3481d2e 100644
>> --- a/include/linux/devm-helpers.h
>> +++ b/include/linux/devm-helpers.h
>> @@ -24,6 +24,7 @@
>>     */
>>
>>    #include <linux/device.h>
>> +#include <linux/mutex.h>
>>    #include <linux/workqueue.h>
>>
>>    static inline void devm_delayed_work_drop(void *res)
>> @@ -76,4 +77,30 @@ static inline int devm_work_autocancel(struct device *dev,
>>           return devm_add_action(dev, devm_work_drop, w);
>>    }
>>
>> +#ifdef mutex_destroy
>> +static inline void devm_mutex_release(void *res)
>> +{
>> +       mutex_destroy(res);
>> +}
>> +#endif
>> +
>> +/**
>> + * 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.
>> + */
>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>> +{
>> +       mutex_init(lock);
>> +#ifdef mutex_destroy
>> +       return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>> +#else
>> +       return 0;
>> +#endif
>> +}
>> +
>>    #endif
>> --
>> 2.25.1
>>

-- 
Best regards
George

Re: [PATCH v3 04/11] leds: aw2013: use devm API to cleanup module's resources

[George Stark]

Hello Nikita

Thanks for the review and testing.

On 12/14/23 08:42, Nikita Travkin wrote:
> George Stark писал(а) 14.12.2023 03:30:
>> 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>
> 
> Thanks for noticing and fixing this!
> Perhaps this patch needs a Fixes tag too, like 1/11?

This patch and the rest of the series depend on new feature 
devm_mutex_init and if I add Fixes tag this feature will need to be 
backported too along with fixes. I'm not sure it's desirable.

> 
> Tested-by: Nikita Travkin <nikita@trvn.ru>
> 
> Btw, seems like (5..11)/11 never arrived to the lists...

Yeah there was a delay, but now all patches from series #3 are online.
> 
> Nikita
> 
>> ---
...

-- 
Best regards
George

Re: [PATCH v3 03/11] devm-helpers: introduce devm_mutex_init

[Christophe Leroy]

Le 14/12/2023 à 13:48, 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/14/23 13:06, Christophe Leroy wrote:
>>
>>
> ...
>>
>> So you abandonned the idea of using mutex.h ?
> 
> I'm not the one who make a choice here. The patch [1] you're talking
> about was seen by everyone but it seems like no one had shown interest.
> For me personally approach with #define mutex_destroy is not very usual
> but if even slight mixing device with mutex.h is unacceptable what else
> can we do? Avoiding the need to allocate devm slot for empty
> mutex_destroy is more important.
> 

Why would a forward declaration of struct device in mutex.h be 
unacceptable when it is done in so many headers ?

$ git grep "struct device;" include/ | wc -l
164



> Should I make series #4 with the patch [1] to give it a last chance?

Yes, lets give it a try

> 
> [1]
> https://lore.kernel.org/lkml/377e4437-7051-4d88-ae68-1460bcd692e1@redhat.com/T/#m3f6df30ffccaccb1df4669a327f349164f572931
> 

Christophe

Re: [PATCH v3 03/11] devm-helpers: introduce devm_mutex_init

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 3:00 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
> Le 14/12/2023 à 13:48, 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 12/14/23 13:06, Christophe Leroy wrote:

...

> >> So you abandonned the idea of using mutex.h ?
> >
> > I'm not the one who make a choice here. The patch [1] you're talking
> > about was seen by everyone but it seems like no one had shown interest.
> > For me personally approach with #define mutex_destroy is not very usual
> > but if even slight mixing device with mutex.h is unacceptable what else
> > can we do? Avoiding the need to allocate devm slot for empty
> > mutex_destroy is more important.
> >
>
> Why would a forward declaration of struct device in mutex.h be
> unacceptable when it is done in so many headers ?
>
> $ git grep "struct device;" include/ | wc -l
> 164

I believe the main misunderstanding here is where to put the
implementation. AFAIU Christophe wants the implementation to be in the
very same _C_-file where mutex_destroy() is defined. mutex.h in this
case indeed requires the only forward declaration and hence doesn't
need to include device.h.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 04/11] leds: aw2013: use devm API to cleanup module's resources

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 03:58:56PM +0300, George Stark wrote:
> On 12/14/23 08:42, Nikita Travkin wrote:
> > George Stark писал(а) 14.12.2023 03:30:

...

> > Btw, seems like (5..11)/11 never arrived to the lists...
> 
> Yeah there was a delay, but now all patches from series #3 are online.

Nikita is right. This patch was the last in the mailing lists. Fix your mail
gateways, it quite likely the mail server in your organisation filters out
some mails as spam or so. I highly recommend to escalate this with your IT
department.

-- 
With Best Regards,
Andy Shevchenko