| Message ID | 20181225204956.17897-1-chunkeey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v1] leds: fix regression in usbport led trigger | expand |
Hi Christian, Thank you for the patch. On 12/25/18 9:49 PM, Christian Lamparter wrote: > 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@linuxfoundation.org> > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Cc: Rafał Miłecki <zajec5@gmail.com> > Fixes: 6f7b0bad8839 ("usb: simplify usbport trigger") > Signed-off-by: Christian Lamparter <chunkeey@gmail.com> > --- > > 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... post_activate() is an option. Otherwise, with your patch, in order to prevent access to the sysfs interface before the trigger is initialized, we would have to implement is_visible op for each struct attribute_group in the triggers with static attrs, and check led_cdev->activated there (it is currently unused and scheduled for removal, but we would have to set it after calling activate() in LED trigger core). It would require more effort but would allow to avoid the addition of a new op. I'd also remove "activated" property from struct led_classdev and made it a flag in leds.h: +#define LED_TRIGGER_ACTIVATED BIT(24) Best regards, Jacek Anaszewski > (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; >
Hello Jacek, On Friday, December 28, 2018 5:29:56 PM CET Jacek Anaszewski wrote: > On 12/25/18 9:49 PM, Christian Lamparter wrote: > > 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@linuxfoundation.org> > > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Cc: Rafał Miłecki <zajec5@gmail.com> > > Fixes: 6f7b0bad8839 ("usb: simplify usbport trigger") > > Signed-off-by: Christian Lamparter <chunkeey@gmail.com> > > --- > > > > 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... > > post_activate() is an option. I was told to wait a bit more because parts of Europe are still enjoying the holidays. Happy New Year. :-D > Otherwise, with your patch, in order to prevent access to the sysfs > interface before the trigger is initialized, we would have to implement > is_visible op for each struct attribute_group in the triggers > with static attrs, and check led_cdev->activated there > (it is currently unused and scheduled for removal, but we > would have to set it after calling activate() in LED trigger core). From what I can tell by looking at a lot of led triggers, only the usbport led trigger is affected. So another sensible option could be to partially revert 6f7b0bad8839 ("usb: simplify usbport trigger") and let the usbport trigger manage its sysfs groups by itself. (see below). What do you think? Best regards, Christian Lamparter --- diff --git a/drivers/usb/core/ledtrig-usbport.c b/drivers/usb/core/ledtrig-usbport.c index dc7f7fd71684..c12ac56606c3 100644 --- a/drivers/usb/core/ledtrig-usbport.c +++ b/drivers/usb/core/ledtrig-usbport.c @@ -119,11 +119,6 @@ static const struct attribute_group ports_group = { .attrs = ports_attrs, }; -static const struct attribute_group *ports_groups[] = { - &ports_group, - NULL -}; - /*************************************** * Adding & removing ports ***************************************/ @@ -307,6 +302,7 @@ static int usbport_trig_notify(struct notifier_block *nb, unsigned long action, static int usbport_trig_activate(struct led_classdev *led_cdev) { struct usbport_trig_data *usbport_data; + int err; usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL); if (!usbport_data) @@ -315,6 +311,9 @@ static int usbport_trig_activate(struct led_classdev *led_cdev) /* List of ports */ INIT_LIST_HEAD(&usbport_data->ports); + err = sysfs_create_group(&led_cdev->dev->kobj, &ports_group); + if (err) + goto err_free; usb_for_each_dev(usbport_data, usbport_trig_add_usb_dev_ports); usbport_trig_update_count(usbport_data); @@ -322,8 +321,11 @@ static int usbport_trig_activate(struct led_classdev *led_cdev) usbport_data->nb.notifier_call = usbport_trig_notify; led_set_trigger_data(led_cdev, usbport_data); usb_register_notify(&usbport_data->nb); - return 0; + +err_free: + kfree(usbport_data); + return err; } static void usbport_trig_deactivate(struct led_classdev *led_cdev) @@ -335,6 +337,8 @@ static void usbport_trig_deactivate(struct led_classdev *led_cdev) usbport_trig_remove_port(usbport_data, port); } + sysfs_remove_group(&led_cdev->dev->kobj, &ports_group); + usb_unregister_notify(&usbport_data->nb); kfree(usbport_data); @@ -344,7 +348,6 @@ static struct led_trigger usbport_led_trigger = { .name = "usbport", .activate = usbport_trig_activate, .deactivate = usbport_trig_deactivate, - .groups = ports_groups, }; static int __init usbport_trig_init(void)
On 1/1/19 3:00 PM, Christian Lamparter wrote: > Hello Jacek, > On Friday, December 28, 2018 5:29:56 PM CET Jacek Anaszewski wrote: >> On 12/25/18 9:49 PM, Christian Lamparter wrote: >>> 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@linuxfoundation.org> >>> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >>> Cc: Rafał Miłecki <zajec5@gmail.com> >>> Fixes: 6f7b0bad8839 ("usb: simplify usbport trigger") >>> Signed-off-by: Christian Lamparter <chunkeey@gmail.com> >>> --- >>> >>> 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... >> >> post_activate() is an option. > I was told to wait a bit more because parts of Europe are > still enjoying the holidays. Happy New Year. :-D Happy New Year :-) >> Otherwise, with your patch, in order to prevent access to the sysfs >> interface before the trigger is initialized, we would have to implement >> is_visible op for each struct attribute_group in the triggers >> with static attrs, and check led_cdev->activated there >> (it is currently unused and scheduled for removal, but we >> would have to set it after calling activate() in LED trigger core). > From what I can tell by looking at a lot of led triggers, only the usbport > led trigger is affected. So another sensible option could be to partially > revert 6f7b0bad8839 ("usb: simplify usbport trigger") and let the usbport > trigger manage its sysfs groups by itself. (see below). What do you think? Works for me, but you'll need the approval from the other side, since this does not belong to the LED subsystem. Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > --- > diff --git a/drivers/usb/core/ledtrig-usbport.c b/drivers/usb/core/ledtrig-usbport.c > index dc7f7fd71684..c12ac56606c3 100644 > --- a/drivers/usb/core/ledtrig-usbport.c > +++ b/drivers/usb/core/ledtrig-usbport.c > @@ -119,11 +119,6 @@ static const struct attribute_group ports_group = { > .attrs = ports_attrs, > }; > > -static const struct attribute_group *ports_groups[] = { > - &ports_group, > - NULL > -}; > - > /*************************************** > * Adding & removing ports > ***************************************/ > @@ -307,6 +302,7 @@ static int usbport_trig_notify(struct notifier_block *nb, unsigned long action, > static int usbport_trig_activate(struct led_classdev *led_cdev) > { > struct usbport_trig_data *usbport_data; > + int err; > > usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL); > if (!usbport_data) > @@ -315,6 +311,9 @@ static int usbport_trig_activate(struct led_classdev *led_cdev) > > /* List of ports */ > INIT_LIST_HEAD(&usbport_data->ports); > + err = sysfs_create_group(&led_cdev->dev->kobj, &ports_group); > + if (err) > + goto err_free; > usb_for_each_dev(usbport_data, usbport_trig_add_usb_dev_ports); > usbport_trig_update_count(usbport_data); > > @@ -322,8 +321,11 @@ static int usbport_trig_activate(struct led_classdev *led_cdev) > usbport_data->nb.notifier_call = usbport_trig_notify; > led_set_trigger_data(led_cdev, usbport_data); > usb_register_notify(&usbport_data->nb); > - > return 0; > + > +err_free: > + kfree(usbport_data); > + return err; > } > > static void usbport_trig_deactivate(struct led_classdev *led_cdev) > @@ -335,6 +337,8 @@ static void usbport_trig_deactivate(struct led_classdev *led_cdev) > usbport_trig_remove_port(usbport_data, port); > } > > + sysfs_remove_group(&led_cdev->dev->kobj, &ports_group); > + > usb_unregister_notify(&usbport_data->nb); > > kfree(usbport_data); > @@ -344,7 +348,6 @@ static struct led_trigger usbport_led_trigger = { > .name = "usbport", > .activate = usbport_trig_activate, > .deactivate = usbport_trig_deactivate, > - .groups = ports_groups, > }; > > static int __init usbport_trig_init(void) > > > > > >
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;
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@linuxfoundation.org> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Cc: Rafał Miłecki <zajec5@gmail.com> Fixes: 6f7b0bad8839 ("usb: simplify usbport trigger") Signed-off-by: Christian Lamparter <chunkeey@gmail.com> --- 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(-)