Patchwork [v2] leds: fix regression in usbport led trigger

login
register
mail settings
Submitter Christian Lamparter
Date Jan. 11, 2019, 4:29 p.m.
Message ID <20190111162945.31612-1-chunkeey@gmail.com>
Download mbox | patch
Permalink /patch/697879/
State New
Headers show

Comments

Christian Lamparter - Jan. 11, 2019, 4:29 p.m.
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(-)
Greg Kroah-Hartman - Jan. 18, 2019, 8:56 a.m.
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
Uwe Kleine-König - Jan. 18, 2019, 9:07 a.m.
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
Greg Kroah-Hartman - Jan. 18, 2019, 9:13 a.m.
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
Uwe Kleine-König - Jan. 18, 2019, 9:22 a.m.
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
Greg Kroah-Hartman - Jan. 18, 2019, 9:28 a.m.
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
Uwe Kleine-König - Jan. 18, 2019, 9:44 a.m.
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

Patch

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)