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/[email protected]/T/
[2] https://lore.kernel.org/lkml/[email protected]/T/#mc132b9b350fa51931b4fcfe14705d9f06e91421f
[3] https://lore.kernel.org/lkml/[email protected]/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 <[email protected]>
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 <[email protected]>
leds* patches
remove #include <linux/devm-helpers.h> due to devm_mutext_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]
[4] https://lore.kernel.org/lkml/[email protected]/T/#mf500af0eda2a9ffc95594607dbe4cb64f2e3c9a8
[5] https://lore.kernel.org/lkml/[email protected]/T/#mc92df4fb4f7d4187fb01cc1144acfa5fb5230dd2
[6] https://lore.kernel.org/lkml/[email protected]/T/#m300df89710c43cc2ab598baa16c68dd0a0d7d681
[7] https://lore.kernel.org/lkml/[email protected]/T/#m8e5c65e0c6b137c91fa00bb9320ad581164d1d0b
[8] https://lore.kernel.org/lkml/[email protected]/T/#m5f84a4a2f387d49678783e652b9e658e02c27450
[9] https://lore.kernel.org/lkml/[email protected]/T/#m19ad1fc04c560012c1e27418e3156d0c9306dd84
[10] https://lore.kernel.org/lkml/[email protected]/T/#m63126025f5d1bdcef69bcad50f2e58274d42e2d
[11] https://lore.kernel.org/lkml/[email protected]/
[12] https://lore.kernel.org/lkml/[email protected]/
George Stark (9):
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 | 11 +++++++++++
10 files changed, 130 insertions(+), 89 deletions(-)
--
2.25.1
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 <[email protected]>
Tested-by: Nikita Travkin <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Thu, Mar 14, 2024 at 10:46 AM George Stark <[email protected]> wrote:
>
> 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.
So, this change is not related to the main purpose of the series...
--
With Best Regards,
Andy Shevchenko
On Thu, Mar 14, 2024 at 10:46 AM George Stark <[email protected]> wrote:
>
> This patch series fixes the problem of devm_led_classdev_register misusing.
>
> The basic problem is described in [1]. Shortly when devm_led_classdev_register()
> is used then led_classdev_unregister() called after driver's remove() callback.
> led_classdev_unregister() calls driver's brightness_set callback and that callback
> may use resources which were destroyed already in driver's remove().
>
> After discussion with maintainers [2] [3] we decided:
> 1) don't touch led subsytem core code and don't remove led_set_brightness() from it
subsystem
> 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.
The leds related changes (except the last one) LGTM, hence FWIW,
Reviewed-by: Andy Shevchenko <[email protected]>
(for patches 2-8)
> [1] https://lore.kernel.org/lkml/[email protected]/T/
> [2] https://lore.kernel.org/lkml/[email protected]/T/#mc132b9b350fa51931b4fcfe14705d9f06e91421f
> [3] https://lore.kernel.org/lkml/[email protected]/T/#mdbf572a85c33f869a553caf986b6228bb65c8383
--
With Best Regards,
Andy Shevchenko