Re: [PATCH 1/4] ACPI / PM: Expose power states of ACPI devices to user space

["Rafael J. Wysocki" <rjw@sisk.pl>]

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@intel.com>
> > 
> > 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@intel.com>
> > ---
> >  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. :-)

So acpi_device_setup_files() is called too late already, right?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.