USB: Use ACPI device information

PATCH In review Samuel Ortiz 3 participants 10 messages Aug 13, 2014 – Sep 3, 2014

Subsystems (3)

USB ACPI ACPI component architecture (acpica)

lore

Share & tools

lore

Samuel Ortiz

author

2014-08-13 17:18 UTC #0 lore raw

PATCH v4 1/2 USB: Use ACPI device information

From: Faouaz TENOUTIT <[email protected]>

Use ACPI device pld information instead of calling acpi_get_physical_device_location.

Signed-off-by Faouaz TENOUTIT <[email protected]>

Acked-by Samuel Ortiz <[email protected]>

Acked-by Greg Kroah-Hartman <[email protected]>

---
 drivers/acpi/scan.c         | 13 +++++++++++++
 drivers/usb/core/usb-acpi.c | 14 +++++---------
 include/acpi/acpi_bus.h     |  1 +
 3 files changed, 19 insertions(+), 9 deletions(-)

drivers/acpi/scan.c +13 -0

index 0a817ad..2ca42d5 100644
@@ -767,6 +767,13 @@ static int acpi_device_setup_files(struct acpi_device *dev)
 						    &dev_attr_real_power_state);
 	}
 
+	/*
+	 * If device has _PLD, initialize the 'pld' struct
+	 */
+	if (acpi_has_method(dev->handle, "_PLD"))
+		acpi_get_physical_device_location(dev->handle,
+						  &dev->pld);
+
 end:
 	return result;
 }

@@ -806,6 +813,12 @@ static void acpi_device_remove_files(struct acpi_device *dev)
 		device_remove_file(&dev->dev, &dev_attr_status);
 	if (dev->handle)
 		device_remove_file(&dev->dev, &dev_attr_path);
+
+	/*
+	 * If device has _PLD, free 'pld' struct
+	 */
+	if (dev->pld)
+		ACPI_FREE(dev->pld);
 }
 /* --------------------------------------------------------------------------
 			ACPI Bus operations

drivers/usb/core/usb-acpi.c +5 -9

index 2776cfe..940b3e5 100644
@@ -157,9 +157,6 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
 	} else if (is_usb_port(dev)) {
 		struct usb_port *port_dev = to_usb_port(dev);
 		int port1 = port_dev->portnum;
-		struct acpi_pld_info *pld;
-		acpi_handle *handle;
-		acpi_status status;
 
 		/* Get the struct usb_device point of port's hub */
 		udev = to_usb_device(dev->parent->parent);

@@ -190,15 +187,14 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
 			if (!adev)
 				return NULL;
 		}
-		handle = adev->handle;
-		status = acpi_get_physical_device_location(handle, &pld);
-		if (ACPI_FAILURE(status) || !pld)
+		if (!adev->pld)
 			return adev;
 
 		port_dev->location = USB_ACPI_LOCATION_VALID
-			| pld->group_token << 8 | pld->group_position;
-		port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
-		ACPI_FREE(pld);
+			| adev->pld->group_token << 8
+			| adev->pld->group_position;
+		port_dev->connect_type = usb_acpi_get_connect_type(adev->handle,
+								   adev->pld);
 
 		return adev;
 	}

include/acpi/acpi_bus.h +1 -0

index bcfd808..e439cfa 100644
@@ -357,6 +357,7 @@ struct acpi_device {
 	struct acpi_scan_handler *handler;
 	struct acpi_hotplug_context *hp;
 	struct acpi_driver *driver;
+	struct acpi_pld_info *pld;
 	void *driver_data;
 	struct device dev;
 	unsigned int physical_node_count;

Signature · 1 line

2.0.0

Samuel Ortiz

author

2014-08-13 17:18 UTC #1 lore raw

PATCH v4 2/2 ACPI: Export PLD (Physical Location of Device)

From: Faouaz TENOUTIT <[email protected]>

This patch exports a sysfs directory with ACPI _PLD information:

$ ls -l /sys/bus/acpi/devices/ACPI-DEV/pld
-r--r--r-- root     root         4096 2014-05-30 08:39 panel
-r--r--r-- root     root         4096 2014-05-30 08:39 revision
-r--r--r-- root     root         4096 2014-05-30 08:39 rotation
-r--r--r-- root     root         4096 2014-05-30 08:39 shape

This information can be used by user applications to:
- Determine which specific connector or device input mechanism may be used for a given task.
- Describes which panel surface of the system's housing the device connection point resides on (Front, Back, ...)

More information about these _PLD fields can be found here:
Documentation/ABI/testing/sysfs-bus-acpi

Signed-off-by Faouaz TENOUTIT <[email protected]>

Acked-by Samuel Ortiz <[email protected]>

Acked-by Andrew J Ross <[email protected]>

---
 Documentation/ABI/testing/sysfs-bus-acpi | 60 ++++++++++++++++++++++++++++++++
 drivers/acpi/scan.c                      | 53 ++++++++++++++++++++++++----
 2 files changed, 107 insertions(+), 6 deletions(-)

Documentation/ABI/testing/sysfs-bus-acpi +60 -0

index 7fa9cbc..be58159 100644
@@ -56,3 +56,63 @@ Description:
 		Writing 1 to this attribute will trigger hot removal of
 		this device object.  This file exists for every device
 		object that has _EJ0 method.
+
+What:		/sys/bus/acpi/devices/.../pld
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		This optional directory provides description of the physical
+		location, orientation and position of external devices.
+		It is for example used to describe how camera sensors are
+		physically mounted and allows user space software to rotate
+		images accordingly.
+
+What:		/sys/bus/acpi/devices/.../pld/revision
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		The current Revision is 0x2
+
+What:		/sys/bus/acpi/devices/.../pld/panel
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		Describes which panel surface of the system's housing the device
+		connection point resides on:
+		0 Top
+		1 Bottom
+		2 Left
+		3 Right
+		4 Front
+		5 Back
+		6 Unknown
+
+What:		/sys/bus/acpi/devices/.../pld/shape
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		Describes the shape of the device connection point:
+		0 Round
+		1 Oval
+		2 Square
+		3 Vertical Rectangle
+		4 Horizontal Rectangle
+		5 Vertical Trapezoid
+		6 Horizontal Trapezoid
+		7 Unknown - Shape rendered as a Rectangle with dotted lines
+		8 Chamfered
+
+What:		/sys/bus/acpi/devices/.../pld/rotation
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		Rotates the Shape clockwise in 45 degree steps around its
+		origin where:
+		0 0°
+		1 45°
+		2 90°
+		3 135°
+		4 180°
+		5 225°
+		6 270°
+		7 315°

drivers/acpi/scan.c +47 -6

74 lines not on this page · raw message

Signature · 6 lines

2.0.0

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Greg Kroah-Hartman

maintainer

2014-08-13 22:06 UTC #2 lore raw

Re: [PATCH v4 2/2] ACPI: Export PLD (Physical Location of Device)

Quoted 142 lines · Samuel Ortiz

From: Faouaz TENOUTIT <[email protected]>

This patch exports a sysfs directory with ACPI _PLD information:

$ ls -l /sys/bus/acpi/devices/ACPI-DEV/pld

(quoted diff) +0 -4

-r--r--r-- root     root         4096 2014-05-30 08:39 panel
-r--r--r-- root     root         4096 2014-05-30 08:39 revision
-r--r--r-- root     root         4096 2014-05-30 08:39 rotation
-r--r--r-- root     root         4096 2014-05-30 08:39 shape

More information about these _PLD fields can be found here:
Documentation/ABI/testing/sysfs-bus-acpi

Signed-off-by Faouaz TENOUTIT <[email protected]>

Acked-by Samuel Ortiz <[email protected]>

Acked-by Andrew J Ross <[email protected]>

---
 Documentation/ABI/testing/sysfs-bus-acpi | 60 ++++++++++++++++++++++++++++++++
 drivers/acpi/scan.c                      | 53 ++++++++++++++++++++++++----
 2 files changed, 107 insertions(+), 6 deletions(-)

Documentation/ABI/testing/sysfs-bus-acpi +60 -0

index 7fa9cbc..be58159 100644
@@ -56,3 +56,63 @@ Description:
 		Writing 1 to this attribute will trigger hot removal of
 		this device object.  This file exists for every device
 		object that has _EJ0 method.
+
+What:		/sys/bus/acpi/devices/.../pld
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		This optional directory provides description of the physical
+		location, orientation and position of external devices.
+		It is for example used to describe how camera sensors are
+		physically mounted and allows user space software to rotate
+		images accordingly.
+
+What:		/sys/bus/acpi/devices/.../pld/revision
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		The current Revision is 0x2
+
+What:		/sys/bus/acpi/devices/.../pld/panel
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		Describes which panel surface of the system's housing the device
+		connection point resides on:
+		0 Top
+		1 Bottom
+		2 Left
+		3 Right
+		4 Front
+		5 Back
+		6 Unknown
+
+What:		/sys/bus/acpi/devices/.../pld/shape
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		Describes the shape of the device connection point:
+		0 Round
+		1 Oval
+		2 Square
+		3 Vertical Rectangle
+		4 Horizontal Rectangle
+		5 Vertical Trapezoid
+		6 Horizontal Trapezoid
+		7 Unknown - Shape rendered as a Rectangle with dotted lines
+		8 Chamfered
+
+What:		/sys/bus/acpi/devices/.../pld/rotation
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		Rotates the Shape clockwise in 45 degree steps around its
+		origin where:
+		0 0°
+		1 45°
+		2 90°
+		3 135°
+		4 180°
+		5 225°
+		6 270°
+		7 315°

drivers/acpi/scan.c +31 -1

index 2ca42d5..19889a3 100644
@@ -686,6 +686,36 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(status);
 
+#define ACPI_SYSFS_PLD_PROP(prop)				\
+	static ssize_t prop##_show(struct device *dev,		\
+			   struct device_attribute *attr,	\
+			   char *buf) {				\
+	struct acpi_device *acpi_dev = to_acpi_device(dev);     \
+	return sprintf(buf, "%d\n", acpi_dev->pld->prop);       \
+};							        \
+static DEVICE_ATTR_RO(prop)
+
+/*
+ * sysfs PLD parameters
+ */
+ACPI_SYSFS_PLD_PROP(revision);
+ACPI_SYSFS_PLD_PROP(panel);
+ACPI_SYSFS_PLD_PROP(shape);
+ACPI_SYSFS_PLD_PROP(rotation);
+
+static struct attribute *acpi_pld_attrs[] = {
+	&dev_attr_revision.attr,
+	&dev_attr_panel.attr,
+	&dev_attr_shape.attr,
+	&dev_attr_rotation.attr,
+	NULL,
+};
+
+static const struct attribute_group acpi_pld_attr_group = {
+	.name = "pld",
+	.attrs = acpi_pld_attrs,
+};
+
 static int acpi_device_setup_files(struct acpi_device *dev)
 {
 	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};

@@ -768,11 +798,20 @@ static int acpi_device_setup_files(struct acpi_device *dev)
 	}
 
 	/*
-	 * If device has _PLD, initialize the 'pld' struct
+	 * If device has _PLD, 'pld' directory is created

drivers/acpi/scan.c static int acpi_device_setup_files(struct acpi_device *dev) · line 808

Quoted 10 lines

drivers/acpi/scan.c +6 -3

 	 */
-	if (acpi_has_method(dev->handle, "_PLD"))
-		acpi_get_physical_device_location(dev->handle,
-						  &dev->pld);
+	if (acpi_has_method(dev->handle, "_PLD")) {
+		status = acpi_get_physical_device_location(dev->handle,
+							   &dev->pld);
+		if (ACPI_SUCCESS(status)) {
+			result = sysfs_create_group(&dev->dev.kobj,
+						    &acpi_pld_attr_group);

It's a device, why not call device_create_groups()?

drivers/acpi/scan.c static int acpi_device_setup_files(struct acpi_device *dev) · line 814

Quoted 6 lines

drivers/acpi/scan.c +6 -0

+			if (result) {
+				ACPI_FREE(dev->pld);
+				dev->pld = NULL;
+			}
+		}
+	}

Is this being called after or before the device in question is registered with the driver core?

Hint, I think I know the answer, but I want you to learn why this code is wrong and will not work properly...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki

2014-08-13 23:56 UTC #3 lore raw

Re: [PATCH v4 2/2] ACPI: Export PLD (Physical Location of Device)

Quoted 167 lines · Greg Kroah-Hartman

Quoted 142 lines · Samuel Ortiz

From: Faouaz TENOUTIT <[email protected]>

This patch exports a sysfs directory with ACPI _PLD information:

$ ls -l /sys/bus/acpi/devices/ACPI-DEV/pld

(quoted diff) +0 -4

-r--r--r-- root     root         4096 2014-05-30 08:39 panel
-r--r--r-- root     root         4096 2014-05-30 08:39 revision
-r--r--r-- root     root         4096 2014-05-30 08:39 rotation
-r--r--r-- root     root         4096 2014-05-30 08:39 shape

More information about these _PLD fields can be found here:
Documentation/ABI/testing/sysfs-bus-acpi

Signed-off-by Faouaz TENOUTIT <[email protected]>

Acked-by Samuel Ortiz <[email protected]>

Acked-by Andrew J Ross <[email protected]>

---
 Documentation/ABI/testing/sysfs-bus-acpi | 60 ++++++++++++++++++++++++++++++++
 drivers/acpi/scan.c                      | 53 ++++++++++++++++++++++++----
 2 files changed, 107 insertions(+), 6 deletions(-)

Documentation/ABI/testing/sysfs-bus-acpi +60 -0

index 7fa9cbc..be58159 100644
@@ -56,3 +56,63 @@ Description:
 		Writing 1 to this attribute will trigger hot removal of
 		this device object.  This file exists for every device
 		object that has _EJ0 method.
+
+What:		/sys/bus/acpi/devices/.../pld
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		This optional directory provides description of the physical
+		location, orientation and position of external devices.
+		It is for example used to describe how camera sensors are
+		physically mounted and allows user space software to rotate
+		images accordingly.
+
+What:		/sys/bus/acpi/devices/.../pld/revision
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		The current Revision is 0x2
+
+What:		/sys/bus/acpi/devices/.../pld/panel
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		Describes which panel surface of the system's housing the device
+		connection point resides on:
+		0 Top
+		1 Bottom
+		2 Left
+		3 Right
+		4 Front
+		5 Back
+		6 Unknown
+
+What:		/sys/bus/acpi/devices/.../pld/shape
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		Describes the shape of the device connection point:
+		0 Round
+		1 Oval
+		2 Square
+		3 Vertical Rectangle
+		4 Horizontal Rectangle
+		5 Vertical Trapezoid
+		6 Horizontal Trapezoid
+		7 Unknown - Shape rendered as a Rectangle with dotted lines
+		8 Chamfered
+
+What:		/sys/bus/acpi/devices/.../pld/rotation
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		Rotates the Shape clockwise in 45 degree steps around its
+		origin where:
+		0 0°
+		1 45°
+		2 90°
+		3 135°
+		4 180°
+		5 225°
+		6 270°
+		7 315°

drivers/acpi/scan.c +31 -1

index 2ca42d5..19889a3 100644
@@ -686,6 +686,36 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(status);
 
+#define ACPI_SYSFS_PLD_PROP(prop)				\
+	static ssize_t prop##_show(struct device *dev,		\
+			   struct device_attribute *attr,	\
+			   char *buf) {				\
+	struct acpi_device *acpi_dev = to_acpi_device(dev);     \
+	return sprintf(buf, "%d\n", acpi_dev->pld->prop);       \
+};							        \
+static DEVICE_ATTR_RO(prop)
+
+/*
+ * sysfs PLD parameters
+ */
+ACPI_SYSFS_PLD_PROP(revision);
+ACPI_SYSFS_PLD_PROP(panel);
+ACPI_SYSFS_PLD_PROP(shape);
+ACPI_SYSFS_PLD_PROP(rotation);
+
+static struct attribute *acpi_pld_attrs[] = {
+	&dev_attr_revision.attr,
+	&dev_attr_panel.attr,
+	&dev_attr_shape.attr,
+	&dev_attr_rotation.attr,
+	NULL,
+};
+
+static const struct attribute_group acpi_pld_attr_group = {
+	.name = "pld",
+	.attrs = acpi_pld_attrs,
+};
+
 static int acpi_device_setup_files(struct acpi_device *dev)
 {
 	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};

@@ -768,11 +798,20 @@ static int acpi_device_setup_files(struct acpi_device *dev)
 	}
 
 	/*
-	 * If device has _PLD, initialize the 'pld' struct
+	 * If device has _PLD, 'pld' directory is created

drivers/acpi/scan.c static int acpi_device_setup_files(struct acpi_device *dev) · line 808

Quoted 10 lines

drivers/acpi/scan.c +6 -3

 	 */
-	if (acpi_has_method(dev->handle, "_PLD"))
-		acpi_get_physical_device_location(dev->handle,
-						  &dev->pld);
+	if (acpi_has_method(dev->handle, "_PLD")) {
+		status = acpi_get_physical_device_location(dev->handle,
+							   &dev->pld);
+		if (ACPI_SUCCESS(status)) {
+			result = sysfs_create_group(&dev->dev.kobj,
+						    &acpi_pld_attr_group);

It's a device, why not call device_create_groups()?

drivers/acpi/scan.c static int acpi_device_setup_files(struct acpi_device *dev) · line 814

Quoted 6 lines

drivers/acpi/scan.c +6 -0

+			if (result) {
+				ACPI_FREE(dev->pld);
+				dev->pld = NULL;
+			}
+		}
+	}

Is this being called after or before the device in question is registered with the driver core?

Quoted 2 lines

Hint, I think I know the answer, but I want you to learn why this code is wrong and will not work properly...

It will work. There are other groups created in the same function and ACPI has a deferred KOBJ_ADD notification.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Greg Kroah-Hartman

maintainer

2014-08-14 00:46 UTC #4 lore raw

Re: [PATCH v4 2/2] ACPI: Export PLD (Physical Location of Device)

Quoted 168 lines · Rafael J. Wysocki

Quoted 167 lines · Greg Kroah-Hartman

Quoted 142 lines · Samuel Ortiz

From: Faouaz TENOUTIT <[email protected]>

This patch exports a sysfs directory with ACPI _PLD information:

$ ls -l /sys/bus/acpi/devices/ACPI-DEV/pld

(quoted diff) +0 -4

-r--r--r-- root     root         4096 2014-05-30 08:39 panel
-r--r--r-- root     root         4096 2014-05-30 08:39 revision
-r--r--r-- root     root         4096 2014-05-30 08:39 rotation
-r--r--r-- root     root         4096 2014-05-30 08:39 shape

More information about these _PLD fields can be found here:
Documentation/ABI/testing/sysfs-bus-acpi

Signed-off-by Faouaz TENOUTIT <[email protected]>

Acked-by Samuel Ortiz <[email protected]>

Acked-by Andrew J Ross <[email protected]>

---
 Documentation/ABI/testing/sysfs-bus-acpi | 60 ++++++++++++++++++++++++++++++++
 drivers/acpi/scan.c                      | 53 ++++++++++++++++++++++++----
 2 files changed, 107 insertions(+), 6 deletions(-)

Documentation/ABI/testing/sysfs-bus-acpi +60 -0

index 7fa9cbc..be58159 100644
@@ -56,3 +56,63 @@ Description:
 		Writing 1 to this attribute will trigger hot removal of
 		this device object.  This file exists for every device
 		object that has _EJ0 method.
+
+What:		/sys/bus/acpi/devices/.../pld
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		This optional directory provides description of the physical
+		location, orientation and position of external devices.
+		It is for example used to describe how camera sensors are
+		physically mounted and allows user space software to rotate
+		images accordingly.
+
+What:		/sys/bus/acpi/devices/.../pld/revision
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		The current Revision is 0x2
+
+What:		/sys/bus/acpi/devices/.../pld/panel
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		Describes which panel surface of the system's housing the device
+		connection point resides on:
+		0 Top
+		1 Bottom
+		2 Left
+		3 Right
+		4 Front
+		5 Back
+		6 Unknown
+
+What:		/sys/bus/acpi/devices/.../pld/shape
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		Describes the shape of the device connection point:
+		0 Round
+		1 Oval
+		2 Square
+		3 Vertical Rectangle
+		4 Horizontal Rectangle
+		5 Vertical Trapezoid
+		6 Horizontal Trapezoid
+		7 Unknown - Shape rendered as a Rectangle with dotted lines
+		8 Chamfered
+
+What:		/sys/bus/acpi/devices/.../pld/rotation
+Date:		August 2014
+Contact:	Faouaz Tenoutit <[email protected]>
+Description:
+		Rotates the Shape clockwise in 45 degree steps around its
+		origin where:
+		0 0°
+		1 45°
+		2 90°
+		3 135°
+		4 180°
+		5 225°
+		6 270°
+		7 315°

drivers/acpi/scan.c +31 -1

index 2ca42d5..19889a3 100644
@@ -686,6 +686,36 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(status);
 
+#define ACPI_SYSFS_PLD_PROP(prop)				\
+	static ssize_t prop##_show(struct device *dev,		\
+			   struct device_attribute *attr,	\
+			   char *buf) {				\
+	struct acpi_device *acpi_dev = to_acpi_device(dev);     \
+	return sprintf(buf, "%d\n", acpi_dev->pld->prop);       \
+};							        \
+static DEVICE_ATTR_RO(prop)
+
+/*
+ * sysfs PLD parameters
+ */
+ACPI_SYSFS_PLD_PROP(revision);
+ACPI_SYSFS_PLD_PROP(panel);
+ACPI_SYSFS_PLD_PROP(shape);
+ACPI_SYSFS_PLD_PROP(rotation);
+
+static struct attribute *acpi_pld_attrs[] = {
+	&dev_attr_revision.attr,
+	&dev_attr_panel.attr,
+	&dev_attr_shape.attr,
+	&dev_attr_rotation.attr,
+	NULL,
+};
+
+static const struct attribute_group acpi_pld_attr_group = {
+	.name = "pld",
+	.attrs = acpi_pld_attrs,
+};
+
 static int acpi_device_setup_files(struct acpi_device *dev)
 {
 	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};

@@ -768,11 +798,20 @@ static int acpi_device_setup_files(struct acpi_device *dev)
 	}
 
 	/*
-	 * If device has _PLD, initialize the 'pld' struct
+	 * If device has _PLD, 'pld' directory is created

drivers/acpi/scan.c static int acpi_device_setup_files(struct acpi_device *dev) · line 808

Quoted 10 lines

drivers/acpi/scan.c +6 -3

 	 */
-	if (acpi_has_method(dev->handle, "_PLD"))
-		acpi_get_physical_device_location(dev->handle,
-						  &dev->pld);
+	if (acpi_has_method(dev->handle, "_PLD")) {
+		status = acpi_get_physical_device_location(dev->handle,
+							   &dev->pld);
+		if (ACPI_SUCCESS(status)) {
+			result = sysfs_create_group(&dev->dev.kobj,
+						    &acpi_pld_attr_group);

It's a device, why not call device_create_groups()?

drivers/acpi/scan.c static int acpi_device_setup_files(struct acpi_device *dev) · line 814

Quoted 6 lines

drivers/acpi/scan.c +6 -0

+			if (result) {
+				ACPI_FREE(dev->pld);
+				dev->pld = NULL;
+			}
+		}
+	}

Is this being called after or before the device in question is registered with the driver core?

Quoted 5 lines

Quoted 2 lines
Hint, I think I know the answer, but I want you to learn why this code is wrong and will not work properly...
It will work. There are other groups created in the same function and ACPI has a deferred KOBJ_ADD notification.

Ah, good, nevermind then.

Well, I'm still curious about the sysfs_create_group() call, please try to use device_* functions when dealing with a struct device, if at all possible.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Greg Kroah-Hartman

maintainer

2014-08-13 22:03 UTC #5 lore raw

In reply to Samuel Ortiz

Quoted 4 lines · Samuel Ortiz

From: Faouaz TENOUTIT <[email protected]>
Use ACPI device pld information instead of calling acpi_get_physical_device_location.

Why? What does this do "better" than the existing code?

thanks,

gre gk-h

Rafael J. Wysocki

2014-08-13 23:59 UTC #6 lore raw

Quoted 7 lines · Greg Kroah-Hartman

Quoted 4 lines · Samuel Ortiz
From: Faouaz TENOUTIT <[email protected]>
Use ACPI device pld information instead of calling acpi_get_physical_device_location.
Why? What does this do "better" than the existing code?

Well, you've acked this patch before. :-)

It just doesn't execute AML from within the USB core. I thought that would be a sufficient reason, wouldn't it?

Rafael

Greg Kroah-Hartman

maintainer

2014-08-14 00:46 UTC #7 lore raw

Quoted 10 lines · Rafael J. Wysocki

Quoted 7 lines · Greg Kroah-Hartman
Quoted 4 lines · Samuel Ortiz
From: Faouaz TENOUTIT <[email protected]>
Use ACPI device pld information instead of calling acpi_get_physical_device_location.
Why? What does this do "better" than the existing code?
Well, you've acked this patch before. :-)

I can't remember what I ate for Lunch yesterday, let alone previous patches I've acked :)

Quoted 2 lines

It just doesn't execute AML from within the USB core. I thought that would be a sufficient reason, wouldn't it?

Yes, that's fine, but a little more description of "why" would have been helpful here...

thanks,

greg k-h

Rafael J. Wysocki

2014-08-14 23:44 UTC #8 lore raw

Quoted 12 lines · Greg Kroah-Hartman

Quoted 10 lines · Rafael J. Wysocki
Quoted 7 lines · Greg Kroah-Hartman
Quoted 4 lines · Samuel Ortiz
From: Faouaz TENOUTIT <[email protected]>
Use ACPI device pld information instead of calling acpi_get_physical_device_location.
Why? What does this do "better" than the existing code?
Well, you've acked this patch before. :-)

Quoted 8 lines

I can't remember what I ate for Lunch yesterday, let alone previous patches I've acked :)
Quoted 2 lines
It just doesn't execute AML from within the USB core. I thought that would be a sufficient reason, wouldn't it?
Yes, that's fine, but a little more description of "why" would have been helpful here...

Since we are going to export _PLD output via sysfs ([PATCH 2/2]) and it's guaranteed to return the same data every time it is evaluated, it simply makes more sense to evaluate it once and cache the data for when it's needed. This particular one makes USB use the cached data.

Rafael

Rafael J. Wysocki

2014-09-03 21:43 UTC #9 lore raw

In reply to Samuel Ortiz

Quoted 8 lines · Samuel Ortiz

From: Faouaz TENOUTIT <[email protected]>
Use ACPI device pld information instead of calling acpi_get_physical_device_location.
Signed-off-by Faouaz TENOUTIT <[email protected]>
Acked-by Samuel Ortiz <[email protected]>
Acked-by Greg Kroah-Hartman <[email protected]>

There is a problem with this approach I've been overlooking, sorry about that.

Namely, the information returned by _PLD is generally not guaranteed to be the same every time that object is evaluated, so either we shouldn't cache it, or we should change the cached data in response to specific device notifications as described in the _PLD section of the spec.

Quoted 85 lines

---
 drivers/acpi/scan.c         | 13 +++++++++++++
 drivers/usb/core/usb-acpi.c | 14 +++++---------
 include/acpi/acpi_bus.h     |  1 +
 3 files changed, 19 insertions(+), 9 deletions(-)

drivers/acpi/scan.c +13 -0

index 0a817ad..2ca42d5 100644
@@ -767,6 +767,13 @@ static int acpi_device_setup_files(struct acpi_device *dev)
 						    &dev_attr_real_power_state);
 	}
 
+	/*
+	 * If device has _PLD, initialize the 'pld' struct
+	 */
+	if (acpi_has_method(dev->handle, "_PLD"))
+		acpi_get_physical_device_location(dev->handle,
+						  &dev->pld);
+
 end:
 	return result;
 }

@@ -806,6 +813,12 @@ static void acpi_device_remove_files(struct acpi_device *dev)
 		device_remove_file(&dev->dev, &dev_attr_status);
 	if (dev->handle)
 		device_remove_file(&dev->dev, &dev_attr_path);
+
+	/*
+	 * If device has _PLD, free 'pld' struct
+	 */
+	if (dev->pld)
+		ACPI_FREE(dev->pld);
 }
 /* --------------------------------------------------------------------------
 			ACPI Bus operations

drivers/usb/core/usb-acpi.c +5 -9

index 2776cfe..940b3e5 100644
@@ -157,9 +157,6 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
 	} else if (is_usb_port(dev)) {
 		struct usb_port *port_dev = to_usb_port(dev);
 		int port1 = port_dev->portnum;
-		struct acpi_pld_info *pld;
-		acpi_handle *handle;
-		acpi_status status;
 
 		/* Get the struct usb_device point of port's hub */
 		udev = to_usb_device(dev->parent->parent);

@@ -190,15 +187,14 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
 			if (!adev)
 				return NULL;
 		}
-		handle = adev->handle;
-		status = acpi_get_physical_device_location(handle, &pld);
-		if (ACPI_FAILURE(status) || !pld)
+		if (!adev->pld)
 			return adev;
 
 		port_dev->location = USB_ACPI_LOCATION_VALID
-			| pld->group_token << 8 | pld->group_position;
-		port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
-		ACPI_FREE(pld);
+			| adev->pld->group_token << 8
+			| adev->pld->group_position;
+		port_dev->connect_type = usb_acpi_get_connect_type(adev->handle,
+								   adev->pld);
 
 		return adev;
 	}

include/acpi/acpi_bus.h +1 -0

index bcfd808..e439cfa 100644
@@ -357,6 +357,7 @@ struct acpi_device {
 	struct acpi_scan_handler *handler;
 	struct acpi_hotplug_context *hp;
 	struct acpi_driver *driver;
+	struct acpi_pld_info *pld;
 	void *driver_data;
 	struct device dev;
 	unsigned int physical_node_count;

Signature · 2 lines

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