LinuxLists.cc - [PATCH v4 00/10] devm_led_classdev

2023-12-14 17:37:10

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

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

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

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

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

--
2.25.1

2023-12-14 17:37:11

by George Stark

[permalink] [raw]

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

2023-12-14 17:37:13

by George Stark

[permalink] [raw]

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

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]>
---
drivers/leds/leds-nic78bx.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-nic78bx.c b/drivers/leds/leds-nic78bx.c
index f196f52eec1e..f3049fa14f04 100644
--- a/drivers/leds/leds-nic78bx.c
+++ b/drivers/leds/leds-nic78bx.c
@@ -118,6 +118,15 @@ static struct nic78bx_led nic78bx_leds[] = {
}
};

+static void lock_led_reg_action(void *data)
+{
+ struct nic78bx_led_data *led_data = (struct nic78bx_led_data *)data;
+
+ /* Lock LED register */
+ outb(NIC78BX_LOCK_VALUE,
+ led_data->io_base + NIC78BX_LOCK_REG_OFFSET);
+}
+
static int nic78bx_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -152,6 +161,10 @@ static int nic78bx_probe(struct platform_device *pdev)
led_data->io_base = io_rc->start;
spin_lock_init(&led_data->lock);

+ ret = devm_add_action(dev, lock_led_reg_action, led_data);
+ if (ret)
+ return ret;
+
for (i = 0; i < ARRAY_SIZE(nic78bx_leds); i++) {
nic78bx_leds[i].data = led_data;

@@ -167,17 +180,6 @@ static int nic78bx_probe(struct platform_device *pdev)
return ret;
}

-static int nic78bx_remove(struct platform_device *pdev)
-{
- struct nic78bx_led_data *led_data = platform_get_drvdata(pdev);
-
- /* Lock LED register */
- outb(NIC78BX_LOCK_VALUE,
- led_data->io_base + NIC78BX_LOCK_REG_OFFSET);
-
- return 0;
-}
-
static const struct acpi_device_id led_device_ids[] = {
{"NIC78B3", 0},
{"", 0},
@@ -186,7 +188,6 @@ MODULE_DEVICE_TABLE(acpi, led_device_ids);

static struct platform_driver led_driver = {
.probe = nic78bx_probe,
- .remove = nic78bx_remove,
.driver = {
.name = KBUILD_MODNAME,
.acpi_match_table = ACPI_PTR(led_device_ids),
--
2.25.1

2023-12-14 17:37:19

by George Stark

[permalink] [raw]

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

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]>
---
drivers/leds/leds-lm3532.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index 13662a4aa1f2..713900947e35 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -3,6 +3,7 @@
// Copyright (C) 2019 Texas Instruments Incorporated - https://www.ti.com/
// https://www.ti.com/lit/ds/symlink/lm3532.pdf

+#include <linux/devm-helpers.h>
#include <linux/i2c.h>
#include <linux/leds.h>
#include <linux/slab.h>
@@ -542,6 +543,13 @@ static int lm3532_parse_als(struct lm3532_data *priv)
return ret;
}

+static void gpio_set_low_action(void *data)
+{
+ struct lm3532_data *priv = (struct lm3532_data *)data;
+
+ gpiod_direction_output(priv->enable_gpio, 0);
+}
+
static int lm3532_parse_node(struct lm3532_data *priv)
{
struct fwnode_handle *child = NULL;
@@ -556,6 +564,12 @@ static int lm3532_parse_node(struct lm3532_data *priv)
if (IS_ERR(priv->enable_gpio))
priv->enable_gpio = NULL;

+ if (priv->enable_gpio) {
+ ret = devm_add_action(&priv->client->dev, gpio_set_low_action, priv);
+ if (ret)
+ return ret;
+ }
+
priv->regulator = devm_regulator_get(&priv->client->dev, "vin");
if (IS_ERR(priv->regulator))
priv->regulator = NULL;
@@ -691,7 +705,10 @@ static int lm3532_probe(struct i2c_client *client)
return ret;
}

- mutex_init(&drvdata->lock);
+ ret = devm_mutex_init(&client->dev, &drvdata->lock);
+ if (ret)
+ return ret;
+
i2c_set_clientdata(client, drvdata);

ret = lm3532_parse_node(drvdata);
@@ -703,16 +720,6 @@ static int lm3532_probe(struct i2c_client *client)
return ret;
}

-static void lm3532_remove(struct i2c_client *client)
-{
- struct lm3532_data *drvdata = i2c_get_clientdata(client);
-
- mutex_destroy(&drvdata->lock);
-
- if (drvdata->enable_gpio)
- gpiod_direction_output(drvdata->enable_gpio, 0);
-}
-
static const struct of_device_id of_lm3532_leds_match[] = {
{ .compatible = "ti,lm3532", },
{},
@@ -727,7 +734,6 @@ MODULE_DEVICE_TABLE(i2c, lm3532_id);

static struct i2c_driver lm3532_i2c_driver = {
.probe = lm3532_probe,
- .remove = lm3532_remove,
.id_table = lm3532_id,
.driver = {
.name = LM3532_NAME,
--
2.25.1

2023-12-14 17:37:22

by George Stark

[permalink] [raw]

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

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 743e2cdd0891..018ec933ac10 100644
--- a/drivers/leds/leds-powernv.c
+++ b/drivers/leds/leds-powernv.c
@@ -30,15 +30,6 @@ static const struct led_type_map led_type_map[] = {
};

struct powernv_led_common {
- /*
- * By default unload path resets all the LEDs. But on PowerNV
- * platform we want to retain LED state across reboot as these
- * are controlled by firmware. Also service processor can modify
- * the LEDs independent of OS. Hence avoid resetting LEDs in
- * unload path.
- */
- bool led_disabled;
-
/* Max supported LED type */
__be64 max_led_type;

@@ -178,10 +169,6 @@ static int powernv_brightness_set(struct led_classdev *led_cdev,
struct powernv_led_common *powernv_led_common = powernv_led->common;
int rc;

- /* Do not modify LED in unload path */
- if (powernv_led_common->led_disabled)
- return 0;
-
mutex_lock(&powernv_led_common->lock);
rc = powernv_led_set(powernv_led, value);
mutex_unlock(&powernv_led_common->lock);
@@ -225,6 +212,14 @@ static int powernv_led_create(struct device *dev,

powernv_led->cdev.brightness_set_blocking = powernv_brightness_set;
powernv_led->cdev.brightness_get = powernv_brightness_get;
+ /*
+ * By default unload path resets all the LEDs. But on PowerNV
+ * platform we want to retain LED state across reboot as these
+ * are controlled by firmware. Also service processor can modify
+ * the LEDs independent of OS. Hence avoid resetting LEDs in
+ * unload path.
+ */
+ powernv_led->cdev.flags = LED_RETAIN_AT_SHUTDOWN;
powernv_led->cdev.brightness = LED_OFF;
powernv_led->cdev.max_brightness = LED_FULL;

@@ -313,9 +308,7 @@ static int powernv_led_remove(struct platform_device *pdev)
{
struct powernv_led_common *powernv_led_common;

- /* Disable LED operation */
powernv_led_common = platform_get_drvdata(pdev);
- powernv_led_common->led_disabled = true;

/* Destroy lock */
mutex_destroy(&powernv_led_common->lock);
--
2.25.1

2023-12-14 17:37:35

by George Stark

[permalink] [raw]

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

2023-12-14 17:37:42

by George Stark

[permalink] [raw]

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

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

2023-12-21 15:11:33

by Lee Jones

[permalink] [raw]

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

On Thu, 14 Dec 2023, George Stark wrote:

> This patch series fixes the problem of devm_led_classdev_register misusing.
>
> The basic problem is described in [1]. Shortly when devm_led_classdev_register()
> is used then led_classdev_unregister() called after driver's remove() callback.
> led_classdev_unregister() calls driver's brightness_set callback and that callback
> may use resources which were destroyed already in driver's remove().
>
> After discussion with maintainers [2] [3] we decided:
> 1) don't touch led subsytem core code and don't remove led_set_brightness() from it
> but fix drivers
> 2) don't use devm_led_classdev_unregister
>
> So the solution is to use devm wrappers for all resources
> driver's brightness_set() depends on. And introduce dedicated devm wrapper
> for mutex as it's often used resource.
>
> [1] https://lore.kernel.org/lkml/[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]>
>
> [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/#m63126025f5d1bdcef69bcad50f2e58274d42e2d7
>
> George Stark (10):
> leds: aw2013: unlock mutex before destroying it
> locking: introduce devm_mutex_init
> leds: aw2013: use devm API to cleanup module's resources
> leds: aw200xx: use devm API to cleanup module's resources
> leds: lp3952: use devm API to cleanup module's resources
> leds: lm3532: use devm API to cleanup module's resources
> leds: nic78bx: use devm API to cleanup module's resources
> leds: mlxreg: use devm_mutex_init for mutex initializtion
> leds: an30259a: use devm_mutext_init for mutext initialization
> leds: powernv: use LED_RETAIN_AT_SHUTDOWN flag for leds
>
> drivers/leds/leds-an30259a.c | 15 +++++----------
> drivers/leds/leds-aw200xx.c | 33 ++++++++++++++++++++++-----------
> drivers/leds/leds-aw2013.c | 27 +++++++++++++++------------
> drivers/leds/leds-lm3532.c | 30 ++++++++++++++++++------------
> drivers/leds/leds-lp3952.c | 21 +++++++++++----------
> drivers/leds/leds-mlxreg.c | 17 ++++++-----------
> drivers/leds/leds-nic78bx.c | 25 +++++++++++++------------
> drivers/leds/leds-powernv.c | 23 ++++++++---------------

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

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

--
Lee Jones [李琼斯]

2024-02-09 17:46:11

by Andy Shevchenko

[permalink] [raw]

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

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

..

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

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

--
With Best Regards,
Andy Shevchenko

2024-02-11 23:52:23

by George Stark

[permalink] [raw]

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

Hello Andy.

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

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

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

--
Best regards
George

2024-02-12 10:04:29

by Andy Shevchenko

[permalink] [raw]

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

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

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

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

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

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

--
With Best Regards,
Andy Shevchenko

2024-02-13 00:14:59

by George Stark

[permalink] [raw]

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

Hello Andy

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

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

[1]
https://lore.kernel.org/lkml/[email protected]/T/#m795b230d662c1debb28463ad721ddba5b384340a

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

Oh ok, that would be great.

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

--
Best regards
George

2024-02-13 10:56:53

by Andy Shevchenko

[permalink] [raw]

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

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

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

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

> [1]
> https://lore.kernel.org/lkml/[email protected]/T/#m795b230d662c1debb28463ad721ddba5b384340a

--
With Best Regards,
Andy Shevchenko

2024-02-13 23:56:50

by George Stark

[permalink] [raw]

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

Hello Andy

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

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

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

[1]
https://lore.kernel.org/all/[email protected]/T/

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

Ok

>
>> [1]
>> https://lore.kernel.org/lkml/[email protected]/T/#m795b230d662c1debb28463ad721ddba5b384340a
>
>

--
Best regards
George