Re: [PATCH v4] ACPI: device_sysfs: Add sysfs support for _PLD

[Rafael J. Wysocki]

On Wed, Feb 2, 2022 at 9:43 PM Won Chung <wonchung@xxxxxxxxxx> wrote:
>
> Hi Heikki,
>
> Thank you for the review!
>
> On Wed, Feb 2, 2022 at 12:19 AM Heikki Krogerus
> <heikki.krogerus@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxxx>
> > > +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@xxxxxxxxxx>
> > > +Description:
> > > +             The current revision is 0x2.
> > > +
> > > +What:           /sys/bus/acpi/devices/.../pld/group_token
> > > +Date:           Feb, 2022
> > > +Contact:        Won Chung <wonchung@xxxxxxxxxx>
> > > +Description:
> > > +             Unique numerical value identifying a group.
> > > +
> > > +What:           /sys/bus/acpi/devices/.../pld/group_position
> > > +Date:           Feb, 2022
> > > +Contact:        Won Chung <wonchung@xxxxxxxxxx>
> > > +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@xxxxxxxxxx>
> > > +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@xxxxxxxxxx>
> > > +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@xxxxxxxxxx>
> > > +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@xxxxxxxxxx>
> > > +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@xxxxxxxxxx>
> > > +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@xxxxxxxxxx>
> > > +Description:
> > > +             0 - Upper
> > > +             1 - Center
> > > +             2 - Lower
> > > +
> > > +What:           /sys/bus/acpi/devices/.../pld/horizontal_position
> > > +Date:           Feb, 2022
> > > +Contact:        Won Chung <wonchung@xxxxxxxxxx>
> > > +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@xxxxxxxxxx>
> > > +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?