diff mbox series

[v3,04/11] leds: aw2013: use devm API to cleanup module's resources

Message ID 20231213223020.2713164-5-gnstark@salutedevices.com
State New
Headers
Series devm_led_classdev_register() usage problem |

Commit Message

George Stark Dec. 13, 2023, 10:30 p.m. UTC 
  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(-)

Comments

Nikita Travkin Dec. 14, 2023, 5:42 a.m. UTC | #1 
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);
George Stark Dec. 14, 2023, 12:58 p.m. UTC | #2 
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
> 
>> ---
...
Andy Shevchenko Dec. 14, 2023, 1:47 p.m. UTC | #3 
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 mbox series

Patch

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