| Message ID | 1477263.uvKKInuF7l@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Jan 22, 2013 at 03:28:23AM +0100, Rafael J. Wysocki wrote: > +static struct attribute *attrs[] = { > + NULL, > +}; That's "odd"... > +static void acpi_power_hide_list(struct acpi_device *adev, > + struct acpi_device_power_state *ps, > + const char *group_name) > { > - if (adev->power.flags.power_resources) { > - struct acpi_device_power_state *ps; > - struct acpi_power_resource_entry *entry; > - > - ps = &adev->power.states[ACPI_STATE_D0]; > - list_for_each_entry(entry, &ps->resources, node) { > - struct acpi_power_resource *resource = entry->resource; > - > - if (add) > - acpi_power_add_dependent(resource, adev); > - else > - acpi_power_remove_dependent(resource, adev); > + struct attribute_group attr_group = { > + .name = group_name, > + .attrs = attrs, > + }; This is on the stack, which seems like it would not be good... > + struct acpi_power_resource_entry *entry; > + > + list_for_each_entry_reverse(entry, &ps->resources, node) { > + struct acpi_device *res_dev = &entry->resource->device; > + > + sysfs_remove_link_from_group(&adev->dev.kobj, group_name, > + dev_name(&res_dev->dev)); > + } > + sysfs_remove_group(&adev->dev.kobj, &attr_group); You aren't removing the same group that you created. Well, kind of, but that's strange, it really works? > +static void acpi_power_expose_list(struct acpi_device *adev, > + struct acpi_device_power_state *ps, > + const char *group_name) > +{ > + struct attribute_group attr_group = { > + .name = group_name, > + .attrs = attrs, > + }; again a structure on the stack. Why not just create the attribute groups as static, instead of "pseudo-dynamically" like you are doing here? I have no idea if sysfs can properly cope with an attribute group pointer that disappears after it has been registered with the sysfs core. That seems ripe for problems, don't you agree? Oh, and same question about racing userspace, you will have problems here in that the symlinks will be showing up after the device is created. Perhaps, to make the whole thing easier, you just change the acpi core code to hold off on the notification until you get all of these links and files set up and then tell userspace. That's probably an easier fix. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, January 22, 2013 03:56:24 PM Greg Kroah-Hartman wrote: > On Tue, Jan 22, 2013 at 03:28:23AM +0100, Rafael J. Wysocki wrote: > > +static struct attribute *attrs[] = { > > + NULL, > > +}; > > That's "odd"... Well, I don't really need file attributes here (at least at the moment), but sysfs_create_group() will complain if I just pass NULL in there. > > +static void acpi_power_hide_list(struct acpi_device *adev, > > + struct acpi_device_power_state *ps, > > + const char *group_name) > > { > > - if (adev->power.flags.power_resources) { > > - struct acpi_device_power_state *ps; > > - struct acpi_power_resource_entry *entry; > > - > > - ps = &adev->power.states[ACPI_STATE_D0]; > > - list_for_each_entry(entry, &ps->resources, node) { > > - struct acpi_power_resource *resource = entry->resource; > > - > > - if (add) > > - acpi_power_add_dependent(resource, adev); > > - else > > - acpi_power_remove_dependent(resource, adev); > > + struct attribute_group attr_group = { > > + .name = group_name, > > + .attrs = attrs, > > + }; > > This is on the stack, which seems like it would not be good... > > > + struct acpi_power_resource_entry *entry; > > + > > + list_for_each_entry_reverse(entry, &ps->resources, node) { > > + struct acpi_device *res_dev = &entry->resource->device; > > + > > + sysfs_remove_link_from_group(&adev->dev.kobj, group_name, > > + dev_name(&res_dev->dev)); > > + } > > + sysfs_remove_group(&adev->dev.kobj, &attr_group); > > You aren't removing the same group that you created. Well, kind of, but > that's strange, it really works? Yes, it does, as far as I can say. > > +static void acpi_power_expose_list(struct acpi_device *adev, > > + struct acpi_device_power_state *ps, > > + const char *group_name) > > +{ > > + struct attribute_group attr_group = { > > + .name = group_name, > > + .attrs = attrs, > > + }; > > again a structure on the stack. > > Why not just create the attribute groups as static, instead of > "pseudo-dynamically" like you are doing here? Basically because I'm lazy. :-) They can be static if that's better. > I have no idea if sysfs > can properly cope with an attribute group pointer that disappears after > it has been registered with the sysfs core. That seems ripe for > problems, don't you agree? I don't think so having looked at the code, but I very well might overlook something. I'll change that. > Oh, and same question about racing userspace, you will have problems > here in that the symlinks will be showing up after the device is > created. Perhaps, to make the whole thing easier, you just change the > acpi core code to hold off on the notification until you get all of > these links and files set up and then tell userspace. That's probably > an easier fix. I suppose so. How can I do that? Rafael
On Wednesday, January 23, 2013 01:08:54 AM Rafael J. Wysocki wrote: > On Tuesday, January 22, 2013 03:56:24 PM Greg Kroah-Hartman wrote: > > On Tue, Jan 22, 2013 at 03:28:23AM +0100, Rafael J. Wysocki wrote: [...] > > Oh, and same question about racing userspace, you will have problems > > here in that the symlinks will be showing up after the device is > > created. Perhaps, to make the whole thing easier, you just change the > > acpi core code to hold off on the notification until you get all of > > these links and files set up and then tell userspace. That's probably > > an easier fix. > > I suppose so. > > How can I do that? Should I set dev->kobj.uevent_suppress before calling device_register() and then clear it and call the kobject_uevent(&dev->kobj, KOBJ_ADD) from the ACPI core after device_register() has returned and the files have been created? Rafael
On Wednesday, January 23, 2013 01:17:47 AM Rafael J. Wysocki wrote: > On Wednesday, January 23, 2013 01:08:54 AM Rafael J. Wysocki wrote: > > On Tuesday, January 22, 2013 03:56:24 PM Greg Kroah-Hartman wrote: > > > On Tue, Jan 22, 2013 at 03:28:23AM +0100, Rafael J. Wysocki wrote: > > [...] > > > > Oh, and same question about racing userspace, you will have problems > > > here in that the symlinks will be showing up after the device is > > > created. Perhaps, to make the whole thing easier, you just change the > > > acpi core code to hold off on the notification until you get all of > > > these links and files set up and then tell userspace. That's probably > > > an easier fix. > > > > I suppose so. > > > > How can I do that? > > Should I set dev->kobj.uevent_suppress before calling device_register() and > then clear it and call the kobject_uevent(&dev->kobj, KOBJ_ADD) from the ACPI > core after device_register() has returned and the files have been created? I suppose along the lines of how firmware_class.c uses uevent_suppress? Is there any safe mechanism for adding/removing sysfs attributes after kobject_uevent(&dev->kobj, KOBJ_ADD) has been called for the given device? Rafael
On Wed, Jan 23, 2013 at 01:17:47AM +0100, Rafael J. Wysocki wrote: > On Wednesday, January 23, 2013 01:08:54 AM Rafael J. Wysocki wrote: > > On Tuesday, January 22, 2013 03:56:24 PM Greg Kroah-Hartman wrote: > > > On Tue, Jan 22, 2013 at 03:28:23AM +0100, Rafael J. Wysocki wrote: > > [...] > > > > Oh, and same question about racing userspace, you will have problems > > > here in that the symlinks will be showing up after the device is > > > created. Perhaps, to make the whole thing easier, you just change the > > > acpi core code to hold off on the notification until you get all of > > > these links and files set up and then tell userspace. That's probably > > > an easier fix. > > > > I suppose so. > > > > How can I do that? > > Should I set dev->kobj.uevent_suppress before calling device_register() and > then clear it and call the kobject_uevent(&dev->kobj, KOBJ_ADD) from the ACPI > core after device_register() has returned and the files have been created? Ick, that might work, but the "traditional" way is to just do the creation of the device in two steps. First call device_initialize(). Then you can do what you want to the device, add sysfs files, etc. Then call device_add() which "finalizes" the device in the driver core and tells userspace all about it. The USB core has been doing this since the beginning of time (well, since we wrote the driver model) and it has worked out pretty well. Calling dev_set_uevent_suppress() would also probably work, like you point out the firmware core uses this. Hm, it uses this to create some sysfs files and then tell userspace about them, even though it uses device_add(), that's "odd". Either way should be fine, you can run 'udevadm monitor' as root to watch the sysfs events be sent from the kernel to make sure you have it all working properly. Hope this helps, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 23, 2013 at 01:28:40AM +0100, Rafael J. Wysocki wrote: > On Wednesday, January 23, 2013 01:17:47 AM Rafael J. Wysocki wrote: > > On Wednesday, January 23, 2013 01:08:54 AM Rafael J. Wysocki wrote: > > > On Tuesday, January 22, 2013 03:56:24 PM Greg Kroah-Hartman wrote: > > > > On Tue, Jan 22, 2013 at 03:28:23AM +0100, Rafael J. Wysocki wrote: > > > > [...] > > > > > > Oh, and same question about racing userspace, you will have problems > > > > here in that the symlinks will be showing up after the device is > > > > created. Perhaps, to make the whole thing easier, you just change the > > > > acpi core code to hold off on the notification until you get all of > > > > these links and files set up and then tell userspace. That's probably > > > > an easier fix. > > > > > > I suppose so. > > > > > > How can I do that? > > > > Should I set dev->kobj.uevent_suppress before calling device_register() and > > then clear it and call the kobject_uevent(&dev->kobj, KOBJ_ADD) from the ACPI > > core after device_register() has returned and the files have been created? > > I suppose along the lines of how firmware_class.c uses uevent_suppress? > > Is there any safe mechanism for adding/removing sysfs attributes after > kobject_uevent(&dev->kobj, KOBJ_ADD) has been called for the given device? Not really. There is the KBOJ_CHANGE event that some subsystems use (hey look, ACPI already uses it), but that's usually used to tell userspace that the kobject has somehow "changed" status, just adding the initial files to it seems like a gratuitous "change", right? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, January 22, 2013 05:03:59 PM Greg Kroah-Hartman wrote: > On Wed, Jan 23, 2013 at 01:17:47AM +0100, Rafael J. Wysocki wrote: > > On Wednesday, January 23, 2013 01:08:54 AM Rafael J. Wysocki wrote: > > > On Tuesday, January 22, 2013 03:56:24 PM Greg Kroah-Hartman wrote: > > > > On Tue, Jan 22, 2013 at 03:28:23AM +0100, Rafael J. Wysocki wrote: > > > > [...] > > > > > > Oh, and same question about racing userspace, you will have problems > > > > here in that the symlinks will be showing up after the device is > > > > created. Perhaps, to make the whole thing easier, you just change the > > > > acpi core code to hold off on the notification until you get all of > > > > these links and files set up and then tell userspace. That's probably > > > > an easier fix. > > > > > > I suppose so. > > > > > > How can I do that? > > > > Should I set dev->kobj.uevent_suppress before calling device_register() and > > then clear it and call the kobject_uevent(&dev->kobj, KOBJ_ADD) from the ACPI > > core after device_register() has returned and the files have been created? > > Ick, that might work, but the "traditional" way is to just do the > creation of the device in two steps. > > First call device_initialize(). Then you can do what you want to the > device, add sysfs files, etc. Then call device_add() which "finalizes" > the device in the driver core and tells userspace all about it. Well, this quite obviously doesn't work. :-) If I call acpi_device_setup_files() between device_initialize() and device_add(), it will trigger the BUG_ON() in sysfs_create_file(), because kobj->sd is still NULL at this point. > The USB core has been doing this since the beginning of time (well, since we > wrote the driver model) and it has worked out pretty well. I guess it does something slightly different from what I need to do, however. > Calling dev_set_uevent_suppress() would also probably work, like you > point out the firmware core uses this. I think I'll need to use this approach. I think that it still will be prudent to call device_initialize() before dev_set_uevent_suppress(), though. I'll post a new series including this later today. > Hm, it uses this to create some sysfs files and then tell userspace about > them, even though it uses device_add(), that's "odd". Well, it seems to have the same problem as I do (described above). > Either way should be fine, you can run 'udevadm monitor' as root to > watch the sysfs events be sent from the kernel to make sure you have it > all working properly. Unfortunately, core ACPI device objects are created way too early for user space to be useful and I don't have any systems with ACPI-based hotplug around here ... Thanks, Rafael
On Tuesday, January 22, 2013 05:05:50 PM Greg Kroah-Hartman wrote: > On Wed, Jan 23, 2013 at 01:28:40AM +0100, Rafael J. Wysocki wrote: > > On Wednesday, January 23, 2013 01:17:47 AM Rafael J. Wysocki wrote: > > > On Wednesday, January 23, 2013 01:08:54 AM Rafael J. Wysocki wrote: > > > > On Tuesday, January 22, 2013 03:56:24 PM Greg Kroah-Hartman wrote: > > > > > On Tue, Jan 22, 2013 at 03:28:23AM +0100, Rafael J. Wysocki wrote: > > > > > > [...] > > > > > > > > Oh, and same question about racing userspace, you will have problems > > > > > here in that the symlinks will be showing up after the device is > > > > > created. Perhaps, to make the whole thing easier, you just change the > > > > > acpi core code to hold off on the notification until you get all of > > > > > these links and files set up and then tell userspace. That's probably > > > > > an easier fix. > > > > > > > > I suppose so. > > > > > > > > How can I do that? > > > > > > Should I set dev->kobj.uevent_suppress before calling device_register() and > > > then clear it and call the kobject_uevent(&dev->kobj, KOBJ_ADD) from the ACPI > > > core after device_register() has returned and the files have been created? > > > > I suppose along the lines of how firmware_class.c uses uevent_suppress? > > > > Is there any safe mechanism for adding/removing sysfs attributes after > > kobject_uevent(&dev->kobj, KOBJ_ADD) has been called for the given device? > > Not really. There is the KBOJ_CHANGE event that some subsystems use > (hey look, ACPI already uses it), but that's usually used to tell > userspace that the kobject has somehow "changed" status, just adding the > initial files to it seems like a gratuitous "change", right? Sure, that was a general question. :-) Thanks, Rafael
Index: linux-pm/drivers/acpi/power.c =================================================================== --- linux-pm.orig/drivers/acpi/power.c +++ linux-pm/drivers/acpi/power.c @@ -417,24 +417,94 @@ static void acpi_power_remove_dependent( } } -void acpi_power_add_remove_device(struct acpi_device *adev, bool add) +static struct attribute *attrs[] = { + NULL, +}; + +static void acpi_power_hide_list(struct acpi_device *adev, + struct acpi_device_power_state *ps, + const char *group_name) { - if (adev->power.flags.power_resources) { - struct acpi_device_power_state *ps; - struct acpi_power_resource_entry *entry; - - ps = &adev->power.states[ACPI_STATE_D0]; - list_for_each_entry(entry, &ps->resources, node) { - struct acpi_power_resource *resource = entry->resource; - - if (add) - acpi_power_add_dependent(resource, adev); - else - acpi_power_remove_dependent(resource, adev); + struct attribute_group attr_group = { + .name = group_name, + .attrs = attrs, + }; + struct acpi_power_resource_entry *entry; + + list_for_each_entry_reverse(entry, &ps->resources, node) { + struct acpi_device *res_dev = &entry->resource->device; + + sysfs_remove_link_from_group(&adev->dev.kobj, group_name, + dev_name(&res_dev->dev)); + } + sysfs_remove_group(&adev->dev.kobj, &attr_group); +} + +static void acpi_power_expose_list(struct acpi_device *adev, + struct acpi_device_power_state *ps, + const char *group_name) +{ + struct attribute_group attr_group = { + .name = group_name, + .attrs = attrs, + }; + struct acpi_power_resource_entry *entry; + int ret; + + ret = sysfs_create_group(&adev->dev.kobj, &attr_group); + if (ret) + return; + + list_for_each_entry(entry, &ps->resources, node) { + struct acpi_device *res_dev = &entry->resource->device; + + ret = sysfs_add_link_to_group(&adev->dev.kobj, group_name, + &res_dev->dev.kobj, + dev_name(&res_dev->dev)); + if (ret) { + acpi_power_hide_list(adev, ps, group_name); + break; } } } +void acpi_power_add_remove_device(struct acpi_device *adev, bool add) +{ + static const char *group_names[ACPI_D_STATE_COUNT] = { + [ACPI_STATE_D0] = "power_resources_D0", + [ACPI_STATE_D1] = "power_resources_D1", + [ACPI_STATE_D2] = "power_resources_D2", + [ACPI_STATE_D3_HOT] = "power_resources_D3hot", + }; + struct acpi_device_power_state *ps; + struct acpi_power_resource_entry *entry; + int state; + + if (!adev->power.flags.power_resources) + return; + + ps = &adev->power.states[ACPI_STATE_D0]; + list_for_each_entry(entry, &ps->resources, node) { + struct acpi_power_resource *resource = entry->resource; + + if (add) + acpi_power_add_dependent(resource, adev); + else + acpi_power_remove_dependent(resource, adev); + } + + for (state = ACPI_STATE_D0; state <= ACPI_STATE_D3_HOT; state++) { + ps = &adev->power.states[state]; + if (list_empty(&ps->resources)) + continue; + + if (add) + acpi_power_expose_list(adev, ps, group_names[state]); + else + acpi_power_hide_list(adev, ps, group_names[state]); + } +} + int acpi_power_min_system_level(struct list_head *list) { struct acpi_power_resource_entry *entry; Index: linux-pm/Documentation/ABI/testing/sysfs-devices-power_resources_D0 =================================================================== --- /dev/null +++ linux-pm/Documentation/ABI/testing/sysfs-devices-power_resources_D0 @@ -0,0 +1,13 @@ +What: /sys/devices/.../power_resources_D0/ +Date: January 2013 +Contact: Rafael J. Wysocki <rafael.j.wysocki@intel.com> +Description: + The /sys/devices/.../power_resources_D0/ directory is only + present for device objects representing ACPI device nodes that + use ACPI power resources for power management. + + If present, it contains symbolic links to device directories + representing ACPI power resources that need to be turned on for + the given device node to be in ACPI power state D0. The names + of the links are the same as the names of the directories they + point to. Index: linux-pm/Documentation/ABI/testing/sysfs-devices-power_resources_D1 =================================================================== --- /dev/null +++ linux-pm/Documentation/ABI/testing/sysfs-devices-power_resources_D1 @@ -0,0 +1,14 @@ +What: /sys/devices/.../power_resources_D1/ +Date: January 2013 +Contact: Rafael J. Wysocki <rafael.j.wysocki@intel.com> +Description: + The /sys/devices/.../power_resources_D1/ directory is only + present for device objects representing ACPI device nodes that + use ACPI power resources for power management and support ACPI + power state D1. + + If present, it contains symbolic links to device directories + representing ACPI power resources that need to be turned on for + the given device node to be in ACPI power state D1. The names + of the links are the same as the names of the directories they + point to. Index: linux-pm/Documentation/ABI/testing/sysfs-devices-power_resources_D2 =================================================================== --- /dev/null +++ linux-pm/Documentation/ABI/testing/sysfs-devices-power_resources_D2 @@ -0,0 +1,14 @@ +What: /sys/devices/.../power_resources_D2/ +Date: January 2013 +Contact: Rafael J. Wysocki <rafael.j.wysocki@intel.com> +Description: + The /sys/devices/.../power_resources_D2/ directory is only + present for device objects representing ACPI device nodes that + use ACPI power resources for power management and support ACPI + power state D2. + + If present, it contains symbolic links to device directories + representing ACPI power resources that need to be turned on for + the given device node to be in ACPI power state D2. The names + of the links are the same as the names of the directories they + point to. Index: linux-pm/Documentation/ABI/testing/sysfs-devices-power_resources_D3hot =================================================================== --- /dev/null +++ linux-pm/Documentation/ABI/testing/sysfs-devices-power_resources_D3hot @@ -0,0 +1,14 @@ +What: /sys/devices/.../power_resources_D3hot/ +Date: January 2013 +Contact: Rafael J. Wysocki <rafael.j.wysocki@intel.com> +Description: + The /sys/devices/.../power_resources_D3hot/ directory is only + present for device objects representing ACPI device nodes that + use ACPI power resources for power management and support ACPI + power state D3hot. + + If present, it contains symbolic links to device directories + representing ACPI power resources that need to be turned on for + the given device node to be in ACPI power state D3hot. The + names of the links are the same as the names of the directories + they point to.