On Wed, Jan 23, 2013 at 01:01:17AM +0100, Rafael J. Wysocki wrote:
> On Tuesday, January 22, 2013 03:47:18 PM Greg Kroah-Hartman wrote:
> > On Tue, Jan 22, 2013 at 03:25:15AM +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > >
> > > Make it possible to retrieve the current power state of a device with
> > > ACPI power management from user space via sysfs by adding two new
> > > attributes, power_state and real_power_state, to the sysfs directory
> > > associated with the struct acpi_device object representing the
> > > device's ACPI node.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > ---
> > > Documentation/ABI/testing/sysfs-devices-power_state | 20 ++++++
> > > Documentation/ABI/testing/sysfs-devices-real_power_state | 23 +++++++
> > > drivers/acpi/scan.c | 49 ++++++++++++++-
> > > 3 files changed, 91 insertions(+), 1 deletion(-)
> > >
> > > Index: linux-pm/drivers/acpi/scan.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/acpi/scan.c
> > > +++ linux-pm/drivers/acpi/scan.c
> > > @@ -178,6 +178,32 @@ err_out:
> > > }
> > > EXPORT_SYMBOL(acpi_bus_hot_remove_device);
> > >
> > > +static ssize_t real_power_state_show(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + struct acpi_device *adev = to_acpi_device(dev);
> > > + int state;
> > > + int ret;
> > > +
> > > + ret = acpi_device_get_power(adev, &state);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return sprintf(buf, "%s\n", acpi_power_state_string(state));
> > > +}
> > > +
> > > +static DEVICE_ATTR(real_power_state, 0444, real_power_state_show, NULL);
> > > +
> > > +static ssize_t power_state_show(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + struct acpi_device *adev = to_acpi_device(dev);
> > > +
> > > + return sprintf(buf, "%s\n", acpi_power_state_string(adev->power.state));
> > > +}
> > > +
> > > +static DEVICE_ATTR(power_state, 0444, power_state_show, NULL);
> > > +
> > > static ssize_t
> > > acpi_eject_store(struct device *d, struct device_attribute *attr,
> > > const char *buf, size_t count)
> > > @@ -369,8 +395,22 @@ static int acpi_device_setup_files(struc
> > > * hot-removal function from userland.
> > > */
> > > status = acpi_get_handle(dev->handle, "_EJ0", &temp);
> > > - if (ACPI_SUCCESS(status))
> > > + if (ACPI_SUCCESS(status)) {
> > > result = device_create_file(&dev->dev, &dev_attr_eject);
> > > + if (result)
> > > + return result;
> > > + }
> > > +
> > > + if (dev->flags.power_manageable) {
> > > + result = device_create_file(&dev->dev, &dev_attr_power_state);
> > > + if (result)
> > > + return result;
> > > +
> > > + if (dev->power.flags.power_resources)
> > > + result = device_create_file(&dev->dev,
> > > + &dev_attr_real_power_state);
> > > + }
> > > +
> >
> > It isn't your fault, but I just noticed this when reading patch 2/4, but
> > I think you are racing userspace here. The device is registered,
> > userspace is notified that the device is present, and _then_ the acpi
> > core creates the sysfs files for the device.
> >
> > That's not good, and needs to be fixed, especially now that you are
> > adding more and more sysfs files for these ACPI devices.
> >
> > To do that, you need to set up "default" files for ACPI devices, they
> > can be enabled or not by the driver core, and will be created before the
> > event that the device is created is sent to userspace.
> >
> > Care to fix that all up in a follow-on patchset before some tools start
> > to get confused?
>
> I will.
>
> That's kind of has been happening for quite a long time, though, as far as
> I can say, so confusion would have ensued already before. Not that I think
> it's not worth fixing, to be sure. :-)
Odds are, not many people have been paying attention to acpi devices,
but now that you are adding "useful" information in their sysfs files,
tools will be written to access that information, so they are the ones
that will start getting confused.
> So acpi_device_setup_files() is called too late already, right?
Yes. But see my email for the 4/4 patch, at the end, there is a
"simple" fix for this that you can do that is much easier than reworking
all of this.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
