[RFC,v1] leds: fix regression in usbport led trigger

[Christian Lamparter <chunkeey@gmail.com>]

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)