| Message ID | 20190111162945.31612-1-chunkeey@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 91f7d2e89868fcac0e750a28230fdb1ad4512137 |
| Headers | show |
| Series | [v2] leds: fix regression in usbport led trigger | expand |
On Fri, Jan 11, 2019 at 05:29:45PM +0100, 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 reverts parts of the "usb: simplify usbport > trigger" patch and restores usbport trigger's functionality. This feels like going backwards, as a driver should not be adding and removing sysfs groups, because you race with userspace. Userspace has no idea that the new sysfs files were added or removed, right? I'll apply this, but this feels like a problem in the led api if the above really is true. also, please wrap your changelogs at 72 columns, making it easier to read. I did that here for you... thanks, greg k-h
On Fri, Jan 18, 2019 at 09:56:52AM +0100, Greg Kroah-Hartman wrote: > On Fri, Jan 11, 2019 at 05:29:45PM +0100, 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 reverts parts of the "usb: simplify usbport > > trigger" patch and restores usbport trigger's functionality. > > This feels like going backwards, as a driver should not be adding and > removing sysfs groups, because you race with userspace. Userspace has > no idea that the new sysfs files were added or removed, right? I think this is not an issue as the led trigger core code calls kobject_uevent_env() when the trigger is set. But I still agree that there seems to be something "unusual" about the usb led trigger ... Instead of providing a sysfs-file per port to make said port trigger the led, I'd prefer a single file that you can use to add and remove ports from the trigger. With this change the driver can properly benefit from the attribute handling of the led-trigger core and is more in line with the other triggers. > I'll apply this, but this feels like a problem in the led api if the > above really is true. > > also, please wrap your changelogs at 72 columns, making it easier to > read. I did that here for you... In the mail I received the changelog looked fine (though I didn't check the actual width). Best regards Uwe
On Fri, Jan 18, 2019 at 10:07:05AM +0100, Uwe Kleine-König wrote: > On Fri, Jan 18, 2019 at 09:56:52AM +0100, Greg Kroah-Hartman wrote: > > On Fri, Jan 11, 2019 at 05:29:45PM +0100, 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 reverts parts of the "usb: simplify usbport > > > trigger" patch and restores usbport trigger's functionality. > > > > This feels like going backwards, as a driver should not be adding and > > removing sysfs groups, because you race with userspace. Userspace has > > no idea that the new sysfs files were added or removed, right? > > I think this is not an issue as the led trigger core code calls > kobject_uevent_env() when the trigger is set. Ok, that helps a lot then. > But I still agree that there seems to be something "unusual" about the > usb led trigger ... > > Instead of providing a sysfs-file per port to make said port trigger the > led, I'd prefer a single file that you can use to add and remove ports > from the trigger. With this change the driver can properly benefit from > the attribute handling of the led-trigger core and is more in line with > the other triggers. But how do you know how many ports are present? And this feels like it ends up being a "custom api" for each different type of led that is present in the system. Or is that already the case? thanks, greg k-h
On Fri, Jan 18, 2019 at 10:13:37AM +0100, Greg Kroah-Hartman wrote: > On Fri, Jan 18, 2019 at 10:07:05AM +0100, Uwe Kleine-König wrote: > > But I still agree that there seems to be something "unusual" about the > > usb led trigger ... > > > > Instead of providing a sysfs-file per port to make said port trigger the > > led, I'd prefer a single file that you can use to add and remove ports > > from the trigger. With this change the driver can properly benefit from > > the attribute handling of the led-trigger core and is more in line with > > the other triggers. > > But how do you know how many ports are present? And this feels like it > ends up being a "custom api" for each different type of led that is I assume you mean "different type of led trigger" here. (For leds that is not true.) > present in the system. Or is that already the case? I imagine something like: # cat available_ports port1 port2 port3 # cat active_ports port1 # echo port2 > active_ports # cat active_ports port1 port2 Regarding the "custom API" point: Sure, it's not possible to entirely prevent this as an UART trigger is about UARTs and an USB trigger is about USB ports. We could argue about a callback that somehow enumerates possible trigger sources, but I'm not sure this simplifies stuff in the end because there are still some special cases. E.g. you might want to have the UART trigger only blink on TX for ttyS2. Best regards Uwe
On Fri, Jan 18, 2019 at 10:22:02AM +0100, Uwe Kleine-König wrote: > On Fri, Jan 18, 2019 at 10:13:37AM +0100, Greg Kroah-Hartman wrote: > > On Fri, Jan 18, 2019 at 10:07:05AM +0100, Uwe Kleine-König wrote: > > > But I still agree that there seems to be something "unusual" about the > > > usb led trigger ... > > > > > > Instead of providing a sysfs-file per port to make said port trigger the > > > led, I'd prefer a single file that you can use to add and remove ports > > > from the trigger. With this change the driver can properly benefit from > > > the attribute handling of the led-trigger core and is more in line with > > > the other triggers. > > > > But how do you know how many ports are present? And this feels like it > > ends up being a "custom api" for each different type of led that is > > I assume you mean "different type of led trigger" here. (For leds that > is not true.) > > > present in the system. Or is that already the case? > > I imagine something like: > > # cat available_ports > port1 port2 port3 > > # cat active_ports > port1 > > # echo port2 > active_ports > # cat active_ports > port1 port2 > > Regarding the "custom API" point: Sure, it's not possible to entirely > prevent this as an UART trigger is about UARTs and an USB trigger is > about USB ports. We could argue about a callback that somehow enumerates > possible trigger sources, but I'm not sure this simplifies stuff in the > end because there are still some special cases. E.g. you might want to > have the UART trigger only blink on TX for ttyS2. But, the trigger is on the specific ttyS2 device, so keeping it on the individual port devices makes a lot of sense. sysfs should be one-value-per-file where ever possible. thanks, greg k-h
On Fri, Jan 18, 2019 at 10:28:34AM +0100, Greg Kroah-Hartman wrote: > On Fri, Jan 18, 2019 at 10:22:02AM +0100, Uwe Kleine-König wrote: > > On Fri, Jan 18, 2019 at 10:13:37AM +0100, Greg Kroah-Hartman wrote: > > > On Fri, Jan 18, 2019 at 10:07:05AM +0100, Uwe Kleine-König wrote: > > > > But I still agree that there seems to be something "unusual" about the > > > > usb led trigger ... > > > > > > > > Instead of providing a sysfs-file per port to make said port trigger the > > > > led, I'd prefer a single file that you can use to add and remove ports > > > > from the trigger. With this change the driver can properly benefit from > > > > the attribute handling of the led-trigger core and is more in line with > > > > the other triggers. > > > > > > But how do you know how many ports are present? And this feels like it > > > ends up being a "custom api" for each different type of led that is > > > > I assume you mean "different type of led trigger" here. (For leds that > > is not true.) > > > > > present in the system. Or is that already the case? > > > > I imagine something like: > > > > # cat available_ports > > port1 port2 port3 > > > > # cat active_ports > > port1 > > > > # echo port2 > active_ports > > # cat active_ports > > port1 port2 > > > > Regarding the "custom API" point: Sure, it's not possible to entirely > > prevent this as an UART trigger is about UARTs and an USB trigger is > > about USB ports. We could argue about a callback that somehow enumerates > > possible trigger sources, but I'm not sure this simplifies stuff in the > > end because there are still some special cases. E.g. you might want to > > have the UART trigger only blink on TX for ttyS2. > > But, the trigger is on the specific ttyS2 device, so keeping it on the (Sticking to the USB trigger, as the tty trigger isn't completed yet.) Not sure I got you right, but the trigger is configured under the sysfs entry of the affected LED, not the triggering device. So currently you have to do: echo usbport > /sys/class/leds/input0::capslock/trigger echo 1 > /sys/class/leds/input0::capslock/ports/myhub-port2 > individual port devices makes a lot of sense. sysfs should be > one-value-per-file where ever possible. So if the above API is considered better above what I suggested, we have to rethink how this can be simplified for trigger drivers in the trigger core. Best regards Uwe
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)
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 reverts parts of the "usb: simplify usbport trigger" patch and restores usbport trigger's functionality. Fixes: 6f7b0bad8839 ("usb: simplify usbport trigger") Signed-off-by: Christian Lamparter <chunkeey@gmail.com> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> --- Let's see if linux-usb is still on holiday or not ;). --- drivers/usb/core/ledtrig-usbport.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)