[RFC][PATCH 0/3] ACPI / PM: Export power resources information to user space

[RFC][PATCH 0/3] ACPI / PM: Export power resources information to user space

[Rafael J. Wysocki]

Hi Greg,

The following patch series is about exporting ACPI power resources to user
space that will be necessary for PM diagnostics on new platforms (powertop is
the target).  Still, existing systems can also benefit from that, like my
oldish HP nx6325 on which the patches have been tested.

Patch [1/3] shouldn't be really controverisal, as it only makes it possible
to read power resources' reference counters from user space in a quite
straightforward way.

Patch [3/3], however, exposes lists of power resources associated with
different power states of devices using ACPI PM as sysfs subdirectories
containing symbolic links and that requires some sysfs changes made by
patch [2/3].

Admittedly, my sysfs skills are limited so please let me know if those
changes make sense and are acceptable from your perspective.  If not, then
please kindly advise what I can do instead of them.

The patches apply on top of the current linux-next branch of the linux-pm.git
tree.

Thanks,
Rafael


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

[RFC][PATCH 1/3] ACPI / PM: Expose reference count values of ACPI power resources

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since ACPI power resources are going to be used more extensively on
new hardware platforms, it becomes necessary for user space (powertop
in particular) to observe some properties of those resources for
diagnostics purposes.

For this reason, export the reference counts of ACPI power resources
to user space by adding a new reference_count attribute to the sysfs
directory representing each power resource.  The value read from
that attribute represents the number of devices using the power
resource at the given time.  If that value is 0, it meas that the
power resource is not used and therefore it has been turned off.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/power.c    |   26 +++++++++++++++++++++++++-
 drivers/acpi/scan.c     |    3 +++
 include/acpi/acpi_bus.h |    1 +
 3 files changed, 29 insertions(+), 1 deletion(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -280,6 +280,7 @@ struct acpi_device {
 	struct mutex physical_node_lock;
 	DECLARE_BITMAP(physical_node_id_bitmap, ACPI_MAX_PHYSICAL_NODE);
 	struct list_head power_dependent;
+	void (*remove)(struct acpi_device *);
 };
 
 static inline void *acpi_driver_data(struct acpi_device *d)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -767,6 +767,9 @@ static void acpi_device_unregister(struc
 
 	acpi_power_add_remove_device(device, false);
 	acpi_device_remove_files(device);
+	if (device->remove)
+		device->remove(device);
+
 	device_unregister(&device->dev);
 	/*
 	 * Drop the reference counts of all power resources the device depends
Index: linux-pm/drivers/acpi/power.c
===================================================================
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -87,6 +87,12 @@ static DEFINE_MUTEX(power_resource_list_
                              Power Resource Management
    -------------------------------------------------------------------------- */
 
+static inline
+struct acpi_power_resource *to_power_resource(struct acpi_device *device)
+{
+	return container_of(device, struct acpi_power_resource, device);
+}
+
 static struct acpi_power_resource *acpi_power_get_context(acpi_handle handle)
 {
 	struct acpi_device *device;
@@ -94,7 +100,7 @@ static struct acpi_power_resource *acpi_
 	if (acpi_bus_get_device(handle, &device))
 		return NULL;
 
-	return container_of(device, struct acpi_power_resource, device);
+	return to_power_resource(device);
 }
 
 static int acpi_power_resources_list_add(acpi_handle handle,
@@ -679,6 +685,21 @@ static void acpi_release_power_resource(
 	kfree(resource);
 }
 
+static ssize_t acpi_power_refcount_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf) {
+	struct acpi_power_resource *resource;
+
+	resource = to_power_resource(to_acpi_device(dev));
+	return sprintf(buf, "%u\n", resource->ref_count);
+}
+static DEVICE_ATTR(reference_count, 0444, acpi_power_refcount_show, NULL);
+
+static void acpi_power_sysfs_remove(struct acpi_device *device)
+{
+	device_remove_file(&device->dev, &dev_attr_reference_count);
+}
+
 int acpi_add_power_resource(acpi_handle handle)
 {
 	struct acpi_power_resource *resource;
@@ -726,6 +747,9 @@ int acpi_add_power_resource(acpi_handle
 	if (result)
 		goto err;
 
+	if (!device_create_file(&device->dev, &dev_attr_reference_count))
+		device->remove = acpi_power_sysfs_remove;
+
 	mutex_lock(&power_resource_list_lock);
 	list_add(&resource->list_node, &acpi_power_resource_list);
 	mutex_unlock(&power_resource_list_lock);

[RFC][PATCH 2/3] sysfs: Functions for adding/removing symlinks to/from attribute groups

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The most convenient way to expose ACPI power resources lists of a
device is to put symbolic links to sysfs directories representing
those resources into special attribute groups in the device's sysfs
directory.  For this purpose, it is necessary to be able to add
symbolic links to attribute groups.

For this reason, add sysfs helper functions for adding/removing
symbolic links to/from attribute groups, sysfs_add_link_to_group()
and sysfs_remove_link_from_group(), respectively.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 fs/sysfs/group.c      |   42 ++++++++++++++++++++++++++++++++++++++++++
 fs/sysfs/symlink.c    |   46 +++++++++++++++++++++++++++++++++-------------
 include/linux/sysfs.h |    6 ++++++
 3 files changed, 81 insertions(+), 13 deletions(-)

Index: linux-pm/fs/sysfs/group.c
===================================================================
--- linux-pm.orig/fs/sysfs/group.c
+++ linux-pm/fs/sysfs/group.c
@@ -205,6 +205,48 @@ void sysfs_unmerge_group(struct kobject
 }
 EXPORT_SYMBOL_GPL(sysfs_unmerge_group);
 
+/**
+ * sysfs_add_link_to_group - add a symlink to an attribute group.
+ * @kobj:	The kobject containing the group.
+ * @group_name:	The name of the group.
+ * @target:	The target kobject of the symlink to create.
+ * @link_name:	The name of the symlink to create.
+ */
+int sysfs_add_link_to_group(struct kobject *kobj, const char *group_name,
+			    struct kobject *target, const char *link_name)
+{
+	struct sysfs_dirent *dir_sd;
+	int error = 0;
+
+	dir_sd = sysfs_get_dirent(kobj->sd, NULL, group_name);
+	if (!dir_sd)
+		return -ENOENT;
+
+	error = sysfs_add_link(dir_sd, target, link_name);
+	sysfs_put(dir_sd);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(sysfs_add_link_to_group);
+
+/**
+ * sysfs_remove_link_from_group - remove a symlink from an attribute group.
+ * @kobj:	The kobject containing the group.
+ * @group_name:	The name of the group.
+ * @link_name:	The name of the symlink to remove.
+ */
+void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
+				  const char *link_name)
+{
+	struct sysfs_dirent *dir_sd;
+
+	dir_sd = sysfs_get_dirent(kobj->sd, NULL, group_name);
+	if (dir_sd) {
+		sysfs_hash_and_remove(dir_sd, NULL, link_name);
+		sysfs_put(dir_sd);
+	}
+}
+EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
 
 EXPORT_SYMBOL_GPL(sysfs_create_group);
 EXPORT_SYMBOL_GPL(sysfs_update_group);
Index: linux-pm/fs/sysfs/symlink.c
===================================================================
--- linux-pm.orig/fs/sysfs/symlink.c
+++ linux-pm/fs/sysfs/symlink.c
@@ -21,26 +21,17 @@
 
 #include "sysfs.h"
 
-static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
-				const char *name, int warn)
+static int sysfs_do_create_link_sd(struct sysfs_dirent *parent_sd,
+				   struct kobject *target,
+				   const char *name, int warn)
 {
-	struct sysfs_dirent *parent_sd = NULL;
 	struct sysfs_dirent *target_sd = NULL;
 	struct sysfs_dirent *sd = NULL;
 	struct sysfs_addrm_cxt acxt;
 	enum kobj_ns_type ns_type;
 	int error;
 
-	BUG_ON(!name);
-
-	if (!kobj)
-		parent_sd = &sysfs_root;
-	else
-		parent_sd = kobj->sd;
-
-	error = -EFAULT;
-	if (!parent_sd)
-		goto out_put;
+	BUG_ON(!name || !parent_sd);
 
 	/* target->sd can go away beneath us but is protected with
 	 * sysfs_assoc_lock.  Fetch target_sd from it.
@@ -96,6 +87,35 @@ static int sysfs_do_create_link(struct k
 }
 
 /**
+ *	sysfs_add_link - create symlink to a given object.
+ *	@sd:		directory we're creating the link in.
+ *	@target:	object we're pointing to.
+ *	@name:		name of the symlink.
+ */
+int sysfs_add_link(struct sysfs_dirent *sd, struct kobject *target,
+		   const char *name)
+{
+	return sysfs_do_create_link_sd(sd, target, name, 1);
+}
+EXPORT_SYMBOL_GPL(sysfs_add_link);
+
+static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
+				const char *name, int warn)
+{
+	struct sysfs_dirent *parent_sd = NULL;
+
+	if (!kobj)
+		parent_sd = &sysfs_root;
+	else
+		parent_sd = kobj->sd;
+
+	if (!parent_sd)
+		return -EFAULT;
+
+	return sysfs_do_create_link_sd(parent_sd, target, name, warn);
+}
+
+/**
  *	sysfs_create_link - create symlink between two objects.
  *	@kobj:	object whose directory we're creating the link in.
  *	@target:	object we're pointing to.
Index: linux-pm/include/linux/sysfs.h
===================================================================
--- linux-pm.orig/include/linux/sysfs.h
+++ linux-pm/include/linux/sysfs.h
@@ -154,6 +154,8 @@ int __must_check sysfs_create_bin_file(s
 void sysfs_remove_bin_file(struct kobject *kobj,
 			   const struct bin_attribute *attr);
 
+int __must_check sysfs_add_link(struct sysfs_dirent *sd, struct kobject *target,
+				const char *name);
 int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
 				   const char *name);
 int __must_check sysfs_create_link_nowarn(struct kobject *kobj,
@@ -181,6 +183,10 @@ int sysfs_merge_group(struct kobject *ko
 		       const struct attribute_group *grp);
 void sysfs_unmerge_group(struct kobject *kobj,
 		       const struct attribute_group *grp);
+int sysfs_add_link_to_group(struct kobject *kobj, const char *group_name,
+			    struct kobject *target, const char *link_name);
+void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
+				  const char *link_name);
 
 void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr);
 void sysfs_notify_dirent(struct sysfs_dirent *sd);

[RFC][PATCH 3/3] ACPI / PM: Expose lists of device power resources to user space

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since ACPI power resources are going to be used more extensively on
new hardware platforms, it is necessary to allow user space (powertop
in particular) to look at the lists of power resources corresponding
to different power states of devices for diagnostics and control
purposes.

For this reason, for each power state of an ACPI device node using
power resources create a special attribute group under the device
node's directory in sysfs containing links to sysfs directories
representing the power resources in that list.  The names of the
new attribute groups are "power_resources_<state>", where <state>
is the state name i.e. "D0", "D1", "D2", or "D3hot".

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/power.c |   96 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 83 insertions(+), 13 deletions(-)

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;

Re: [RFC][PATCH 0/3] ACPI / PM: Export power resources information to user space

[Greg Kroah-Hartman]

On Sun, Jan 20, 2013 at 03:51:40PM +0100, Rafael J. Wysocki wrote:
> Hi Greg,
> 
> The following patch series is about exporting ACPI power resources to user
> space that will be necessary for PM diagnostics on new platforms (powertop is
> the target).  Still, existing systems can also benefit from that, like my
> oldish HP nx6325 on which the patches have been tested.
> 
> Patch [1/3] shouldn't be really controverisal, as it only makes it possible
> to read power resources' reference counters from user space in a quite
> straightforward way.

You need to add a Documentation/ABI/ entry for the new file you created
here.  Same thing for 3/3, which will help me understand exactly what
you are trying to do here with sysfs.  Care to update that so I can try
to figure out if the sysfs code is correct?

thanks,

greg k-h

Re: [RFC][PATCH 0/3] ACPI / PM: Export power resources information to user space

[Rafael J. Wysocki]

On Sunday, January 20, 2013 03:37:35 PM Greg Kroah-Hartman wrote:
> On Sun, Jan 20, 2013 at 03:51:40PM +0100, Rafael J. Wysocki wrote:
> > Hi Greg,
> > 
> > The following patch series is about exporting ACPI power resources to user
> > space that will be necessary for PM diagnostics on new platforms (powertop is
> > the target).  Still, existing systems can also benefit from that, like my
> > oldish HP nx6325 on which the patches have been tested.
> > 
> > Patch [1/3] shouldn't be really controverisal, as it only makes it possible
> > to read power resources' reference counters from user space in a quite
> > straightforward way.
> 
> You need to add a Documentation/ABI/ entry for the new file you created
> here.  Same thing for 3/3, which will help me understand exactly what
> you are trying to do here with sysfs.  Care to update that so I can try
> to figure out if the sysfs code is correct?

I will.

Thanks,
Rafael


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

[RFC][Update][PATCH 0/3] ACPI / PM: Export power resources information to user space

[Rafael J. Wysocki]

On Sunday, January 20, 2013 03:51:40 PM Rafael J. Wysocki wrote:
> Hi Greg,
> 
> The following patch series is about exporting ACPI power resources to user
> space that will be necessary for PM diagnostics on new platforms (powertop is
> the target).  Still, existing systems can also benefit from that, like my
> oldish HP nx6325 on which the patches have been tested.
> 
> Patch [1/3] shouldn't be really controverisal, as it only makes it possible
> to read power resources' reference counters from user space in a quite
> straightforward way.
> 
> Patch [3/3], however, exposes lists of power resources associated with
> different power states of devices using ACPI PM as sysfs subdirectories
> containing symbolic links and that requires some sysfs changes made by
> patch [2/3].
> 
> Admittedly, my sysfs skills are limited so please let me know if those
> changes make sense and are acceptable from your perspective.  If not, then
> please kindly advise what I can do instead of them.
> 
> The patches apply on top of the current linux-next branch of the linux-pm.git
> tree.

Refreshed with Documentation/ABI files as requested.

Thanks,
Rafael


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

[RFC][Update][PATCH 1/3] ACPI / PM: Expose reference count values of ACPI power resources

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since ACPI power resources are going to be used more extensively on
new hardware platforms, it becomes necessary for user space (powertop
in particular) to observe some properties of those resources for
diagnostics purposes.

For this reason, export the reference counts of ACPI power resources
to user space by adding a new reference_count attribute to the sysfs
directory representing each power resource.  The value read from
that attribute represents the number of devices using the power
resource at the given time.  If that value is 0, it meas that the
power resource is not used and therefore it has been turned off.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/ABI/testing/sysfs-devices-reference_count |   13 ++++++++
 drivers/acpi/power.c                                    |   26 +++++++++++++++-
 drivers/acpi/scan.c                                     |    3 +
 include/acpi/acpi_bus.h                                 |    1 
 4 files changed, 42 insertions(+), 1 deletion(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -280,6 +280,7 @@ struct acpi_device {
 	struct mutex physical_node_lock;
 	DECLARE_BITMAP(physical_node_id_bitmap, ACPI_MAX_PHYSICAL_NODE);
 	struct list_head power_dependent;
+	void (*remove)(struct acpi_device *);
 };
 
 static inline void *acpi_driver_data(struct acpi_device *d)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -767,6 +767,9 @@ static void acpi_device_unregister(struc
 
 	acpi_power_add_remove_device(device, false);
 	acpi_device_remove_files(device);
+	if (device->remove)
+		device->remove(device);
+
 	device_unregister(&device->dev);
 	/*
 	 * Drop the reference counts of all power resources the device depends
Index: linux-pm/drivers/acpi/power.c
===================================================================
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -87,6 +87,12 @@ static DEFINE_MUTEX(power_resource_list_
                              Power Resource Management
    -------------------------------------------------------------------------- */
 
+static inline
+struct acpi_power_resource *to_power_resource(struct acpi_device *device)
+{
+	return container_of(device, struct acpi_power_resource, device);
+}
+
 static struct acpi_power_resource *acpi_power_get_context(acpi_handle handle)
 {
 	struct acpi_device *device;
@@ -94,7 +100,7 @@ static struct acpi_power_resource *acpi_
 	if (acpi_bus_get_device(handle, &device))
 		return NULL;
 
-	return container_of(device, struct acpi_power_resource, device);
+	return to_power_resource(device);
 }
 
 static int acpi_power_resources_list_add(acpi_handle handle,
@@ -679,6 +685,21 @@ static void acpi_release_power_resource(
 	kfree(resource);
 }
 
+static ssize_t acpi_power_refcount_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf) {
+	struct acpi_power_resource *resource;
+
+	resource = to_power_resource(to_acpi_device(dev));
+	return sprintf(buf, "%u\n", resource->ref_count);
+}
+static DEVICE_ATTR(reference_count, 0444, acpi_power_refcount_show, NULL);
+
+static void acpi_power_sysfs_remove(struct acpi_device *device)
+{
+	device_remove_file(&device->dev, &dev_attr_reference_count);
+}
+
 int acpi_add_power_resource(acpi_handle handle)
 {
 	struct acpi_power_resource *resource;
@@ -726,6 +747,9 @@ int acpi_add_power_resource(acpi_handle
 	if (result)
 		goto err;
 
+	if (!device_create_file(&device->dev, &dev_attr_reference_count))
+		device->remove = acpi_power_sysfs_remove;
+
 	mutex_lock(&power_resource_list_lock);
 	list_add(&resource->list_node, &acpi_power_resource_list);
 	mutex_unlock(&power_resource_list_lock);
Index: linux-pm/Documentation/ABI/testing/sysfs-devices-reference_count
===================================================================
--- /dev/null
+++ linux-pm/Documentation/ABI/testing/sysfs-devices-reference_count
@@ -0,0 +1,13 @@
+What:		/sys/devices/.../reference_count
+Date:		January 2012
+Contact:	Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+Description:
+		The /sys/devices/.../reference_count attribute allows user
+		space to retrieve the current number of devices using the ACPI
+		power resource represented by the given device object (whose
+		directory contains that attribute).  If the read value is 0,
+		the power resource is currently not in use and therefore it
+		has been turned off.
+
+		This attribute is read-only.  It only is present for device
+		objects representing ACPI power resources.

[RFC][Update][PATCH 2/3] sysfs: Functions for adding/removing symlinks to/from attribute groups

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The most convenient way to expose ACPI power resources lists of a
device is to put symbolic links to sysfs directories representing
those resources into special attribute groups in the device's sysfs
directory.  For this purpose, it is necessary to be able to add
symbolic links to attribute groups.

For this reason, add sysfs helper functions for adding/removing
symbolic links to/from attribute groups, sysfs_add_link_to_group()
and sysfs_remove_link_from_group(), respectively.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 fs/sysfs/group.c      |   42 ++++++++++++++++++++++++++++++++++++++++++
 fs/sysfs/symlink.c    |   46 +++++++++++++++++++++++++++++++++-------------
 include/linux/sysfs.h |    6 ++++++
 3 files changed, 81 insertions(+), 13 deletions(-)

Index: linux-pm/fs/sysfs/group.c
===================================================================
--- linux-pm.orig/fs/sysfs/group.c
+++ linux-pm/fs/sysfs/group.c
@@ -205,6 +205,48 @@ void sysfs_unmerge_group(struct kobject
 }
 EXPORT_SYMBOL_GPL(sysfs_unmerge_group);
 
+/**
+ * sysfs_add_link_to_group - add a symlink to an attribute group.
+ * @kobj:	The kobject containing the group.
+ * @group_name:	The name of the group.
+ * @target:	The target kobject of the symlink to create.
+ * @link_name:	The name of the symlink to create.
+ */
+int sysfs_add_link_to_group(struct kobject *kobj, const char *group_name,
+			    struct kobject *target, const char *link_name)
+{
+	struct sysfs_dirent *dir_sd;
+	int error = 0;
+
+	dir_sd = sysfs_get_dirent(kobj->sd, NULL, group_name);
+	if (!dir_sd)
+		return -ENOENT;
+
+	error = sysfs_add_link(dir_sd, target, link_name);
+	sysfs_put(dir_sd);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(sysfs_add_link_to_group);
+
+/**
+ * sysfs_remove_link_from_group - remove a symlink from an attribute group.
+ * @kobj:	The kobject containing the group.
+ * @group_name:	The name of the group.
+ * @link_name:	The name of the symlink to remove.
+ */
+void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
+				  const char *link_name)
+{
+	struct sysfs_dirent *dir_sd;
+
+	dir_sd = sysfs_get_dirent(kobj->sd, NULL, group_name);
+	if (dir_sd) {
+		sysfs_hash_and_remove(dir_sd, NULL, link_name);
+		sysfs_put(dir_sd);
+	}
+}
+EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
 
 EXPORT_SYMBOL_GPL(sysfs_create_group);
 EXPORT_SYMBOL_GPL(sysfs_update_group);
Index: linux-pm/fs/sysfs/symlink.c
===================================================================
--- linux-pm.orig/fs/sysfs/symlink.c
+++ linux-pm/fs/sysfs/symlink.c
@@ -21,26 +21,17 @@
 
 #include "sysfs.h"
 
-static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
-				const char *name, int warn)
+static int sysfs_do_create_link_sd(struct sysfs_dirent *parent_sd,
+				   struct kobject *target,
+				   const char *name, int warn)
 {
-	struct sysfs_dirent *parent_sd = NULL;
 	struct sysfs_dirent *target_sd = NULL;
 	struct sysfs_dirent *sd = NULL;
 	struct sysfs_addrm_cxt acxt;
 	enum kobj_ns_type ns_type;
 	int error;
 
-	BUG_ON(!name);
-
-	if (!kobj)
-		parent_sd = &sysfs_root;
-	else
-		parent_sd = kobj->sd;
-
-	error = -EFAULT;
-	if (!parent_sd)
-		goto out_put;
+	BUG_ON(!name || !parent_sd);
 
 	/* target->sd can go away beneath us but is protected with
 	 * sysfs_assoc_lock.  Fetch target_sd from it.
@@ -96,6 +87,35 @@ static int sysfs_do_create_link(struct k
 }
 
 /**
+ *	sysfs_add_link - create symlink to a given object.
+ *	@sd:		directory we're creating the link in.
+ *	@target:	object we're pointing to.
+ *	@name:		name of the symlink.
+ */
+int sysfs_add_link(struct sysfs_dirent *sd, struct kobject *target,
+		   const char *name)
+{
+	return sysfs_do_create_link_sd(sd, target, name, 1);
+}
+EXPORT_SYMBOL_GPL(sysfs_add_link);
+
+static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
+				const char *name, int warn)
+{
+	struct sysfs_dirent *parent_sd = NULL;
+
+	if (!kobj)
+		parent_sd = &sysfs_root;
+	else
+		parent_sd = kobj->sd;
+
+	if (!parent_sd)
+		return -EFAULT;
+
+	return sysfs_do_create_link_sd(parent_sd, target, name, warn);
+}
+
+/**
  *	sysfs_create_link - create symlink between two objects.
  *	@kobj:	object whose directory we're creating the link in.
  *	@target:	object we're pointing to.
Index: linux-pm/include/linux/sysfs.h
===================================================================
--- linux-pm.orig/include/linux/sysfs.h
+++ linux-pm/include/linux/sysfs.h
@@ -154,6 +154,8 @@ int __must_check sysfs_create_bin_file(s
 void sysfs_remove_bin_file(struct kobject *kobj,
 			   const struct bin_attribute *attr);
 
+int __must_check sysfs_add_link(struct sysfs_dirent *sd, struct kobject *target,
+				const char *name);
 int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
 				   const char *name);
 int __must_check sysfs_create_link_nowarn(struct kobject *kobj,
@@ -181,6 +183,10 @@ int sysfs_merge_group(struct kobject *ko
 		       const struct attribute_group *grp);
 void sysfs_unmerge_group(struct kobject *kobj,
 		       const struct attribute_group *grp);
+int sysfs_add_link_to_group(struct kobject *kobj, const char *group_name,
+			    struct kobject *target, const char *link_name);
+void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
+				  const char *link_name);
 
 void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr);
 void sysfs_notify_dirent(struct sysfs_dirent *sd);

[RFC][Update][PATCH 3/3] ACPI / PM: Expose lists of device power resources to user space

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since ACPI power resources are going to be used more extensively on
new hardware platforms, it is necessary to allow user space (powertop
in particular) to look at the lists of power resources corresponding
to different power states of devices for diagnostics and control
purposes.

For this reason, for each power state of an ACPI device node using
power resources create a special attribute group under the device
node's directory in sysfs containing links to sysfs directories
representing the power resources in that list.  The names of the
new attribute groups are "power_resources_<state>", where <state>
is the state name i.e. "D0", "D1", "D2", or "D3hot".

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/ABI/testing/sysfs-devices-power_resources_D0    |   12 +
 Documentation/ABI/testing/sysfs-devices-power_resources_D1    |   12 +
 Documentation/ABI/testing/sysfs-devices-power_resources_D2    |   12 +
 Documentation/ABI/testing/sysfs-devices-power_resources_D3hot |   12 +
 drivers/acpi/power.c                                          |   96 ++++++++--
 5 files changed, 131 insertions(+), 13 deletions(-)

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,12 @@
+What:		/sys/devices/.../power_resources_D0/
+Date:		January 2012
+Contact:	Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+Description:
+		The /sys/devices/.../power_resources_D0/ directory contains a
+		list of symbolic links to device object directories representing
+		ACPI power resources that need to be turned on for the given
+		device to be in ACPI power state D0.
+
+		This directory is only present for device objects representing
+		ACPI device nodes and only if there are power resources needed
+		by the given device node to be in power state D0.
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,12 @@
+What:		/sys/devices/.../power_resources_D1/
+Date:		January 2012
+Contact:	Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+Description:
+		The /sys/devices/.../power_resources_D1/ directory contains a
+		list of symbolic links to device object directories representing
+		ACPI power resources that need to be turned on for the given
+		device to be in ACPI power state D1.
+
+		This directory is only present for device objects representing
+		ACPI device nodes and only if there are power resources needed
+		by the given device node to be in power state D1.
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,12 @@
+What:		/sys/devices/.../power_resources_D2/
+Date:		January 2012
+Contact:	Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+Description:
+		The /sys/devices/.../power_resources_D2/ directory contains a
+		list of symbolic links to device object directories representing
+		ACPI power resources that need to be turned on for the given
+		device to be in ACPI power state D2.
+
+		This directory is only present for device objects representing
+		ACPI device nodes and only if there are power resources needed
+		by the given device node to be in power state D2.
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,12 @@
+What:		/sys/devices/.../power_resources_D3hot/
+Date:		January 2012
+Contact:	Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+Description:
+		The /sys/devices/.../power_resources_D3hot/ directory contains a
+		list of symbolic links to device object directories representing
+		ACPI power resources that need to be turned on for the given
+		device to be in ACPI power state D3hot.
+
+		This directory is only present for device objects representing
+		ACPI device nodes and only if there are power resources needed
+		by the given device node to be in power state D3hot.

[RFC][Update 2][PATCH 0/4] ACPI / PM: Export power information to user space

[Rafael J. Wysocki]

On Monday, January 21, 2013 01:48:55 AM Rafael J. Wysocki wrote:
> On Sunday, January 20, 2013 03:51:40 PM Rafael J. Wysocki wrote:
> > Hi Greg,
> > 
> > The following patch series is about exporting ACPI power resources to user
> > space that will be necessary for PM diagnostics on new platforms (powertop is
> > the target).  Still, existing systems can also benefit from that, like my
> > oldish HP nx6325 on which the patches have been tested.
> > 
> > Patch [1/3] shouldn't be really controverisal, as it only makes it possible
> > to read power resources' reference counters from user space in a quite
> > straightforward way.
> > 
> > Patch [3/3], however, exposes lists of power resources associated with
> > different power states of devices using ACPI PM as sysfs subdirectories
> > containing symbolic links and that requires some sysfs changes made by
> > patch [2/3].
> > 
> > Admittedly, my sysfs skills are limited so please let me know if those
> > changes make sense and are acceptable from your perspective.  If not, then
> > please kindly advise what I can do instead of them.
> > 
> > The patches apply on top of the current linux-next branch of the linux-pm.git
> > tree.
> 
> Refreshed with Documentation/ABI files as requested.

Well, that version wasn't really a good one, due to mistakes in the docs
mostly, so a new one follows (with better docs hopefully).

Besides, I thought I'd include a patch that exposes ACPI power state
information, because it kind of belongs to the lot.  It goes as the new [1/4].

[2/4] is the previous [1/3] and so on.

Thanks,
Rafael


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

[RFC][Update 2][PATCH 1/4] ACPI / PM: Export power states of ACPI devices via sysfs

[Rafael J. Wysocki]

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 a new
attribute 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 |   21 ++++++++++++++
 drivers/acpi/scan.c                                 |   29 +++++++++++++++++++-
 2 files changed, 49 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,23 @@ err_out:
 }
 EXPORT_SYMBOL(acpi_bus_hot_remove_device);
 
+static ssize_t 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 %s\n", acpi_power_state_string(state),
+		       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 +386,15 @@ 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)
+			goto end;
+	}
+
+	if (dev->flags.power_manageable)
+		result = device_create_file(&dev->dev, &dev_attr_power_state);
+
 end:
 	return result;
 }
@@ -380,6 +404,9 @@ static void acpi_device_remove_files(str
 	acpi_status status;
 	acpi_handle temp;
 
+	if (dev->flags.power_manageable)
+		device_remove_file(&dev->dev, &dev_attr_power_state);
+
 	/*
 	 * If device has _STR, remove 'description' file
 	 */
Index: linux-pm/Documentation/ABI/testing/sysfs-devices-power_state
===================================================================
--- /dev/null
+++ linux-pm/Documentation/ABI/testing/sysfs-devices-power_state
@@ -0,0 +1,21 @@
+What:		/sys/devices/.../power_state
+Date:		January 2013
+Contact:	Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+Description:
+		The /sys/devices/.../power_state attribute is only present for
+		device objects representing ACPI device nodes that provide power
+		management methods.
+
+		If present, it contains a pair of strings representing the
+		current ACPI power state of the given device node and the ACPI
+		power state the device node would be in if it did not share
+		power resources with other device nodes, respectively.  If the
+		given device node does not share power resources with other
+		device nodes or it does not use power resource objects for power
+		management, the strings are always the same.
+
+		For each of the strings the possible values are "D0", "D1",
+		"D2", "D3hot", and "D3cold" which reflect the power state names
+		defined by the ACPI specification (4.0 and above).
+
+		This attribute is read-only.

Re: [RFC][Update 2][PATCH 1/4] ACPI / PM: Export power states of ACPI devices via sysfs

[Greg Kroah-Hartman]

On Mon, Jan 21, 2013 at 02:04:32PM +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 a new
> attribute 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 |   21 ++++++++++++++
>  drivers/acpi/scan.c                                 |   29 +++++++++++++++++++-
>  2 files changed, 49 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,23 @@ err_out:
>  }
>  EXPORT_SYMBOL(acpi_bus_hot_remove_device);
>  
> +static ssize_t 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 %s\n", acpi_power_state_string(state),
> +		       acpi_power_state_string(adev->power.state));
> +}

You are showing 2 different things here in a single sysfs file, which is
really frowned apon.  Any chance to split this up into two different
sysfs files instead?

thanks,

greg k-h

Re: [RFC][Update 2][PATCH 1/4] ACPI / PM: Export power states of ACPI devices via sysfs

[Rafael J. Wysocki]

On Monday, January 21, 2013 12:53:05 PM Greg Kroah-Hartman wrote:
> On Mon, Jan 21, 2013 at 02:04:32PM +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 a new
> > attribute 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 |   21 ++++++++++++++
> >  drivers/acpi/scan.c                                 |   29 +++++++++++++++++++-
> >  2 files changed, 49 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,23 @@ err_out:
> >  }
> >  EXPORT_SYMBOL(acpi_bus_hot_remove_device);
> >  
> > +static ssize_t 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 %s\n", acpi_power_state_string(state),
> > +		       acpi_power_state_string(adev->power.state));
> > +}
> 
> You are showing 2 different things here in a single sysfs file, which is
> really frowned apon.  Any chance to split this up into two different
> sysfs files instead?

Well, I can, but I'm not sure how to call the other one.  "sw_power_state"
perhaps?

Rafael


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

Re: [RFC][Update 2][PATCH 1/4] ACPI / PM: Export power states of ACPI devices via sysfs

[Greg Kroah-Hartman]

On Mon, Jan 21, 2013 at 11:27:32PM +0100, Rafael J. Wysocki wrote:
> On Monday, January 21, 2013 12:53:05 PM Greg Kroah-Hartman wrote:
> > On Mon, Jan 21, 2013 at 02:04:32PM +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 a new
> > > attribute 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 |   21 ++++++++++++++
> > >  drivers/acpi/scan.c                                 |   29 +++++++++++++++++++-
> > >  2 files changed, 49 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,23 @@ err_out:
> > >  }
> > >  EXPORT_SYMBOL(acpi_bus_hot_remove_device);
> > >  
> > > +static ssize_t 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 %s\n", acpi_power_state_string(state),
> > > +		       acpi_power_state_string(adev->power.state));
> > > +}
> > 
> > You are showing 2 different things here in a single sysfs file, which is
> > really frowned apon.  Any chance to split this up into two different
> > sysfs files instead?
> 
> Well, I can, but I'm not sure how to call the other one.  "sw_power_state"
> perhaps?

I don't know, as I'm not quite sure what it is supposed to represent :)

Re: [RFC][Update 2][PATCH 1/4] ACPI / PM: Export power states of ACPI devices via sysfs

[Rafael J. Wysocki]

On Monday, January 21, 2013 02:26:47 PM Greg Kroah-Hartman wrote:
> On Mon, Jan 21, 2013 at 11:27:32PM +0100, Rafael J. Wysocki wrote:
> > On Monday, January 21, 2013 12:53:05 PM Greg Kroah-Hartman wrote:
> > > On Mon, Jan 21, 2013 at 02:04:32PM +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 a new
> > > > attribute 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 |   21 ++++++++++++++
> > > >  drivers/acpi/scan.c                                 |   29 +++++++++++++++++++-
> > > >  2 files changed, 49 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,23 @@ err_out:
> > > >  }
> > > >  EXPORT_SYMBOL(acpi_bus_hot_remove_device);
> > > >  
> > > > +static ssize_t 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 %s\n", acpi_power_state_string(state),
> > > > +		       acpi_power_state_string(adev->power.state));
> > > > +}
> > > 
> > > You are showing 2 different things here in a single sysfs file, which is
> > > really frowned apon.  Any chance to split this up into two different
> > > sysfs files instead?
> > 
> > Well, I can, but I'm not sure how to call the other one.  "sw_power_state"
> > perhaps?
> 
> I don't know, as I'm not quite sure what it is supposed to represent :)

The first one is power state as read using _PSC or inferred from power
resources on/off configuration.  That's easy.

Now, if power resources are shared between two or more devices, it is possible
that one device will be in, say, D3hot from the software point of view, but its
real ("physical") power state will be different, because the other devices
still keep the shared resource "on".  In that case, if the "software" power
state of a device is lower-power (higher-number) than its real power state,
we know that that particular device doesn't prevent the shared resource from
being turned off.  This is good to know, I think. :-)

Thanks,
Rafael


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

Re: [RFC][Update 2][PATCH 1/4] ACPI / PM: Export power states of ACPI devices via sysfs

[Greg Kroah-Hartman]

On Mon, Jan 21, 2013 at 11:59:12PM +0100, Rafael J. Wysocki wrote:
> On Monday, January 21, 2013 02:26:47 PM Greg Kroah-Hartman wrote:
> > On Mon, Jan 21, 2013 at 11:27:32PM +0100, Rafael J. Wysocki wrote:
> > > On Monday, January 21, 2013 12:53:05 PM Greg Kroah-Hartman wrote:
> > > > On Mon, Jan 21, 2013 at 02:04:32PM +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 a new
> > > > > attribute 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 |   21 ++++++++++++++
> > > > >  drivers/acpi/scan.c                                 |   29 +++++++++++++++++++-
> > > > >  2 files changed, 49 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,23 @@ err_out:
> > > > >  }
> > > > >  EXPORT_SYMBOL(acpi_bus_hot_remove_device);
> > > > >  
> > > > > +static ssize_t 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 %s\n", acpi_power_state_string(state),
> > > > > +		       acpi_power_state_string(adev->power.state));
> > > > > +}
> > > > 
> > > > You are showing 2 different things here in a single sysfs file, which is
> > > > really frowned apon.  Any chance to split this up into two different
> > > > sysfs files instead?
> > > 
> > > Well, I can, but I'm not sure how to call the other one.  "sw_power_state"
> > > perhaps?
> > 
> > I don't know, as I'm not quite sure what it is supposed to represent :)
> 
> The first one is power state as read using _PSC or inferred from power
> resources on/off configuration.  That's easy.
> 
> Now, if power resources are shared between two or more devices, it is possible
> that one device will be in, say, D3hot from the software point of view, but its
> real ("physical") power state will be different, because the other devices
> still keep the shared resource "on".  In that case, if the "software" power
> state of a device is lower-power (higher-number) than its real power state,
> we know that that particular device doesn't prevent the shared resource from
> being turned off.  This is good to know, I think. :-)

I agree that it's good to know, just what to call it.  I think you just
named it with "real_power_state", right?

thanks,

greg k-h

Re: [RFC][Update 2][PATCH 1/4] ACPI / PM: Export power states of ACPI devices via sysfs

[Rafael J. Wysocki]

On Monday, January 21, 2013 03:08:04 PM Greg Kroah-Hartman wrote:
> On Mon, Jan 21, 2013 at 11:59:12PM +0100, Rafael J. Wysocki wrote:
> > On Monday, January 21, 2013 02:26:47 PM Greg Kroah-Hartman wrote:
> > > On Mon, Jan 21, 2013 at 11:27:32PM +0100, Rafael J. Wysocki wrote:
> > > > On Monday, January 21, 2013 12:53:05 PM Greg Kroah-Hartman wrote:
> > > > > On Mon, Jan 21, 2013 at 02:04:32PM +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 a new
> > > > > > attribute 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 |   21 ++++++++++++++
> > > > > >  drivers/acpi/scan.c                                 |   29 +++++++++++++++++++-
> > > > > >  2 files changed, 49 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,23 @@ err_out:
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(acpi_bus_hot_remove_device);
> > > > > >  
> > > > > > +static ssize_t 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 %s\n", acpi_power_state_string(state),
> > > > > > +		       acpi_power_state_string(adev->power.state));
> > > > > > +}
> > > > > 
> > > > > You are showing 2 different things here in a single sysfs file, which is
> > > > > really frowned apon.  Any chance to split this up into two different
> > > > > sysfs files instead?
> > > > 
> > > > Well, I can, but I'm not sure how to call the other one.  "sw_power_state"
> > > > perhaps?
> > > 
> > > I don't know, as I'm not quite sure what it is supposed to represent :)
> > 
> > The first one is power state as read using _PSC or inferred from power
> > resources on/off configuration.  That's easy.
> > 
> > Now, if power resources are shared between two or more devices, it is possible
> > that one device will be in, say, D3hot from the software point of view, but its
> > real ("physical") power state will be different, because the other devices
> > still keep the shared resource "on".  In that case, if the "software" power
> > state of a device is lower-power (higher-number) than its real power state,
> > we know that that particular device doesn't prevent the shared resource from
> > being turned off.  This is good to know, I think. :-)
> 
> I agree that it's good to know, just what to call it.  I think you just
> named it with "real_power_state", right?

Yes, I think I'll just call them "power_state" and "real_power_state".  That
should be clear enough.

Thanks,
Rafael


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

[RFC][Update 2][PATCH 2/4] ACPI / PM: Expose reference count values of ACPI power resources

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since ACPI power resources are going to be used more extensively on
new hardware platforms, it becomes necessary for user space (powertop
in particular) to observe some properties of those resources for
diagnostics purposes.

For this reason, export the reference counts of ACPI power resources
to user space by adding a new reference_count attribute to the sysfs
directory representing each power resource.  The value read from
that attribute represents the number of devices using the power
resource at the given time.  If that value is 0, it meas that the
power resource is not used and therefore it has been turned off.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/ABI/testing/sysfs-devices-reference_count |   13 ++++++++
 drivers/acpi/power.c                                    |   26 +++++++++++++++-
 drivers/acpi/scan.c                                     |    3 +
 include/acpi/acpi_bus.h                                 |    1 
 4 files changed, 42 insertions(+), 1 deletion(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -280,6 +280,7 @@ struct acpi_device {
 	struct mutex physical_node_lock;
 	DECLARE_BITMAP(physical_node_id_bitmap, ACPI_MAX_PHYSICAL_NODE);
 	struct list_head power_dependent;
+	void (*remove)(struct acpi_device *);
 };
 
 static inline void *acpi_driver_data(struct acpi_device *d)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -771,6 +771,9 @@ static void acpi_device_unregister(struc
 
 	acpi_power_add_remove_device(device, false);
 	acpi_device_remove_files(device);
+	if (device->remove)
+		device->remove(device);
+
 	device_unregister(&device->dev);
 	/*
 	 * Drop the reference counts of all power resources the device depends
Index: linux-pm/drivers/acpi/power.c
===================================================================
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -87,6 +87,12 @@ static DEFINE_MUTEX(power_resource_list_
                              Power Resource Management
    -------------------------------------------------------------------------- */
 
+static inline
+struct acpi_power_resource *to_power_resource(struct acpi_device *device)
+{
+	return container_of(device, struct acpi_power_resource, device);
+}
+
 static struct acpi_power_resource *acpi_power_get_context(acpi_handle handle)
 {
 	struct acpi_device *device;
@@ -94,7 +100,7 @@ static struct acpi_power_resource *acpi_
 	if (acpi_bus_get_device(handle, &device))
 		return NULL;
 
-	return container_of(device, struct acpi_power_resource, device);
+	return to_power_resource(device);
 }
 
 static int acpi_power_resources_list_add(acpi_handle handle,
@@ -679,6 +685,21 @@ static void acpi_release_power_resource(
 	kfree(resource);
 }
 
+static ssize_t acpi_power_refcount_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf) {
+	struct acpi_power_resource *resource;
+
+	resource = to_power_resource(to_acpi_device(dev));
+	return sprintf(buf, "%u\n", resource->ref_count);
+}
+static DEVICE_ATTR(reference_count, 0444, acpi_power_refcount_show, NULL);
+
+static void acpi_power_sysfs_remove(struct acpi_device *device)
+{
+	device_remove_file(&device->dev, &dev_attr_reference_count);
+}
+
 int acpi_add_power_resource(acpi_handle handle)
 {
 	struct acpi_power_resource *resource;
@@ -726,6 +747,9 @@ int acpi_add_power_resource(acpi_handle
 	if (result)
 		goto err;
 
+	if (!device_create_file(&device->dev, &dev_attr_reference_count))
+		device->remove = acpi_power_sysfs_remove;
+
 	mutex_lock(&power_resource_list_lock);
 	list_add(&resource->list_node, &acpi_power_resource_list);
 	mutex_unlock(&power_resource_list_lock);
Index: linux-pm/Documentation/ABI/testing/sysfs-devices-reference_count
===================================================================
--- /dev/null
+++ linux-pm/Documentation/ABI/testing/sysfs-devices-reference_count
@@ -0,0 +1,13 @@
+What:		/sys/devices/.../reference_count
+Date:		January 2013
+Contact:	Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+Description:
+		The /sys/devices/.../reference_count attribute is only present
+		for device objects representing ACPI power resources.
+
+		If present, it contains the current number of devices using the
+		ACPI power resource represented by the given device object.  If
+		that number is 0, the power resource is currently not in use and
+		therefore it has been turned off.
+
+		This attribute is read-only.

Re: [RFC][Update 2][PATCH 2/4] ACPI / PM: Expose reference count values of ACPI power resources

[Greg Kroah-Hartman]

On Mon, Jan 21, 2013 at 02:05:47PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since ACPI power resources are going to be used more extensively on
> new hardware platforms, it becomes necessary for user space (powertop
> in particular) to observe some properties of those resources for
> diagnostics purposes.
> 
> For this reason, export the reference counts of ACPI power resources
> to user space by adding a new reference_count attribute to the sysfs
> directory representing each power resource.  The value read from
> that attribute represents the number of devices using the power
> resource at the given time.  If that value is 0, it meas that the
> power resource is not used and therefore it has been turned off.

Why does userspace need to know a reference count?  Is it so that if it
is not 0, it can work to try to lower it to 0?  Or something else?

thanks,

greg k-h

Re: [RFC][Update 2][PATCH 2/4] ACPI / PM: Expose reference count values of ACPI power resources

[Rafael J. Wysocki]

On Monday, January 21, 2013 12:53:57 PM Greg Kroah-Hartman wrote:
> On Mon, Jan 21, 2013 at 02:05:47PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Since ACPI power resources are going to be used more extensively on
> > new hardware platforms, it becomes necessary for user space (powertop
> > in particular) to observe some properties of those resources for
> > diagnostics purposes.
> > 
> > For this reason, export the reference counts of ACPI power resources
> > to user space by adding a new reference_count attribute to the sysfs
> > directory representing each power resource.  The value read from
> > that attribute represents the number of devices using the power
> > resource at the given time.  If that value is 0, it meas that the
> > power resource is not used and therefore it has been turned off.
> 
> Why does userspace need to know a reference count?  Is it so that if it
> is not 0, it can work to try to lower it to 0?  Or something else?

Yes, this information is needed to say if (1) the power resource is in use
and (2) how many users there are at the moment, so that we can go and look
if it really has to be in use.

Perhaps I can just expose the "in use"/"not in use" information.  I don't
think it will be much less convenient, because we should scan all of the
possible users anyway in case they are coming and going frequently.

So should I do that and rename the attribute to "resource_in_use" (or something
similar)?

Rafael


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

Re: [RFC][Update 2][PATCH 2/4] ACPI / PM: Expose reference count values of ACPI power resources

[Greg Kroah-Hartman]

On Mon, Jan 21, 2013 at 11:35:51PM +0100, Rafael J. Wysocki wrote:
> On Monday, January 21, 2013 12:53:57 PM Greg Kroah-Hartman wrote:
> > On Mon, Jan 21, 2013 at 02:05:47PM +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > Since ACPI power resources are going to be used more extensively on
> > > new hardware platforms, it becomes necessary for user space (powertop
> > > in particular) to observe some properties of those resources for
> > > diagnostics purposes.
> > > 
> > > For this reason, export the reference counts of ACPI power resources
> > > to user space by adding a new reference_count attribute to the sysfs
> > > directory representing each power resource.  The value read from
> > > that attribute represents the number of devices using the power
> > > resource at the given time.  If that value is 0, it meas that the
> > > power resource is not used and therefore it has been turned off.
> > 
> > Why does userspace need to know a reference count?  Is it so that if it
> > is not 0, it can work to try to lower it to 0?  Or something else?
> 
> Yes, this information is needed to say if (1) the power resource is in use
> and (2) how many users there are at the moment, so that we can go and look
> if it really has to be in use.
> 
> Perhaps I can just expose the "in use"/"not in use" information.  I don't
> think it will be much less convenient, because we should scan all of the
> possible users anyway in case they are coming and going frequently.
> 
> So should I do that and rename the attribute to "resource_in_use" (or something
> similar)?

That makes a bit more sense to do, thanks.

greg k-h

[RFC][Update 2][PATCH 3/4] sysfs: Functions for adding/removing symlinks to/from attribute groups

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The most convenient way to expose ACPI power resources lists of a
device is to put symbolic links to sysfs directories representing
those resources into special attribute groups in the device's sysfs
directory.  For this purpose, it is necessary to be able to add
symbolic links to attribute groups.

For this reason, add sysfs helper functions for adding/removing
symbolic links to/from attribute groups, sysfs_add_link_to_group()
and sysfs_remove_link_from_group(), respectively.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 fs/sysfs/group.c      |   42 ++++++++++++++++++++++++++++++++++++++++++
 fs/sysfs/symlink.c    |   46 +++++++++++++++++++++++++++++++++-------------
 include/linux/sysfs.h |    6 ++++++
 3 files changed, 81 insertions(+), 13 deletions(-)

Index: linux-pm/fs/sysfs/group.c
===================================================================
--- linux-pm.orig/fs/sysfs/group.c
+++ linux-pm/fs/sysfs/group.c
@@ -205,6 +205,48 @@ void sysfs_unmerge_group(struct kobject
 }
 EXPORT_SYMBOL_GPL(sysfs_unmerge_group);
 
+/**
+ * sysfs_add_link_to_group - add a symlink to an attribute group.
+ * @kobj:	The kobject containing the group.
+ * @group_name:	The name of the group.
+ * @target:	The target kobject of the symlink to create.
+ * @link_name:	The name of the symlink to create.
+ */
+int sysfs_add_link_to_group(struct kobject *kobj, const char *group_name,
+			    struct kobject *target, const char *link_name)
+{
+	struct sysfs_dirent *dir_sd;
+	int error = 0;
+
+	dir_sd = sysfs_get_dirent(kobj->sd, NULL, group_name);
+	if (!dir_sd)
+		return -ENOENT;
+
+	error = sysfs_add_link(dir_sd, target, link_name);
+	sysfs_put(dir_sd);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(sysfs_add_link_to_group);
+
+/**
+ * sysfs_remove_link_from_group - remove a symlink from an attribute group.
+ * @kobj:	The kobject containing the group.
+ * @group_name:	The name of the group.
+ * @link_name:	The name of the symlink to remove.
+ */
+void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
+				  const char *link_name)
+{
+	struct sysfs_dirent *dir_sd;
+
+	dir_sd = sysfs_get_dirent(kobj->sd, NULL, group_name);
+	if (dir_sd) {
+		sysfs_hash_and_remove(dir_sd, NULL, link_name);
+		sysfs_put(dir_sd);
+	}
+}
+EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
 
 EXPORT_SYMBOL_GPL(sysfs_create_group);
 EXPORT_SYMBOL_GPL(sysfs_update_group);
Index: linux-pm/fs/sysfs/symlink.c
===================================================================
--- linux-pm.orig/fs/sysfs/symlink.c
+++ linux-pm/fs/sysfs/symlink.c
@@ -21,26 +21,17 @@
 
 #include "sysfs.h"
 
-static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
-				const char *name, int warn)
+static int sysfs_do_create_link_sd(struct sysfs_dirent *parent_sd,
+				   struct kobject *target,
+				   const char *name, int warn)
 {
-	struct sysfs_dirent *parent_sd = NULL;
 	struct sysfs_dirent *target_sd = NULL;
 	struct sysfs_dirent *sd = NULL;
 	struct sysfs_addrm_cxt acxt;
 	enum kobj_ns_type ns_type;
 	int error;
 
-	BUG_ON(!name);
-
-	if (!kobj)
-		parent_sd = &sysfs_root;
-	else
-		parent_sd = kobj->sd;
-
-	error = -EFAULT;
-	if (!parent_sd)
-		goto out_put;
+	BUG_ON(!name || !parent_sd);
 
 	/* target->sd can go away beneath us but is protected with
 	 * sysfs_assoc_lock.  Fetch target_sd from it.
@@ -96,6 +87,35 @@ static int sysfs_do_create_link(struct k
 }
 
 /**
+ *	sysfs_add_link - create symlink to a given object.
+ *	@sd:		directory we're creating the link in.
+ *	@target:	object we're pointing to.
+ *	@name:		name of the symlink.
+ */
+int sysfs_add_link(struct sysfs_dirent *sd, struct kobject *target,
+		   const char *name)
+{
+	return sysfs_do_create_link_sd(sd, target, name, 1);
+}
+EXPORT_SYMBOL_GPL(sysfs_add_link);
+
+static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
+				const char *name, int warn)
+{
+	struct sysfs_dirent *parent_sd = NULL;
+
+	if (!kobj)
+		parent_sd = &sysfs_root;
+	else
+		parent_sd = kobj->sd;
+
+	if (!parent_sd)
+		return -EFAULT;
+
+	return sysfs_do_create_link_sd(parent_sd, target, name, warn);
+}
+
+/**
  *	sysfs_create_link - create symlink between two objects.
  *	@kobj:	object whose directory we're creating the link in.
  *	@target:	object we're pointing to.
Index: linux-pm/include/linux/sysfs.h
===================================================================
--- linux-pm.orig/include/linux/sysfs.h
+++ linux-pm/include/linux/sysfs.h
@@ -154,6 +154,8 @@ int __must_check sysfs_create_bin_file(s
 void sysfs_remove_bin_file(struct kobject *kobj,
 			   const struct bin_attribute *attr);
 
+int __must_check sysfs_add_link(struct sysfs_dirent *sd, struct kobject *target,
+				const char *name);
 int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
 				   const char *name);
 int __must_check sysfs_create_link_nowarn(struct kobject *kobj,
@@ -181,6 +183,10 @@ int sysfs_merge_group(struct kobject *ko
 		       const struct attribute_group *grp);
 void sysfs_unmerge_group(struct kobject *kobj,
 		       const struct attribute_group *grp);
+int sysfs_add_link_to_group(struct kobject *kobj, const char *group_name,
+			    struct kobject *target, const char *link_name);
+void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
+				  const char *link_name);
 
 void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr);
 void sysfs_notify_dirent(struct sysfs_dirent *sd);

Re: [RFC][Update 2][PATCH 3/4] sysfs: Functions for adding/removing symlinks to/from attribute groups

[Greg Kroah-Hartman]

On Mon, Jan 21, 2013 at 02:06:27PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The most convenient way to expose ACPI power resources lists of a
> device is to put symbolic links to sysfs directories representing
> those resources into special attribute groups in the device's sysfs
> directory.  For this purpose, it is necessary to be able to add
> symbolic links to attribute groups.
> 
> For this reason, add sysfs helper functions for adding/removing
> symbolic links to/from attribute groups, sysfs_add_link_to_group()
> and sysfs_remove_link_from_group(), respectively.

Those functions are fine, but why sysfs_add_link()?  It looks identical
to sysfs_create_link(), why would you call one and not the other?  Why
would anything outside of the sysfs core code ever have a pointer to a
sysfs_dirent?

thanks,

greg k-h

Re: [RFC][Update 2][PATCH 3/4] sysfs: Functions for adding/removing symlinks to/from attribute groups

[Rafael J. Wysocki]

On Monday, January 21, 2013 12:58:20 PM Greg Kroah-Hartman wrote:
> On Mon, Jan 21, 2013 at 02:06:27PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > The most convenient way to expose ACPI power resources lists of a
> > device is to put symbolic links to sysfs directories representing
> > those resources into special attribute groups in the device's sysfs
> > directory.  For this purpose, it is necessary to be able to add
> > symbolic links to attribute groups.
> > 
> > For this reason, add sysfs helper functions for adding/removing
> > symbolic links to/from attribute groups, sysfs_add_link_to_group()
> > and sysfs_remove_link_from_group(), respectively.
> 
> Those functions are fine, but why sysfs_add_link()?

Do you mean the helper?  I couldn't invent a better name. :-)

Would sysfs_create_link_sd() be better?

> It looks identical to sysfs_create_link(), why would you call one and not
> the other?

Because sysfs_create_link() takes a kobject as the first argument and I want
a sysfs_dirent.

> Why would anything outside of the sysfs core code ever have a pointer to a
> sysfs_dirent?

I don't see a reason and that sysfs_add_link() really is internal (it shouldn't
be exported then, yes).

I need to make a link from an attribute group under a kobject and not from the
kobject itself, that's all.

Thanks,
Rafael


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

Re: [RFC][Update 2][PATCH 3/4] sysfs: Functions for adding/removing symlinks to/from attribute groups

[Greg Kroah-Hartman]

On Mon, Jan 21, 2013 at 11:41:35PM +0100, Rafael J. Wysocki wrote:
> On Monday, January 21, 2013 12:58:20 PM Greg Kroah-Hartman wrote:
> > On Mon, Jan 21, 2013 at 02:06:27PM +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > The most convenient way to expose ACPI power resources lists of a
> > > device is to put symbolic links to sysfs directories representing
> > > those resources into special attribute groups in the device's sysfs
> > > directory.  For this purpose, it is necessary to be able to add
> > > symbolic links to attribute groups.
> > > 
> > > For this reason, add sysfs helper functions for adding/removing
> > > symbolic links to/from attribute groups, sysfs_add_link_to_group()
> > > and sysfs_remove_link_from_group(), respectively.
> > 
> > Those functions are fine, but why sysfs_add_link()?
> 
> Do you mean the helper?  I couldn't invent a better name. :-)
> 
> Would sysfs_create_link_sd() be better?

That's a bit more descriptive, yes, and don't export it and I'll have no
objections.

thanks,

greg k-h

[RFC][Update 2][PATCH 4/4] ACPI / PM: Expose lists of device power resources to user space

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since ACPI power resources are going to be used more extensively on
new hardware platforms, it is necessary to allow user space (powertop
in particular) to look at the lists of power resources corresponding
to different power states of devices for diagnostics and control
purposes.

For this reason, for each power state of an ACPI device node using
power resources create a special attribute group under the device
node's directory in sysfs containing links to sysfs directories
representing the power resources in that list.  The names of the
new attribute groups are "power_resources_<state>", where <state>
is the state name i.e. "D0", "D1", "D2", or "D3hot".

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/ABI/testing/sysfs-devices-power_resources_D0    |   13 +
 Documentation/ABI/testing/sysfs-devices-power_resources_D1    |   14 +
 Documentation/ABI/testing/sysfs-devices-power_resources_D2    |   14 +
 Documentation/ABI/testing/sysfs-devices-power_resources_D3hot |   14 +
 drivers/acpi/power.c                                          |   96 ++++++++--
 5 files changed, 138 insertions(+), 13 deletions(-)

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.

Re: [RFC][Update 2][PATCH 4/4] ACPI / PM: Expose lists of device power resources to user space

[Greg Kroah-Hartman]

On Mon, Jan 21, 2013 at 02:08:16PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since ACPI power resources are going to be used more extensively on
> new hardware platforms, it is necessary to allow user space (powertop
> in particular) to look at the lists of power resources corresponding
> to different power states of devices for diagnostics and control
> purposes.
> 
> For this reason, for each power state of an ACPI device node using
> power resources create a special attribute group under the device
> node's directory in sysfs containing links to sysfs directories
> representing the power resources in that list.  The names of the
> new attribute groups are "power_resources_<state>", where <state>
> is the state name i.e. "D0", "D1", "D2", or "D3hot".
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

You aren't calling sysfs_add_link() here, so why did you export it in
the previous patch?

thanks,

greg k-h

Re: [RFC][Update 2][PATCH 4/4] ACPI / PM: Expose lists of device power resources to user space

[Rafael J. Wysocki]

On Monday, January 21, 2013 12:58:46 PM Greg Kroah-Hartman wrote:
> On Mon, Jan 21, 2013 at 02:08:16PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Since ACPI power resources are going to be used more extensively on
> > new hardware platforms, it is necessary to allow user space (powertop
> > in particular) to look at the lists of power resources corresponding
> > to different power states of devices for diagnostics and control
> > purposes.
> > 
> > For this reason, for each power state of an ACPI device node using
> > power resources create a special attribute group under the device
> > node's directory in sysfs containing links to sysfs directories
> > representing the power resources in that list.  The names of the
> > new attribute groups are "power_resources_<state>", where <state>
> > is the state name i.e. "D0", "D1", "D2", or "D3hot".
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> You aren't calling sysfs_add_link() here, so why did you export it in
> the previous patch?

Well, unnecessarily.

Thanks,
Rafael


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

[PATCH 0/4] ACPI / PM: Export power information to user space

[Rafael J. Wysocki]

On Monday, January 21, 2013 02:03:03 PM Rafael J. Wysocki wrote:
> On Monday, January 21, 2013 01:48:55 AM Rafael J. Wysocki wrote:
> > On Sunday, January 20, 2013 03:51:40 PM Rafael J. Wysocki wrote:
> > > Hi Greg,
> > > 
> > > The following patch series is about exporting ACPI power resources to user
> > > space that will be necessary for PM diagnostics on new platforms (powertop is
> > > the target).  Still, existing systems can also benefit from that, like my
> > > oldish HP nx6325 on which the patches have been tested.
> > > 
> > > Patch [1/3] shouldn't be really controverisal, as it only makes it possible
> > > to read power resources' reference counters from user space in a quite
> > > straightforward way.
> > > 
> > > Patch [3/3], however, exposes lists of power resources associated with
> > > different power states of devices using ACPI PM as sysfs subdirectories
> > > containing symbolic links and that requires some sysfs changes made by
> > > patch [2/3].
> > > 
> > > Admittedly, my sysfs skills are limited so please let me know if those
> > > changes make sense and are acceptable from your perspective.  If not, then
> > > please kindly advise what I can do instead of them.
> > > 
> > > The patches apply on top of the current linux-next branch of the linux-pm.git
> > > tree.
> > 
> > Refreshed with Documentation/ABI files as requested.
> 
> Well, that version wasn't really a good one, due to mistakes in the docs
> mostly, so a new one follows (with better docs hopefully).
> 
> Besides, I thought I'd include a patch that exposes ACPI power state
> information, because it kind of belongs to the lot.  It goes as the new [1/4].
> 
> [2/4] is the previous [1/3] and so on.

Here it goes again after the changes we discussed earlier today.

I think I've addressed all of the comments, but if I overlooked something,
please let me know.

Thanks,
Rafael


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

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

[Rafael J. Wysocki]

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);
+	}
+
 end:
 	return result;
 }
@@ -380,6 +420,13 @@ static void acpi_device_remove_files(str
 	acpi_status status;
 	acpi_handle temp;
 
+	if (dev->flags.power_manageable) {
+		device_remove_file(&dev->dev, &dev_attr_power_state);
+		if (dev->power.flags.power_resources)
+			device_remove_file(&dev->dev,
+					   &dev_attr_real_power_state);
+	}
+
 	/*
 	 * If device has _STR, remove 'description' file
 	 */
Index: linux-pm/Documentation/ABI/testing/sysfs-devices-power_state
===================================================================
--- /dev/null
+++ linux-pm/Documentation/ABI/testing/sysfs-devices-power_state
@@ -0,0 +1,20 @@
+What:		/sys/devices/.../power_state
+Date:		January 2013
+Contact:	Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+Description:
+		The /sys/devices/.../power_state attribute is only present for
+		device objects representing ACPI device nodes that provide power
+		management methods.
+
+		If present, it contains a string representing the current ACPI
+		power state of the given device node.  Its possible values,
+		"D0", "D1", "D2", "D3hot", and "D3cold", reflect the power state
+		names defined by the ACPI specification (ACPI 4 and above).
+
+		If the device node uses shared ACPI power resources, this state
+		determines a list of power resources required not to be turned
+		off.  However, some power resources needed by the device node in
+		higher-power (lower-number) states may also be ON because of
+		some other devices using them at the moment.
+
+		This attribute is read-only.
Index: linux-pm/Documentation/ABI/testing/sysfs-devices-real_power_state
===================================================================
--- /dev/null
+++ linux-pm/Documentation/ABI/testing/sysfs-devices-real_power_state
@@ -0,0 +1,23 @@
+What:		/sys/devices/.../real_power_state
+Date:		January 2013
+Contact:	Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+Description:
+		The /sys/devices/.../real_power_state attribute is only present
+		for device objects representing ACPI device nodes that provide
+		power management methods and use ACPI power resources for power
+		management.
+
+		If present, it contains a string representing the real ACPI
+		power state of the given device node as returned by the _PSC
+		control method or inferred from the configuration of power
+		resources.  Its possible values, "D0", "D1", "D2", "D3hot", and
+		"D3cold", reflect the power state names defined by the ACPI
+		specification (ACPI 4 and above).
+
+		In some situations the value of this attribute may be different
+		from the value of the /sys/devices/.../power_state attribute for
+		the same device object.  If that happens, some shared power
+		resources used by the device node are only ON because of some
+		other devices using them at the moment.
+
+		This attribute is read-only.

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

[Greg Kroah-Hartman]

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>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

[Greg Kroah-Hartman]

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?

thanks,

greg k-h

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

[Rafael J. Wysocki]

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.

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

[Greg Kroah-Hartman]

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

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

[PATCH 2/4] ACPI / PM: Expose current status of ACPI power resources

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since ACPI power resources are going to be used more extensively on
new hardware platforms, it becomes necessary for user space (powertop
in particular) to observe some properties of those resources for
diagnostics purposes.

For this reason, expose the current status of each ACPI power
resource to user space via sysfs by adding a new resource_in_use
attribute to the sysfs directory representing the given power
resource.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/ABI/testing/sysfs-devices-resource_in_use |   12 +++++++
 drivers/acpi/power.c                                    |   26 +++++++++++++++-
 drivers/acpi/scan.c                                     |    3 +
 include/acpi/acpi_bus.h                                 |    1 
 4 files changed, 41 insertions(+), 1 deletion(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -280,6 +280,7 @@ struct acpi_device {
 	struct mutex physical_node_lock;
 	DECLARE_BITMAP(physical_node_id_bitmap, ACPI_MAX_PHYSICAL_NODE);
 	struct list_head power_dependent;
+	void (*remove)(struct acpi_device *);
 };
 
 static inline void *acpi_driver_data(struct acpi_device *d)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -790,6 +790,9 @@ static void acpi_device_unregister(struc
 
 	acpi_power_add_remove_device(device, false);
 	acpi_device_remove_files(device);
+	if (device->remove)
+		device->remove(device);
+
 	device_unregister(&device->dev);
 	/*
 	 * Drop the reference counts of all power resources the device depends
Index: linux-pm/drivers/acpi/power.c
===================================================================
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -87,6 +87,12 @@ static DEFINE_MUTEX(power_resource_list_
                              Power Resource Management
    -------------------------------------------------------------------------- */
 
+static inline
+struct acpi_power_resource *to_power_resource(struct acpi_device *device)
+{
+	return container_of(device, struct acpi_power_resource, device);
+}
+
 static struct acpi_power_resource *acpi_power_get_context(acpi_handle handle)
 {
 	struct acpi_device *device;
@@ -94,7 +100,7 @@ static struct acpi_power_resource *acpi_
 	if (acpi_bus_get_device(handle, &device))
 		return NULL;
 
-	return container_of(device, struct acpi_power_resource, device);
+	return to_power_resource(device);
 }
 
 static int acpi_power_resources_list_add(acpi_handle handle,
@@ -676,6 +682,21 @@ static void acpi_release_power_resource(
 	kfree(resource);
 }
 
+static ssize_t acpi_power_in_use_show(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buf) {
+	struct acpi_power_resource *resource;
+
+	resource = to_power_resource(to_acpi_device(dev));
+	return sprintf(buf, "%u\n", !!resource->ref_count);
+}
+static DEVICE_ATTR(resource_in_use, 0444, acpi_power_in_use_show, NULL);
+
+static void acpi_power_sysfs_remove(struct acpi_device *device)
+{
+	device_remove_file(&device->dev, &dev_attr_resource_in_use);
+}
+
 int acpi_add_power_resource(acpi_handle handle)
 {
 	struct acpi_power_resource *resource;
@@ -723,6 +744,9 @@ int acpi_add_power_resource(acpi_handle
 	if (result)
 		goto err;
 
+	if (!device_create_file(&device->dev, &dev_attr_resource_in_use))
+		device->remove = acpi_power_sysfs_remove;
+
 	mutex_lock(&power_resource_list_lock);
 	list_add(&resource->list_node, &acpi_power_resource_list);
 	mutex_unlock(&power_resource_list_lock);
Index: linux-pm/Documentation/ABI/testing/sysfs-devices-resource_in_use
===================================================================
--- /dev/null
+++ linux-pm/Documentation/ABI/testing/sysfs-devices-resource_in_use
@@ -0,0 +1,12 @@
+What:		/sys/devices/.../resource_in_use
+Date:		January 2013
+Contact:	Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+Description:
+		The /sys/devices/.../resource_in_use attribute is only present
+		for device objects representing ACPI power resources.
+
+		If present, it contains a number (0 or 1) representing the
+		current status of the given power resource (0 means that the
+		resource is not in use and therefore it has been turned off).
+
+		This attribute is read-only.

Re: [PATCH 2/4] ACPI / PM: Expose current status of ACPI power resources

[Greg Kroah-Hartman]

On Tue, Jan 22, 2013 at 03:26:29AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since ACPI power resources are going to be used more extensively on
> new hardware platforms, it becomes necessary for user space (powertop
> in particular) to observe some properties of those resources for
> diagnostics purposes.
> 
> For this reason, expose the current status of each ACPI power
> resource to user space via sysfs by adding a new resource_in_use
> attribute to the sysfs directory representing the given power
> resource.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

But, as with patch 1/4, this is racy:

> @@ -723,6 +744,9 @@ int acpi_add_power_resource(acpi_handle
>  	if (result)
>  		goto err;
>  
> +	if (!device_create_file(&device->dev, &dev_attr_resource_in_use))
> +		device->remove = acpi_power_sysfs_remove;
> +

You are creating the file after userspace has been told that the device
is present.

As this looks like the first file this type of device has, maybe you
should just fix this up "properly" here, and not wait to do it later?

Your choice,

greg k-h

[PATCH 3/4] sysfs: Functions for adding/removing symlinks to/from attribute groups

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The most convenient way to expose ACPI power resources lists of a
device is to put symbolic links to sysfs directories representing
those resources into special attribute groups in the device's sysfs
directory.  For this purpose, it is necessary to be able to add
symbolic links to attribute groups.

For this reason, add sysfs helper functions for adding/removing
symbolic links to/from attribute groups, sysfs_add_link_to_group()
and sysfs_remove_link_from_group(), respectively.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 fs/sysfs/group.c      |   42 ++++++++++++++++++++++++++++++++++++++++++
 fs/sysfs/symlink.c    |   45 ++++++++++++++++++++++++++++++++-------------
 include/linux/sysfs.h |    6 ++++++
 3 files changed, 80 insertions(+), 13 deletions(-)

Index: linux-pm/fs/sysfs/group.c
===================================================================
--- linux-pm.orig/fs/sysfs/group.c
+++ linux-pm/fs/sysfs/group.c
@@ -205,6 +205,48 @@ void sysfs_unmerge_group(struct kobject
 }
 EXPORT_SYMBOL_GPL(sysfs_unmerge_group);
 
+/**
+ * sysfs_add_link_to_group - add a symlink to an attribute group.
+ * @kobj:	The kobject containing the group.
+ * @group_name:	The name of the group.
+ * @target:	The target kobject of the symlink to create.
+ * @link_name:	The name of the symlink to create.
+ */
+int sysfs_add_link_to_group(struct kobject *kobj, const char *group_name,
+			    struct kobject *target, const char *link_name)
+{
+	struct sysfs_dirent *dir_sd;
+	int error = 0;
+
+	dir_sd = sysfs_get_dirent(kobj->sd, NULL, group_name);
+	if (!dir_sd)
+		return -ENOENT;
+
+	error = sysfs_create_link_sd(dir_sd, target, link_name);
+	sysfs_put(dir_sd);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(sysfs_add_link_to_group);
+
+/**
+ * sysfs_remove_link_from_group - remove a symlink from an attribute group.
+ * @kobj:	The kobject containing the group.
+ * @group_name:	The name of the group.
+ * @link_name:	The name of the symlink to remove.
+ */
+void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
+				  const char *link_name)
+{
+	struct sysfs_dirent *dir_sd;
+
+	dir_sd = sysfs_get_dirent(kobj->sd, NULL, group_name);
+	if (dir_sd) {
+		sysfs_hash_and_remove(dir_sd, NULL, link_name);
+		sysfs_put(dir_sd);
+	}
+}
+EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
 
 EXPORT_SYMBOL_GPL(sysfs_create_group);
 EXPORT_SYMBOL_GPL(sysfs_update_group);
Index: linux-pm/fs/sysfs/symlink.c
===================================================================
--- linux-pm.orig/fs/sysfs/symlink.c
+++ linux-pm/fs/sysfs/symlink.c
@@ -21,26 +21,17 @@
 
 #include "sysfs.h"
 
-static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
-				const char *name, int warn)
+static int sysfs_do_create_link_sd(struct sysfs_dirent *parent_sd,
+				   struct kobject *target,
+				   const char *name, int warn)
 {
-	struct sysfs_dirent *parent_sd = NULL;
 	struct sysfs_dirent *target_sd = NULL;
 	struct sysfs_dirent *sd = NULL;
 	struct sysfs_addrm_cxt acxt;
 	enum kobj_ns_type ns_type;
 	int error;
 
-	BUG_ON(!name);
-
-	if (!kobj)
-		parent_sd = &sysfs_root;
-	else
-		parent_sd = kobj->sd;
-
-	error = -EFAULT;
-	if (!parent_sd)
-		goto out_put;
+	BUG_ON(!name || !parent_sd);
 
 	/* target->sd can go away beneath us but is protected with
 	 * sysfs_assoc_lock.  Fetch target_sd from it.
@@ -96,6 +87,34 @@ static int sysfs_do_create_link(struct k
 }
 
 /**
+ *	sysfs_create_link_sd - create symlink to a given object.
+ *	@sd:		directory we're creating the link in.
+ *	@target:	object we're pointing to.
+ *	@name:		name of the symlink.
+ */
+int sysfs_create_link_sd(struct sysfs_dirent *sd, struct kobject *target,
+			 const char *name)
+{
+	return sysfs_do_create_link_sd(sd, target, name, 1);
+}
+
+static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
+				const char *name, int warn)
+{
+	struct sysfs_dirent *parent_sd = NULL;
+
+	if (!kobj)
+		parent_sd = &sysfs_root;
+	else
+		parent_sd = kobj->sd;
+
+	if (!parent_sd)
+		return -EFAULT;
+
+	return sysfs_do_create_link_sd(parent_sd, target, name, warn);
+}
+
+/**
  *	sysfs_create_link - create symlink between two objects.
  *	@kobj:	object whose directory we're creating the link in.
  *	@target:	object we're pointing to.
Index: linux-pm/include/linux/sysfs.h
===================================================================
--- linux-pm.orig/include/linux/sysfs.h
+++ linux-pm/include/linux/sysfs.h
@@ -154,6 +154,8 @@ int __must_check sysfs_create_bin_file(s
 void sysfs_remove_bin_file(struct kobject *kobj,
 			   const struct bin_attribute *attr);
 
+int sysfs_create_link_sd(struct sysfs_dirent *sd, struct kobject *target,
+			 const char *name);
 int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
 				   const char *name);
 int __must_check sysfs_create_link_nowarn(struct kobject *kobj,
@@ -181,6 +183,10 @@ int sysfs_merge_group(struct kobject *ko
 		       const struct attribute_group *grp);
 void sysfs_unmerge_group(struct kobject *kobj,
 		       const struct attribute_group *grp);
+int sysfs_add_link_to_group(struct kobject *kobj, const char *group_name,
+			    struct kobject *target, const char *link_name);
+void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
+				  const char *link_name);
 
 void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr);
 void sysfs_notify_dirent(struct sysfs_dirent *sd);

Re: [PATCH 3/4] sysfs: Functions for adding/removing symlinks to/from attribute groups

[Greg Kroah-Hartman]

On Tue, Jan 22, 2013 at 03:27:12AM +0100, Rafael J. Wysocki wrote:
> --- linux-pm.orig/include/linux/sysfs.h
> +++ linux-pm/include/linux/sysfs.h
> @@ -154,6 +154,8 @@ int __must_check sysfs_create_bin_file(s
>  void sysfs_remove_bin_file(struct kobject *kobj,
>  			   const struct bin_attribute *attr);
>  
> +int sysfs_create_link_sd(struct sysfs_dirent *sd, struct kobject *target,
> +			 const char *name);

This should go into fs/sysfs/sysfs.h not include/linux/sysfs.h as it's
an "internal" sysfs function.

thanks,

greg k-h

Re: [PATCH 3/4] sysfs: Functions for adding/removing symlinks to/from attribute groups

[Rafael J. Wysocki]

On Tuesday, January 22, 2013 03:51:01 PM Greg Kroah-Hartman wrote:
> On Tue, Jan 22, 2013 at 03:27:12AM +0100, Rafael J. Wysocki wrote:
> > --- linux-pm.orig/include/linux/sysfs.h
> > +++ linux-pm/include/linux/sysfs.h
> > @@ -154,6 +154,8 @@ int __must_check sysfs_create_bin_file(s
> >  void sysfs_remove_bin_file(struct kobject *kobj,
> >  			   const struct bin_attribute *attr);
> >  
> > +int sysfs_create_link_sd(struct sysfs_dirent *sd, struct kobject *target,
> > +			 const char *name);
> 
> This should go into fs/sysfs/sysfs.h not include/linux/sysfs.h as it's
> an "internal" sysfs function.

Damn, right.  Will fix.

Thanks,
Rafael


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

[PATCH 4/4] ACPI / PM: Expose lists of device power resources to user space

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since ACPI power resources are going to be used more extensively on
new hardware platforms, it is necessary to allow user space (powertop
in particular) to look at the lists of power resources corresponding
to different power states of devices for diagnostics and control
purposes.

For this reason, for each power state of an ACPI device node using
power resources create a special attribute group under the device
node's directory in sysfs containing links to sysfs directories
representing the power resources in that list.  The names of the
new attribute groups are "power_resources_<state>", where <state>
is the state name i.e. "D0", "D1", "D2", or "D3hot".

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/ABI/testing/sysfs-devices-power_resources_D0    |   13 +
 Documentation/ABI/testing/sysfs-devices-power_resources_D1    |   14 +
 Documentation/ABI/testing/sysfs-devices-power_resources_D2    |   14 +
 Documentation/ABI/testing/sysfs-devices-power_resources_D3hot |   14 +
 drivers/acpi/power.c                                          |   96 ++++++++--
 5 files changed, 138 insertions(+), 13 deletions(-)

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.

Re: [PATCH 4/4] ACPI / PM: Expose lists of device power resources to user space

[Greg Kroah-Hartman]

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

Re: [PATCH 4/4] ACPI / PM: Expose lists of device power resources to user space

[Rafael J. Wysocki]

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


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

Re: [PATCH 4/4] ACPI / PM: Expose lists of device power resources to user space

[Rafael J. Wysocki]

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


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

Re: [PATCH 4/4] ACPI / PM: Expose lists of device power resources to user space

[Rafael J. Wysocki]

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


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

Re: [PATCH 4/4] ACPI / PM: Expose lists of device power resources to user space

[Greg Kroah-Hartman]

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

Re: [PATCH 4/4] ACPI / PM: Expose lists of device power resources to user space

[Rafael J. Wysocki]

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


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

Re: [PATCH 4/4] ACPI / PM: Expose lists of device power resources to user space

[Greg Kroah-Hartman]

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

Re: [PATCH 4/4] ACPI / PM: Expose lists of device power resources to user space

[Rafael J. Wysocki]

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


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

[Update][PATCH 0/5] ACPI / PM: Export power information to user space

[Rafael J. Wysocki]

On Tuesday, January 22, 2013 03:15:52 AM Rafael J. Wysocki wrote:
> On Monday, January 21, 2013 02:03:03 PM Rafael J. Wysocki wrote:
> > On Monday, January 21, 2013 01:48:55 AM Rafael J. Wysocki wrote:
> > > On Sunday, January 20, 2013 03:51:40 PM Rafael J. Wysocki wrote:
> > > > Hi Greg,
> > > > 
> > > > The following patch series is about exporting ACPI power resources to user
> > > > space that will be necessary for PM diagnostics on new platforms (powertop is
> > > > the target).  Still, existing systems can also benefit from that, like my
> > > > oldish HP nx6325 on which the patches have been tested.
> > > > 
> > > > Patch [1/3] shouldn't be really controverisal, as it only makes it possible
> > > > to read power resources' reference counters from user space in a quite
> > > > straightforward way.
> > > > 
> > > > Patch [3/3], however, exposes lists of power resources associated with
> > > > different power states of devices using ACPI PM as sysfs subdirectories
> > > > containing symbolic links and that requires some sysfs changes made by
> > > > patch [2/3].
> > > > 
> > > > Admittedly, my sysfs skills are limited so please let me know if those
> > > > changes make sense and are acceptable from your perspective.  If not, then
> > > > please kindly advise what I can do instead of them.
> > > > 
> > > > The patches apply on top of the current linux-next branch of the linux-pm.git
> > > > tree.
> > > 
> > > Refreshed with Documentation/ABI files as requested.
> > 
> > Well, that version wasn't really a good one, due to mistakes in the docs
> > mostly, so a new one follows (with better docs hopefully).
> > 
> > Besides, I thought I'd include a patch that exposes ACPI power state
> > information, because it kind of belongs to the lot.  It goes as the new [1/4].
> > 
> > [2/4] is the previous [1/3] and so on.
> 
> Here it goes again after the changes we discussed earlier today.
> 
> I think I've addressed all of the comments, but if I overlooked something,
> please let me know.

So, the following is an update containing a patch ([1/5]) that fixes the
"creation of sysfs files after the device have been registered" problem.
Hopefully. :-)

Patches [2-3/5] are the previous [1-2/4] and they are almost identical as
before, so I added the ACKs you gave for them.  Patch [4/5] is the sysfs one
with the internal function placed in the internal header.  Patch [5/5] is
a rework of the one exposing the lists of power resources as directories
containing symbolic links.

Please let me know if you are fine with this.

Thanks,
Rafael


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

[Update][PATCH 1/5] ACPI / scan: Prevent device add uevents from racing with user space

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

ACPI core adds sysfs device files after the given devices have been
registered with device_register(), which is not appropriate, because
it may lead to race conditions with user space tools using those
files.

Fix the problem by delaying the KOBJ_ADD uevent for ACPI devices
until after all of the devices' sysfs files have been created.

This also fixes a use-after-free in acpi_device_unregister().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/internal.h |    5 +++--
 drivers/acpi/power.c    |    3 ++-
 drivers/acpi/scan.c     |   20 +++++++++++++++-----
 3 files changed, 20 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -40,10 +40,11 @@ static inline void acpi_debugfs_init(voi
 #define ACPI_STA_DEFAULT (ACPI_STA_DEVICE_PRESENT | ACPI_STA_DEVICE_ENABLED | \
 			  ACPI_STA_DEVICE_UI | ACPI_STA_DEVICE_FUNCTIONING)
 
-int acpi_device_register(struct acpi_device *device,
-			 void (*release)(struct device *));
+int acpi_device_add(struct acpi_device *device,
+		    void (*release)(struct device *));
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 			     int type, unsigned long long sta);
+void acpi_device_add_finalize(struct acpi_device *device);
 void acpi_free_ids(struct acpi_device *device);
 
 /* --------------------------------------------------------------------------
Index: linux-pm/drivers/acpi/power.c
===================================================================
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -719,13 +719,14 @@ int acpi_add_power_resource(acpi_handle
 	       acpi_device_bid(device), state ? "on" : "off");
 
 	device->flags.match_driver = true;
-	result = acpi_device_register(device, acpi_release_power_resource);
+	result = acpi_device_add(device, acpi_release_power_resource);
 	if (result)
 		goto err;
 
 	mutex_lock(&power_resource_list_lock);
 	list_add(&resource->list_node, &acpi_power_resource_list);
 	mutex_unlock(&power_resource_list_lock);
+	acpi_device_add_finalize(device);
 	return 0;
 
  err:
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -633,8 +633,8 @@ struct bus_type acpi_bus_type = {
 	.uevent		= acpi_device_uevent,
 };
 
-int acpi_device_register(struct acpi_device *device,
-			 void (*release)(struct device *))
+int acpi_device_add(struct acpi_device *device,
+		    void (*release)(struct device *))
 {
 	int result;
 	struct acpi_device_bus_id *acpi_device_bus_id, *new_bus_id;
@@ -704,7 +704,7 @@ int acpi_device_register(struct acpi_dev
 		device->dev.parent = &device->parent->dev;
 	device->dev.bus = &acpi_bus_type;
 	device->dev.release = release;
-	result = device_register(&device->dev);
+	result = device_add(&device->dev);
 	if (result) {
 		dev_err(&device->dev, "Error registering device\n");
 		goto err;
@@ -743,12 +743,13 @@ static void acpi_device_unregister(struc
 
 	acpi_power_add_remove_device(device, false);
 	acpi_device_remove_files(device);
-	device_unregister(&device->dev);
+	device_del(&device->dev);
 	/*
 	 * Drop the reference counts of all power resources the device depends
 	 * on and turn off the ones that have no more references.
 	 */
 	acpi_power_transition(device, ACPI_STATE_D3_COLD);
+	put_device(&device->dev);
 }
 
 /* --------------------------------------------------------------------------
@@ -1390,6 +1391,14 @@ void acpi_init_device_object(struct acpi
 	acpi_device_get_busid(device);
 	acpi_device_set_id(device);
 	acpi_bus_get_flags(device);
+	device_initialize(&device->dev);
+	dev_set_uevent_suppress(&device->dev, true);
+}
+
+void acpi_device_add_finalize(struct acpi_device *device)
+{
+	dev_set_uevent_suppress(&device->dev, false);
+	kobject_uevent(&device->dev.kobj, KOBJ_ADD);
 }
 
 static int acpi_add_single_object(struct acpi_device **child,
@@ -1411,13 +1420,14 @@ static int acpi_add_single_object(struct
 	acpi_bus_get_wakeup_device_flags(device);
 
 	device->flags.match_driver = match_driver;
-	result = acpi_device_register(device, acpi_device_release);
+	result = acpi_device_add(device, acpi_device_release);
 	if (result) {
 		acpi_device_release(&device->dev);
 		return result;
 	}
 
 	acpi_power_add_remove_device(device, true);
+	acpi_device_add_finalize(device);
 	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Added %s [%s] parent %s\n",
 		dev_name(&device->dev), (char *) buffer.pointer,

Re: [Update][PATCH 1/5] ACPI / scan: Prevent device add uevents from racing with user space

[Greg Kroah-Hartman]

On Wed, Jan 23, 2013 at 06:58:31PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> ACPI core adds sysfs device files after the given devices have been
> registered with device_register(), which is not appropriate, because
> it may lead to race conditions with user space tools using those
> files.
> 
> Fix the problem by delaying the KOBJ_ADD uevent for ACPI devices
> until after all of the devices' sysfs files have been created.
> 
> This also fixes a use-after-free in acpi_device_unregister().
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Nice fix.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[Update][PATCH 2/5] ACPI / PM: Expose power states of ACPI devices to user space

[Rafael J. Wysocki]

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>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 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);
+	}
+
 end:
 	return result;
 }
@@ -380,6 +420,13 @@ static void acpi_device_remove_files(str
 	acpi_status status;
 	acpi_handle temp;
 
+	if (dev->flags.power_manageable) {
+		device_remove_file(&dev->dev, &dev_attr_power_state);
+		if (dev->power.flags.power_resources)
+			device_remove_file(&dev->dev,
+					   &dev_attr_real_power_state);
+	}
+
 	/*
 	 * If device has _STR, remove 'description' file
 	 */
Index: linux-pm/Documentation/ABI/testing/sysfs-devices-power_state
===================================================================
--- /dev/null
+++ linux-pm/Documentation/ABI/testing/sysfs-devices-power_state
@@ -0,0 +1,20 @@
+What:		/sys/devices/.../power_state
+Date:		January 2013
+Contact:	Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+Description:
+		The /sys/devices/.../power_state attribute is only present for
+		device objects representing ACPI device nodes that provide power
+		management methods.
+
+		If present, it contains a string representing the current ACPI
+		power state of the given device node.  Its possible values,
+		"D0", "D1", "D2", "D3hot", and "D3cold", reflect the power state
+		names defined by the ACPI specification (ACPI 4 and above).
+
+		If the device node uses shared ACPI power resources, this state
+		determines a list of power resources required not to be turned
+		off.  However, some power resources needed by the device node in
+		higher-power (lower-number) states may also be ON because of
+		some other devices using them at the moment.
+
+		This attribute is read-only.
Index: linux-pm/Documentation/ABI/testing/sysfs-devices-real_power_state
===================================================================
--- /dev/null
+++ linux-pm/Documentation/ABI/testing/sysfs-devices-real_power_state
@@ -0,0 +1,23 @@
+What:		/sys/devices/.../real_power_state
+Date:		January 2013
+Contact:	Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+Description:
+		The /sys/devices/.../real_power_state attribute is only present
+		for device objects representing ACPI device nodes that provide
+		power management methods and use ACPI power resources for power
+		management.
+
+		If present, it contains a string representing the real ACPI
+		power state of the given device node as returned by the _PSC
+		control method or inferred from the configuration of power
+		resources.  Its possible values, "D0", "D1", "D2", "D3hot", and
+		"D3cold", reflect the power state names defined by the ACPI
+		specification (ACPI 4 and above).
+
+		In some situations the value of this attribute may be different
+		from the value of the /sys/devices/.../power_state attribute for
+		the same device object.  If that happens, some shared power
+		resources used by the device node are only ON because of some
+		other devices using them at the moment.
+
+		This attribute is read-only.

[Update][PATCH 3/5] ACPI / PM: Expose current status of ACPI power resources

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since ACPI power resources are going to be used more extensively on
new hardware platforms, it becomes necessary for user space (powertop
in particular) to observe some properties of those resources for
diagnostics purposes.

For this reason, expose the current status of each ACPI power
resource to user space via sysfs by adding a new resource_in_use
attribute to the sysfs directory representing the given power
resource.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 Documentation/ABI/testing/sysfs-devices-resource_in_use |   12 +++++++
 drivers/acpi/power.c                                    |   26 +++++++++++++++-
 drivers/acpi/scan.c                                     |    3 +
 include/acpi/acpi_bus.h                                 |    1 
 4 files changed, 41 insertions(+), 1 deletion(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -278,6 +278,7 @@ struct acpi_device {
 	struct mutex physical_node_lock;
 	DECLARE_BITMAP(physical_node_id_bitmap, ACPI_MAX_PHYSICAL_NODE);
 	struct list_head power_dependent;
+	void (*remove)(struct acpi_device *);
 };
 
 static inline void *acpi_driver_data(struct acpi_device *d)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -790,6 +790,9 @@ static void acpi_device_unregister(struc
 
 	acpi_power_add_remove_device(device, false);
 	acpi_device_remove_files(device);
+	if (device->remove)
+		device->remove(device);
+
 	device_del(&device->dev);
 	/*
 	 * Drop the reference counts of all power resources the device depends
Index: linux-pm/drivers/acpi/power.c
===================================================================
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -87,6 +87,12 @@ static DEFINE_MUTEX(power_resource_list_
                              Power Resource Management
    -------------------------------------------------------------------------- */
 
+static inline
+struct acpi_power_resource *to_power_resource(struct acpi_device *device)
+{
+	return container_of(device, struct acpi_power_resource, device);
+}
+
 static struct acpi_power_resource *acpi_power_get_context(acpi_handle handle)
 {
 	struct acpi_device *device;
@@ -94,7 +100,7 @@ static struct acpi_power_resource *acpi_
 	if (acpi_bus_get_device(handle, &device))
 		return NULL;
 
-	return container_of(device, struct acpi_power_resource, device);
+	return to_power_resource(device);
 }
 
 static int acpi_power_resources_list_add(acpi_handle handle,
@@ -676,6 +682,21 @@ static void acpi_release_power_resource(
 	kfree(resource);
 }
 
+static ssize_t acpi_power_in_use_show(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buf) {
+	struct acpi_power_resource *resource;
+
+	resource = to_power_resource(to_acpi_device(dev));
+	return sprintf(buf, "%u\n", !!resource->ref_count);
+}
+static DEVICE_ATTR(resource_in_use, 0444, acpi_power_in_use_show, NULL);
+
+static void acpi_power_sysfs_remove(struct acpi_device *device)
+{
+	device_remove_file(&device->dev, &dev_attr_resource_in_use);
+}
+
 int acpi_add_power_resource(acpi_handle handle)
 {
 	struct acpi_power_resource *resource;
@@ -723,6 +744,9 @@ int acpi_add_power_resource(acpi_handle
 	if (result)
 		goto err;
 
+	if (!device_create_file(&device->dev, &dev_attr_resource_in_use))
+		device->remove = acpi_power_sysfs_remove;
+
 	mutex_lock(&power_resource_list_lock);
 	list_add(&resource->list_node, &acpi_power_resource_list);
 	mutex_unlock(&power_resource_list_lock);
Index: linux-pm/Documentation/ABI/testing/sysfs-devices-resource_in_use
===================================================================
--- /dev/null
+++ linux-pm/Documentation/ABI/testing/sysfs-devices-resource_in_use
@@ -0,0 +1,12 @@
+What:		/sys/devices/.../resource_in_use
+Date:		January 2013
+Contact:	Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+Description:
+		The /sys/devices/.../resource_in_use attribute is only present
+		for device objects representing ACPI power resources.
+
+		If present, it contains a number (0 or 1) representing the
+		current status of the given power resource (0 means that the
+		resource is not in use and therefore it has been turned off).
+
+		This attribute is read-only.

[Update][PATCH 4/5] sysfs: Functions for adding/removing symlinks to/from attribute groups

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The most convenient way to expose ACPI power resources lists of a
device is to put symbolic links to sysfs directories representing
those resources into special attribute groups in the device's sysfs
directory.  For this purpose, it is necessary to be able to add
symbolic links to attribute groups.

For this reason, add sysfs helper functions for adding/removing
symbolic links to/from attribute groups, sysfs_add_link_to_group()
and sysfs_remove_link_from_group(), respectively.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 fs/sysfs/group.c      |   42 ++++++++++++++++++++++++++++++++++++++++++
 fs/sysfs/symlink.c    |   45 ++++++++++++++++++++++++++++++++-------------
 fs/sysfs/sysfs.h      |    2 ++
 include/linux/sysfs.h |    4 ++++
 4 files changed, 80 insertions(+), 13 deletions(-)

Index: linux-pm/fs/sysfs/group.c
===================================================================
--- linux-pm.orig/fs/sysfs/group.c
+++ linux-pm/fs/sysfs/group.c
@@ -205,6 +205,48 @@ void sysfs_unmerge_group(struct kobject
 }
 EXPORT_SYMBOL_GPL(sysfs_unmerge_group);
 
+/**
+ * sysfs_add_link_to_group - add a symlink to an attribute group.
+ * @kobj:	The kobject containing the group.
+ * @group_name:	The name of the group.
+ * @target:	The target kobject of the symlink to create.
+ * @link_name:	The name of the symlink to create.
+ */
+int sysfs_add_link_to_group(struct kobject *kobj, const char *group_name,
+			    struct kobject *target, const char *link_name)
+{
+	struct sysfs_dirent *dir_sd;
+	int error = 0;
+
+	dir_sd = sysfs_get_dirent(kobj->sd, NULL, group_name);
+	if (!dir_sd)
+		return -ENOENT;
+
+	error = sysfs_create_link_sd(dir_sd, target, link_name);
+	sysfs_put(dir_sd);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(sysfs_add_link_to_group);
+
+/**
+ * sysfs_remove_link_from_group - remove a symlink from an attribute group.
+ * @kobj:	The kobject containing the group.
+ * @group_name:	The name of the group.
+ * @link_name:	The name of the symlink to remove.
+ */
+void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
+				  const char *link_name)
+{
+	struct sysfs_dirent *dir_sd;
+
+	dir_sd = sysfs_get_dirent(kobj->sd, NULL, group_name);
+	if (dir_sd) {
+		sysfs_hash_and_remove(dir_sd, NULL, link_name);
+		sysfs_put(dir_sd);
+	}
+}
+EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
 
 EXPORT_SYMBOL_GPL(sysfs_create_group);
 EXPORT_SYMBOL_GPL(sysfs_update_group);
Index: linux-pm/fs/sysfs/symlink.c
===================================================================
--- linux-pm.orig/fs/sysfs/symlink.c
+++ linux-pm/fs/sysfs/symlink.c
@@ -21,26 +21,17 @@
 
 #include "sysfs.h"
 
-static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
-				const char *name, int warn)
+static int sysfs_do_create_link_sd(struct sysfs_dirent *parent_sd,
+				   struct kobject *target,
+				   const char *name, int warn)
 {
-	struct sysfs_dirent *parent_sd = NULL;
 	struct sysfs_dirent *target_sd = NULL;
 	struct sysfs_dirent *sd = NULL;
 	struct sysfs_addrm_cxt acxt;
 	enum kobj_ns_type ns_type;
 	int error;
 
-	BUG_ON(!name);
-
-	if (!kobj)
-		parent_sd = &sysfs_root;
-	else
-		parent_sd = kobj->sd;
-
-	error = -EFAULT;
-	if (!parent_sd)
-		goto out_put;
+	BUG_ON(!name || !parent_sd);
 
 	/* target->sd can go away beneath us but is protected with
 	 * sysfs_assoc_lock.  Fetch target_sd from it.
@@ -96,6 +87,34 @@ static int sysfs_do_create_link(struct k
 }
 
 /**
+ *	sysfs_create_link_sd - create symlink to a given object.
+ *	@sd:		directory we're creating the link in.
+ *	@target:	object we're pointing to.
+ *	@name:		name of the symlink.
+ */
+int sysfs_create_link_sd(struct sysfs_dirent *sd, struct kobject *target,
+			 const char *name)
+{
+	return sysfs_do_create_link_sd(sd, target, name, 1);
+}
+
+static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
+				const char *name, int warn)
+{
+	struct sysfs_dirent *parent_sd = NULL;
+
+	if (!kobj)
+		parent_sd = &sysfs_root;
+	else
+		parent_sd = kobj->sd;
+
+	if (!parent_sd)
+		return -EFAULT;
+
+	return sysfs_do_create_link_sd(parent_sd, target, name, warn);
+}
+
+/**
  *	sysfs_create_link - create symlink between two objects.
  *	@kobj:	object whose directory we're creating the link in.
  *	@target:	object we're pointing to.
Index: linux-pm/include/linux/sysfs.h
===================================================================
--- linux-pm.orig/include/linux/sysfs.h
+++ linux-pm/include/linux/sysfs.h
@@ -181,6 +181,10 @@ int sysfs_merge_group(struct kobject *ko
 		       const struct attribute_group *grp);
 void sysfs_unmerge_group(struct kobject *kobj,
 		       const struct attribute_group *grp);
+int sysfs_add_link_to_group(struct kobject *kobj, const char *group_name,
+			    struct kobject *target, const char *link_name);
+void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
+				  const char *link_name);
 
 void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr);
 void sysfs_notify_dirent(struct sysfs_dirent *sd);
Index: linux-pm/fs/sysfs/sysfs.h
===================================================================
--- linux-pm.orig/fs/sysfs/sysfs.h
+++ linux-pm/fs/sysfs/sysfs.h
@@ -240,3 +240,5 @@ void unmap_bin_file(struct sysfs_dirent
  * symlink.c
  */
 extern const struct inode_operations sysfs_symlink_inode_operations;
+int sysfs_create_link_sd(struct sysfs_dirent *sd, struct kobject *target,
+			 const char *name);

Re: [Update][PATCH 4/5] sysfs: Functions for adding/removing symlinks to/from attribute groups

[Greg Kroah-Hartman]

On Wed, Jan 23, 2013 at 07:00:54PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The most convenient way to expose ACPI power resources lists of a
> device is to put symbolic links to sysfs directories representing
> those resources into special attribute groups in the device's sysfs
> directory.  For this purpose, it is necessary to be able to add
> symbolic links to attribute groups.
> 
> For this reason, add sysfs helper functions for adding/removing
> symbolic links to/from attribute groups, sysfs_add_link_to_group()
> and sysfs_remove_link_from_group(), respectively.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[patch] sysfs: Fix build when sysfs is disabled

[David Rientjes]

"sysfs: Functions for adding/removing symlinks to/from attribute groups" 
causes a build failure when CONFIG_SYSFS is disabled:

drivers/acpi/power.c: In function 'acpi_power_hide_list':
drivers/acpi/power.c:463: error: implicit declaration of function 'sysfs_remove_link_from_group'
drivers/acpi/power.c: In function 'acpi_power_expose_list':
drivers/acpi/power.c:486: error: implicit declaration of function 'sysfs_add_link_to_group'

We need to define empty functions for this config to avoid ifdefs in the 
code.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/sysfs.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -330,6 +330,18 @@ static inline void sysfs_unmerge_group(struct kobject *kobj,
 {
 }
 
+static inline int sysfs_add_link_to_group(struct kobject *kobj,
+		const char *group_name, struct kobject *target,
+		const char *link_name)
+{
+	return 0;
+}
+
+static inline void sysfs_remove_link_from_group(struct kobject *kobj,
+		const char *group_name, const char *link_name)
+{
+}
+
 static inline void sysfs_notify(struct kobject *kobj, const char *dir,
 				const char *attr)
 {

Re: [patch] sysfs: Fix build when sysfs is disabled

[Greg Kroah-Hartman]

On Fri, Jan 25, 2013 at 12:13:59PM -0800, David Rientjes wrote:
> "sysfs: Functions for adding/removing symlinks to/from attribute groups" 
> causes a build failure when CONFIG_SYSFS is disabled:
> 
> drivers/acpi/power.c: In function 'acpi_power_hide_list':
> drivers/acpi/power.c:463: error: implicit declaration of function 'sysfs_remove_link_from_group'
> drivers/acpi/power.c: In function 'acpi_power_expose_list':
> drivers/acpi/power.c:486: error: implicit declaration of function 'sysfs_add_link_to_group'
> 
> We need to define empty functions for this config to avoid ifdefs in the 
> code.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  include/linux/sysfs.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Rafael has to take this through his tree.

Re: [patch] sysfs: Fix build when sysfs is disabled

[Rafael J. Wysocki]

On Friday, January 25, 2013 12:42:34 PM Greg Kroah-Hartman wrote:
> On Fri, Jan 25, 2013 at 12:13:59PM -0800, David Rientjes wrote:
> > "sysfs: Functions for adding/removing symlinks to/from attribute groups" 
> > causes a build failure when CONFIG_SYSFS is disabled:
> > 
> > drivers/acpi/power.c: In function 'acpi_power_hide_list':
> > drivers/acpi/power.c:463: error: implicit declaration of function 'sysfs_remove_link_from_group'
> > drivers/acpi/power.c: In function 'acpi_power_expose_list':
> > drivers/acpi/power.c:486: error: implicit declaration of function 'sysfs_add_link_to_group'
> > 
> > We need to define empty functions for this config to avoid ifdefs in the 
> > code.
> > 
> > Signed-off-by: David Rientjes <rientjes@google.com>
> > ---
> >  include/linux/sysfs.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> 
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Rafael has to take this through his tree.

Thanks for the patch and review.

I will fold it into the original patch to reduce bisection pain.

Thanks,
Rafael


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

Re: [patch] sysfs: Fix build when sysfs is disabled

[Rafael J. Wysocki]

On Friday, January 25, 2013 12:13:59 PM David Rientjes wrote:
> "sysfs: Functions for adding/removing symlinks to/from attribute groups" 
> causes a build failure when CONFIG_SYSFS is disabled:
> 
> drivers/acpi/power.c: In function 'acpi_power_hide_list':
> drivers/acpi/power.c:463: error: implicit declaration of function 'sysfs_remove_link_from_group'
> drivers/acpi/power.c: In function 'acpi_power_expose_list':
> drivers/acpi/power.c:486: error: implicit declaration of function 'sysfs_add_link_to_group'
> 
> We need to define empty functions for this config to avoid ifdefs in the 
> code.

Can you please send me the .config that triggered those errors?  It will be
useful for future build testing.

Thanks,
Rafael


> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  include/linux/sysfs.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -330,6 +330,18 @@ static inline void sysfs_unmerge_group(struct kobject *kobj,
>  {
>  }
>  
> +static inline int sysfs_add_link_to_group(struct kobject *kobj,
> +		const char *group_name, struct kobject *target,
> +		const char *link_name)
> +{
> +	return 0;
> +}
> +
> +static inline void sysfs_remove_link_from_group(struct kobject *kobj,
> +		const char *group_name, const char *link_name)
> +{
> +}
> +
>  static inline void sysfs_notify(struct kobject *kobj, const char *dir,
>  				const char *attr)
>  {
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [patch] sysfs: Fix build when sysfs is disabled

[David Rientjes]

On Fri, 25 Jan 2013, Rafael J. Wysocki wrote:

> Can you please send me the .config that triggered those errors?  It will be
> useful for future build testing.
> 

Anything with CONFIG_SYSFS disabled.

[Update][PATCH 5/5] ACPI / PM: Expose lists of device power resources to user space

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since ACPI power resources are going to be used more extensively on
new hardware platforms, it is necessary to allow user space (powertop
in particular) to look at the lists of power resources corresponding
to different power states of devices for diagnostics and control
purposes.

For this reason, for each power state of an ACPI device node using
power resources create a special attribute group under the device
node's directory in sysfs containing links to sysfs directories
representing the power resources in that list.  The names of the
new attribute groups are "power_resources_<state>", where <state>
is the state name i.e. "D0", "D1", "D2", or "D3hot".

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/ABI/testing/sysfs-devices-power_resources_D0    |   13 +
 Documentation/ABI/testing/sysfs-devices-power_resources_D1    |   14 +
 Documentation/ABI/testing/sysfs-devices-power_resources_D2    |   14 +
 Documentation/ABI/testing/sysfs-devices-power_resources_D3hot |   14 +
 drivers/acpi/power.c                                          |  104 ++++++++--
 5 files changed, 146 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/acpi/power.c
===================================================================
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -41,6 +41,7 @@
 #include <linux/types.h>
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
+#include <linux/sysfs.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
 #include "sleep.h"
@@ -417,24 +418,101 @@ static void acpi_power_remove_dependent(
 	}
 }
 
-void acpi_power_add_remove_device(struct acpi_device *adev, bool add)
+static struct attribute *attrs[] = {
+	NULL,
+};
+
+static struct attribute_group attr_groups[] = {
+	[ACPI_STATE_D0] = {
+		.name = "power_resources_D0",
+		.attrs = attrs,
+	},
+	[ACPI_STATE_D1] = {
+		.name = "power_resources_D1",
+		.attrs = attrs,
+	},
+	[ACPI_STATE_D2] = {
+		.name = "power_resources_D2",
+		.attrs = attrs,
+	},
+	[ACPI_STATE_D3_HOT] = {
+		.name = "power_resources_D3hot",
+		.attrs = attrs,
+	},
+};
+
+static void acpi_power_hide_list(struct acpi_device *adev, int state)
+{
+	struct acpi_device_power_state *ps = &adev->power.states[state];
+	struct acpi_power_resource_entry *entry;
+
+	if (list_empty(&ps->resources))
+		return;
+
+	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,
+					     attr_groups[state].name,
+					     dev_name(&res_dev->dev));
+	}
+	sysfs_remove_group(&adev->dev.kobj, &attr_groups[state]);
+}
+
+static void acpi_power_expose_list(struct acpi_device *adev, int state)
 {
-	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 acpi_device_power_state *ps = &adev->power.states[state];
+	struct acpi_power_resource_entry *entry;
+	int ret;
+
+	if (list_empty(&ps->resources))
+		return;
+
+	ret = sysfs_create_group(&adev->dev.kobj, &attr_groups[state]);
+	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,
+					      attr_groups[state].name,
+					      &res_dev->dev.kobj,
+					      dev_name(&res_dev->dev));
+		if (ret) {
+			acpi_power_hide_list(adev, state);
+			break;
 		}
 	}
 }
 
+void acpi_power_add_remove_device(struct acpi_device *adev, bool add)
+{
+	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++) {
+		if (add)
+			acpi_power_expose_list(adev, state);
+		else
+			acpi_power_hide_list(adev, 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.

Re: [Update][PATCH 5/5] ACPI / PM: Expose lists of device power resources to user space

[Greg Kroah-Hartman]

On Wed, Jan 23, 2013 at 07:01:37PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since ACPI power resources are going to be used more extensively on
> new hardware platforms, it is necessary to allow user space (powertop
> in particular) to look at the lists of power resources corresponding
> to different power states of devices for diagnostics and control
> purposes.
> 
> For this reason, for each power state of an ACPI device node using
> power resources create a special attribute group under the device
> node's directory in sysfs containing links to sysfs directories
> representing the power resources in that list.  The names of the
> new attribute groups are "power_resources_<state>", where <state>
> is the state name i.e. "D0", "D1", "D2", or "D3hot".
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks for fixing this up.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [Update][PATCH 0/5] ACPI / PM: Export power information to user space

[Greg Kroah-Hartman]

On Wed, Jan 23, 2013 at 06:56:43PM +0100, Rafael J. Wysocki wrote:
> On Tuesday, January 22, 2013 03:15:52 AM Rafael J. Wysocki wrote:
> > On Monday, January 21, 2013 02:03:03 PM Rafael J. Wysocki wrote:
> > > On Monday, January 21, 2013 01:48:55 AM Rafael J. Wysocki wrote:
> > > > On Sunday, January 20, 2013 03:51:40 PM Rafael J. Wysocki wrote:
> > > > > Hi Greg,
> > > > > 
> > > > > The following patch series is about exporting ACPI power resources to user
> > > > > space that will be necessary for PM diagnostics on new platforms (powertop is
> > > > > the target).  Still, existing systems can also benefit from that, like my
> > > > > oldish HP nx6325 on which the patches have been tested.
> > > > > 
> > > > > Patch [1/3] shouldn't be really controverisal, as it only makes it possible
> > > > > to read power resources' reference counters from user space in a quite
> > > > > straightforward way.
> > > > > 
> > > > > Patch [3/3], however, exposes lists of power resources associated with
> > > > > different power states of devices using ACPI PM as sysfs subdirectories
> > > > > containing symbolic links and that requires some sysfs changes made by
> > > > > patch [2/3].
> > > > > 
> > > > > Admittedly, my sysfs skills are limited so please let me know if those
> > > > > changes make sense and are acceptable from your perspective.  If not, then
> > > > > please kindly advise what I can do instead of them.
> > > > > 
> > > > > The patches apply on top of the current linux-next branch of the linux-pm.git
> > > > > tree.
> > > > 
> > > > Refreshed with Documentation/ABI files as requested.
> > > 
> > > Well, that version wasn't really a good one, due to mistakes in the docs
> > > mostly, so a new one follows (with better docs hopefully).
> > > 
> > > Besides, I thought I'd include a patch that exposes ACPI power state
> > > information, because it kind of belongs to the lot.  It goes as the new [1/4].
> > > 
> > > [2/4] is the previous [1/3] and so on.
> > 
> > Here it goes again after the changes we discussed earlier today.
> > 
> > I think I've addressed all of the comments, but if I overlooked something,
> > please let me know.
> 
> So, the following is an update containing a patch ([1/5]) that fixes the
> "creation of sysfs files after the device have been registered" problem.
> Hopefully. :-)
> 
> Patches [2-3/5] are the previous [1-2/4] and they are almost identical as
> before, so I added the ACKs you gave for them.  Patch [4/5] is the sysfs one
> with the internal function placed in the internal header.  Patch [5/5] is
> a rework of the one exposing the lists of power resources as directories
> containing symbolic links.
> 
> Please let me know if you are fine with this.

These all look great, thanks for fixing up the acpi device notification
mess, and the sysfs header file locations.  Feel free to take all of
these through your tree.

greg k-h

Re: [Update][PATCH 0/5] ACPI / PM: Export power information to user space

[Rafael J. Wysocki]

On Wednesday, January 23, 2013 04:34:40 PM Greg Kroah-Hartman wrote:
> On Wed, Jan 23, 2013 at 06:56:43PM +0100, Rafael J. Wysocki wrote:
> > On Tuesday, January 22, 2013 03:15:52 AM Rafael J. Wysocki wrote:
> > > On Monday, January 21, 2013 02:03:03 PM Rafael J. Wysocki wrote:
> > > > On Monday, January 21, 2013 01:48:55 AM Rafael J. Wysocki wrote:
> > > > > On Sunday, January 20, 2013 03:51:40 PM Rafael J. Wysocki wrote:
> > > > > > Hi Greg,
> > > > > > 
> > > > > > The following patch series is about exporting ACPI power resources to user
> > > > > > space that will be necessary for PM diagnostics on new platforms (powertop is
> > > > > > the target).  Still, existing systems can also benefit from that, like my
> > > > > > oldish HP nx6325 on which the patches have been tested.
> > > > > > 
> > > > > > Patch [1/3] shouldn't be really controverisal, as it only makes it possible
> > > > > > to read power resources' reference counters from user space in a quite
> > > > > > straightforward way.
> > > > > > 
> > > > > > Patch [3/3], however, exposes lists of power resources associated with
> > > > > > different power states of devices using ACPI PM as sysfs subdirectories
> > > > > > containing symbolic links and that requires some sysfs changes made by
> > > > > > patch [2/3].
> > > > > > 
> > > > > > Admittedly, my sysfs skills are limited so please let me know if those
> > > > > > changes make sense and are acceptable from your perspective.  If not, then
> > > > > > please kindly advise what I can do instead of them.
> > > > > > 
> > > > > > The patches apply on top of the current linux-next branch of the linux-pm.git
> > > > > > tree.
> > > > > 
> > > > > Refreshed with Documentation/ABI files as requested.
> > > > 
> > > > Well, that version wasn't really a good one, due to mistakes in the docs
> > > > mostly, so a new one follows (with better docs hopefully).
> > > > 
> > > > Besides, I thought I'd include a patch that exposes ACPI power state
> > > > information, because it kind of belongs to the lot.  It goes as the new [1/4].
> > > > 
> > > > [2/4] is the previous [1/3] and so on.
> > > 
> > > Here it goes again after the changes we discussed earlier today.
> > > 
> > > I think I've addressed all of the comments, but if I overlooked something,
> > > please let me know.
> > 
> > So, the following is an update containing a patch ([1/5]) that fixes the
> > "creation of sysfs files after the device have been registered" problem.
> > Hopefully. :-)
> > 
> > Patches [2-3/5] are the previous [1-2/4] and they are almost identical as
> > before, so I added the ACKs you gave for them.  Patch [4/5] is the sysfs one
> > with the internal function placed in the internal header.  Patch [5/5] is
> > a rework of the one exposing the lists of power resources as directories
> > containing symbolic links.
> > 
> > Please let me know if you are fine with this.
> 
> These all look great, thanks for fixing up the acpi device notification
> mess, and the sysfs header file locations.  Feel free to take all of
> these through your tree.

I will, thanks a lot for reviewing!


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