[PATCH v7 0/8] devm_led_classdev_register() usage problem

[PATCH v7 0/8] 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 subsystem 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 one line
	- 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 one line
	- 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 initialization
leds: an30259a: use devm_mutext_init for mutext initialization
leds: powernv: add LED_RETAIN_AT_SHUTDOWN flag for leds
	- those patches 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_mutex_init() in mutex.h now

v5->v6:
locking/mutex: move mutex_destroy() definition lower [11]
	drop the patch due to devm_mutex_init block is big enough to be declared standalone.

locking/mutex: introduce devm_mutex_init
	redesign devm_mutex_init function to macro to keep lockdep feature working
	use typeof to redeclare mutex var in macro body (thanks to checkpatch)
	previous version [12]

v6->v7:
locking/mutex: introduce devm_mutex_init
	fix comment at __devm_mutex_init
	move #include <linux/device.h> upper
	commit message: change devm_mutex_init -> devm_mutex_init(), add point in the end
	fix and move up tag Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
	add tags (in the order received):
	Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
	Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
	Reviewed-by: Marek Behún <kabel@kernel.org>
	Acked-by: Waiman Long <longman@redhat.com>

leds: powernv: use LED_RETAIN_AT_SHUTDOWN flag for leds
	remove the patch from this series to send it separately

leds: mlxreg: use devm_mutex_init() for mutex initialization
leds: an30259a: use devm_mutex_init() for mutex initialization
	commit message: change devm_mutex_init -> devm_mutex_init()
	add tag Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

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
	add tag Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

[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
[11] https://lore.kernel.org/lkml/20240307024034.1548605-2-gnstark@salutedevices.com/
[12] https://lore.kernel.org/lkml/20240307024034.1548605-3-gnstark@salutedevices.com/

George Stark (8):
  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 initialization
  leds: an30259a: use devm_mutex_init() for mutex initialization

 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 +++++++++++++----------
 include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
 kernel/locking/mutex-debug.c | 11 +++++++++++
 9 files changed, 122 insertions(+), 74 deletions(-)

--
2.25.1

[PATCH v7 1/8] 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().

Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Signed-off-by: George Stark <gnstark@salutedevices.com>
Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Marek Behún <kabel@kernel.org>
Acked-by: Waiman Long <longman@redhat.com>
---
 include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
 kernel/locking/mutex-debug.c | 11 +++++++++++
 2 files changed, 38 insertions(+)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 67edc4ca2bee..a561c629d89f 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -22,6 +22,8 @@
 #include <linux/cleanup.h>
 #include <linux/mutex_types.h>
 
+struct device;
+
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 # define __DEP_MAP_MUTEX_INITIALIZER(lockname)			\
 		, .dep_map = {					\
@@ -117,6 +119,31 @@ do {							\
 } while (0)
 #endif /* CONFIG_PREEMPT_RT */
 
+#ifdef CONFIG_DEBUG_MUTEXES
+
+int __devm_mutex_init(struct device *dev, struct mutex *lock);
+
+#else
+
+static inline int __devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+	/*
+	 * When CONFIG_DEBUG_MUTEXES is off mutex_destroy() is just a nop so
+	 * no really need to register it in the devm subsystem.
+	 */
+	return 0;
+}
+
+#endif
+
+#define devm_mutex_init(dev, mutex)			\
+({							\
+	typeof(mutex) mutex_ = (mutex);			\
+							\
+	mutex_init(mutex_);				\
+	__devm_mutex_init(dev, mutex_);			\
+})
+
 /*
  * See kernel/locking/mutex.c for detailed documentation of these APIs.
  * Also see Documentation/locking/mutex-design.rst.
diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
index bc8abb8549d2..f7e848defd50 100644
--- a/kernel/locking/mutex-debug.c
+++ b/kernel/locking/mutex-debug.c
@@ -12,6 +12,7 @@
  */
 #include <linux/mutex.h>
 #include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/export.h>
 #include <linux/poison.h>
 #include <linux/sched.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

[PATCH v7 2/8] 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 v7 3/8] 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 v7 4/8] 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 v7 5/8] 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 v7 6/8] 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 v7 7/8] leds: mlxreg: use devm_mutex_init() for mutex 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 v7 8/8] leds: an30259a: use devm_mutex_init() for mutex 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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

Re: [PATCH v7 1/8] locking/mutex: introduce devm_mutex_init()

[Andy Shevchenko]

On Thu, Mar 14, 2024 at 10:19 PM 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().

...

>  /***

Unrelated to your patch, but there are too many asterisks :-)

>   * mutex_destroy - mark a mutex unusable
>   * @lock: the mutex to be destroyed

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 4/8] leds: lp3952: use devm API to cleanup module's resources

[Andy Shevchenko]

On Thu, Mar 14, 2024 at 10:19 PM George Stark <gnstark@salutedevices.com> wrote:
>
> 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().

...

> +static void gpio_set_low_action(void *data)
> +{
> +       struct lp3952_led_array *priv = (struct lp3952_led_array *)data;

In case of new series, drop these castings in patches 4-6.

> +       gpiod_set_value(priv->enable_gpio, 0);
> +}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 0/8] devm_led_classdev_register() usage problem

[Lee Jones]

On Thu, 14 Mar 2024, George Stark wrote:

> This patch series fixes the problem of devm_led_classdev_register misusing.
> 
> The basic problem is described in [1]. Shortly when devm_led_classdev_register()
> is used then led_classdev_unregister() called after driver's remove() callback.
> led_classdev_unregister() calls driver's brightness_set callback and that callback
> may use resources which were destroyed already in driver's remove().
> 
> After discussion with maintainers [2] [3] we decided:
> 1) don't touch led subsystem 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 one line
> 	- 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 one line
> 	- 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 initialization
> leds: an30259a: use devm_mutext_init for mutext initialization
> leds: powernv: add LED_RETAIN_AT_SHUTDOWN flag for leds
> 	- those patches 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_mutex_init() in mutex.h now
> 
> v5->v6:
> locking/mutex: move mutex_destroy() definition lower [11]
> 	drop the patch due to devm_mutex_init block is big enough to be declared standalone.
> 
> locking/mutex: introduce devm_mutex_init
> 	redesign devm_mutex_init function to macro to keep lockdep feature working
> 	use typeof to redeclare mutex var in macro body (thanks to checkpatch)
> 	previous version [12]
> 
> v6->v7:
> locking/mutex: introduce devm_mutex_init
> 	fix comment at __devm_mutex_init
> 	move #include <linux/device.h> upper
> 	commit message: change devm_mutex_init -> devm_mutex_init(), add point in the end
> 	fix and move up tag Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> 	add tags (in the order received):
> 	Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> 	Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 	Reviewed-by: Marek Behún <kabel@kernel.org>
> 	Acked-by: Waiman Long <longman@redhat.com>
> 
> leds: powernv: use LED_RETAIN_AT_SHUTDOWN flag for leds
> 	remove the patch from this series to send it separately
> 
> leds: mlxreg: use devm_mutex_init() for mutex initialization
> leds: an30259a: use devm_mutex_init() for mutex initialization
> 	commit message: change devm_mutex_init -> devm_mutex_init()
> 	add tag Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> 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
> 	add tag Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> [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
> [11] https://lore.kernel.org/lkml/20240307024034.1548605-2-gnstark@salutedevices.com/
> [12] https://lore.kernel.org/lkml/20240307024034.1548605-3-gnstark@salutedevices.com/
> 
> George Stark (8):
>   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 initialization
>   leds: an30259a: use devm_mutex_init() for mutex initialization
> 
>  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 +++++++++++++----------
>  include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
>  kernel/locking/mutex-debug.c | 11 +++++++++++
>  9 files changed, 122 insertions(+), 74 deletions(-)

Doesn't apply to v6.8.

What base was used for this?

-- 
Lee Jones [李琼斯]

Re: [PATCH v7 0/8] devm_led_classdev_register() usage problem

[George Stark]

Hello Lee

On 3/21/24 21:11, Lee Jones wrote:
> On Thu, 14 Mar 2024, George Stark wrote:
> 
>> This patch series fixes the problem of devm_led_classdev_register misusing.
>>
>> The basic problem is described in [1]. Shortly when devm_led_classdev_register()
>> is used then led_classdev_unregister() called after driver's remove() callback.
>> led_classdev_unregister() calls driver's brightness_set callback and that callback
>> may use resources which were destroyed already in driver's remove().
>>
>> After discussion with maintainers [2] [3] we decided:
>> 1) don't touch led subsystem 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.

...

>>    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 initialization
>>    leds: an30259a: use devm_mutex_init() for mutex initialization
>>
>>   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 +++++++++++++----------
>>   include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
>>   kernel/locking/mutex-debug.c | 11 +++++++++++
>>   9 files changed, 122 insertions(+), 74 deletions(-)
> 
> Doesn't apply to v6.8.
> 
> What base was used for this?

I've just pulled git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
and v7 was applied cleanly. linux-next is ok too.

v6.8 is lack of recent patch 6969d0a2ba1adc9ba6a49b9805f24080896c255c
v7's patch #2 depends on it

-- 
Best regards
George

Re: [PATCH v7 0/8] devm_led_classdev_register() usage problem

[Lee Jones]

On Fri, 22 Mar 2024, George Stark wrote:

> Hello Lee
> 
> On 3/21/24 21:11, Lee Jones wrote:
> > On Thu, 14 Mar 2024, George Stark wrote:
> > 
> > > This patch series fixes the problem of devm_led_classdev_register misusing.
> > > 
> > > The basic problem is described in [1]. Shortly when devm_led_classdev_register()
> > > is used then led_classdev_unregister() called after driver's remove() callback.
> > > led_classdev_unregister() calls driver's brightness_set callback and that callback
> > > may use resources which were destroyed already in driver's remove().
> > > 
> > > After discussion with maintainers [2] [3] we decided:
> > > 1) don't touch led subsystem 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.
> 
> ...
> 
> > >    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 initialization
> > >    leds: an30259a: use devm_mutex_init() for mutex initialization
> > > 
> > >   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 +++++++++++++----------
> > >   include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
> > >   kernel/locking/mutex-debug.c | 11 +++++++++++
> > >   9 files changed, 122 insertions(+), 74 deletions(-)
> > 
> > Doesn't apply to v6.8.
> > 
> > What base was used for this?
> 
> I've just pulled git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> and v7 was applied cleanly. linux-next is ok too.
> 
> v6.8 is lack of recent patch 6969d0a2ba1adc9ba6a49b9805f24080896c255c
> v7's patch #2 depends on it

No problem.  I'll wait for v6.9-rc1.

-- 
Lee Jones [李琼斯]

Re: [PATCH v7 0/8] devm_led_classdev_register() usage problem

[George Stark]

Hello Lee

On 3/22/24 13:43, Lee Jones wrote:
> On Fri, 22 Mar 2024, George Stark wrote:
> 
>> Hello Lee
>>
>> On 3/21/24 21:11, Lee Jones wrote:
>>> On Thu, 14 Mar 2024, George Stark wrote:
>>>
>>>> This patch series fixes the problem of devm_led_classdev_register misusing.
>>>>
>>>> The basic problem is described in [1]. Shortly when devm_led_classdev_register()
>>>> is used then led_classdev_unregister() called after driver's remove() callback.
>>>> led_classdev_unregister() calls driver's brightness_set callback and that callback
>>>> may use resources which were destroyed already in driver's remove().
>>>>
>>>> After discussion with maintainers [2] [3] we decided:
>>>> 1) don't touch led subsystem 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.
>>
>> ...
>>
>>>>     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 initialization
>>>>     leds: an30259a: use devm_mutex_init() for mutex initialization
>>>>
>>>>    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 +++++++++++++----------
>>>>    include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
>>>>    kernel/locking/mutex-debug.c | 11 +++++++++++
>>>>    9 files changed, 122 insertions(+), 74 deletions(-)
>>>
>>> Doesn't apply to v6.8.
>>>
>>> What base was used for this?
>>
>> I've just pulled git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>> and v7 was applied cleanly. linux-next is ok too.
>>
>> v6.8 is lack of recent patch 6969d0a2ba1adc9ba6a49b9805f24080896c255c
>> v7's patch #2 depends on it
> 
> No problem.  I'll wait for v6.9-rc1.
> 

Just checked the v7 patch series on released 6.9-rc1 and it's applied
cleanly. If anything i can help please let me know.

-- 
Best regards
George

Re: [PATCH v7 0/8] devm_led_classdev_register() usage problem

[Lee Jones]

On Thu, 14 Mar 2024 23:18:48 +0300, George Stark wrote:
> This patch series fixes the problem of devm_led_classdev_register misusing.
> 
> The basic problem is described in [1]. Shortly when devm_led_classdev_register()
> is used then led_classdev_unregister() called after driver's remove() callback.
> led_classdev_unregister() calls driver's brightness_set callback and that callback
> may use resources which were destroyed already in driver's remove().
> 
> [...]

Applied, thanks!

[1/8] locking/mutex: introduce devm_mutex_init()
      commit: c1a17857f8e333aa597ce0099c8728888e0c5037
[2/8] leds: aw2013: use devm API to cleanup module's resources
      commit: aa2fb50d9f17185e799a5969d0d357842d07450b
[3/8] leds: aw200xx: use devm API to cleanup module's resources
      commit: 517d9d3c408369cfa552652ac06294410c570095
[4/8] leds: lp3952: use devm API to cleanup module's resources
      commit: f8cf710c971e021e5ca39832adde32dfa241e081
[5/8] leds: lm3532: use devm API to cleanup module's resources
      commit: d265d86a2725e38c4f13ef79fc4e685b7234e7a3
[6/8] leds: nic78bx: use devm API to cleanup module's resources
      commit: 917676a8961e18e4dd5b17411317e3261473762a
[7/8] leds: mlxreg: use devm_mutex_init() for mutex initialization
      commit: 0548081cb25fbd0eb3a605cb6647dd166368da32
[8/8] leds: an30259a: use devm_mutex_init() for mutex initialization
      commit: c898cf6eb464d4172c79e925ea4d8007e856e10c

--
Lee Jones [李琼斯]

Re: [PATCH v7 0/8] devm_led_classdev_register() usage problem

[Lee Jones]

On Thu, 11 Apr 2024, Lee Jones wrote:

> On Thu, 14 Mar 2024 23:18:48 +0300, George Stark wrote:
> > This patch series fixes the problem of devm_led_classdev_register misusing.
> > 
> > The basic problem is described in [1]. Shortly when devm_led_classdev_register()
> > is used then led_classdev_unregister() called after driver's remove() callback.
> > led_classdev_unregister() calls driver's brightness_set callback and that callback
> > may use resources which were destroyed already in driver's remove().
> > 
> > [...]
> 
> Applied, thanks!
> 
> [1/8] locking/mutex: introduce devm_mutex_init()
>       commit: c1a17857f8e333aa597ce0099c8728888e0c5037
> [2/8] leds: aw2013: use devm API to cleanup module's resources
>       commit: aa2fb50d9f17185e799a5969d0d357842d07450b
> [3/8] leds: aw200xx: use devm API to cleanup module's resources
>       commit: 517d9d3c408369cfa552652ac06294410c570095
> [4/8] leds: lp3952: use devm API to cleanup module's resources
>       commit: f8cf710c971e021e5ca39832adde32dfa241e081
> [5/8] leds: lm3532: use devm API to cleanup module's resources
>       commit: d265d86a2725e38c4f13ef79fc4e685b7234e7a3
> [6/8] leds: nic78bx: use devm API to cleanup module's resources
>       commit: 917676a8961e18e4dd5b17411317e3261473762a
> [7/8] leds: mlxreg: use devm_mutex_init() for mutex initialization
>       commit: 0548081cb25fbd0eb3a605cb6647dd166368da32
> [8/8] leds: an30259a: use devm_mutex_init() for mutex initialization
>       commit: c898cf6eb464d4172c79e925ea4d8007e856e10c

Submitted for build testing.

I'll follow-up with a pull-request once everything is in order.

-- 
Lee Jones [李琼斯]

Re: [PATCH v7 0/8] devm_led_classdev_register() usage problem

[Lee Jones]

On Fri, 29 Mar 2024, George Stark wrote:

> Hello Lee
> 
> On 3/22/24 13:43, Lee Jones wrote:
> > On Fri, 22 Mar 2024, George Stark wrote:
> > 
> > > Hello Lee
> > > 
> > > On 3/21/24 21:11, Lee Jones wrote:
> > > > On Thu, 14 Mar 2024, George Stark wrote:
> > > > 
> > > > > This patch series fixes the problem of devm_led_classdev_register misusing.
> > > > > 
> > > > > The basic problem is described in [1]. Shortly when devm_led_classdev_register()
> > > > > is used then led_classdev_unregister() called after driver's remove() callback.
> > > > > led_classdev_unregister() calls driver's brightness_set callback and that callback
> > > > > may use resources which were destroyed already in driver's remove().
> > > > > 
> > > > > After discussion with maintainers [2] [3] we decided:
> > > > > 1) don't touch led subsystem 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.
> > > 
> > > ...
> > > 
> > > > >     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 initialization
> > > > >     leds: an30259a: use devm_mutex_init() for mutex initialization
> > > > > 
> > > > >    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 +++++++++++++----------
> > > > >    include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
> > > > >    kernel/locking/mutex-debug.c | 11 +++++++++++
> > > > >    9 files changed, 122 insertions(+), 74 deletions(-)
> > > > 
> > > > Doesn't apply to v6.8.
> > > > 
> > > > What base was used for this?
> > > 
> > > I've just pulled git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > > and v7 was applied cleanly. linux-next is ok too.
> > > 
> > > v6.8 is lack of recent patch 6969d0a2ba1adc9ba6a49b9805f24080896c255c
> > > v7's patch #2 depends on it
> > 
> > No problem.  I'll wait for v6.9-rc1.
> > 
> 
> Just checked the v7 patch series on released 6.9-rc1 and it's applied
> cleanly. If anything i can help please let me know.

It applies, but doesn't seem to build:

make --silent --keep-going --jobs=8 O=../build ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- 'CC=sccache aarch64-linux-gnu-gcc' 'HOSTCC=sccache gcc' allmodconfig
make --silent --keep-going --jobs=8 O=../build ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- 'CC=sccache aarch64-linux-gnu-gcc' 'HOSTCC=sccache gcc'
ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-an30259a.ko] undefined!
ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-aw200xx.ko] undefined!
ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-aw2013.ko] undefined!
ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-lm3532.ko] undefined!
ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-mlxreg.ko] undefined!
make[3]: *** [/builds/linux/scripts/Makefile.modpost:145: Module.symvers] Error 1

Did you forget to export it?

-- 
Lee Jones [李琼斯]

Re: [PATCH v7 0/8] devm_led_classdev_register() usage problem

[George Stark]

Hello Lee

On 4/11/24 16:45, Lee Jones wrote:
> On Fri, 29 Mar 2024, George Stark wrote:
> 
>> Hello Lee
>>
>> On 3/22/24 13:43, Lee Jones wrote:
>>> On Fri, 22 Mar 2024, George Stark wrote:
>>>
>>>> Hello Lee
>>>>
>>>> On 3/21/24 21:11, Lee Jones wrote:
>>>>> On Thu, 14 Mar 2024, George Stark wrote:
>>>>>
>>>>>> This patch series fixes the problem of devm_led_classdev_register misusing.
>>>>>>
>>>>>> The basic problem is described in [1]. Shortly when devm_led_classdev_register()
>>>>>> is used then led_classdev_unregister() called after driver's remove() callback.
>>>>>> led_classdev_unregister() calls driver's brightness_set callback and that callback
>>>>>> may use resources which were destroyed already in driver's remove().
>>>>>>
>>>>>> After discussion with maintainers [2] [3] we decided:
>>>>>> 1) don't touch led subsystem 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.
>>>>
>>>> ...
>>>>
>>>>>>      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 initialization
>>>>>>      leds: an30259a: use devm_mutex_init() for mutex initialization
>>>>>>
>>>>>>     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 +++++++++++++----------
>>>>>>     include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
>>>>>>     kernel/locking/mutex-debug.c | 11 +++++++++++
>>>>>>     9 files changed, 122 insertions(+), 74 deletions(-)
>>>>>
>>>>> Doesn't apply to v6.8.
>>>>>
>>>>> What base was used for this?
>>>>
>>>> I've just pulled git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>>>> and v7 was applied cleanly. linux-next is ok too.
>>>>
>>>> v6.8 is lack of recent patch 6969d0a2ba1adc9ba6a49b9805f24080896c255c
>>>> v7's patch #2 depends on it
>>>
>>> No problem.  I'll wait for v6.9-rc1.
>>>
>>
>> Just checked the v7 patch series on released 6.9-rc1 and it's applied
>> cleanly. If anything i can help please let me know.
> 
> It applies, but doesn't seem to build:
> 
> make --silent --keep-going --jobs=8 O=../build ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- 'CC=sccache aarch64-linux-gnu-gcc' 'HOSTCC=sccache gcc' allmodconfig
> make --silent --keep-going --jobs=8 O=../build ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- 'CC=sccache aarch64-linux-gnu-gcc' 'HOSTCC=sccache gcc'
> ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-an30259a.ko] undefined!
> ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-aw200xx.ko] undefined!
> ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-aw2013.ko] undefined!
> ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-lm3532.ko] undefined!
> ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-mlxreg.ko] undefined!
> make[3]: *** [/builds/linux/scripts/Makefile.modpost:145: Module.symvers] Error 1
> 
> Did you forget to export it?
> 

Yes, my bad. I tested *DEBUG* configs but without modules enabled.
Adding EXPORT_SYMBOL_GPL(__devm_mutex_init); fixes the problem,
v8 is on the way.

-- 
Best regards
George