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@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...
>
> 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)
