diff mbox series

[2/8] leds: nic78bx: explicitly unregister LEDs at module's shutdown

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

Commit Message

George Stark Oct. 25, 2023, 1:07 p.m. UTC 
  LEDs are registered using devm_led_classdev_register() and automatically
unregistered after module's remove(). led_classdev_unregister() calls
led_set_brightness() to turn off the LEDs and module's appropriate callback
uses resources those were destroyed already in module's remove().
So explicitly unregister LEDs at module shutdown.

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 drivers/leds/leds-nic78bx.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Christophe Leroy Nov. 6, 2023, 8:13 a.m. UTC | #1 
Le 25/10/2023 à 15:07, George Stark a écrit :
> LEDs are registered using devm_led_classdev_register() and automatically
> unregistered after module's remove(). led_classdev_unregister() calls
> led_set_brightness() to turn off the LEDs and module's appropriate callback
> uses resources those were destroyed already in module's remove().
> So explicitly unregister LEDs at module shutdown.
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> ---
>   drivers/leds/leds-nic78bx.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/leds/leds-nic78bx.c b/drivers/leds/leds-nic78bx.c
> index f196f52eec1e..12b70fcad37f 100644
> --- a/drivers/leds/leds-nic78bx.c
> +++ b/drivers/leds/leds-nic78bx.c
> @@ -170,6 +170,10 @@ static int nic78bx_probe(struct platform_device *pdev)
>   static int nic78bx_remove(struct platform_device *pdev)
>   {
>   	struct nic78bx_led_data *led_data = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(nic78bx_leds); i++)
> +		devm_led_classdev_unregister(&pdev->dev, &nic78bx_leds[i].cdev);

The whole purpose of devm_ functions is that you don't need to call 
unregister when removing the driver as the dev core will do it for you. 
I understand your problem but I think this is not the solution.

>   
>   	/* Lock LED register */
>   	outb(NIC78BX_LOCK_VALUE,
George Stark Nov. 9, 2023, 11:28 p.m. UTC | #2 
Hello Christophe.

Thanks for the review.


On 11/6/23 11:13, Christophe Leroy wrote:
 >
 >
 > Le 25/10/2023 à 15:07, George Stark a écrit :
 >> LEDs are registered using devm_led_classdev_register() and automatically
 >> unregistered after module's remove(). led_classdev_unregister() calls
 >> led_set_brightness() to turn off the LEDs and module's appropriate 
callback
 >> uses resources those were destroyed already in module's remove().
 >> So explicitly unregister LEDs at module shutdown.
 >>
 >> Signed-off-by: George Stark <gnstark@salutedevices.com>
 >> ---
 >>    drivers/leds/leds-nic78bx.c | 4 ++++
 >>    1 file changed, 4 insertions(+)
 >>
 >> diff --git a/drivers/leds/leds-nic78bx.c b/drivers/leds/leds-nic78bx.c
 >> index f196f52eec1e..12b70fcad37f 100644
 >> --- a/drivers/leds/leds-nic78bx.c
 >> +++ b/drivers/leds/leds-nic78bx.c
 >> @@ -170,6 +170,10 @@ static int nic78bx_probe(struct platform_device 
*pdev)
 >>    static int nic78bx_remove(struct platform_device *pdev)
 >>    {
 >>    	struct nic78bx_led_data *led_data = platform_get_drvdata(pdev);
 >> +	int i;
 >> +
 >> +	for (i = 0; i < ARRAY_SIZE(nic78bx_leds); i++)
 >> +		devm_led_classdev_unregister(&pdev->dev, &nic78bx_leds[i].cdev);
 >
 > The whole purpose of devm_ functions is that you don't need to call
 > unregister when removing the driver as the dev core will do it for you.
 > I understand your problem but I think this is not the solution.

I agree my solution is questionable although 
devm_led_classdev_unregister() is exists for some reason.

Probably it's not the best solution to remove led_set_brightness() from 
led_classdev_unregister() either.
Or we'll have to patch a lot of drivers which use led subsystem to call 
led_set_brightness() manually to keep leds' previous behavior.

Well if we can't easily unregister leds before module's remove() 
callback is completed may be we can get rid of remove() itself and 
manage all resources using devm API. In that case by the time 
led_set_brightness() is called from led_classdev_unregister() all 
dependent resources will be alive.
I'll try it in the next patch series.

 >
 >>
 >>    	/* Lock LED register */
 >>    	outb(NIC78BX_LOCK_VALUE,
diff mbox series

Patch

diff --git a/drivers/leds/leds-nic78bx.c b/drivers/leds/leds-nic78bx.c
index f196f52eec1e..12b70fcad37f 100644
--- a/drivers/leds/leds-nic78bx.c
+++ b/drivers/leds/leds-nic78bx.c
@@ -170,6 +170,10 @@  static int nic78bx_probe(struct platform_device *pdev)
 static int nic78bx_remove(struct platform_device *pdev)
 {
 	struct nic78bx_led_data *led_data = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(nic78bx_leds); i++)
+		devm_led_classdev_unregister(&pdev->dev, &nic78bx_leds[i].cdev);
 
 	/* Lock LED register */
 	outb(NIC78BX_LOCK_VALUE,