Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1214285 > unrolled thread

[PATCH 0/6] ACPI / properties: Hierarchical properties support

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] ACPI / properties: Hierarchical properties support "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-08-27 04:20 +0200
    [PATCH 5/6] ACPI / gpio: Split acpi_get_gpiod_by_index() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-08-27 04:20 +0200
    [PATCH 3/6] ACPI / property: Expose data-only subnodes via sysfs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-08-27 04:20 +0200
    [PATCH 1/6] ACPI / property: Add routine for extraction of _DSD properties "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-08-27 04:20 +0200
    Re: [PATCH 0/6] ACPI / properties: Hierarchical properties support Dustin Byford <dustin@cumulusnetworks.com> - 2015-08-28 02:20 +0200
      Re: [PATCH 0/6] ACPI / properties: Hierarchical properties support "Rafael J. Wysocki" <rafael@kernel.org> - 2015-08-28 03:40 +0200

#1214285 — [PATCH 0/6] ACPI / properties: Hierarchical properties support

Hi Everyone,

This has been in the works for some time, but the official document it is
based on was not quite ready before the last week.  It now is available at

http://www.uefi.org/sites/default/files/resources/_DSD-hierarchical-data-extension-UUID-v1.pdf

The issue at hand is that we need to be able to support hierarchical device
properties in ACPI, like when the set of properties of an entity called "Fred"
may include a subset called "dog" containing the properties of the Fred's dog
rather than those of Fred himself.  And it may make sense to have the same
property, like "hair color", for both Fred and the dog, but with different
values.

We (I, Darren and Dave at least) have explored many possible ways to deal with
that in ACPI, but the majority of them turned out to be unattractive for various
reasons.  Our first take was to use ACPI device objects to make the "child"
property sets available via _DSD, but that approach is generally incompatible
with the PnP Manager in Windows following the notion that all device objects
in ACPI tables are supposed to represent real devices.  It can still be made
work by adding _STA that returns 0 to those "property-only" device objects,
but that leads to complications in other places and is error prone (if the _STA
is forgotten, for example).  Moreover, it adds quite a bit of overhead even in
Linux (an ACPICA representation, struct acpi_device, driver core mechanics etc)
for things that are only supposed to represent sets of device properties.  And,
in addition to that, we'd need to figure out how to give those things arbitrary
names in a consistent way.  All of that caused us to drop the approach based on
device objects and look for other options.

One of those was to nest the "child" property sets within _DSD packages, but it
follows from experience that this is error prone too (firmware people tend to have
problems with getting deeply nested packages right in ASL) and we wanted to be
able to visually distinguish those sets as separate entities in ASL code.  That
led us to the directory concept defined by the document mentioned above.

The idea is that _DSD may return a package containing the properties of the
device it belongs to along with a directory of objects that need to be evaluated
in order to obtain the "child" property sets of it.  That directory needs to be
present in a separate section of the _DSD package (after the new UUID defined in
the above document) and is a list of two-element sub-packages (entries) where
the first element is the name of the entry (the name of the "child" property set
represented by it) and the second element is a "pointer" (essentially, a path
or "namestring" in the ACPI terminology) to the control method or a static
data package that needs to be evaluated to obtain the entry's data.  The data
returned by it is then interpreted in the same way as a _DSD return package,
so it may also contain properties and a directory of its own "child" property
sets.

As an example, consider the following ASL from an experimental MinnowBoard
firmware:

        Device (LEDS)
        {
            Name (_HID, "PRP0001")

            Name (_CRS, ResourceTemplate () {
                GpioIo (Exclusive, PullDown, 0, 0, IoRestrictionOutputOnly,
                        "\\_SB.PCI0.LPC", 0, ResourceConsumer) {10}
                GpioIo (Exclusive, PullDown, 0, 0, IoRestrictionOutputOnly,
                        "\\_SB.PCI0.LPC", 0, ResourceConsumer) {11}
            })

            Name (_DSD, Package () {
                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                Package () {
                    Package () {"compatible", Package () {"gpio-leds"}},
                },
                // Data extension
                ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
                Package () {
                    Package () {"heartbeat", "LEDH"},
                    Package () {"mmc-activity", "LEDM"},
                }
            })

            Name (LEDH, Package () {
                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                Package () {
                    Package () {"label", "Heartbeat"},
                    Package () {"gpios", Package () {^LEDS, 0, 0, 0}},
                    Package () {"linux,default-trigger", "heartbeat"},
                    Package () {"linux,default-state", "off"},
                    Package () {"linux,retain-state-suspended", 1},
                }
            })

            Name (LEDM, Package () {
                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                Package () {
                    Package () {"label", Package () {"MMC0 Activity"}},
                    Package () {"gpios", Package () {^LEDS, 1, 0, 0}},
                    Package () {"linux,default-trigger", Package () {"mmc0"}},
                    Package () {"linux,default-state", "off"},
                    Package () {"linux,retain-state-suspended", 1},
                }
            })
        }

where each LED in a GPIO LED array is represented by a "child" property set of
the master device object LEDS.  There are two "child" sets called "heartbeat"
and "mmc-activity" whose data come from the LEDH and LEDM static data
packages under LEDS, respectively. 

The patch series introduces an infrastructure allowing "child" property
sets like the above to be accessed via the generic device properties API.
It represents those property sets as structures extending struct fwnode_handle
with the new type FWNODE_ACPI_DATA and reworks the ACPI property handling
code to do the right thing if an fwnode_handle of that type is passed to it
(please refer to the patch changelogs for details).

Please note that this new mechanism is not a replacement for anything.  It
simply adds general support for representing hierarchical properties of
devices in a new way, but things that worked previously should still work.

Also please let me know if you have objections agaist this approach or
suggestions on improving the code.

Although the 4.3 merge window has not officially opened yet, I'm not regarding
this as 4.3 material unless someone wants it in 4.3 really badly, in which
case please let me know ASAP as well.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1214288 — [PATCH 5/6] ACPI / gpio: Split acpi_get_gpiod_by_index()

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

Split acpi_get_gpiod_by_index() into three smaller routines to
allow the subsequent change of the generic firmware node properties
code to be more strarightforward.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c |  113 +++++++++++++++++++++++++-------------------
 1 file changed, 65 insertions(+), 48 deletions(-)

Index: linux-pm/drivers/gpio/gpiolib-acpi.c
===================================================================
--- linux-pm.orig/drivers/gpio/gpiolib-acpi.c
+++ linux-pm/drivers/gpio/gpiolib-acpi.c
@@ -389,6 +389,8 @@ struct acpi_gpio_lookup {
 	struct acpi_gpio_info info;
 	int index;
 	int pin_index;
+	bool active_low;
+	struct acpi_device *adev;
 	struct gpio_desc *desc;
 	int n;
 };
@@ -425,6 +427,59 @@ static int acpi_find_gpio(struct acpi_re
 	return 1;
 }
 
+static int acpi_gpio_resource_lookup(struct acpi_gpio_lookup *lookup,
+				     struct acpi_gpio_info *info)
+{
+	struct list_head res_list;
+	int ret;
+
+	INIT_LIST_HEAD(&res_list);
+
+	ret = acpi_dev_get_resources(lookup->adev, &res_list, acpi_find_gpio,
+				     lookup);
+	if (ret < 0)
+		return ret;
+
+	acpi_dev_free_resource_list(&res_list);
+
+	if (!lookup->desc)
+		return -ENOENT;
+
+	if (info) {
+		*info = lookup->info;
+		if (lookup->active_low)
+			info->active_low = lookup->active_low;
+	}
+	return 0;
+}
+
+static int acpi_gpio_property_lookup(struct acpi_device *adev,
+				     const char *propname, int index,
+				     struct acpi_gpio_lookup *lookup)
+{
+	struct acpi_reference_args args;
+	int ret;
+
+	memset(&args, 0, sizeof(args));
+	ret = acpi_dev_get_property_reference(adev, propname, index, &args);
+	if (ret && !acpi_get_driver_gpio_data(adev, propname, index, &args))
+		return ret;
+
+	/*
+	 * The property was found and resolved, so need to lookup the GPIO based
+	 * on returned args.
+	 */
+	lookup->adev = args.adev;
+	if (args.nargs >= 2) {
+		lookup->index = args.args[0];
+		lookup->pin_index = args.args[1];
+		/* 3rd argument, if present is used to specify active_low. */
+		if (args.nargs >= 3)
+			lookup->active_low = !!args.args[2];
+	}
+	return 0;
+}
+
 /**
  * acpi_get_gpiod_by_index() - get a GPIO descriptor from device resources
  * @adev: pointer to a ACPI device to get GPIO from
@@ -452,8 +507,6 @@ struct gpio_desc *acpi_get_gpiod_by_inde
 					  struct acpi_gpio_info *info)
 {
 	struct acpi_gpio_lookup lookup;
-	struct list_head resource_list;
-	bool active_low = false;
 	int ret;
 
 	if (!adev)
@@ -463,58 +516,22 @@ struct gpio_desc *acpi_get_gpiod_by_inde
 	lookup.index = index;
 
 	if (propname) {
-		struct acpi_reference_args args;
-
 		dev_dbg(&adev->dev, "GPIO: looking up %s\n", propname);
 
-		memset(&args, 0, sizeof(args));
-		ret = acpi_dev_get_property_reference(adev, propname,
-						      index, &args);
-		if (ret) {
-			bool found = acpi_get_driver_gpio_data(adev, propname,
-							       index, &args);
-			if (!found)
-				return ERR_PTR(ret);
-		}
-
-		/*
-		 * The property was found and resolved so need to
-		 * lookup the GPIO based on returned args instead.
-		 */
-		adev = args.adev;
-		if (args.nargs >= 2) {
-			lookup.index = args.args[0];
-			lookup.pin_index = args.args[1];
-			/*
-			 * 3rd argument, if present is used to
-			 * specify active_low.
-			 */
-			if (args.nargs >= 3)
-				active_low = !!args.args[2];
-		}
-
-		dev_dbg(&adev->dev, "GPIO: _DSD returned %s %zd %llu %llu %llu\n",
-			dev_name(&adev->dev), args.nargs,
-			args.args[0], args.args[1], args.args[2]);
+		ret = acpi_gpio_property_lookup(adev, propname, index, &lookup);
+		if (ret)
+			return ERR_PTR(ret);
+
+		dev_dbg(&adev->dev, "GPIO: _DSD returned %s %d %d %u\n",
+			dev_name(&lookup.adev->dev), lookup.index,
+			lookup.pin_index, lookup.active_low);
 	} else {
 		dev_dbg(&adev->dev, "GPIO: looking up %d in _CRS\n", index);
+		lookup.adev = adev;
 	}
 
-	INIT_LIST_HEAD(&resource_list);
-	ret = acpi_dev_get_resources(adev, &resource_list, acpi_find_gpio,
-				     &lookup);
-	if (ret < 0)
-		return ERR_PTR(ret);
-
-	acpi_dev_free_resource_list(&resource_list);
-
-	if (lookup.desc && info) {
-		*info = lookup.info;
-		if (active_low)
-			info->active_low = active_low;
-	}
-
-	return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT);
+	ret = acpi_gpio_resource_lookup(&lookup, info);
+	return ret ? ERR_PTR(ret) : lookup.desc;
 }
 
 /**

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214290 — [PATCH 3/6] ACPI / property: Expose data-only subnodes via sysfs

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

Add infrastructure needed to expose data-only subnodes of ACPI
device objects introduced previously via sysfs.

Each data-only subnode is represented as a sysfs directory under
the directory corresponding to its parent object (a device or a
data-only subnode).  Each of them has a "path" attribute (containing
the full ACPI namespace path to the object the subnode data come from)
at this time.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/device_sysfs.c |  120 +++++++++++++++++++++++++++++++++++++++-----
 drivers/acpi/property.c     |    8 +-
 include/acpi/acpi_bus.h     |    3 +
 3 files changed, 116 insertions(+), 15 deletions(-)

Index: linux-pm/drivers/acpi/device_sysfs.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_sysfs.c
+++ linux-pm/drivers/acpi/device_sysfs.c
@@ -26,6 +26,106 @@
 
 #include "internal.h"
 
+static ssize_t acpi_object_path(acpi_handle handle, char *buf)
+{
+	struct acpi_buffer path = {ACPI_ALLOCATE_BUFFER, NULL};
+	int result;
+
+	result = acpi_get_name(handle, ACPI_FULL_PATHNAME, &path);
+	if (result)
+		return result;
+
+	result = sprintf(buf, "%s\n", (char*)path.pointer);
+	kfree(path.pointer);
+	return result;
+}
+
+struct acpi_data_node_attr {
+	struct attribute attr;
+	ssize_t (*show)(struct acpi_data_node *, char *);
+	ssize_t (*store)(struct acpi_data_node *, const char *, size_t count);
+};
+
+#define DATA_NODE_ATTR(_name)			\
+	static struct acpi_data_node_attr data_node_##_name =	\
+		__ATTR(_name, 0444, data_node_show_##_name, NULL)
+
+static ssize_t data_node_show_path(struct acpi_data_node *dn, char *buf)
+{
+	return acpi_object_path(dn->handle, buf);
+}
+
+DATA_NODE_ATTR(path);
+
+static struct attribute *acpi_data_node_default_attrs[] = {
+	&data_node_path.attr,
+	NULL
+};
+
+#define to_data_node(k) container_of(k, struct acpi_data_node, kobj)
+#define to_attr(a) container_of(a, struct acpi_data_node_attr, attr)
+
+static ssize_t acpi_data_node_attr_show(struct kobject *kobj,
+					struct attribute *attr, char *buf)
+{
+	struct acpi_data_node *dn = to_data_node(kobj);
+	struct acpi_data_node_attr *dn_attr = to_attr(attr);
+
+	return dn_attr->show ? dn_attr->show(dn, buf) : -ENXIO;
+}
+
+static const struct sysfs_ops acpi_data_node_sysfs_ops = {
+	.show	= acpi_data_node_attr_show,
+};
+
+static void acpi_data_node_release(struct kobject *kobj)
+{
+	struct acpi_data_node *dn = to_data_node(kobj);
+	complete(&dn->kobj_done);
+}
+
+static struct kobj_type acpi_data_node_ktype = {
+	.sysfs_ops = &acpi_data_node_sysfs_ops,
+	.default_attrs = acpi_data_node_default_attrs,
+	.release = acpi_data_node_release,
+};
+
+static void acpi_expose_nondev_subnodes(struct kobject *kobj,
+					struct acpi_device_data *data)
+{
+	struct list_head *list = &data->subnodes;
+	struct acpi_data_node *dn;
+
+	if (list_empty(list))
+		return;
+
+	list_for_each_entry(dn, list, sibling) {
+		int ret;
+
+		init_completion(&dn->kobj_done);
+		ret = kobject_init_and_add(&dn->kobj, &acpi_data_node_ktype,
+					   kobj, dn->name);
+		if (ret)
+			acpi_handle_err(dn->handle, "Failed to expose (%d)\n", ret);
+		else
+			acpi_expose_nondev_subnodes(&dn->kobj, &dn->data);
+	}
+}
+
+static void acpi_hide_nondev_subnodes(struct acpi_device_data *data)
+{
+	struct list_head *list = &data->subnodes;
+	struct acpi_data_node *dn;
+
+	if (list_empty(list))
+		return;
+
+	list_for_each_entry_reverse(dn, list, sibling) {
+		acpi_hide_nondev_subnodes(&dn->data);
+		kobject_put(&dn->kobj);
+	}
+}
+
 /**
  * create_pnp_modalias - Create hid/cid(s) string for modalias and uevent
  * @acpi_dev: ACPI device object.
@@ -323,20 +423,12 @@ static ssize_t acpi_device_adr_show(stru
 }
 static DEVICE_ATTR(adr, 0444, acpi_device_adr_show, NULL);
 
-static ssize_t
-acpi_device_path_show(struct device *dev, struct device_attribute *attr, char *buf) {
+static ssize_t acpi_device_path_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
 	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	struct acpi_buffer path = {ACPI_ALLOCATE_BUFFER, NULL};
-	int result;
-
-	result = acpi_get_name(acpi_dev->handle, ACPI_FULL_PATHNAME, &path);
-	if (result)
-		goto end;
 
-	result = sprintf(buf, "%s\n", (char*)path.pointer);
-	kfree(path.pointer);
-end:
-	return result;
+	return acpi_object_path(acpi_dev->handle, buf);
 }
 static DEVICE_ATTR(path, 0444, acpi_device_path_show, NULL);
 
@@ -475,6 +567,8 @@ int acpi_device_setup_files(struct acpi_
 						    &dev_attr_real_power_state);
 	}
 
+	acpi_expose_nondev_subnodes(&dev->dev.kobj, &dev->data);
+
 end:
 	return result;
 }
@@ -485,6 +579,8 @@ end:
  */
 void acpi_device_remove_files(struct acpi_device *dev)
 {
+	acpi_hide_nondev_subnodes(&dev->data);
+
 	if (dev->flags.power_manageable) {
 		device_remove_file(&dev->dev, &dev_attr_power_state);
 		if (dev->power.flags.power_resources)
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -382,9 +382,12 @@ struct acpi_device {
 /* Non-device subnode */
 struct acpi_data_node {
 	const char *name;
+	acpi_handle handle;
 	struct fwnode_handle fwnode;
 	struct acpi_device_data data;
 	struct list_head sibling;
+	struct kobject kobj;
+	struct completion kobj_done;
 };
 
 static inline bool acpi_check_dma(struct acpi_device *adev, bool *coherent)
Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -64,12 +64,13 @@ static bool acpi_nondev_subnode_ok(acpi_
 		goto fail;
 
 	if (acpi_extract_properties(buf.pointer, &dn->data))
-		dn->data.pointer = buf.pointer;
+		dn->handle = handle;
 
 	if (acpi_enumerate_nondev_subnodes(scope, buf.pointer, &dn->data))
-		dn->data.pointer = buf.pointer;
+		dn->handle = handle;
 
-	if (dn->data.pointer) {
+	if (dn->handle) {
+		dn->data.pointer = buf.pointer;
 		list_add_tail(&dn->sibling, list);
 		return true;
 	}
@@ -302,6 +303,7 @@ static void acpi_destroy_nondev_subnodes
 
 	list_for_each_entry_safe_reverse(dn, next, list, sibling) {
 		acpi_destroy_nondev_subnodes(&dn->data.subnodes);
+		wait_for_completion(&dn->kobj_done);
 		list_del(&dn->sibling);
 		ACPI_FREE((void *)dn->data.pointer);
 		kfree(dn);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214291 — [PATCH 1/6] ACPI / property: Add routine for extraction of _DSD properties

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

Move the extraction of _DSD properties from acpi_init_properties()
to a separate routine called acpi_extract_properties() to make the
subsequent changes more straightforward.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/property.c |   69 +++++++++++++++++++++++++-----------------------
 1 file changed, 37 insertions(+), 32 deletions(-)

Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -100,34 +100,13 @@ static void acpi_init_of_compatible(stru
 	adev->flags.of_compatible_ok = 1;
 }
 
-void acpi_init_properties(struct acpi_device *adev)
+static bool acpi_extract_properties(const union acpi_object *desc,
+				    struct acpi_device_data *data)
 {
-	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
-	bool acpi_of = false;
-	struct acpi_hardware_id *hwid;
-	const union acpi_object *desc;
-	acpi_status status;
 	int i;
 
-	/*
-	 * Check if ACPI_DT_NAMESPACE_HID is present and inthat case we fill in
-	 * Device Tree compatible properties for this device.
-	 */
-	list_for_each_entry(hwid, &adev->pnp.ids, list) {
-		if (!strcmp(hwid->id, ACPI_DT_NAMESPACE_HID)) {
-			acpi_of = true;
-			break;
-		}
-	}
-
-	status = acpi_evaluate_object_typed(adev->handle, "_DSD", NULL, &buf,
-					    ACPI_TYPE_PACKAGE);
-	if (ACPI_FAILURE(status))
-		goto out;
-
-	desc = buf.pointer;
 	if (desc->package.count % 2)
-		goto fail;
+		return false;
 
 	/* Look for the device properties UUID. */
 	for (i = 0; i < desc->package.count; i += 2) {
@@ -154,18 +133,44 @@ void acpi_init_properties(struct acpi_de
 		if (!acpi_properties_format_valid(properties))
 			break;
 
-		adev->data.pointer = buf.pointer;
-		adev->data.properties = properties;
+		data->properties = properties;
+		return true;
+	}
 
-		if (acpi_of)
-			acpi_init_of_compatible(adev);
+	return false;
+}
 
-		goto out;
+void acpi_init_properties(struct acpi_device *adev)
+{
+	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
+	struct acpi_hardware_id *hwid;
+	acpi_status status;
+	bool acpi_of = false;
+
+	/*
+	 * Check if ACPI_DT_NAMESPACE_HID is present and inthat case we fill in
+	 * Device Tree compatible properties for this device.
+	 */
+	list_for_each_entry(hwid, &adev->pnp.ids, list) {
+		if (!strcmp(hwid->id, ACPI_DT_NAMESPACE_HID)) {
+			acpi_of = true;
+			break;
+		}
 	}
 
- fail:
-	dev_dbg(&adev->dev, "Returned _DSD data is not valid, skipping\n");
-	ACPI_FREE(buf.pointer);
+	status = acpi_evaluate_object_typed(adev->handle, "_DSD", NULL, &buf,
+					    ACPI_TYPE_PACKAGE);
+	if (ACPI_FAILURE(status))
+		goto out;
+
+	if (acpi_extract_properties(buf.pointer, &adev->data)) {
+		adev->data.pointer = buf.pointer;
+		if (acpi_of)
+			acpi_init_of_compatible(adev);
+	} else {
+		acpi_handle_debug(adev->handle, "Invalid _DSD data, skipping\n");
+		ACPI_FREE(buf.pointer);
+	}
 
  out:
 	if (acpi_of && !adev->flags.of_compatible_ok)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1215017

On Thu Aug 27 04:34, Rafael J. Wysocki wrote:

Hi Rafael,

> The issue at hand is that we need to be able to support hierarchical device
> properties in ACPI, like when the set of properties of an entity called "Fred"
> may include a subset called "dog" containing the properties of the Fred's dog
> rather than those of Fred himself.  And it may make sense to have the same
> property, like "hair color", for both Fred and the dog, but with different
> values.

OK.  I have a couple of questions.

> We (I, Darren and Dave at least) have explored many possible ways to deal with
> that in ACPI, but the majority of them turned out to be unattractive for various
> reasons.  Our first take was to use ACPI device objects to make the "child"
> property sets available via _DSD, but that approach is generally incompatible
> with the PnP Manager in Windows following the notion that all device objects
> in ACPI tables are supposed to represent real devices.  It can still be made
> work by adding _STA that returns 0 to those "property-only" device objects,
> but that leads to complications in other places and is error prone (if the _STA
> is forgotten, for example).  Moreover, it adds quite a bit of overhead even in
> Linux (an ACPICA representation, struct acpi_device, driver core mechanics etc)
> for things that are only supposed to represent sets of device properties.  And,
> in addition to that, we'd need to figure out how to give those things arbitrary
> names in a consistent way.  All of that caused us to drop the approach based on
> device objects and look for other options.

What's the overhead/effect on Windows for an ACPI object without a _HID (_ADR
only)?  That seems like a case where the OS shouldn't be expecting to load
another driver for the ACPI object and the _ADR gives each node a unique name
(even if it is an unfriendly integer)

> The idea is that _DSD may return a package containing the properties of the
> device it belongs to along with a directory of objects that need to be evaluated
> in order to obtain the "child" property sets of it.  That directory needs to be
> present in a separate section of the _DSD package (after the new UUID defined in
> the above document) and is a list of two-element sub-packages (entries) where
> the first element is the name of the entry (the name of the "child" property set
> represented by it) and the second element is a "pointer" (essentially, a path
> or "namestring" in the ACPI terminology) to the control method or a static
> data package that needs to be evaluated to obtain the entry's data.  The data
> returned by it is then interpreted in the same way as a _DSD return package,
> so it may also contain properties and a directory of its own "child" property
> sets.

Do you expect there to be cases where using an ACPI device object is more
desirable than hierarchical properties?  Or is it just impossible given the PNP
manager in Windows?

The best example I can think of is perhaps a multi function device where each
sub-function really does look like a separate device and you probably want to
reference that sub-device, as a device, in other ASL code.

Stating the above more generally, by taking this approach you loose the ability
to reference these child nodes as a device object.  In this LED example, I
think it would be nice to set the "trigger" for the led by adding a reference
to the LED from another device (such as a NIC).

Device (NIC0)
{
    ...
    Name (_DSD, Package () {
        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package () {
            Package () {"activity-led", LEDS.LEDM },
            Package () {"link-led", LEDS.LEDH },
        },
    }
}

I'm not sure that's even supported in devicetree or LEDs are the best example
of this, but the pattern seems generally useful.

Without a device you're also forced to use a "label" property instead of a
_STR.

> As an example, consider the following ASL from an experimental MinnowBoard
> firmware:
> 
>         Device (LEDS)
>         {
>             Name (_HID, "PRP0001")
> 
>             Name (_CRS, ResourceTemplate () {
>                 GpioIo (Exclusive, PullDown, 0, 0, IoRestrictionOutputOnly,
>                         "\\_SB.PCI0.LPC", 0, ResourceConsumer) {10}
>                 GpioIo (Exclusive, PullDown, 0, 0, IoRestrictionOutputOnly,
>                         "\\_SB.PCI0.LPC", 0, ResourceConsumer) {11}
>             })
> 
>             Name (_DSD, Package () {
>                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                 Package () {
>                     Package () {"compatible", Package () {"gpio-leds"}},
>                 },
>                 // Data extension
>                 ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>                 Package () {
>                     Package () {"heartbeat", "LEDH"},
>                     Package () {"mmc-activity", "LEDM"},

I guess LEDH and LEDM have to be strings here.  It would be nice if the
compiler could verify the path resolves.  I suppose it's more incentive to keep
these in the same scope.

>            Name (LEDH, Package () {
>                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                Package () {
>                    Package () {"label", "Heartbeat"},
>                    Package () {"gpios", Package () {^LEDS, 0, 0, 0}},
>                    Package () {"linux,default-trigger", "heartbeat"},
>                    Package () {"linux,default-state", "off"},
>                    Package () {"linux,retain-state-suspended", 1},
>                }
>            })
>            
>            Name (LEDM, Package () {
>                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                Package () {
>                    Package () {"label", Package () {"MMC0 Activity"}},
>                    Package () {"gpios", Package () {^LEDS, 1, 0, 0}},
>                    Package () {"linux,default-trigger", Package () {"mmc0"}},
>                    Package () {"linux,default-state", "off"},
>                    Package () {"linux,retain-state-suspended", 1},
>                }
>            })
>        }

I suspect you've thought of all of this.  Thanks in advance for any
explanations.

                --Dustin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1215050

Hi,

On Fri, Aug 28, 2015 at 2:10 AM, Dustin Byford
<dustin@cumulusnetworks.com> wrote:
> On Thu Aug 27 04:34, Rafael J. Wysocki wrote:
>
> Hi Rafael,
>
>> The issue at hand is that we need to be able to support hierarchical device
>> properties in ACPI, like when the set of properties of an entity called "Fred"
>> may include a subset called "dog" containing the properties of the Fred's dog
>> rather than those of Fred himself.  And it may make sense to have the same
>> property, like "hair color", for both Fred and the dog, but with different
>> values.
>
> OK.  I have a couple of questions.
>
>> We (I, Darren and Dave at least) have explored many possible ways to deal with
>> that in ACPI, but the majority of them turned out to be unattractive for various
>> reasons.  Our first take was to use ACPI device objects to make the "child"
>> property sets available via _DSD, but that approach is generally incompatible
>> with the PnP Manager in Windows following the notion that all device objects
>> in ACPI tables are supposed to represent real devices.  It can still be made
>> work by adding _STA that returns 0 to those "property-only" device objects,
>> but that leads to complications in other places and is error prone (if the _STA
>> is forgotten, for example).  Moreover, it adds quite a bit of overhead even in
>> Linux (an ACPICA representation, struct acpi_device, driver core mechanics etc)
>> for things that are only supposed to represent sets of device properties.  And,
>> in addition to that, we'd need to figure out how to give those things arbitrary
>> names in a consistent way.  All of that caused us to drop the approach based on
>> device objects and look for other options.
>
> What's the overhead/effect on Windows for an ACPI object without a _HID (_ADR
> only)?  That seems like a case where the OS shouldn't be expecting to load
> another driver for the ACPI object and the _ADR gives each node a unique name
> (even if it is an unfriendly integer)

I'm not sure about the driver thing in Windows in the case of _ADR
devices to be honest.  That said, by the spec you're only supposed to
use _ADR with things listed in Table 6-168 (page 278) as of ACPI 6.
So that already is a stretch to use _ADR for something not listed
there.

The fact that this is an integer rather than a name is slightly
problematic too, because it does not fit into some use cases we
already have drivers for.  In some cases the "child" entities are
referred to by names, because things have been developed with DT in
mind.  We generally would like to avoid having to provide a separate
ACPI code path at least in some of those cases if possible.

As for the overhead, if something is a device object, it already has a
scope associated with it in the ACPI namespace which adds complexity.
Further, we create a struct acpi_device for it, which is based on
struct device and registered with the driver core etc.  All of those
things are not small objects so they take up quite a bit of memory.
Not to mention sysfs interfaces created for them and so on.  They also
are taken into account by things like acpi_walk_namespace() and
generally cause those operations to be slightly slower.  The are taken
into account by the system suspend/resume code too.

>> The idea is that _DSD may return a package containing the properties of the
>> device it belongs to along with a directory of objects that need to be evaluated
>> in order to obtain the "child" property sets of it.  That directory needs to be
>> present in a separate section of the _DSD package (after the new UUID defined in
>> the above document) and is a list of two-element sub-packages (entries) where
>> the first element is the name of the entry (the name of the "child" property set
>> represented by it) and the second element is a "pointer" (essentially, a path
>> or "namestring" in the ACPI terminology) to the control method or a static
>> data package that needs to be evaluated to obtain the entry's data.  The data
>> returned by it is then interpreted in the same way as a _DSD return package,
>> so it may also contain properties and a directory of its own "child" property
>> sets.
>
> Do you expect there to be cases where using an ACPI device object is more
> desirable than hierarchical properties?  Or is it just impossible given the PNP
> manager in Windows?
>
> The best example I can think of is perhaps a multi function device where each
> sub-function really does look like a separate device and you probably want to
> reference that sub-device, as a device, in other ASL code.

In those cases you may need to use device objects, but you can still
do that just fine.  It is just more heavy-weight and may be
problematic for other OSes at least in principle, so by doing that you
may end up with Linux-specific firmware.

The new mechanism introduced here is for the situations in which you
don't have to do that and by using it you can avoid having to worry
about things like the Windows PnP manager entirely.

> Stating the above more generally, by taking this approach you loose the ability
> to reference these child nodes as a device object.  In this LED example, I
> think it would be nice to set the "trigger" for the led by adding a reference
> to the LED from another device (such as a NIC).
>
> Device (NIC0)
> {
>     ...
>     Name (_DSD, Package () {
>         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>         Package () {
>             Package () {"activity-led", LEDS.LEDM },
>             Package () {"link-led", LEDS.LEDH },
>         },
>     }
> }
>
> I'm not sure that's even supported in devicetree or LEDs are the best example
> of this, but the pattern seems generally useful.

I don't see why you can't do that (except that the paths need to be
represented as strings and they are relative to the current scope by
default, so here they need to be absolute or use the "one level up"
operator).

> Without a device you're also forced to use a "label" property instead of a _STR.

Well, you are.

Why exactly does that matter?

>> As an example, consider the following ASL from an experimental MinnowBoard
>> firmware:
>>
>>         Device (LEDS)
>>         {
>>             Name (_HID, "PRP0001")
>>
>>             Name (_CRS, ResourceTemplate () {
>>                 GpioIo (Exclusive, PullDown, 0, 0, IoRestrictionOutputOnly,
>>                         "\\_SB.PCI0.LPC", 0, ResourceConsumer) {10}
>>                 GpioIo (Exclusive, PullDown, 0, 0, IoRestrictionOutputOnly,
>>                         "\\_SB.PCI0.LPC", 0, ResourceConsumer) {11}
>>             })
>>
>>             Name (_DSD, Package () {
>>                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>                 Package () {
>>                     Package () {"compatible", Package () {"gpio-leds"}},
>>                 },
>>                 // Data extension
>>                 ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>>                 Package () {
>>                     Package () {"heartbeat", "LEDH"},
>>                     Package () {"mmc-activity", "LEDM"},
>
> I guess LEDH and LEDM have to be strings here.

Yes, they have to be strings.

We attempted to use references for that, but those are replaced with
the target objects in some cases when the package is being created.

> It would be nice if the compiler could verify the path resolves.

That only is possible at run time when the namespace has been created
(think about stuff coming from different SSDTs, for example).

> I suppose it's more incentive to keep these in the same scope.

That is recommended even. :-)

>>            Name (LEDH, Package () {
>>                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>                Package () {
>>                    Package () {"label", "Heartbeat"},
>>                    Package () {"gpios", Package () {^LEDS, 0, 0, 0}},
>>                    Package () {"linux,default-trigger", "heartbeat"},
>>                    Package () {"linux,default-state", "off"},
>>                    Package () {"linux,retain-state-suspended", 1},
>>                }
>>            })
>>
>>            Name (LEDM, Package () {
>>                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>                Package () {
>>                    Package () {"label", Package () {"MMC0 Activity"}},
>>                    Package () {"gpios", Package () {^LEDS, 1, 0, 0}},
>>                    Package () {"linux,default-trigger", Package () {"mmc0"}},
>>                    Package () {"linux,default-state", "off"},
>>                    Package () {"linux,retain-state-suspended", 1},
>>                }
>>            })
>>        }
>
> I suspect you've thought of all of this.  Thanks in advance for any explanations.

No problem at all.  Please let me know if the above is sufficient.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web