When ACPI table includes _PLD fields for a device, create a new
directory (pld) in sysfs to share _PLD fields.
Signed-off-by: Won Chung <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-acpi | 107 +++++++++++++++++++++++
drivers/acpi/device_sysfs.c | 55 ++++++++++++
include/acpi/acpi_bus.h | 1 +
3 files changed, 163 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
index 58abacf59b2a..b8b71c8f3cfd 100644
--- a/Documentation/ABI/testing/sysfs-bus-acpi
+++ b/Documentation/ABI/testing/sysfs-bus-acpi
@@ -96,3 +96,110 @@ Description:
hardware, if the _HRV control method is present. It is mostly
useful for non-PCI devices because lspci can list the hardware
version for PCI devices.
+
+What: /sys/bus/acpi/devices/.../pld/
+Date: Feb, 2022
+Contact: Won Chung <[email protected]>
+Description:
+ This directory contains the output of the device object's _PLD
+ control method, if present. This information provides details
+ on physical location of a device.
+
+What: /sys/bus/acpi/devices/.../pld/revision
+Date: Feb, 2022
+Contact: Won Chung <[email protected]>
+Description:
+ The current revision is 0x2.
+
+What: /sys/bus/acpi/devices/.../pld/group_token
+Date: Feb, 2022
+Contact: Won Chung <[email protected]>
+Description:
+ Unique numerical value identifying a group.
+
+What: /sys/bus/acpi/devices/.../pld/group_position
+Date: Feb, 2022
+Contact: Won Chung <[email protected]>
+Description:
+ Identifies this device connection point’s position in the group.
+
+What: /sys/bus/acpi/devices/.../pld/user_visible
+Date: Feb, 2022
+Contact: Won Chung <[email protected]>
+Description:
+ Set if the device connection point can be seen by the user
+ without disassembly.
+
+What: /sys/bus/acpi/devices/.../pld/dock
+Date: Feb, 2022
+Contact: Won Chung <[email protected]>
+Description:
+ Set if the device connection point resides in a docking station
+ or port replicator.
+
+What: /sys/bus/acpi/devices/.../pld/bay
+Date: Feb, 2022
+Contact: Won Chung <[email protected]>
+Description:
+ Set if describing a device in a bay or if device connection
+ point is a bay.
+
+What: /sys/bus/acpi/devices/.../pld/lid
+Date: Feb, 2022
+Contact: Won Chung <[email protected]>
+Description:
+ Set if this device connection point resides on the lid of
+ laptop system.
+
+What: /sys/bus/acpi/devices/.../pld/panel
+Date: Feb, 2022
+Contact: Won Chung <[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 (Vertical Position and Horizontal Position will be
+ ignored)
+
+What: /sys/bus/acpi/devices/.../pld/vertical_position
+Date: Feb, 2022
+Contact: Won Chung <[email protected]>
+Description:
+ 0 - Upper
+ 1 - Center
+ 2 - Lower
+
+What: /sys/bus/acpi/devices/.../pld/horizontal_position
+Date: Feb, 2022
+Contact: Won Chung <[email protected]>
+Description:
+ ACPI specification does not define horizontal position field.
+ Can be used as either
+ 0 - Left
+ 1 - Center
+ 2 - Right
+ or
+ 0 - Leftmost
+ and higher numbers going toward the right.
+
+What: /sys/bus/acpi/devices/.../pld/shape
+Date: Feb, 2022
+Contact: Won Chung <[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
+ 15:9 - Reserved
+
diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index d5d6403ba07b..610be93635a0 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -509,6 +509,49 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
}
static DEVICE_ATTR_RO(status);
+#define DEV_ATTR_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); \
+ if (acpi_dev->pld == NULL) \
+ return -EIO; \
+ return sprintf(buf, "%u\n", acpi_dev->pld->prop); \
+}; \
+static DEVICE_ATTR_RO(prop)
+
+DEV_ATTR_PLD_PROP(revision);
+DEV_ATTR_PLD_PROP(group_token);
+DEV_ATTR_PLD_PROP(group_position);
+DEV_ATTR_PLD_PROP(user_visible);
+DEV_ATTR_PLD_PROP(dock);
+DEV_ATTR_PLD_PROP(bay);
+DEV_ATTR_PLD_PROP(lid);
+DEV_ATTR_PLD_PROP(panel);
+DEV_ATTR_PLD_PROP(vertical_position);
+DEV_ATTR_PLD_PROP(horizontal_position);
+DEV_ATTR_PLD_PROP(shape);
+
+static struct attribute *dev_attr_pld[] = {
+ &dev_attr_revision.attr,
+ &dev_attr_group_token.attr,
+ &dev_attr_group_position.attr,
+ &dev_attr_user_visible.attr,
+ &dev_attr_dock.attr,
+ &dev_attr_bay.attr,
+ &dev_attr_lid.attr,
+ &dev_attr_panel.attr,
+ &dev_attr_vertical_position.attr,
+ &dev_attr_horizontal_position.attr,
+ &dev_attr_shape.attr,
+ NULL,
+};
+
+static struct attribute_group dev_attr_pld_group = {
+ .name = "pld",
+ .attrs = dev_attr_pld,
+};
+
/**
* acpi_device_setup_files - Create sysfs attributes of an ACPI device.
* @dev: ACPI device object.
@@ -595,6 +638,16 @@ int acpi_device_setup_files(struct acpi_device *dev)
&dev_attr_real_power_state);
}
+ if (acpi_has_method(dev->handle, "_PLD")) {
+ status = acpi_get_physical_device_location(dev->handle,
+ &dev->pld);
+ if (ACPI_FAILURE(status))
+ goto end;
+ result = device_add_group(&dev->dev, &dev_attr_pld_group);
+ if (result)
+ goto end;
+ }
+
acpi_expose_nondev_subnodes(&dev->dev.kobj, &dev->data);
end:
@@ -645,4 +698,6 @@ 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 (acpi_has_method(dev->handle, "_PLD"))
+ device_remove_group(&dev->dev, &dev_attr_pld_group);
}
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index ca88c4706f2b..929e726a666b 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -381,6 +381,7 @@ struct acpi_device {
struct acpi_hotplug_context *hp;
struct acpi_driver *driver;
const struct acpi_gpio_mapping *driver_gpios;
+ struct acpi_pld_info *pld;
void *driver_data;
struct device dev;
unsigned int physical_node_count;
--
2.35.0.rc2.247.g8bbb082509-goog
Hi Won,
On Tue, Feb 01, 2022 at 01:55:18AM +0000, Won Chung wrote:
> When ACPI table includes _PLD fields for a device, create a new
> directory (pld) in sysfs to share _PLD fields.
I think you need to explain what needs this information in user space.
> Signed-off-by: Won Chung <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-bus-acpi | 107 +++++++++++++++++++++++
> drivers/acpi/device_sysfs.c | 55 ++++++++++++
> include/acpi/acpi_bus.h | 1 +
> 3 files changed, 163 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> index 58abacf59b2a..b8b71c8f3cfd 100644
> --- a/Documentation/ABI/testing/sysfs-bus-acpi
> +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> @@ -96,3 +96,110 @@ Description:
> hardware, if the _HRV control method is present. It is mostly
> useful for non-PCI devices because lspci can list the hardware
> version for PCI devices.
> +
> +What: /sys/bus/acpi/devices/.../pld/
> +Date: Feb, 2022
> +Contact: Won Chung <[email protected]>
> +Description:
> + This directory contains the output of the device object's _PLD
> + control method, if present. This information provides details
> + on physical location of a device.
> +
> +What: /sys/bus/acpi/devices/.../pld/revision
> +Date: Feb, 2022
> +Contact: Won Chung <[email protected]>
> +Description:
> + The current revision is 0x2.
> +
> +What: /sys/bus/acpi/devices/.../pld/group_token
> +Date: Feb, 2022
> +Contact: Won Chung <[email protected]>
> +Description:
> + Unique numerical value identifying a group.
> +
> +What: /sys/bus/acpi/devices/.../pld/group_position
> +Date: Feb, 2022
> +Contact: Won Chung <[email protected]>
> +Description:
> + Identifies this device connection point’s position in the group.
> +
> +What: /sys/bus/acpi/devices/.../pld/user_visible
> +Date: Feb, 2022
> +Contact: Won Chung <[email protected]>
> +Description:
> + Set if the device connection point can be seen by the user
> + without disassembly.
> +
> +What: /sys/bus/acpi/devices/.../pld/dock
> +Date: Feb, 2022
> +Contact: Won Chung <[email protected]>
> +Description:
> + Set if the device connection point resides in a docking station
> + or port replicator.
> +
> +What: /sys/bus/acpi/devices/.../pld/bay
> +Date: Feb, 2022
> +Contact: Won Chung <[email protected]>
> +Description:
> + Set if describing a device in a bay or if device connection
> + point is a bay.
> +
> +What: /sys/bus/acpi/devices/.../pld/lid
> +Date: Feb, 2022
> +Contact: Won Chung <[email protected]>
> +Description:
> + Set if this device connection point resides on the lid of
> + laptop system.
> +
> +What: /sys/bus/acpi/devices/.../pld/panel
> +Date: Feb, 2022
> +Contact: Won Chung <[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 (Vertical Position and Horizontal Position will be
> + ignored)
> +
> +What: /sys/bus/acpi/devices/.../pld/vertical_position
> +Date: Feb, 2022
> +Contact: Won Chung <[email protected]>
> +Description:
> + 0 - Upper
> + 1 - Center
> + 2 - Lower
> +
> +What: /sys/bus/acpi/devices/.../pld/horizontal_position
> +Date: Feb, 2022
> +Contact: Won Chung <[email protected]>
> +Description:
> + ACPI specification does not define horizontal position field.
> + Can be used as either
> + 0 - Left
> + 1 - Center
> + 2 - Right
> + or
> + 0 - Leftmost
> + and higher numbers going toward the right.
> +
> +What: /sys/bus/acpi/devices/.../pld/shape
> +Date: Feb, 2022
> +Contact: Won Chung <[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
> + 15:9 - Reserved
> +
> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index d5d6403ba07b..610be93635a0 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -509,6 +509,49 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> }
> static DEVICE_ATTR_RO(status);
>
> +#define DEV_ATTR_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); \
> + if (acpi_dev->pld == NULL) \
> + return -EIO; \
> + return sprintf(buf, "%u\n", acpi_dev->pld->prop); \
> +}; \
Ah, you are storing the _PLD below. Before there were concerns about
the memory that the cached _PLD information would consume. Another way
of doing this would be to just always evaluate the _PLD here.
Rafael needs to comment on this. My personal opinion is that let's
just store the thing.
> +static DEVICE_ATTR_RO(prop)
> +
> +DEV_ATTR_PLD_PROP(revision);
> +DEV_ATTR_PLD_PROP(group_token);
> +DEV_ATTR_PLD_PROP(group_position);
> +DEV_ATTR_PLD_PROP(user_visible);
> +DEV_ATTR_PLD_PROP(dock);
> +DEV_ATTR_PLD_PROP(bay);
> +DEV_ATTR_PLD_PROP(lid);
> +DEV_ATTR_PLD_PROP(panel);
> +DEV_ATTR_PLD_PROP(vertical_position);
> +DEV_ATTR_PLD_PROP(horizontal_position);
> +DEV_ATTR_PLD_PROP(shape);
> +
> +static struct attribute *dev_attr_pld[] = {
> + &dev_attr_revision.attr,
> + &dev_attr_group_token.attr,
> + &dev_attr_group_position.attr,
> + &dev_attr_user_visible.attr,
> + &dev_attr_dock.attr,
> + &dev_attr_bay.attr,
> + &dev_attr_lid.attr,
> + &dev_attr_panel.attr,
> + &dev_attr_vertical_position.attr,
> + &dev_attr_horizontal_position.attr,
> + &dev_attr_shape.attr,
> + NULL,
> +};
> +
> +static struct attribute_group dev_attr_pld_group = {
> + .name = "pld",
> + .attrs = dev_attr_pld,
> +};
> +
> /**
> * acpi_device_setup_files - Create sysfs attributes of an ACPI device.
> * @dev: ACPI device object.
> @@ -595,6 +638,16 @@ int acpi_device_setup_files(struct acpi_device *dev)
> &dev_attr_real_power_state);
> }
>
> + if (acpi_has_method(dev->handle, "_PLD")) {
> + status = acpi_get_physical_device_location(dev->handle,
> + &dev->pld);
> + if (ACPI_FAILURE(status))
> + goto end;
> + result = device_add_group(&dev->dev, &dev_attr_pld_group);
> + if (result)
> + goto end;
> + }
You probable want to store the pld in acpi_store_pld_crc(). Perhaps
also rename that function to just acpi_store_pld() at the same time.
Here you just want to create the sysfs group.
> acpi_expose_nondev_subnodes(&dev->dev.kobj, &dev->data);
>
> end:
> @@ -645,4 +698,6 @@ 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 (acpi_has_method(dev->handle, "_PLD"))
> + device_remove_group(&dev->dev, &dev_attr_pld_group);
> }
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index ca88c4706f2b..929e726a666b 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -381,6 +381,7 @@ struct acpi_device {
> struct acpi_hotplug_context *hp;
> struct acpi_driver *driver;
> const struct acpi_gpio_mapping *driver_gpios;
> + struct acpi_pld_info *pld;
> void *driver_data;
> struct device dev;
> unsigned int physical_node_count;
> --
thanks,
--
heikki
Hi Heikki,
Thank you for the review!
On Wed, Feb 2, 2022 at 12:19 AM Heikki Krogerus
<[email protected]> wrote:
>
> Hi Won,
>
> On Tue, Feb 01, 2022 at 01:55:18AM +0000, Won Chung wrote:
> > When ACPI table includes _PLD fields for a device, create a new
> > directory (pld) in sysfs to share _PLD fields.
>
> I think you need to explain what needs this information in user space.
>
> > Signed-off-by: Won Chung <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-bus-acpi | 107 +++++++++++++++++++++++
> > drivers/acpi/device_sysfs.c | 55 ++++++++++++
> > include/acpi/acpi_bus.h | 1 +
> > 3 files changed, 163 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> > index 58abacf59b2a..b8b71c8f3cfd 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-acpi
> > +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> > @@ -96,3 +96,110 @@ Description:
> > hardware, if the _HRV control method is present. It is mostly
> > useful for non-PCI devices because lspci can list the hardware
> > version for PCI devices.
> > +
> > +What: /sys/bus/acpi/devices/.../pld/
> > +Date: Feb, 2022
> > +Contact: Won Chung <[email protected]>
> > +Description:
> > + This directory contains the output of the device object's _PLD
> > + control method, if present. This information provides details
> > + on physical location of a device.
> > +
> > +What: /sys/bus/acpi/devices/.../pld/revision
> > +Date: Feb, 2022
> > +Contact: Won Chung <[email protected]>
> > +Description:
> > + The current revision is 0x2.
> > +
> > +What: /sys/bus/acpi/devices/.../pld/group_token
> > +Date: Feb, 2022
> > +Contact: Won Chung <[email protected]>
> > +Description:
> > + Unique numerical value identifying a group.
> > +
> > +What: /sys/bus/acpi/devices/.../pld/group_position
> > +Date: Feb, 2022
> > +Contact: Won Chung <[email protected]>
> > +Description:
> > + Identifies this device connection point’s position in the group.
> > +
> > +What: /sys/bus/acpi/devices/.../pld/user_visible
> > +Date: Feb, 2022
> > +Contact: Won Chung <[email protected]>
> > +Description:
> > + Set if the device connection point can be seen by the user
> > + without disassembly.
> > +
> > +What: /sys/bus/acpi/devices/.../pld/dock
> > +Date: Feb, 2022
> > +Contact: Won Chung <[email protected]>
> > +Description:
> > + Set if the device connection point resides in a docking station
> > + or port replicator.
> > +
> > +What: /sys/bus/acpi/devices/.../pld/bay
> > +Date: Feb, 2022
> > +Contact: Won Chung <[email protected]>
> > +Description:
> > + Set if describing a device in a bay or if device connection
> > + point is a bay.
> > +
> > +What: /sys/bus/acpi/devices/.../pld/lid
> > +Date: Feb, 2022
> > +Contact: Won Chung <[email protected]>
> > +Description:
> > + Set if this device connection point resides on the lid of
> > + laptop system.
> > +
> > +What: /sys/bus/acpi/devices/.../pld/panel
> > +Date: Feb, 2022
> > +Contact: Won Chung <[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 (Vertical Position and Horizontal Position will be
> > + ignored)
> > +
> > +What: /sys/bus/acpi/devices/.../pld/vertical_position
> > +Date: Feb, 2022
> > +Contact: Won Chung <[email protected]>
> > +Description:
> > + 0 - Upper
> > + 1 - Center
> > + 2 - Lower
> > +
> > +What: /sys/bus/acpi/devices/.../pld/horizontal_position
> > +Date: Feb, 2022
> > +Contact: Won Chung <[email protected]>
> > +Description:
> > + ACPI specification does not define horizontal position field.
> > + Can be used as either
> > + 0 - Left
> > + 1 - Center
> > + 2 - Right
> > + or
> > + 0 - Leftmost
> > + and higher numbers going toward the right.
> > +
> > +What: /sys/bus/acpi/devices/.../pld/shape
> > +Date: Feb, 2022
> > +Contact: Won Chung <[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
> > + 15:9 - Reserved
> > +
> > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> > index d5d6403ba07b..610be93635a0 100644
> > --- a/drivers/acpi/device_sysfs.c
> > +++ b/drivers/acpi/device_sysfs.c
> > @@ -509,6 +509,49 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> > }
> > static DEVICE_ATTR_RO(status);
> >
> > +#define DEV_ATTR_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); \
> > + if (acpi_dev->pld == NULL) \
> > + return -EIO; \
> > + return sprintf(buf, "%u\n", acpi_dev->pld->prop); \
> > +}; \
>
> Ah, you are storing the _PLD below. Before there were concerns about
> the memory that the cached _PLD information would consume. Another way
> of doing this would be to just always evaluate the _PLD here.
>
> Rafael needs to comment on this. My personal opinion is that let's
> just store the thing.
>
By "always evaluate the _PLD here", you mean something like
acpi_get_physical_device_location(dev->handle, &pld)
for every _PLD field, right?
I will wait for Rafael's comment on this.
> > +static DEVICE_ATTR_RO(prop)
> > +
> > +DEV_ATTR_PLD_PROP(revision);
> > +DEV_ATTR_PLD_PROP(group_token);
> > +DEV_ATTR_PLD_PROP(group_position);
> > +DEV_ATTR_PLD_PROP(user_visible);
> > +DEV_ATTR_PLD_PROP(dock);
> > +DEV_ATTR_PLD_PROP(bay);
> > +DEV_ATTR_PLD_PROP(lid);
> > +DEV_ATTR_PLD_PROP(panel);
> > +DEV_ATTR_PLD_PROP(vertical_position);
> > +DEV_ATTR_PLD_PROP(horizontal_position);
> > +DEV_ATTR_PLD_PROP(shape);
> > +
> > +static struct attribute *dev_attr_pld[] = {
> > + &dev_attr_revision.attr,
> > + &dev_attr_group_token.attr,
> > + &dev_attr_group_position.attr,
> > + &dev_attr_user_visible.attr,
> > + &dev_attr_dock.attr,
> > + &dev_attr_bay.attr,
> > + &dev_attr_lid.attr,
> > + &dev_attr_panel.attr,
> > + &dev_attr_vertical_position.attr,
> > + &dev_attr_horizontal_position.attr,
> > + &dev_attr_shape.attr,
> > + NULL,
> > +};
> > +
> > +static struct attribute_group dev_attr_pld_group = {
> > + .name = "pld",
> > + .attrs = dev_attr_pld,
> > +};
> > +
> > /**
> > * acpi_device_setup_files - Create sysfs attributes of an ACPI device.
> > * @dev: ACPI device object.
> > @@ -595,6 +638,16 @@ int acpi_device_setup_files(struct acpi_device *dev)
> > &dev_attr_real_power_state);
> > }
> >
> > + if (acpi_has_method(dev->handle, "_PLD")) {
> > + status = acpi_get_physical_device_location(dev->handle,
> > + &dev->pld);
> > + if (ACPI_FAILURE(status))
> > + goto end;
> > + result = device_add_group(&dev->dev, &dev_attr_pld_group);
> > + if (result)
> > + goto end;
> > + }
>
> You probable want to store the pld in acpi_store_pld_crc(). Perhaps
> also rename that function to just acpi_store_pld() at the same time.
>
> Here you just want to create the sysfs group.
>
Got it. I will send v5 with this change.
> > acpi_expose_nondev_subnodes(&dev->dev.kobj, &dev->data);
> >
> > end:
> > @@ -645,4 +698,6 @@ 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 (acpi_has_method(dev->handle, "_PLD"))
> > + device_remove_group(&dev->dev, &dev_attr_pld_group);
> > }
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > index ca88c4706f2b..929e726a666b 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -381,6 +381,7 @@ struct acpi_device {
> > struct acpi_hotplug_context *hp;
> > struct acpi_driver *driver;
> > const struct acpi_gpio_mapping *driver_gpios;
> > + struct acpi_pld_info *pld;
> > void *driver_data;
> > struct device dev;
> > unsigned int physical_node_count;
> > --
>
> thanks,
>
> --
> heikki
Thank you,
Won
On Wed, Feb 2, 2022 at 9:43 PM Won Chung <[email protected]> wrote:
>
> Hi Heikki,
>
> Thank you for the review!
>
> On Wed, Feb 2, 2022 at 12:19 AM Heikki Krogerus
> <[email protected]> wrote:
> >
> > Hi Won,
> >
> > On Tue, Feb 01, 2022 at 01:55:18AM +0000, Won Chung wrote:
> > > When ACPI table includes _PLD fields for a device, create a new
> > > directory (pld) in sysfs to share _PLD fields.
> >
> > I think you need to explain what needs this information in user space.
> >
> > > Signed-off-by: Won Chung <[email protected]>
> > > ---
> > > Documentation/ABI/testing/sysfs-bus-acpi | 107 +++++++++++++++++++++++
> > > drivers/acpi/device_sysfs.c | 55 ++++++++++++
> > > include/acpi/acpi_bus.h | 1 +
> > > 3 files changed, 163 insertions(+)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> > > index 58abacf59b2a..b8b71c8f3cfd 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-acpi
> > > +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> > > @@ -96,3 +96,110 @@ Description:
> > > hardware, if the _HRV control method is present. It is mostly
> > > useful for non-PCI devices because lspci can list the hardware
> > > version for PCI devices.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <[email protected]>
> > > +Description:
> > > + This directory contains the output of the device object's _PLD
> > > + control method, if present. This information provides details
> > > + on physical location of a device.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/revision
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <[email protected]>
> > > +Description:
> > > + The current revision is 0x2.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/group_token
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <[email protected]>
> > > +Description:
> > > + Unique numerical value identifying a group.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/group_position
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <[email protected]>
> > > +Description:
> > > + Identifies this device connection point’s position in the group.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/user_visible
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <[email protected]>
> > > +Description:
> > > + Set if the device connection point can be seen by the user
> > > + without disassembly.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/dock
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <[email protected]>
> > > +Description:
> > > + Set if the device connection point resides in a docking station
> > > + or port replicator.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/bay
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <[email protected]>
> > > +Description:
> > > + Set if describing a device in a bay or if device connection
> > > + point is a bay.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/lid
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <[email protected]>
> > > +Description:
> > > + Set if this device connection point resides on the lid of
> > > + laptop system.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/panel
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <[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 (Vertical Position and Horizontal Position will be
> > > + ignored)
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/vertical_position
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <[email protected]>
> > > +Description:
> > > + 0 - Upper
> > > + 1 - Center
> > > + 2 - Lower
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/horizontal_position
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <[email protected]>
> > > +Description:
> > > + ACPI specification does not define horizontal position field.
> > > + Can be used as either
> > > + 0 - Left
> > > + 1 - Center
> > > + 2 - Right
> > > + or
> > > + 0 - Leftmost
> > > + and higher numbers going toward the right.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/shape
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <[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
> > > + 15:9 - Reserved
> > > +
> > > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> > > index d5d6403ba07b..610be93635a0 100644
> > > --- a/drivers/acpi/device_sysfs.c
> > > +++ b/drivers/acpi/device_sysfs.c
> > > @@ -509,6 +509,49 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> > > }
> > > static DEVICE_ATTR_RO(status);
> > >
> > > +#define DEV_ATTR_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); \
> > > + if (acpi_dev->pld == NULL) \
> > > + return -EIO; \
> > > + return sprintf(buf, "%u\n", acpi_dev->pld->prop); \
> > > +}; \
> >
> > Ah, you are storing the _PLD below. Before there were concerns about
> > the memory that the cached _PLD information would consume. Another way
> > of doing this would be to just always evaluate the _PLD here.
> >
> > Rafael needs to comment on this. My personal opinion is that let's
> > just store the thing.
> >
>
> By "always evaluate the _PLD here", you mean something like
> acpi_get_physical_device_location(dev->handle, &pld)
> for every _PLD field, right?
>
> I will wait for Rafael's comment on this.
So I would like to get back to the very beginning: Do you need full
_PLD output to address the issue at hand?
If so, do you really need it for all devices that have _PLD?
If not, then why waste memory for all that stuff?
On Fri, Feb 4, 2022 at 11:18 AM Rafael J. Wysocki <[email protected]> wrote:
>
> On Wed, Feb 2, 2022 at 9:43 PM Won Chung <[email protected]> wrote:
> >
> > Hi Heikki,
> >
> > Thank you for the review!
> >
> > On Wed, Feb 2, 2022 at 12:19 AM Heikki Krogerus
> > <[email protected]> wrote:
> > >
> > > Hi Won,
> > >
> > > On Tue, Feb 01, 2022 at 01:55:18AM +0000, Won Chung wrote:
> > > > When ACPI table includes _PLD fields for a device, create a new
> > > > directory (pld) in sysfs to share _PLD fields.
> > >
> > > I think you need to explain what needs this information in user space.
> > >
> > > > Signed-off-by: Won Chung <[email protected]>
> > > > ---
> > > > Documentation/ABI/testing/sysfs-bus-acpi | 107 +++++++++++++++++++++++
> > > > drivers/acpi/device_sysfs.c | 55 ++++++++++++
> > > > include/acpi/acpi_bus.h | 1 +
> > > > 3 files changed, 163 insertions(+)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> > > > index 58abacf59b2a..b8b71c8f3cfd 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-acpi
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> > > > @@ -96,3 +96,110 @@ Description:
> > > > hardware, if the _HRV control method is present. It is mostly
> > > > useful for non-PCI devices because lspci can list the hardware
> > > > version for PCI devices.
> > > > +
> > > > +What: /sys/bus/acpi/devices/.../pld/
> > > > +Date: Feb, 2022
> > > > +Contact: Won Chung <[email protected]>
> > > > +Description:
> > > > + This directory contains the output of the device object's _PLD
> > > > + control method, if present. This information provides details
> > > > + on physical location of a device.
> > > > +
> > > > +What: /sys/bus/acpi/devices/.../pld/revision
> > > > +Date: Feb, 2022
> > > > +Contact: Won Chung <[email protected]>
> > > > +Description:
> > > > + The current revision is 0x2.
> > > > +
> > > > +What: /sys/bus/acpi/devices/.../pld/group_token
> > > > +Date: Feb, 2022
> > > > +Contact: Won Chung <[email protected]>
> > > > +Description:
> > > > + Unique numerical value identifying a group.
> > > > +
> > > > +What: /sys/bus/acpi/devices/.../pld/group_position
> > > > +Date: Feb, 2022
> > > > +Contact: Won Chung <[email protected]>
> > > > +Description:
> > > > + Identifies this device connection point’s position in the group.
> > > > +
> > > > +What: /sys/bus/acpi/devices/.../pld/user_visible
> > > > +Date: Feb, 2022
> > > > +Contact: Won Chung <[email protected]>
> > > > +Description:
> > > > + Set if the device connection point can be seen by the user
> > > > + without disassembly.
> > > > +
> > > > +What: /sys/bus/acpi/devices/.../pld/dock
> > > > +Date: Feb, 2022
> > > > +Contact: Won Chung <[email protected]>
> > > > +Description:
> > > > + Set if the device connection point resides in a docking station
> > > > + or port replicator.
> > > > +
> > > > +What: /sys/bus/acpi/devices/.../pld/bay
> > > > +Date: Feb, 2022
> > > > +Contact: Won Chung <[email protected]>
> > > > +Description:
> > > > + Set if describing a device in a bay or if device connection
> > > > + point is a bay.
> > > > +
> > > > +What: /sys/bus/acpi/devices/.../pld/lid
> > > > +Date: Feb, 2022
> > > > +Contact: Won Chung <[email protected]>
> > > > +Description:
> > > > + Set if this device connection point resides on the lid of
> > > > + laptop system.
> > > > +
> > > > +What: /sys/bus/acpi/devices/.../pld/panel
> > > > +Date: Feb, 2022
> > > > +Contact: Won Chung <[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 (Vertical Position and Horizontal Position will be
> > > > + ignored)
> > > > +
> > > > +What: /sys/bus/acpi/devices/.../pld/vertical_position
> > > > +Date: Feb, 2022
> > > > +Contact: Won Chung <[email protected]>
> > > > +Description:
> > > > + 0 - Upper
> > > > + 1 - Center
> > > > + 2 - Lower
> > > > +
> > > > +What: /sys/bus/acpi/devices/.../pld/horizontal_position
> > > > +Date: Feb, 2022
> > > > +Contact: Won Chung <[email protected]>
> > > > +Description:
> > > > + ACPI specification does not define horizontal position field.
> > > > + Can be used as either
> > > > + 0 - Left
> > > > + 1 - Center
> > > > + 2 - Right
> > > > + or
> > > > + 0 - Leftmost
> > > > + and higher numbers going toward the right.
> > > > +
> > > > +What: /sys/bus/acpi/devices/.../pld/shape
> > > > +Date: Feb, 2022
> > > > +Contact: Won Chung <[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
> > > > + 15:9 - Reserved
> > > > +
> > > > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> > > > index d5d6403ba07b..610be93635a0 100644
> > > > --- a/drivers/acpi/device_sysfs.c
> > > > +++ b/drivers/acpi/device_sysfs.c
> > > > @@ -509,6 +509,49 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> > > > }
> > > > static DEVICE_ATTR_RO(status);
> > > >
> > > > +#define DEV_ATTR_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); \
> > > > + if (acpi_dev->pld == NULL) \
> > > > + return -EIO; \
> > > > + return sprintf(buf, "%u\n", acpi_dev->pld->prop); \
> > > > +}; \
> > >
> > > Ah, you are storing the _PLD below. Before there were concerns about
> > > the memory that the cached _PLD information would consume. Another way
> > > of doing this would be to just always evaluate the _PLD here.
> > >
> > > Rafael needs to comment on this. My personal opinion is that let's
> > > just store the thing.
> > >
> >
> > By "always evaluate the _PLD here", you mean something like
> > acpi_get_physical_device_location(dev->handle, &pld)
> > for every _PLD field, right?
> >
> > I will wait for Rafael's comment on this.
>
Hi Rafael,
Thank you for the review.
> So I would like to get back to the very beginning: Do you need full
> _PLD output to address the issue at hand?
I do not think we need full _PLD output. In fact, the fields I am
adding are many but not all _PLD fields. To explain reasons behind
each field added:
*revision: current pld revision
*group token and position: like an identifier for the device
*panel, horizontal position, vertical position: to locate which
physical device corresponds to this device
*user visible, dock, bay, lid: to determine if and how the device can
be located by the user.
*shape: helps determine connection type
So, for example, let's say there are multiple Type C connectors (C0,
C1, and C2). We have 2 Type C ports on the left panel and 1 Type C
port on the right panel of the laptop. We can use PLD fields: panel
and horizontal position to determine which connector actually
corresponds to which physical port. Having this information, we can
determine which specific port (out of three) is doing what. (like
charging cable connected to the port on the left side of the left
panel)
The fields that are in _PLD output but not included here are:
ignore_color, color, width, height, ejectable, OSPM ejection required,
cabinet_number, card_cage_number, reference, group orientation,
rotation, order, vertical offset, horizontal offset.
These are not included because they do not help specify the device to
which one in specific physical location.
>
> If so, do you really need it for all devices that have _PLD?
Benson and Prashant, please correct me if I am wrong:
I do not think we need to store PLD for all devices, I think just user
visible devices to locate them physically. So, I think we can check if
device_type is ACPI_BUS_TYPE_DEVICE and user_visible pld field is 1.
Otherwise, I do not think we need to store pld information nor create
sysfs files.
What do you think?
>
> If not, then why waste memory for all that stuff?
If this sounds okay with you, I can make changes and send v6. (This
comment is on v4 but I already sent v5)
Thank you,
Won