| Message ID | 20231025130737.2015468-3-gnstark@salutedevices.com |
|---|---|
| State | New |
| Headers | show |
| Series | devm_led_classdev_register() usage problem | expand |
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,
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 --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,
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(+)