| Message ID | 20220201015518.3118404-1-wonchung@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] ACPI: device_sysfs: Add sysfs support for _PLD | expand |
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 <wonchung@google.com> > --- > 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 <wonchung@google.com> > +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 <wonchung@google.com> > +Description: > + The current revision is 0x2. > + > +What: /sys/bus/acpi/devices/.../pld/group_token > +Date: Feb, 2022 > +Contact: Won Chung <wonchung@google.com> > +Description: > + Unique numerical value identifying a group. > + > +What: /sys/bus/acpi/devices/.../pld/group_position > +Date: Feb, 2022 > +Contact: Won Chung <wonchung@google.com> > +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 <wonchung@google.com> > +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 <wonchung@google.com> > +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 <wonchung@google.com> > +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 <wonchung@google.com> > +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 <wonchung@google.com> > +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 <wonchung@google.com> > +Description: > + 0 - Upper > + 1 - Center > + 2 - Lower > + > +What: /sys/bus/acpi/devices/.../pld/horizontal_position > +Date: Feb, 2022 > +Contact: Won Chung <wonchung@google.com> > +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 <wonchung@google.com> > +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,
Hi Heikki, Thank you for the review! On Wed, Feb 2, 2022 at 12:19 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> 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 <wonchung@google.com> > > --- > > 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 <wonchung@google.com> > > +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 <wonchung@google.com> > > +Description: > > + The current revision is 0x2. > > + > > +What: /sys/bus/acpi/devices/.../pld/group_token > > +Date: Feb, 2022 > > +Contact: Won Chung <wonchung@google.com> > > +Description: > > + Unique numerical value identifying a group. > > + > > +What: /sys/bus/acpi/devices/.../pld/group_position > > +Date: Feb, 2022 > > +Contact: Won Chung <wonchung@google.com> > > +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 <wonchung@google.com> > > +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 <wonchung@google.com> > > +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 <wonchung@google.com> > > +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 <wonchung@google.com> > > +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 <wonchung@google.com> > > +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 <wonchung@google.com> > > +Description: > > + 0 - Upper > > + 1 - Center > > + 2 - Lower > > + > > +What: /sys/bus/acpi/devices/.../pld/horizontal_position > > +Date: Feb, 2022 > > +Contact: Won Chung <wonchung@google.com> > > +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 <wonchung@google.com> > > +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 <wonchung@google.com> wrote: > > Hi Heikki, > > Thank you for the review! > > On Wed, Feb 2, 2022 at 12:19 AM Heikki Krogerus > <heikki.krogerus@linux.intel.com> 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 <wonchung@google.com> > > > --- > > > 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 <wonchung@google.com> > > > +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 <wonchung@google.com> > > > +Description: > > > + The current revision is 0x2. > > > + > > > +What: /sys/bus/acpi/devices/.../pld/group_token > > > +Date: Feb, 2022 > > > +Contact: Won Chung <wonchung@google.com> > > > +Description: > > > + Unique numerical value identifying a group. > > > + > > > +What: /sys/bus/acpi/devices/.../pld/group_position > > > +Date: Feb, 2022 > > > +Contact: Won Chung <wonchung@google.com> > > > +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 <wonchung@google.com> > > > +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 <wonchung@google.com> > > > +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 <wonchung@google.com> > > > +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 <wonchung@google.com> > > > +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 <wonchung@google.com> > > > +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 <wonchung@google.com> > > > +Description: > > > + 0 - Upper > > > + 1 - Center > > > + 2 - Lower > > > + > > > +What: /sys/bus/acpi/devices/.../pld/horizontal_position > > > +Date: Feb, 2022 > > > +Contact: Won Chung <wonchung@google.com> > > > +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 <wonchung@google.com> > > > +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 <rafael@kernel.org> wrote: > > On Wed, Feb 2, 2022 at 9:43 PM Won Chung <wonchung@google.com> wrote: > > > > Hi Heikki, > > > > Thank you for the review! > > > > On Wed, Feb 2, 2022 at 12:19 AM Heikki Krogerus > > <heikki.krogerus@linux.intel.com> 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 <wonchung@google.com> > > > > --- > > > > 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 <wonchung@google.com> > > > > +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 <wonchung@google.com> > > > > +Description: > > > > + The current revision is 0x2. > > > > + > > > > +What: /sys/bus/acpi/devices/.../pld/group_token > > > > +Date: Feb, 2022 > > > > +Contact: Won Chung <wonchung@google.com> > > > > +Description: > > > > + Unique numerical value identifying a group. > > > > + > > > > +What: /sys/bus/acpi/devices/.../pld/group_position > > > > +Date: Feb, 2022 > > > > +Contact: Won Chung <wonchung@google.com> > > > > +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 <wonchung@google.com> > > > > +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 <wonchung@google.com> > > > > +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 <wonchung@google.com> > > > > +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 <wonchung@google.com> > > > > +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 <wonchung@google.com> > > > > +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 <wonchung@google.com> > > > > +Description: > > > > + 0 - Upper > > > > + 1 - Center > > > > + 2 - Lower > > > > + > > > > +What: /sys/bus/acpi/devices/.../pld/horizontal_position > > > > +Date: Feb, 2022 > > > > +Contact: Won Chung <wonchung@google.com> > > > > +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 <wonchung@google.com> > > > > +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
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 <wonchung@google.com> +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 <wonchung@google.com> +Description: + The current revision is 0x2. + +What: /sys/bus/acpi/devices/.../pld/group_token +Date: Feb, 2022 +Contact: Won Chung <wonchung@google.com> +Description: + Unique numerical value identifying a group. + +What: /sys/bus/acpi/devices/.../pld/group_position +Date: Feb, 2022 +Contact: Won Chung <wonchung@google.com> +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 <wonchung@google.com> +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 <wonchung@google.com> +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 <wonchung@google.com> +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 <wonchung@google.com> +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 <wonchung@google.com> +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 <wonchung@google.com> +Description: + 0 - Upper + 1 - Center + 2 - Lower + +What: /sys/bus/acpi/devices/.../pld/horizontal_position +Date: Feb, 2022 +Contact: Won Chung <wonchung@google.com> +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 <wonchung@google.com> +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;
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 <wonchung@google.com> --- Documentation/ABI/testing/sysfs-bus-acpi | 107 +++++++++++++++++++++++ drivers/acpi/device_sysfs.c | 55 ++++++++++++ include/acpi/acpi_bus.h | 1 + 3 files changed, 163 insertions(+)