| Message ID | 20231213223020.2713164-5-gnstark@salutedevices.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | devm_led_classdev_register() usage problem | expand |
George Stark писал(а) 14.12.2023 03:30: > In this driver LEDs are registered using devm_led_classdev_register() > so they are automatically unregistered after module's remove() is done. > led_classdev_unregister() calls module's led_set_brightness() to turn off > the LEDs and that callback uses resources which were destroyed already > in module's remove() so use devm API instead of remove(). > > Signed-off-by: George Stark <gnstark@salutedevices.com> Thanks for noticing and fixing this! Perhaps this patch needs a Fixes tag too, like 1/11? Tested-by: Nikita Travkin <nikita@trvn.ru> Btw, seems like (5..11)/11 never arrived to the lists... Nikita > --- > drivers/leds/leds-aw2013.c | 26 ++++++++++++++------------ > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c > index c2bc0782c0cd..863aeb02f278 100644 > --- a/drivers/leds/leds-aw2013.c > +++ b/drivers/leds/leds-aw2013.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0+ > // Driver for Awinic AW2013 3-channel LED driver > > +#include <linux/devm-helpers.h> > #include <linux/i2c.h> > #include <linux/leds.h> > #include <linux/module.h> > @@ -318,6 +319,11 @@ static int aw2013_probe_dt(struct aw2013 *chip) > return 0; > } > > +static void aw2013_chip_disable_action(void *data) > +{ > + aw2013_chip_disable(data); > +} > + > static const struct regmap_config aw2013_regmap_config = { > .reg_bits = 8, > .val_bits = 8, > @@ -334,7 +340,10 @@ static int aw2013_probe(struct i2c_client *client) > if (!chip) > return -ENOMEM; > > - mutex_init(&chip->mutex); > + ret = devm_mutex_init(&client->dev, &chip->mutex); > + if (ret) > + return ret; > + > mutex_lock(&chip->mutex); > > chip->client = client; > @@ -378,6 +387,10 @@ static int aw2013_probe(struct i2c_client *client) > goto error_reg; > } > > + ret = devm_add_action(&client->dev, aw2013_chip_disable_action, chip); > + if (ret) > + goto error_reg; > + > ret = aw2013_probe_dt(chip); > if (ret < 0) > goto error_reg; > @@ -398,19 +411,9 @@ static int aw2013_probe(struct i2c_client *client) > > error: > mutex_unlock(&chip->mutex); > - mutex_destroy(&chip->mutex); > return ret; > } > > -static void aw2013_remove(struct i2c_client *client) > -{ > - struct aw2013 *chip = i2c_get_clientdata(client); > - > - aw2013_chip_disable(chip); > - > - mutex_destroy(&chip->mutex); > -} > - > static const struct of_device_id aw2013_match_table[] = { > { .compatible = "awinic,aw2013", }, > { /* sentinel */ }, > @@ -424,7 +427,6 @@ static struct i2c_driver aw2013_driver = { > .of_match_table = of_match_ptr(aw2013_match_table), > }, > .probe = aw2013_probe, > - .remove = aw2013_remove, > }; > > module_i2c_driver(aw2013_driver);
Hello Nikita Thanks for the review and testing. On 12/14/23 08:42, Nikita Travkin wrote: > George Stark писал(а) 14.12.2023 03:30: >> In this driver LEDs are registered using devm_led_classdev_register() >> so they are automatically unregistered after module's remove() is done. >> led_classdev_unregister() calls module's led_set_brightness() to turn off >> the LEDs and that callback uses resources which were destroyed already >> in module's remove() so use devm API instead of remove(). >> >> Signed-off-by: George Stark <gnstark@salutedevices.com> > > Thanks for noticing and fixing this! > Perhaps this patch needs a Fixes tag too, like 1/11? This patch and the rest of the series depend on new feature devm_mutex_init and if I add Fixes tag this feature will need to be backported too along with fixes. I'm not sure it's desirable. > > Tested-by: Nikita Travkin <nikita@trvn.ru> > > Btw, seems like (5..11)/11 never arrived to the lists... Yeah there was a delay, but now all patches from series #3 are online. > > Nikita > >> --- ...
On Thu, Dec 14, 2023 at 03:58:56PM +0300, George Stark wrote: > On 12/14/23 08:42, Nikita Travkin wrote: > > George Stark писал(а) 14.12.2023 03:30: ... > > Btw, seems like (5..11)/11 never arrived to the lists... > > Yeah there was a delay, but now all patches from series #3 are online. Nikita is right. This patch was the last in the mailing lists. Fix your mail gateways, it quite likely the mail server in your organisation filters out some mails as spam or so. I highly recommend to escalate this with your IT department.
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);
In this driver LEDs are registered using devm_led_classdev_register() so they are automatically unregistered after module's remove() is done. led_classdev_unregister() calls module's led_set_brightness() to turn off the LEDs and that callback uses resources which were destroyed already in module's remove() so use devm API instead of remove(). Signed-off-by: George Stark <gnstark@salutedevices.com> --- drivers/leds/leds-aw2013.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)