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/[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 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 <[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_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 <[email protected]>
add tags (in the order received):
Reviewed-by: Christophe Leroy <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Marek Behún <[email protected]>
Acked-by: Waiman Long <[email protected]>
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 <[email protected]>
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 <[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/#m63126025f5d1bdcef69bcad50f2e58274d42e2d
[11] https://lore.kernel.org/lkml/[email protected]/
[12] https://lore.kernel.org/lkml/[email protected]/
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
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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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
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]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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
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]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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
On Thu, Mar 14, 2024 at 10:19 PM George Stark <[email protected]> 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
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/[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 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 <[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_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 <[email protected]>
> add tags (in the order received):
> Reviewed-by: Christophe Leroy <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Marek Behún <[email protected]>
> Acked-by: Waiman Long <[email protected]>
>
> 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 <[email protected]>
>
> 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 <[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/#m63126025f5d1bdcef69bcad50f2e58274d42e2d
> [11] https://lore.kernel.org/lkml/[email protected]/
> [12] https://lore.kernel.org/lkml/[email protected]/
>
> 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 [李琼斯]
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
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 [李琼斯]
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
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 [李琼斯]
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 [李琼斯]
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 [李琼斯]
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