The patch "usb: simplify usbport trigger" together with
"leds: triggers: add device attribute support" caused an
regression for the usbport trigger. it will no longer
enumerate any active usb hub ports under the "ports"
directory in the sysfs class directory, if the usb host
drivers are fully initialized before the usbport trigger
was loaded.
The reason is that the usbport driver tries to register
the sysfs entries during the activate() callback. And this
will fail with -2 / ENOENT because the patch
"leds: triggers: add device attribute support" made it so
that the sysfs "ports" group was only being added after
the activate() callback succeeded.
This version of the patch moves the device_add_groups()
in front of the call to the trigger's activate() function
in order to solve the problem.
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Cc: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
Cc: Rafał Miłecki <zajec5@xxxxxxxxx>
Fixes: 6f7b0bad8839 ("usb: simplify usbport trigger")
Signed-off-by: Christian Lamparter <chunkeey@xxxxxxxxx>
---
This version of the patch ... of which will be many more?!
yeah, device_add_groups() in front of activate() works
for the usbport driver since it dynamically registers
the entries in "ports". However, if a trigger has a
static list of entities this can get more complicated,
since I think the sysfs entries will now be available
before the activate() call even started and this can
end up badly. So, is there any better approach?
Introduce a "post_activate()" callback? Or use the event
system and make usbport trigger on the KOBJ_CHANGE? use
a (delayed) work_struct in usbport to register the ports
at a slightly later time? etc...
(Note: I'm hitting this on 4.19 too, so whatever the
real fix will look like it should be backported)
---
drivers/leds/led-triggers.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 17d73db1456e..08e7c724a9dc 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -134,6 +134,12 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
led_set_brightness(led_cdev, LED_OFF);
}
if (trig) {
+ ret = device_add_groups(led_cdev->dev, trig->groups);
+ if (ret) {
+ dev_err(led_cdev->dev, "Failed to add trigger attributes\n");
+ goto err_add_groups;
+ }
+
write_lock_irqsave(&trig->leddev_list_lock, flags);
list_add_tail(&led_cdev->trig_list, &trig->led_cdevs);
write_unlock_irqrestore(&trig->leddev_list_lock, flags);
@@ -146,12 +152,6 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
if (ret)
goto err_activate;
-
- ret = device_add_groups(led_cdev->dev, trig->groups);
- if (ret) {
- dev_err(led_cdev->dev, "Failed to add trigger attributes\n");
- goto err_add_groups;
- }
}
if (event) {
@@ -165,17 +165,18 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
return 0;
-err_add_groups:
-
+err_activate:
+ device_remove_groups(led_cdev->dev, trig->groups);
if (trig->deactivate)
trig->deactivate(led_cdev);
-err_activate:
led_cdev->trigger = NULL;
led_cdev->trigger_data = NULL;
write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
list_del(&led_cdev->trig_list);
write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock, flags);
+
+err_add_groups:
led_set_brightness(led_cdev, LED_OFF);
return ret;
--
2.20.1
