[PATCH v6] ACPI: device_sysfs: Add sysfs support for _PLD

[PATCH v6] ACPI: device_sysfs: Add sysfs support for _PLD

[Won Chung]

When ACPI table includes _PLD fields for a device, create a new
directory (pld) in sysfs to share _PLD fields.

Currently without PLD information, when there are multiple of same
devices, it is hard to distinguish which device corresponds to which
physical device in which location. For example, when there are two Type
C connectors, it is hard to find out which connector corresponds to the
Type C port on the left panel versus the Type C port on the right panel.
With PLD information provided, we can determine which specific device at
which location is doing what.

Since PLD information is to be used for finding where physical device is
located in user's perspective, pld sysfs directory will only be created
for ACPI_BUS_TYPE_DEVICE with user_visible field set as 1.

Signed-off-by: Won Chung <wonchung@google.com>
---
Changes in v6:
- Add pld to acpi_device only if it is a user visible device.

Changes in v5:
- Store pld to acpi_device in acpi_store_pld_crc() and rename it as
  acpi_store_pld().

Changes in v4:
- Create seperate files for each _PLD fields instead of having a single
  file with all fields included.
- Create a new directory pld that contains _PLD fields

 Documentation/ABI/testing/sysfs-bus-acpi | 107 +++++++++++++++++++++++
 drivers/acpi/device_sysfs.c              |  51 +++++++++++
 drivers/acpi/scan.c                      |  12 ++-
 include/acpi/acpi_bus.h                  |   1 +
 4 files changed, 168 insertions(+), 3 deletions(-)

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..2702f78a2503 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,12 @@ int acpi_device_setup_files(struct acpi_device *dev)
 						    &dev_attr_real_power_state);
 	}
 
+	if (acpi_has_method(dev->handle, "_PLD") && dev->pld != NULL) {
+		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 +694,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") && dev->pld != NULL)
+		device_remove_group(&dev->dev, &dev_attr_pld_group);
 }
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 1331756d4cfc..35ac7bcc05ed 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -668,7 +668,7 @@ static int acpi_tie_acpi_dev(struct acpi_device *adev)
 	return 0;
 }
 
-static void acpi_store_pld_crc(struct acpi_device *adev)
+static void acpi_store_pld(struct acpi_device *adev)
 {
 	struct acpi_pld_info *pld;
 	acpi_status status;
@@ -678,7 +678,13 @@ static void acpi_store_pld_crc(struct acpi_device *adev)
 		return;
 
 	adev->pld_crc = crc32(~0, pld, sizeof(*pld));
-	ACPI_FREE(pld);
+
+	if (adev->device_type == ACPI_BUS_TYPE_DEVICE && pld->user_visible) {
+		adev->pld = pld;
+	} else {
+		adev->pld = NULL;
+		ACPI_FREE(pld);
+	}
 }
 
 static int __acpi_device_add(struct acpi_device *device,
@@ -739,7 +745,7 @@ static int __acpi_device_add(struct acpi_device *device,
 	if (device->wakeup.flags.valid)
 		list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list);
 
-	acpi_store_pld_crc(device);
+	acpi_store_pld(device);
 
 	mutex_unlock(&acpi_device_lock);
 
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.1.265.g69c8d7142f-goog

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

[Rafael J. Wysocki]

On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@google.com> wrote:
>
> When ACPI table includes _PLD fields for a device, create a new
> directory (pld) in sysfs to share _PLD fields.

This version of the patch loos better to me, but I'm not sure if it
goes into the right direction overall.

> Currently without PLD information, when there are multiple of same
> devices, it is hard to distinguish which device corresponds to which
> physical device in which location. For example, when there are two Type
> C connectors, it is hard to find out which connector corresponds to the
> Type C port on the left panel versus the Type C port on the right panel.

So I think that this is your primary use case and I'm wondering if
this is the best way to address it.

Namely, by exposing _PLD information under the ACPI device object,
you'll make user space wanting to use that information depend on this
interface, but the problem is not ACPI-specific (inevitably, it will
appear on systems using DT, sooner or later) and making the user space
interface related to it depend on ACPI doesn't look like a perfect
choice.

IOW, why don't you create a proper ABI for this in the Type C
subsystem and expose the information needed by user space in a generic
way that can be based on the _PLD information on systems with ACPI?

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

[Won Chung]

On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@google.com> wrote:
> >
> > When ACPI table includes _PLD fields for a device, create a new
> > directory (pld) in sysfs to share _PLD fields.
>
> This version of the patch loos better to me, but I'm not sure if it
> goes into the right direction overall.
>
> > Currently without PLD information, when there are multiple of same
> > devices, it is hard to distinguish which device corresponds to which
> > physical device in which location. For example, when there are two Type
> > C connectors, it is hard to find out which connector corresponds to the
> > Type C port on the left panel versus the Type C port on the right panel.
>
> So I think that this is your primary use case and I'm wondering if
> this is the best way to address it.
>
> Namely, by exposing _PLD information under the ACPI device object,
> you'll make user space wanting to use that information depend on this
> interface, but the problem is not ACPI-specific (inevitably, it will
> appear on systems using DT, sooner or later) and making the user space
> interface related to it depend on ACPI doesn't look like a perfect
> choice.
>
> IOW, why don't you create a proper ABI for this in the Type C
> subsystem and expose the information needed by user space in a generic
> way that can be based on the _PLD information on systems with ACPI?

Hi Rafael,

Thank you for the review.

I was thinking that _PLD info is specific to ACPI since it is part of
the ACPI table. Could you explain a little bit more on why you think
exposing _PLD fields is not an ACPI-specific problem?

I gave an example of how _PLD fields can be used for specifying Type C
connectors, but it is not Type C specific. For Chrome OS, we plan to
initially add PLD to not only Type C connectors but also USB port
devices (including Type C and Type A). Also, PLD can be used in the
future for describing other types of ports too like HDMI. (Benson and
Prashant, please correct or add if I am wrong or missing some
information) Maybe my commit message was not detailed enough..

I am also curious what Heikki thinks about this. Heikki, can you take
a look and share your thoughts?

Thank you,
Won

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

[Won Chung]

On Mon, Feb 14, 2022 at 12:30 PM Won Chung <wonchung@google.com> wrote:
>
> On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@google.com> wrote:
> > >
> > > When ACPI table includes _PLD fields for a device, create a new
> > > directory (pld) in sysfs to share _PLD fields.
> >
> > This version of the patch loos better to me, but I'm not sure if it
> > goes into the right direction overall.
> >
> > > Currently without PLD information, when there are multiple of same
> > > devices, it is hard to distinguish which device corresponds to which
> > > physical device in which location. For example, when there are two Type
> > > C connectors, it is hard to find out which connector corresponds to the
> > > Type C port on the left panel versus the Type C port on the right panel.
> >
> > So I think that this is your primary use case and I'm wondering if
> > this is the best way to address it.
> >
> > Namely, by exposing _PLD information under the ACPI device object,
> > you'll make user space wanting to use that information depend on this
> > interface, but the problem is not ACPI-specific (inevitably, it will
> > appear on systems using DT, sooner or later) and making the user space
> > interface related to it depend on ACPI doesn't look like a perfect
> > choice.
> >
> > IOW, why don't you create a proper ABI for this in the Type C
> > subsystem and expose the information needed by user space in a generic
> > way that can be based on the _PLD information on systems with ACPI?
>
> Hi Rafael,
>
> Thank you for the review.
>
> I was thinking that _PLD info is specific to ACPI since it is part of
> the ACPI table. Could you explain a little bit more on why you think
> exposing _PLD fields is not an ACPI-specific problem?

Hi Rafael again,

Sorry for the silly question here. I misunderstood your comment a bit,
but I talked to Benson and Prashant for clarification. I understand
now what you mean by it is not an ACPI-specific problem and exposing
PLD would depend on ACPI.

>
> I gave an example of how _PLD fields can be used for specifying Type C
> connectors, but it is not Type C specific. For Chrome OS, we plan to
> initially add PLD to not only Type C connectors but also USB port
> devices (including Type C and Type A). Also, PLD can be used in the
> future for describing other types of ports too like HDMI. (Benson and
> Prashant, please correct or add if I am wrong or missing some
> information) Maybe my commit message was not detailed enough..
>
> I am also curious what Heikki thinks about this. Heikki, can you take
> a look and share your thoughts?

I am still curious what you and Heikki think about this since it may
not be a Type C specific issue. We can start from adding generic
location info to Type C subsystem first, as you suggested, then
consider how to do the same for USB devices and Type A ports
afterwards. I would appreciate sharing any thoughts or feedback. Thank
you very much!

Won

>
> Thank you,
> Won

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

[Heikki Krogerus]

On Mon, Feb 14, 2022 at 02:58:44PM -0800, Won Chung wrote:
> On Mon, Feb 14, 2022 at 12:30 PM Won Chung <wonchung@google.com> wrote:
> >
> > On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@google.com> wrote:
> > > >
> > > > When ACPI table includes _PLD fields for a device, create a new
> > > > directory (pld) in sysfs to share _PLD fields.
> > >
> > > This version of the patch loos better to me, but I'm not sure if it
> > > goes into the right direction overall.
> > >
> > > > Currently without PLD information, when there are multiple of same
> > > > devices, it is hard to distinguish which device corresponds to which
> > > > physical device in which location. For example, when there are two Type
> > > > C connectors, it is hard to find out which connector corresponds to the
> > > > Type C port on the left panel versus the Type C port on the right panel.
> > >
> > > So I think that this is your primary use case and I'm wondering if
> > > this is the best way to address it.
> > >
> > > Namely, by exposing _PLD information under the ACPI device object,
> > > you'll make user space wanting to use that information depend on this
> > > interface, but the problem is not ACPI-specific (inevitably, it will
> > > appear on systems using DT, sooner or later) and making the user space
> > > interface related to it depend on ACPI doesn't look like a perfect
> > > choice.
> > >
> > > IOW, why don't you create a proper ABI for this in the Type C
> > > subsystem and expose the information needed by user space in a generic
> > > way that can be based on the _PLD information on systems with ACPI?
> >
> > Hi Rafael,
> >
> > Thank you for the review.
> >
> > I was thinking that _PLD info is specific to ACPI since it is part of
> > the ACPI table. Could you explain a little bit more on why you think
> > exposing _PLD fields is not an ACPI-specific problem?
> 
> Hi Rafael again,
> 
> Sorry for the silly question here. I misunderstood your comment a bit,
> but I talked to Benson and Prashant for clarification. I understand
> now what you mean by it is not an ACPI-specific problem and exposing
> PLD would depend on ACPI.
> 
> >
> > I gave an example of how _PLD fields can be used for specifying Type C
> > connectors, but it is not Type C specific. For Chrome OS, we plan to
> > initially add PLD to not only Type C connectors but also USB port
> > devices (including Type C and Type A). Also, PLD can be used in the
> > future for describing other types of ports too like HDMI. (Benson and
> > Prashant, please correct or add if I am wrong or missing some
> > information) Maybe my commit message was not detailed enough..
> >
> > I am also curious what Heikki thinks about this. Heikki, can you take
> > a look and share your thoughts?
> 
> I am still curious what you and Heikki think about this since it may
> not be a Type C specific issue. We can start from adding generic
> location info to Type C subsystem first, as you suggested, then
> consider how to do the same for USB devices and Type A ports
> afterwards. I would appreciate sharing any thoughts or feedback. Thank
> you very much!

Like you said, _PLD is not Type-C specific. We can't limit it to any
specific device class. For example, I'm pretty ssure that sooner or
later we want to get this information in user space also with camera
sensors, and probable with a few other things as well.

I think the question here is, can we create a some kind of an
abstraction layer for the user space that exposes the device location
details in generic Linux specific way - so with ACPI it would utilise
the _PLD, and with DT something else (today AFAIK DT does not have
any way to describe locations of the devices). Maybe I'm wrong?

But if that is the question, then IMO the answer is: maybe one day,
but not today, and even if we one day can come up with something like
that, we still should expose the _PLD as ACPI specific information to
the user space as is.

Even if one day we have common sysfs attributes for all the devices
that contain the location of the device in some form, those attributes
will almost certainly have only a sub-set of the _PLD details, a
sub-set that works also with DT.
IMO the user space should always have access to all the necessary _PLD
details in their raw form if needed, even if those common device
location attributes exist - duplicated information or not. And debugfs
unfortunately is also not OK for that, because the user space needs to
be able to also rely on access to the additional details if needed.

We can limit the _PLD fields that we expose to the ones that we know
we need today (and probable should limit them to those), and we can of
course have a Kconfig option for the _PLD sysfs information if we want
to, but let's not start this by trying to figure out what kind of
abstraction we want for this. Right now we simply can not do that.

thanks,

-- 
heikki

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

[Rafael J. Wysocki]

On Tue, Feb 15, 2022 at 11:14 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Mon, Feb 14, 2022 at 02:58:44PM -0800, Won Chung wrote:
> > On Mon, Feb 14, 2022 at 12:30 PM Won Chung <wonchung@google.com> wrote:
> > >
> > > On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@google.com> wrote:
> > > > >
> > > > > When ACPI table includes _PLD fields for a device, create a new
> > > > > directory (pld) in sysfs to share _PLD fields.
> > > >
> > > > This version of the patch loos better to me, but I'm not sure if it
> > > > goes into the right direction overall.
> > > >
> > > > > Currently without PLD information, when there are multiple of same
> > > > > devices, it is hard to distinguish which device corresponds to which
> > > > > physical device in which location. For example, when there are two Type
> > > > > C connectors, it is hard to find out which connector corresponds to the
> > > > > Type C port on the left panel versus the Type C port on the right panel.
> > > >
> > > > So I think that this is your primary use case and I'm wondering if
> > > > this is the best way to address it.
> > > >
> > > > Namely, by exposing _PLD information under the ACPI device object,
> > > > you'll make user space wanting to use that information depend on this
> > > > interface, but the problem is not ACPI-specific (inevitably, it will
> > > > appear on systems using DT, sooner or later) and making the user space
> > > > interface related to it depend on ACPI doesn't look like a perfect
> > > > choice.
> > > >
> > > > IOW, why don't you create a proper ABI for this in the Type C
> > > > subsystem and expose the information needed by user space in a generic
> > > > way that can be based on the _PLD information on systems with ACPI?
> > >
> > > Hi Rafael,
> > >
> > > Thank you for the review.
> > >
> > > I was thinking that _PLD info is specific to ACPI since it is part of
> > > the ACPI table. Could you explain a little bit more on why you think
> > > exposing _PLD fields is not an ACPI-specific problem?
> >
> > Hi Rafael again,
> >
> > Sorry for the silly question here. I misunderstood your comment a bit,
> > but I talked to Benson and Prashant for clarification. I understand
> > now what you mean by it is not an ACPI-specific problem and exposing
> > PLD would depend on ACPI.
> >
> > >
> > > I gave an example of how _PLD fields can be used for specifying Type C
> > > connectors, but it is not Type C specific. For Chrome OS, we plan to
> > > initially add PLD to not only Type C connectors but also USB port
> > > devices (including Type C and Type A). Also, PLD can be used in the
> > > future for describing other types of ports too like HDMI. (Benson and
> > > Prashant, please correct or add if I am wrong or missing some
> > > information) Maybe my commit message was not detailed enough..
> > >
> > > I am also curious what Heikki thinks about this. Heikki, can you take
> > > a look and share your thoughts?
> >
> > I am still curious what you and Heikki think about this since it may
> > not be a Type C specific issue. We can start from adding generic
> > location info to Type C subsystem first, as you suggested, then
> > consider how to do the same for USB devices and Type A ports
> > afterwards. I would appreciate sharing any thoughts or feedback. Thank
> > you very much!
>
> Like you said, _PLD is not Type-C specific. We can't limit it to any
> specific device class. For example, I'm pretty sure that sooner or
> later we want to get this information in user space also with camera
> sensors, and probable with a few other things as well.
>
> I think the question here is, can we create a some kind of an
> abstraction layer for the user space that exposes the device location
> details in generic Linux specific way - so with ACPI it would utilise
> the _PLD, and with DT something else (today AFAIK DT does not have
> any way to describe locations of the devices). Maybe I'm wrong?

No, you aren't.

> But if that is the question, then IMO the answer is: maybe one day,
> but not today,

Why not?

> and even if we one day can come up with something like
> that, we still should expose the _PLD as ACPI specific information to
> the user space as is.

Why would it need that information in this particular format?

> Even if one day we have common sysfs attributes for all the devices
> that contain the location of the device in some form, those attributes
> will almost certainly have only a sub-set of the _PLD details, a
> sub-set that works also with DT.

That doesn't have to be the case.

However, things linke cpuidle have been invented to provide user space
interfaces for features that previously were only available on systems
with ACPI.  Why is _PLD different?

> IMO the user space should always have access to all the necessary _PLD
> details in their raw form if needed, even if those common device
> location attributes exist - duplicated information or not.

Again, why would it need that information?

> And debugfs
> unfortunately is also not OK for that, because the user space needs to
> be able to also rely on access to the additional details if needed.

What additional details do you mean?

> We can limit the _PLD fields that we expose to the ones that we know
> we need today (and probable should limit them to those), and we can of
> course have a Kconfig option for the _PLD sysfs information if we want
> to, but let's not start this by trying to figure out what kind of
> abstraction we want for this. Right now we simply can not do that.

Why can't we?

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

[Heikki Krogerus]

On Tue, Feb 15, 2022 at 02:54:11PM +0100, Rafael J. Wysocki wrote:
> On Tue, Feb 15, 2022 at 11:14 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Mon, Feb 14, 2022 at 02:58:44PM -0800, Won Chung wrote:
> > > On Mon, Feb 14, 2022 at 12:30 PM Won Chung <wonchung@google.com> wrote:
> > > >
> > > > On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@google.com> wrote:
> > > > > >
> > > > > > When ACPI table includes _PLD fields for a device, create a new
> > > > > > directory (pld) in sysfs to share _PLD fields.
> > > > >
> > > > > This version of the patch loos better to me, but I'm not sure if it
> > > > > goes into the right direction overall.
> > > > >
> > > > > > Currently without PLD information, when there are multiple of same
> > > > > > devices, it is hard to distinguish which device corresponds to which
> > > > > > physical device in which location. For example, when there are two Type
> > > > > > C connectors, it is hard to find out which connector corresponds to the
> > > > > > Type C port on the left panel versus the Type C port on the right panel.
> > > > >
> > > > > So I think that this is your primary use case and I'm wondering if
> > > > > this is the best way to address it.
> > > > >
> > > > > Namely, by exposing _PLD information under the ACPI device object,
> > > > > you'll make user space wanting to use that information depend on this
> > > > > interface, but the problem is not ACPI-specific (inevitably, it will
> > > > > appear on systems using DT, sooner or later) and making the user space
> > > > > interface related to it depend on ACPI doesn't look like a perfect
> > > > > choice.
> > > > >
> > > > > IOW, why don't you create a proper ABI for this in the Type C
> > > > > subsystem and expose the information needed by user space in a generic
> > > > > way that can be based on the _PLD information on systems with ACPI?
> > > >
> > > > Hi Rafael,
> > > >
> > > > Thank you for the review.
> > > >
> > > > I was thinking that _PLD info is specific to ACPI since it is part of
> > > > the ACPI table. Could you explain a little bit more on why you think
> > > > exposing _PLD fields is not an ACPI-specific problem?
> > >
> > > Hi Rafael again,
> > >
> > > Sorry for the silly question here. I misunderstood your comment a bit,
> > > but I talked to Benson and Prashant for clarification. I understand
> > > now what you mean by it is not an ACPI-specific problem and exposing
> > > PLD would depend on ACPI.
> > >
> > > >
> > > > I gave an example of how _PLD fields can be used for specifying Type C
> > > > connectors, but it is not Type C specific. For Chrome OS, we plan to
> > > > initially add PLD to not only Type C connectors but also USB port
> > > > devices (including Type C and Type A). Also, PLD can be used in the
> > > > future for describing other types of ports too like HDMI. (Benson and
> > > > Prashant, please correct or add if I am wrong or missing some
> > > > information) Maybe my commit message was not detailed enough..
> > > >
> > > > I am also curious what Heikki thinks about this. Heikki, can you take
> > > > a look and share your thoughts?
> > >
> > > I am still curious what you and Heikki think about this since it may
> > > not be a Type C specific issue. We can start from adding generic
> > > location info to Type C subsystem first, as you suggested, then
> > > consider how to do the same for USB devices and Type A ports
> > > afterwards. I would appreciate sharing any thoughts or feedback. Thank
> > > you very much!
> >
> > Like you said, _PLD is not Type-C specific. We can't limit it to any
> > specific device class. For example, I'm pretty sure that sooner or
> > later we want to get this information in user space also with camera
> > sensors, and probable with a few other things as well.
> >
> > I think the question here is, can we create a some kind of an
> > abstraction layer for the user space that exposes the device location
> > details in generic Linux specific way - so with ACPI it would utilise
> > the _PLD, and with DT something else (today AFAIK DT does not have
> > any way to describe locations of the devices). Maybe I'm wrong?
> 
> No, you aren't.
> 
> > But if that is the question, then IMO the answer is: maybe one day,
> > but not today,
> 
> Why not?
> 
> > and even if we one day can come up with something like
> > that, we still should expose the _PLD as ACPI specific information to
> > the user space as is.
> 
> Why would it need that information in this particular format?
> 
> > Even if one day we have common sysfs attributes for all the devices
> > that contain the location of the device in some form, those attributes
> > will almost certainly have only a sub-set of the _PLD details, a
> > sub-set that works also with DT.
> 
> That doesn't have to be the case.
> 
> However, things linke cpuidle have been invented to provide user space
> interfaces for features that previously were only available on systems
> with ACPI.  Why is _PLD different?
> 
> > IMO the user space should always have access to all the necessary _PLD
> > details in their raw form if needed, even if those common device
> > location attributes exist - duplicated information or not.
> 
> Again, why would it need that information?

We don't know if we'll need that in the future, and that's the point.

> > And debugfs
> > unfortunately is also not OK for that, because the user space needs to
> > be able to also rely on access to the additional details if needed.
> 
> What additional details do you mean?
> 
> > We can limit the _PLD fields that we expose to the ones that we know
> > we need today (and probable should limit them to those), and we can of
> > course have a Kconfig option for the _PLD sysfs information if we want
> > to, but let's not start this by trying to figure out what kind of
> > abstraction we want for this. Right now we simply can not do that.
> 
> Why can't we?

Right now we can't say for sure if DT can even supply the details that
we need from _PLD. I don't think we can at the moment even say are the
DT guys willing to support this at all.

To play it safe, I would just supply the needed _PLD fields as part of
the ACPI device nodes (under /sys/bus/acpi). There we can guarantee
that we'll always be able to supply all the information in the _PDL if
needed. Since we would add these to the ACPI nodes, it would be
crystal clear to the userspace that this information is only available
on ACPI platforms.

Then if, and only if, we know that DT can supply the same information
(at least to some of it) I would start thinking about the alternative
interface to this information that we make part of the actual devices.
Since at this point we have already the primary ACPI specific
interface to this same information that guarantees that it can supply
all the details if necessary, we don't have to worry about having to
be able to do the same with this new interface. This interface can
just expose the common details that we know for sure that both ACPI
and DT can always supply.

thanks,

-- 
heikki

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

[Rafael J. Wysocki]

On Wed, Feb 16, 2022 at 12:34 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Tue, Feb 15, 2022 at 02:54:11PM +0100, Rafael J. Wysocki wrote:
> > On Tue, Feb 15, 2022 at 11:14 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > On Mon, Feb 14, 2022 at 02:58:44PM -0800, Won Chung wrote:
> > > > On Mon, Feb 14, 2022 at 12:30 PM Won Chung <wonchung@google.com> wrote:
> > > > >
> > > > > On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > >
> > > > > > On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@google.com> wrote:
> > > > > > >
> > > > > > > When ACPI table includes _PLD fields for a device, create a new
> > > > > > > directory (pld) in sysfs to share _PLD fields.
> > > > > >
> > > > > > This version of the patch loos better to me, but I'm not sure if it
> > > > > > goes into the right direction overall.
> > > > > >
> > > > > > > Currently without PLD information, when there are multiple of same
> > > > > > > devices, it is hard to distinguish which device corresponds to which
> > > > > > > physical device in which location. For example, when there are two Type
> > > > > > > C connectors, it is hard to find out which connector corresponds to the
> > > > > > > Type C port on the left panel versus the Type C port on the right panel.
> > > > > >
> > > > > > So I think that this is your primary use case and I'm wondering if
> > > > > > this is the best way to address it.
> > > > > >
> > > > > > Namely, by exposing _PLD information under the ACPI device object,
> > > > > > you'll make user space wanting to use that information depend on this
> > > > > > interface, but the problem is not ACPI-specific (inevitably, it will
> > > > > > appear on systems using DT, sooner or later) and making the user space
> > > > > > interface related to it depend on ACPI doesn't look like a perfect
> > > > > > choice.
> > > > > >
> > > > > > IOW, why don't you create a proper ABI for this in the Type C
> > > > > > subsystem and expose the information needed by user space in a generic
> > > > > > way that can be based on the _PLD information on systems with ACPI?
> > > > >
> > > > > Hi Rafael,
> > > > >
> > > > > Thank you for the review.
> > > > >
> > > > > I was thinking that _PLD info is specific to ACPI since it is part of
> > > > > the ACPI table. Could you explain a little bit more on why you think
> > > > > exposing _PLD fields is not an ACPI-specific problem?
> > > >
> > > > Hi Rafael again,
> > > >
> > > > Sorry for the silly question here. I misunderstood your comment a bit,
> > > > but I talked to Benson and Prashant for clarification. I understand
> > > > now what you mean by it is not an ACPI-specific problem and exposing
> > > > PLD would depend on ACPI.
> > > >
> > > > >
> > > > > I gave an example of how _PLD fields can be used for specifying Type C
> > > > > connectors, but it is not Type C specific. For Chrome OS, we plan to
> > > > > initially add PLD to not only Type C connectors but also USB port
> > > > > devices (including Type C and Type A). Also, PLD can be used in the
> > > > > future for describing other types of ports too like HDMI. (Benson and
> > > > > Prashant, please correct or add if I am wrong or missing some
> > > > > information) Maybe my commit message was not detailed enough..
> > > > >
> > > > > I am also curious what Heikki thinks about this. Heikki, can you take
> > > > > a look and share your thoughts?
> > > >
> > > > I am still curious what you and Heikki think about this since it may
> > > > not be a Type C specific issue. We can start from adding generic
> > > > location info to Type C subsystem first, as you suggested, then
> > > > consider how to do the same for USB devices and Type A ports
> > > > afterwards. I would appreciate sharing any thoughts or feedback. Thank
> > > > you very much!
> > >
> > > Like you said, _PLD is not Type-C specific. We can't limit it to any
> > > specific device class. For example, I'm pretty sure that sooner or
> > > later we want to get this information in user space also with camera
> > > sensors, and probable with a few other things as well.
> > >
> > > I think the question here is, can we create a some kind of an
> > > abstraction layer for the user space that exposes the device location
> > > details in generic Linux specific way - so with ACPI it would utilise
> > > the _PLD, and with DT something else (today AFAIK DT does not have
> > > any way to describe locations of the devices). Maybe I'm wrong?
> >
> > No, you aren't.
> >
> > > But if that is the question, then IMO the answer is: maybe one day,
> > > but not today,
> >
> > Why not?
> >
> > > and even if we one day can come up with something like
> > > that, we still should expose the _PLD as ACPI specific information to
> > > the user space as is.
> >
> > Why would it need that information in this particular format?
> >
> > > Even if one day we have common sysfs attributes for all the devices
> > > that contain the location of the device in some form, those attributes
> > > will almost certainly have only a sub-set of the _PLD details, a
> > > sub-set that works also with DT.
> >
> > That doesn't have to be the case.
> >
> > However, things linke cpuidle have been invented to provide user space
> > interfaces for features that previously were only available on systems
> > with ACPI.  Why is _PLD different?
> >
> > > IMO the user space should always have access to all the necessary _PLD
> > > details in their raw form if needed, even if those common device
> > > location attributes exist - duplicated information or not.
> >
> > Again, why would it need that information?
>
> We don't know if we'll need that in the future, and that's the point.

Well, for me that would be a good enough reason for avoiding to expose it.

If there is no particular reason for exposing any information to user
space, I don't see why it should be exposed at all.

There is some cost of exposing things to user space, so why pay it for
no benefit?

> > > And debugfs
> > > unfortunately is also not OK for that, because the user space needs to
> > > be able to also rely on access to the additional details if needed.
> >
> > What additional details do you mean?
> >
> > > We can limit the _PLD fields that we expose to the ones that we know
> > > we need today (and probable should limit them to those), and we can of
> > > course have a Kconfig option for the _PLD sysfs information if we want
> > > to, but let's not start this by trying to figure out what kind of
> > > abstraction we want for this. Right now we simply can not do that.
> >
> > Why can't we?
>
> Right now we can't say for sure if DT can even supply the details that
> we need from _PLD. I don't think we can at the moment even say are the
> DT guys willing to support this at all.
>
> To play it safe, I would just supply the needed _PLD fields as part of
> the ACPI device nodes (under /sys/bus/acpi).

That would be suboptimal for a few reasons:

1. The interface is potentially confusing.  User space would first
need to locate the ACPI device interface corresponding to the given
"real" device in order to use that information.
2. It doesn't scale beyond ACPI.
3. From the ACPI subsystem's perspective the choice of the "relevant"
_PLD fields is arbitrary and exposing all of them is overkill for any
use cases known to me.
4. The ACPI subsystem doesn't know the devices for which _PLD
information should be exposed and there are some devices for which it
is just not useful.

> There we can guarantee
> that we'll always be able to supply all the information in the _PDL if
> needed. Since we would add these to the ACPI nodes, it would be
> crystal clear to the userspace that this information is only available
> on ACPI platforms.

I'm not considering this as a feature.

> Then if, and only if, we know that DT can supply the same information
> (at least to some of it) I would start thinking about the alternative
> interface to this information that we make part of the actual devices.
> Since at this point we have already the primary ACPI specific
> interface to this same information that guarantees that it can supply
> all the details if necessary, we don't have to worry about having to
> be able to do the same with this new interface. This interface can
> just expose the common details that we know for sure that both ACPI
> and DT can always supply.

Well, there is another possible approach: Expose the information
needed to address a particular use case in a way that doesn't strictly
depend on ACPI and make this use ACPI as a backend.  Don't worry about
the DT side of things.  If the generic interface is there and it is
suitable enough, DT will be in the receiving end position with much
less of a freedom to introduce a new interface for the same purpose.

On the other hand, if _PLD information is exposed in an ACPI-specific
way, it is almost guaranteed that there will be a DT-specific
interface for the same thing and utilities wanting to be generic will
need to support both of them which will be extra pain.  Some of them
will choose to support the DT-specific interface only and we'll end up
with utilities that can't be used on ACPI-based systems because of
incompatible interfaces.  Been there already.  Thanks, but no thanks!

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

[Won Chung]

On Wed, Feb 16, 2022 at 8:39 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Feb 16, 2022 at 12:34 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Tue, Feb 15, 2022 at 02:54:11PM +0100, Rafael J. Wysocki wrote:
> > > On Tue, Feb 15, 2022 at 11:14 AM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > On Mon, Feb 14, 2022 at 02:58:44PM -0800, Won Chung wrote:
> > > > > On Mon, Feb 14, 2022 at 12:30 PM Won Chung <wonchung@google.com> wrote:
> > > > > >
> > > > > > On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > > >
> > > > > > > On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@google.com> wrote:
> > > > > > > >
> > > > > > > > When ACPI table includes _PLD fields for a device, create a new
> > > > > > > > directory (pld) in sysfs to share _PLD fields.
> > > > > > >
> > > > > > > This version of the patch loos better to me, but I'm not sure if it
> > > > > > > goes into the right direction overall.
> > > > > > >
> > > > > > > > Currently without PLD information, when there are multiple of same
> > > > > > > > devices, it is hard to distinguish which device corresponds to which
> > > > > > > > physical device in which location. For example, when there are two Type
> > > > > > > > C connectors, it is hard to find out which connector corresponds to the
> > > > > > > > Type C port on the left panel versus the Type C port on the right panel.
> > > > > > >
> > > > > > > So I think that this is your primary use case and I'm wondering if
> > > > > > > this is the best way to address it.
> > > > > > >
> > > > > > > Namely, by exposing _PLD information under the ACPI device object,
> > > > > > > you'll make user space wanting to use that information depend on this
> > > > > > > interface, but the problem is not ACPI-specific (inevitably, it will
> > > > > > > appear on systems using DT, sooner or later) and making the user space
> > > > > > > interface related to it depend on ACPI doesn't look like a perfect
> > > > > > > choice.
> > > > > > >
> > > > > > > IOW, why don't you create a proper ABI for this in the Type C
> > > > > > > subsystem and expose the information needed by user space in a generic
> > > > > > > way that can be based on the _PLD information on systems with ACPI?
> > > > > >
> > > > > > Hi Rafael,
> > > > > >
> > > > > > Thank you for the review.
> > > > > >
> > > > > > I was thinking that _PLD info is specific to ACPI since it is part of
> > > > > > the ACPI table. Could you explain a little bit more on why you think
> > > > > > exposing _PLD fields is not an ACPI-specific problem?
> > > > >
> > > > > Hi Rafael again,
> > > > >
> > > > > Sorry for the silly question here. I misunderstood your comment a bit,
> > > > > but I talked to Benson and Prashant for clarification. I understand
> > > > > now what you mean by it is not an ACPI-specific problem and exposing
> > > > > PLD would depend on ACPI.
> > > > >
> > > > > >
> > > > > > I gave an example of how _PLD fields can be used for specifying Type C
> > > > > > connectors, but it is not Type C specific. For Chrome OS, we plan to
> > > > > > initially add PLD to not only Type C connectors but also USB port
> > > > > > devices (including Type C and Type A). Also, PLD can be used in the
> > > > > > future for describing other types of ports too like HDMI. (Benson and
> > > > > > Prashant, please correct or add if I am wrong or missing some
> > > > > > information) Maybe my commit message was not detailed enough..
> > > > > >
> > > > > > I am also curious what Heikki thinks about this. Heikki, can you take
> > > > > > a look and share your thoughts?
> > > > >
> > > > > I am still curious what you and Heikki think about this since it may
> > > > > not be a Type C specific issue. We can start from adding generic
> > > > > location info to Type C subsystem first, as you suggested, then
> > > > > consider how to do the same for USB devices and Type A ports
> > > > > afterwards. I would appreciate sharing any thoughts or feedback. Thank
> > > > > you very much!
> > > >
> > > > Like you said, _PLD is not Type-C specific. We can't limit it to any
> > > > specific device class. For example, I'm pretty sure that sooner or
> > > > later we want to get this information in user space also with camera
> > > > sensors, and probable with a few other things as well.
> > > >
> > > > I think the question here is, can we create a some kind of an
> > > > abstraction layer for the user space that exposes the device location
> > > > details in generic Linux specific way - so with ACPI it would utilise
> > > > the _PLD, and with DT something else (today AFAIK DT does not have
> > > > any way to describe locations of the devices). Maybe I'm wrong?
> > >
> > > No, you aren't.
> > >
> > > > But if that is the question, then IMO the answer is: maybe one day,
> > > > but not today,
> > >
> > > Why not?
> > >
> > > > and even if we one day can come up with something like
> > > > that, we still should expose the _PLD as ACPI specific information to
> > > > the user space as is.
> > >
> > > Why would it need that information in this particular format?
> > >
> > > > Even if one day we have common sysfs attributes for all the devices
> > > > that contain the location of the device in some form, those attributes
> > > > will almost certainly have only a sub-set of the _PLD details, a
> > > > sub-set that works also with DT.
> > >
> > > That doesn't have to be the case.
> > >
> > > However, things linke cpuidle have been invented to provide user space
> > > interfaces for features that previously were only available on systems
> > > with ACPI.  Why is _PLD different?
> > >
> > > > IMO the user space should always have access to all the necessary _PLD
> > > > details in their raw form if needed, even if those common device
> > > > location attributes exist - duplicated information or not.
> > >
> > > Again, why would it need that information?
> >
> > We don't know if we'll need that in the future, and that's the point.
>
> Well, for me that would be a good enough reason for avoiding to expose it.
>
> If there is no particular reason for exposing any information to user
> space, I don't see why it should be exposed at all.
>
> There is some cost of exposing things to user space, so why pay it for
> no benefit?
>
> > > > And debugfs
> > > > unfortunately is also not OK for that, because the user space needs to
> > > > be able to also rely on access to the additional details if needed.
> > >
> > > What additional details do you mean?
> > >
> > > > We can limit the _PLD fields that we expose to the ones that we know
> > > > we need today (and probable should limit them to those), and we can of
> > > > course have a Kconfig option for the _PLD sysfs information if we want
> > > > to, but let's not start this by trying to figure out what kind of
> > > > abstraction we want for this. Right now we simply can not do that.
> > >
> > > Why can't we?
> >
> > Right now we can't say for sure if DT can even supply the details that
> > we need from _PLD. I don't think we can at the moment even say are the
> > DT guys willing to support this at all.
> >
> > To play it safe, I would just supply the needed _PLD fields as part of
> > the ACPI device nodes (under /sys/bus/acpi).
>
> That would be suboptimal for a few reasons:
>
> 1. The interface is potentially confusing.  User space would first
> need to locate the ACPI device interface corresponding to the given
> "real" device in order to use that information.
> 2. It doesn't scale beyond ACPI.
> 3. From the ACPI subsystem's perspective the choice of the "relevant"
> _PLD fields is arbitrary and exposing all of them is overkill for any
> use cases known to me.
> 4. The ACPI subsystem doesn't know the devices for which _PLD
> information should be exposed and there are some devices for which it
> is just not useful.
>
> > There we can guarantee
> > that we'll always be able to supply all the information in the _PDL if
> > needed. Since we would add these to the ACPI nodes, it would be
> > crystal clear to the userspace that this information is only available
> > on ACPI platforms.
>
> I'm not considering this as a feature.
>
> > Then if, and only if, we know that DT can supply the same information
> > (at least to some of it) I would start thinking about the alternative
> > interface to this information that we make part of the actual devices.
> > Since at this point we have already the primary ACPI specific
> > interface to this same information that guarantees that it can supply
> > all the details if necessary, we don't have to worry about having to
> > be able to do the same with this new interface. This interface can
> > just expose the common details that we know for sure that both ACPI
> > and DT can always supply.
>
> Well, there is another possible approach: Expose the information
> needed to address a particular use case in a way that doesn't strictly
> depend on ACPI and make this use ACPI as a backend.  Don't worry about
> the DT side of things.  If the generic interface is there and it is
> suitable enough, DT will be in the receiving end position with much
> less of a freedom to introduce a new interface for the same purpose.
>
> On the other hand, if _PLD information is exposed in an ACPI-specific
> way, it is almost guaranteed that there will be a DT-specific
> interface for the same thing and utilities wanting to be generic will
> need to support both of them which will be extra pain.  Some of them
> will choose to support the DT-specific interface only and we'll end up
> with utilities that can't be used on ACPI-based systems because of
> incompatible interfaces.  Been there already.  Thanks, but no thanks!

Hi Rafael,

Thank you for the feedback. If we add a generic location to type c
connector, would you suggest we do something similar to other devices
that would use PLD information? (like USB devices, HDMI ports, and so
on) Also, I am curious what you think about how to add generic
locations for Type A ports which I believe do not have connectors like
Type C. I would appreciate it if anyone can share any ideas. Thank you
very much!

Won

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

[Rafael J. Wysocki]

On Fri, Feb 18, 2022 at 2:15 AM Won Chung <wonchung@google.com> wrote:
>
> On Wed, Feb 16, 2022 at 8:39 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Feb 16, 2022 at 12:34 PM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > On Tue, Feb 15, 2022 at 02:54:11PM +0100, Rafael J. Wysocki wrote:
> > > > On Tue, Feb 15, 2022 at 11:14 AM Heikki Krogerus
> > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > >
> > > > > On Mon, Feb 14, 2022 at 02:58:44PM -0800, Won Chung wrote:
> > > > > > On Mon, Feb 14, 2022 at 12:30 PM Won Chung <wonchung@google.com> wrote:
> > > > > > >
> > > > > > > On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@google.com> wrote:
> > > > > > > > >
> > > > > > > > > When ACPI table includes _PLD fields for a device, create a new
> > > > > > > > > directory (pld) in sysfs to share _PLD fields.
> > > > > > > >
> > > > > > > > This version of the patch loos better to me, but I'm not sure if it
> > > > > > > > goes into the right direction overall.
> > > > > > > >
> > > > > > > > > Currently without PLD information, when there are multiple of same
> > > > > > > > > devices, it is hard to distinguish which device corresponds to which
> > > > > > > > > physical device in which location. For example, when there are two Type
> > > > > > > > > C connectors, it is hard to find out which connector corresponds to the
> > > > > > > > > Type C port on the left panel versus the Type C port on the right panel.
> > > > > > > >
> > > > > > > > So I think that this is your primary use case and I'm wondering if
> > > > > > > > this is the best way to address it.
> > > > > > > >
> > > > > > > > Namely, by exposing _PLD information under the ACPI device object,
> > > > > > > > you'll make user space wanting to use that information depend on this
> > > > > > > > interface, but the problem is not ACPI-specific (inevitably, it will
> > > > > > > > appear on systems using DT, sooner or later) and making the user space
> > > > > > > > interface related to it depend on ACPI doesn't look like a perfect
> > > > > > > > choice.
> > > > > > > >
> > > > > > > > IOW, why don't you create a proper ABI for this in the Type C
> > > > > > > > subsystem and expose the information needed by user space in a generic
> > > > > > > > way that can be based on the _PLD information on systems with ACPI?
> > > > > > >
> > > > > > > Hi Rafael,
> > > > > > >
> > > > > > > Thank you for the review.
> > > > > > >
> > > > > > > I was thinking that _PLD info is specific to ACPI since it is part of
> > > > > > > the ACPI table. Could you explain a little bit more on why you think
> > > > > > > exposing _PLD fields is not an ACPI-specific problem?
> > > > > >
> > > > > > Hi Rafael again,
> > > > > >
> > > > > > Sorry for the silly question here. I misunderstood your comment a bit,
> > > > > > but I talked to Benson and Prashant for clarification. I understand
> > > > > > now what you mean by it is not an ACPI-specific problem and exposing
> > > > > > PLD would depend on ACPI.
> > > > > >
> > > > > > >
> > > > > > > I gave an example of how _PLD fields can be used for specifying Type C
> > > > > > > connectors, but it is not Type C specific. For Chrome OS, we plan to
> > > > > > > initially add PLD to not only Type C connectors but also USB port
> > > > > > > devices (including Type C and Type A). Also, PLD can be used in the
> > > > > > > future for describing other types of ports too like HDMI. (Benson and
> > > > > > > Prashant, please correct or add if I am wrong or missing some
> > > > > > > information) Maybe my commit message was not detailed enough..
> > > > > > >
> > > > > > > I am also curious what Heikki thinks about this. Heikki, can you take
> > > > > > > a look and share your thoughts?
> > > > > >
> > > > > > I am still curious what you and Heikki think about this since it may
> > > > > > not be a Type C specific issue. We can start from adding generic
> > > > > > location info to Type C subsystem first, as you suggested, then
> > > > > > consider how to do the same for USB devices and Type A ports
> > > > > > afterwards. I would appreciate sharing any thoughts or feedback. Thank
> > > > > > you very much!
> > > > >
> > > > > Like you said, _PLD is not Type-C specific. We can't limit it to any
> > > > > specific device class. For example, I'm pretty sure that sooner or
> > > > > later we want to get this information in user space also with camera
> > > > > sensors, and probable with a few other things as well.
> > > > >
> > > > > I think the question here is, can we create a some kind of an
> > > > > abstraction layer for the user space that exposes the device location
> > > > > details in generic Linux specific way - so with ACPI it would utilise
> > > > > the _PLD, and with DT something else (today AFAIK DT does not have
> > > > > any way to describe locations of the devices). Maybe I'm wrong?
> > > >
> > > > No, you aren't.
> > > >
> > > > > But if that is the question, then IMO the answer is: maybe one day,
> > > > > but not today,
> > > >
> > > > Why not?
> > > >
> > > > > and even if we one day can come up with something like
> > > > > that, we still should expose the _PLD as ACPI specific information to
> > > > > the user space as is.
> > > >
> > > > Why would it need that information in this particular format?
> > > >
> > > > > Even if one day we have common sysfs attributes for all the devices
> > > > > that contain the location of the device in some form, those attributes
> > > > > will almost certainly have only a sub-set of the _PLD details, a
> > > > > sub-set that works also with DT.
> > > >
> > > > That doesn't have to be the case.
> > > >
> > > > However, things linke cpuidle have been invented to provide user space
> > > > interfaces for features that previously were only available on systems
> > > > with ACPI.  Why is _PLD different?
> > > >
> > > > > IMO the user space should always have access to all the necessary _PLD
> > > > > details in their raw form if needed, even if those common device
> > > > > location attributes exist - duplicated information or not.
> > > >
> > > > Again, why would it need that information?
> > >
> > > We don't know if we'll need that in the future, and that's the point.
> >
> > Well, for me that would be a good enough reason for avoiding to expose it.
> >
> > If there is no particular reason for exposing any information to user
> > space, I don't see why it should be exposed at all.
> >
> > There is some cost of exposing things to user space, so why pay it for
> > no benefit?
> >
> > > > > And debugfs
> > > > > unfortunately is also not OK for that, because the user space needs to
> > > > > be able to also rely on access to the additional details if needed.
> > > >
> > > > What additional details do you mean?
> > > >
> > > > > We can limit the _PLD fields that we expose to the ones that we know
> > > > > we need today (and probable should limit them to those), and we can of
> > > > > course have a Kconfig option for the _PLD sysfs information if we want
> > > > > to, but let's not start this by trying to figure out what kind of
> > > > > abstraction we want for this. Right now we simply can not do that.
> > > >
> > > > Why can't we?
> > >
> > > Right now we can't say for sure if DT can even supply the details that
> > > we need from _PLD. I don't think we can at the moment even say are the
> > > DT guys willing to support this at all.
> > >
> > > To play it safe, I would just supply the needed _PLD fields as part of
> > > the ACPI device nodes (under /sys/bus/acpi).
> >
> > That would be suboptimal for a few reasons:
> >
> > 1. The interface is potentially confusing.  User space would first
> > need to locate the ACPI device interface corresponding to the given
> > "real" device in order to use that information.
> > 2. It doesn't scale beyond ACPI.
> > 3. From the ACPI subsystem's perspective the choice of the "relevant"
> > _PLD fields is arbitrary and exposing all of them is overkill for any
> > use cases known to me.
> > 4. The ACPI subsystem doesn't know the devices for which _PLD
> > information should be exposed and there are some devices for which it
> > is just not useful.
> >
> > > There we can guarantee
> > > that we'll always be able to supply all the information in the _PDL if
> > > needed. Since we would add these to the ACPI nodes, it would be
> > > crystal clear to the userspace that this information is only available
> > > on ACPI platforms.
> >
> > I'm not considering this as a feature.
> >
> > > Then if, and only if, we know that DT can supply the same information
> > > (at least to some of it) I would start thinking about the alternative
> > > interface to this information that we make part of the actual devices.
> > > Since at this point we have already the primary ACPI specific
> > > interface to this same information that guarantees that it can supply
> > > all the details if necessary, we don't have to worry about having to
> > > be able to do the same with this new interface. This interface can
> > > just expose the common details that we know for sure that both ACPI
> > > and DT can always supply.
> >
> > Well, there is another possible approach: Expose the information
> > needed to address a particular use case in a way that doesn't strictly
> > depend on ACPI and make this use ACPI as a backend.  Don't worry about
> > the DT side of things.  If the generic interface is there and it is
> > suitable enough, DT will be in the receiving end position with much
> > less of a freedom to introduce a new interface for the same purpose.
> >
> > On the other hand, if _PLD information is exposed in an ACPI-specific
> > way, it is almost guaranteed that there will be a DT-specific
> > interface for the same thing and utilities wanting to be generic will
> > need to support both of them which will be extra pain.  Some of them
> > will choose to support the DT-specific interface only and we'll end up
> > with utilities that can't be used on ACPI-based systems because of
> > incompatible interfaces.  Been there already.  Thanks, but no thanks!
>
> Hi Rafael,
>
> Thank you for the feedback. If we add a generic location to type c
> connector, would you suggest we do something similar to other devices
> that would use PLD information? (like USB devices, HDMI ports, and so
> on).

If there is a specific use case for exposing that information to user
space, then yes, but it all depends on how user space is going to use
that information (or how you envision the usage of that information in
user space).

> Also, I am curious what you think about how to add generic
> locations for Type A ports which I believe do not have connectors like
> Type C. I would appreciate it if anyone can share any ideas. Thank you
> very much!

I'm not sure I understand the question correctly.  Can you clarify,
please?  Or better, give an example of what exactly you are referring
to?

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

[Won Chung]

On Fri, Feb 18, 2022 at 9:25 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Feb 18, 2022 at 2:15 AM Won Chung <wonchung@google.com> wrote:
> >
> > On Wed, Feb 16, 2022 at 8:39 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Feb 16, 2022 at 12:34 PM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > On Tue, Feb 15, 2022 at 02:54:11PM +0100, Rafael J. Wysocki wrote:
> > > > > On Tue, Feb 15, 2022 at 11:14 AM Heikki Krogerus
> > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > >
> > > > > > On Mon, Feb 14, 2022 at 02:58:44PM -0800, Won Chung wrote:
> > > > > > > On Mon, Feb 14, 2022 at 12:30 PM Won Chung <wonchung@google.com> wrote:
> > > > > > > >
> > > > > > > > On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@google.com> wrote:
> > > > > > > > > >
> > > > > > > > > > When ACPI table includes _PLD fields for a device, create a new
> > > > > > > > > > directory (pld) in sysfs to share _PLD fields.
> > > > > > > > >
> > > > > > > > > This version of the patch loos better to me, but I'm not sure if it
> > > > > > > > > goes into the right direction overall.
> > > > > > > > >
> > > > > > > > > > Currently without PLD information, when there are multiple of same
> > > > > > > > > > devices, it is hard to distinguish which device corresponds to which
> > > > > > > > > > physical device in which location. For example, when there are two Type
> > > > > > > > > > C connectors, it is hard to find out which connector corresponds to the
> > > > > > > > > > Type C port on the left panel versus the Type C port on the right panel.
> > > > > > > > >
> > > > > > > > > So I think that this is your primary use case and I'm wondering if
> > > > > > > > > this is the best way to address it.
> > > > > > > > >
> > > > > > > > > Namely, by exposing _PLD information under the ACPI device object,
> > > > > > > > > you'll make user space wanting to use that information depend on this
> > > > > > > > > interface, but the problem is not ACPI-specific (inevitably, it will
> > > > > > > > > appear on systems using DT, sooner or later) and making the user space
> > > > > > > > > interface related to it depend on ACPI doesn't look like a perfect
> > > > > > > > > choice.
> > > > > > > > >
> > > > > > > > > IOW, why don't you create a proper ABI for this in the Type C
> > > > > > > > > subsystem and expose the information needed by user space in a generic
> > > > > > > > > way that can be based on the _PLD information on systems with ACPI?
> > > > > > > >
> > > > > > > > Hi Rafael,
> > > > > > > >
> > > > > > > > Thank you for the review.
> > > > > > > >
> > > > > > > > I was thinking that _PLD info is specific to ACPI since it is part of
> > > > > > > > the ACPI table. Could you explain a little bit more on why you think
> > > > > > > > exposing _PLD fields is not an ACPI-specific problem?
> > > > > > >
> > > > > > > Hi Rafael again,
> > > > > > >
> > > > > > > Sorry for the silly question here. I misunderstood your comment a bit,
> > > > > > > but I talked to Benson and Prashant for clarification. I understand
> > > > > > > now what you mean by it is not an ACPI-specific problem and exposing
> > > > > > > PLD would depend on ACPI.
> > > > > > >
> > > > > > > >
> > > > > > > > I gave an example of how _PLD fields can be used for specifying Type C
> > > > > > > > connectors, but it is not Type C specific. For Chrome OS, we plan to
> > > > > > > > initially add PLD to not only Type C connectors but also USB port
> > > > > > > > devices (including Type C and Type A). Also, PLD can be used in the
> > > > > > > > future for describing other types of ports too like HDMI. (Benson and
> > > > > > > > Prashant, please correct or add if I am wrong or missing some
> > > > > > > > information) Maybe my commit message was not detailed enough..
> > > > > > > >
> > > > > > > > I am also curious what Heikki thinks about this. Heikki, can you take
> > > > > > > > a look and share your thoughts?
> > > > > > >
> > > > > > > I am still curious what you and Heikki think about this since it may
> > > > > > > not be a Type C specific issue. We can start from adding generic
> > > > > > > location info to Type C subsystem first, as you suggested, then
> > > > > > > consider how to do the same for USB devices and Type A ports
> > > > > > > afterwards. I would appreciate sharing any thoughts or feedback. Thank
> > > > > > > you very much!
> > > > > >
> > > > > > Like you said, _PLD is not Type-C specific. We can't limit it to any
> > > > > > specific device class. For example, I'm pretty sure that sooner or
> > > > > > later we want to get this information in user space also with camera
> > > > > > sensors, and probable with a few other things as well.
> > > > > >
> > > > > > I think the question here is, can we create a some kind of an
> > > > > > abstraction layer for the user space that exposes the device location
> > > > > > details in generic Linux specific way - so with ACPI it would utilise
> > > > > > the _PLD, and with DT something else (today AFAIK DT does not have
> > > > > > any way to describe locations of the devices). Maybe I'm wrong?
> > > > >
> > > > > No, you aren't.
> > > > >
> > > > > > But if that is the question, then IMO the answer is: maybe one day,
> > > > > > but not today,
> > > > >
> > > > > Why not?
> > > > >
> > > > > > and even if we one day can come up with something like
> > > > > > that, we still should expose the _PLD as ACPI specific information to
> > > > > > the user space as is.
> > > > >
> > > > > Why would it need that information in this particular format?
> > > > >
> > > > > > Even if one day we have common sysfs attributes for all the devices
> > > > > > that contain the location of the device in some form, those attributes
> > > > > > will almost certainly have only a sub-set of the _PLD details, a
> > > > > > sub-set that works also with DT.
> > > > >
> > > > > That doesn't have to be the case.
> > > > >
> > > > > However, things linke cpuidle have been invented to provide user space
> > > > > interfaces for features that previously were only available on systems
> > > > > with ACPI.  Why is _PLD different?
> > > > >
> > > > > > IMO the user space should always have access to all the necessary _PLD
> > > > > > details in their raw form if needed, even if those common device
> > > > > > location attributes exist - duplicated information or not.
> > > > >
> > > > > Again, why would it need that information?
> > > >
> > > > We don't know if we'll need that in the future, and that's the point.
> > >
> > > Well, for me that would be a good enough reason for avoiding to expose it.
> > >
> > > If there is no particular reason for exposing any information to user
> > > space, I don't see why it should be exposed at all.
> > >
> > > There is some cost of exposing things to user space, so why pay it for
> > > no benefit?
> > >
> > > > > > And debugfs
> > > > > > unfortunately is also not OK for that, because the user space needs to
> > > > > > be able to also rely on access to the additional details if needed.
> > > > >
> > > > > What additional details do you mean?
> > > > >
> > > > > > We can limit the _PLD fields that we expose to the ones that we know
> > > > > > we need today (and probable should limit them to those), and we can of
> > > > > > course have a Kconfig option for the _PLD sysfs information if we want
> > > > > > to, but let's not start this by trying to figure out what kind of
> > > > > > abstraction we want for this. Right now we simply can not do that.
> > > > >
> > > > > Why can't we?
> > > >
> > > > Right now we can't say for sure if DT can even supply the details that
> > > > we need from _PLD. I don't think we can at the moment even say are the
> > > > DT guys willing to support this at all.
> > > >
> > > > To play it safe, I would just supply the needed _PLD fields as part of
> > > > the ACPI device nodes (under /sys/bus/acpi).
> > >
> > > That would be suboptimal for a few reasons:
> > >
> > > 1. The interface is potentially confusing.  User space would first
> > > need to locate the ACPI device interface corresponding to the given
> > > "real" device in order to use that information.
> > > 2. It doesn't scale beyond ACPI.
> > > 3. From the ACPI subsystem's perspective the choice of the "relevant"
> > > _PLD fields is arbitrary and exposing all of them is overkill for any
> > > use cases known to me.
> > > 4. The ACPI subsystem doesn't know the devices for which _PLD
> > > information should be exposed and there are some devices for which it
> > > is just not useful.
> > >
> > > > There we can guarantee
> > > > that we'll always be able to supply all the information in the _PDL if
> > > > needed. Since we would add these to the ACPI nodes, it would be
> > > > crystal clear to the userspace that this information is only available
> > > > on ACPI platforms.
> > >
> > > I'm not considering this as a feature.
> > >
> > > > Then if, and only if, we know that DT can supply the same information
> > > > (at least to some of it) I would start thinking about the alternative
> > > > interface to this information that we make part of the actual devices.
> > > > Since at this point we have already the primary ACPI specific
> > > > interface to this same information that guarantees that it can supply
> > > > all the details if necessary, we don't have to worry about having to
> > > > be able to do the same with this new interface. This interface can
> > > > just expose the common details that we know for sure that both ACPI
> > > > and DT can always supply.
> > >
> > > Well, there is another possible approach: Expose the information
> > > needed to address a particular use case in a way that doesn't strictly
> > > depend on ACPI and make this use ACPI as a backend.  Don't worry about
> > > the DT side of things.  If the generic interface is there and it is
> > > suitable enough, DT will be in the receiving end position with much
> > > less of a freedom to introduce a new interface for the same purpose.
> > >
> > > On the other hand, if _PLD information is exposed in an ACPI-specific
> > > way, it is almost guaranteed that there will be a DT-specific
> > > interface for the same thing and utilities wanting to be generic will
> > > need to support both of them which will be extra pain.  Some of them
> > > will choose to support the DT-specific interface only and we'll end up
> > > with utilities that can't be used on ACPI-based systems because of
> > > incompatible interfaces.  Been there already.  Thanks, but no thanks!
> >
> > Hi Rafael,
> >
> > Thank you for the feedback. If we add a generic location to type c
> > connector, would you suggest we do something similar to other devices
> > that would use PLD information? (like USB devices, HDMI ports, and so
> > on).
>
> If there is a specific use case for exposing that information to user
> space, then yes, but it all depends on how user space is going to use
> that information (or how you envision the usage of that information in
> user space).
>
> > Also, I am curious what you think about how to add generic
> > locations for Type A ports which I believe do not have connectors like
> > Type C. I would appreciate it if anyone can share any ideas. Thank you
> > very much!
>
> I'm not sure I understand the question correctly.  Can you clarify,
> please?  Or better, give an example of what exactly you are referring
> to?

Hi Rafael,

For Type C ports, we have Type C connectors at /sys/class/typec in
which we can add generic location information, as you suggested.
However, since Type A ports do not have such connectors, I was curious
what would be a good way to add a generic location, instead of
exposing _PLD directly in the USB-A port's ACPI device.

Now that I think about it again and look through sysfs, I think we can
also add generic location to
/sys/bus/usb/devices/.../<hub_interface>/port<X>, some of which
represents Type A ports. Benson, do you think this could be a good way
for Type A ports? Who would be a good person to get feedback on this?

Heikki, I am also convinced by Rafael's feedback since userspace code
would also be quite ACPI-specific to access _PLD fields from ACPI
device sysfs. Would you agree? Regarding your concerns with DT, we can
look for some ways to have similar location information on systems
with DT. Would it sound okay to you to add generic location in Type C
connectors? If it does, I can start working on it and send patches for
review. If it does not, I would appreciate it if you can share your
thoughts on possible alternative approach.

Thank you very much all for the feedback!
Won

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

[Won Chung]

On Fri, Feb 18, 2022 at 11:48 AM Won Chung <wonchung@google.com> wrote:
>
> On Fri, Feb 18, 2022 at 9:25 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Fri, Feb 18, 2022 at 2:15 AM Won Chung <wonchung@google.com> wrote:
> > >
> > > On Wed, Feb 16, 2022 at 8:39 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Wed, Feb 16, 2022 at 12:34 PM Heikki Krogerus
> > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > >
> > > > > On Tue, Feb 15, 2022 at 02:54:11PM +0100, Rafael J. Wysocki wrote:
> > > > > > On Tue, Feb 15, 2022 at 11:14 AM Heikki Krogerus
> > > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > >
> > > > > > > On Mon, Feb 14, 2022 at 02:58:44PM -0800, Won Chung wrote:
> > > > > > > > On Mon, Feb 14, 2022 at 12:30 PM Won Chung <wonchung@google.com> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > > > > > >
> > > > > > > > > > On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@google.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > When ACPI table includes _PLD fields for a device, create a new
> > > > > > > > > > > directory (pld) in sysfs to share _PLD fields.
> > > > > > > > > >
> > > > > > > > > > This version of the patch loos better to me, but I'm not sure if it
> > > > > > > > > > goes into the right direction overall.
> > > > > > > > > >
> > > > > > > > > > > Currently without PLD information, when there are multiple of same
> > > > > > > > > > > devices, it is hard to distinguish which device corresponds to which
> > > > > > > > > > > physical device in which location. For example, when there are two Type
> > > > > > > > > > > C connectors, it is hard to find out which connector corresponds to the
> > > > > > > > > > > Type C port on the left panel versus the Type C port on the right panel.
> > > > > > > > > >
> > > > > > > > > > So I think that this is your primary use case and I'm wondering if
> > > > > > > > > > this is the best way to address it.
> > > > > > > > > >
> > > > > > > > > > Namely, by exposing _PLD information under the ACPI device object,
> > > > > > > > > > you'll make user space wanting to use that information depend on this
> > > > > > > > > > interface, but the problem is not ACPI-specific (inevitably, it will
> > > > > > > > > > appear on systems using DT, sooner or later) and making the user space
> > > > > > > > > > interface related to it depend on ACPI doesn't look like a perfect
> > > > > > > > > > choice.
> > > > > > > > > >
> > > > > > > > > > IOW, why don't you create a proper ABI for this in the Type C
> > > > > > > > > > subsystem and expose the information needed by user space in a generic
> > > > > > > > > > way that can be based on the _PLD information on systems with ACPI?
> > > > > > > > >
> > > > > > > > > Hi Rafael,
> > > > > > > > >
> > > > > > > > > Thank you for the review.
> > > > > > > > >
> > > > > > > > > I was thinking that _PLD info is specific to ACPI since it is part of
> > > > > > > > > the ACPI table. Could you explain a little bit more on why you think
> > > > > > > > > exposing _PLD fields is not an ACPI-specific problem?
> > > > > > > >
> > > > > > > > Hi Rafael again,
> > > > > > > >
> > > > > > > > Sorry for the silly question here. I misunderstood your comment a bit,
> > > > > > > > but I talked to Benson and Prashant for clarification. I understand
> > > > > > > > now what you mean by it is not an ACPI-specific problem and exposing
> > > > > > > > PLD would depend on ACPI.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > I gave an example of how _PLD fields can be used for specifying Type C
> > > > > > > > > connectors, but it is not Type C specific. For Chrome OS, we plan to
> > > > > > > > > initially add PLD to not only Type C connectors but also USB port
> > > > > > > > > devices (including Type C and Type A). Also, PLD can be used in the
> > > > > > > > > future for describing other types of ports too like HDMI. (Benson and
> > > > > > > > > Prashant, please correct or add if I am wrong or missing some
> > > > > > > > > information) Maybe my commit message was not detailed enough..
> > > > > > > > >
> > > > > > > > > I am also curious what Heikki thinks about this. Heikki, can you take
> > > > > > > > > a look and share your thoughts?
> > > > > > > >
> > > > > > > > I am still curious what you and Heikki think about this since it may
> > > > > > > > not be a Type C specific issue. We can start from adding generic
> > > > > > > > location info to Type C subsystem first, as you suggested, then
> > > > > > > > consider how to do the same for USB devices and Type A ports
> > > > > > > > afterwards. I would appreciate sharing any thoughts or feedback. Thank
> > > > > > > > you very much!
> > > > > > >
> > > > > > > Like you said, _PLD is not Type-C specific. We can't limit it to any
> > > > > > > specific device class. For example, I'm pretty sure that sooner or
> > > > > > > later we want to get this information in user space also with camera
> > > > > > > sensors, and probable with a few other things as well.
> > > > > > >
> > > > > > > I think the question here is, can we create a some kind of an
> > > > > > > abstraction layer for the user space that exposes the device location
> > > > > > > details in generic Linux specific way - so with ACPI it would utilise
> > > > > > > the _PLD, and with DT something else (today AFAIK DT does not have
> > > > > > > any way to describe locations of the devices). Maybe I'm wrong?
> > > > > >
> > > > > > No, you aren't.
> > > > > >
> > > > > > > But if that is the question, then IMO the answer is: maybe one day,
> > > > > > > but not today,
> > > > > >
> > > > > > Why not?
> > > > > >
> > > > > > > and even if we one day can come up with something like
> > > > > > > that, we still should expose the _PLD as ACPI specific information to
> > > > > > > the user space as is.
> > > > > >
> > > > > > Why would it need that information in this particular format?
> > > > > >
> > > > > > > Even if one day we have common sysfs attributes for all the devices
> > > > > > > that contain the location of the device in some form, those attributes
> > > > > > > will almost certainly have only a sub-set of the _PLD details, a
> > > > > > > sub-set that works also with DT.
> > > > > >
> > > > > > That doesn't have to be the case.
> > > > > >
> > > > > > However, things linke cpuidle have been invented to provide user space
> > > > > > interfaces for features that previously were only available on systems
> > > > > > with ACPI.  Why is _PLD different?
> > > > > >
> > > > > > > IMO the user space should always have access to all the necessary _PLD
> > > > > > > details in their raw form if needed, even if those common device
> > > > > > > location attributes exist - duplicated information or not.
> > > > > >
> > > > > > Again, why would it need that information?
> > > > >
> > > > > We don't know if we'll need that in the future, and that's the point.
> > > >
> > > > Well, for me that would be a good enough reason for avoiding to expose it.
> > > >
> > > > If there is no particular reason for exposing any information to user
> > > > space, I don't see why it should be exposed at all.
> > > >
> > > > There is some cost of exposing things to user space, so why pay it for
> > > > no benefit?
> > > >
> > > > > > > And debugfs
> > > > > > > unfortunately is also not OK for that, because the user space needs to
> > > > > > > be able to also rely on access to the additional details if needed.
> > > > > >
> > > > > > What additional details do you mean?
> > > > > >
> > > > > > > We can limit the _PLD fields that we expose to the ones that we know
> > > > > > > we need today (and probable should limit them to those), and we can of
> > > > > > > course have a Kconfig option for the _PLD sysfs information if we want
> > > > > > > to, but let's not start this by trying to figure out what kind of
> > > > > > > abstraction we want for this. Right now we simply can not do that.
> > > > > >
> > > > > > Why can't we?
> > > > >
> > > > > Right now we can't say for sure if DT can even supply the details that
> > > > > we need from _PLD. I don't think we can at the moment even say are the
> > > > > DT guys willing to support this at all.
> > > > >
> > > > > To play it safe, I would just supply the needed _PLD fields as part of
> > > > > the ACPI device nodes (under /sys/bus/acpi).
> > > >
> > > > That would be suboptimal for a few reasons:
> > > >
> > > > 1. The interface is potentially confusing.  User space would first
> > > > need to locate the ACPI device interface corresponding to the given
> > > > "real" device in order to use that information.
> > > > 2. It doesn't scale beyond ACPI.
> > > > 3. From the ACPI subsystem's perspective the choice of the "relevant"
> > > > _PLD fields is arbitrary and exposing all of them is overkill for any
> > > > use cases known to me.
> > > > 4. The ACPI subsystem doesn't know the devices for which _PLD
> > > > information should be exposed and there are some devices for which it
> > > > is just not useful.
> > > >
> > > > > There we can guarantee
> > > > > that we'll always be able to supply all the information in the _PDL if
> > > > > needed. Since we would add these to the ACPI nodes, it would be
> > > > > crystal clear to the userspace that this information is only available
> > > > > on ACPI platforms.
> > > >
> > > > I'm not considering this as a feature.
> > > >
> > > > > Then if, and only if, we know that DT can supply the same information
> > > > > (at least to some of it) I would start thinking about the alternative
> > > > > interface to this information that we make part of the actual devices.
> > > > > Since at this point we have already the primary ACPI specific
> > > > > interface to this same information that guarantees that it can supply
> > > > > all the details if necessary, we don't have to worry about having to
> > > > > be able to do the same with this new interface. This interface can
> > > > > just expose the common details that we know for sure that both ACPI
> > > > > and DT can always supply.
> > > >
> > > > Well, there is another possible approach: Expose the information
> > > > needed to address a particular use case in a way that doesn't strictly
> > > > depend on ACPI and make this use ACPI as a backend.  Don't worry about
> > > > the DT side of things.  If the generic interface is there and it is
> > > > suitable enough, DT will be in the receiving end position with much
> > > > less of a freedom to introduce a new interface for the same purpose.
> > > >
> > > > On the other hand, if _PLD information is exposed in an ACPI-specific
> > > > way, it is almost guaranteed that there will be a DT-specific
> > > > interface for the same thing and utilities wanting to be generic will
> > > > need to support both of them which will be extra pain.  Some of them
> > > > will choose to support the DT-specific interface only and we'll end up
> > > > with utilities that can't be used on ACPI-based systems because of
> > > > incompatible interfaces.  Been there already.  Thanks, but no thanks!
> > >
> > > Hi Rafael,
> > >
> > > Thank you for the feedback. If we add a generic location to type c
> > > connector, would you suggest we do something similar to other devices
> > > that would use PLD information? (like USB devices, HDMI ports, and so
> > > on).
> >
> > If there is a specific use case for exposing that information to user
> > space, then yes, but it all depends on how user space is going to use
> > that information (or how you envision the usage of that information in
> > user space).
> >
> > > Also, I am curious what you think about how to add generic
> > > locations for Type A ports which I believe do not have connectors like
> > > Type C. I would appreciate it if anyone can share any ideas. Thank you
> > > very much!
> >
> > I'm not sure I understand the question correctly.  Can you clarify,
> > please?  Or better, give an example of what exactly you are referring
> > to?
>
> Hi Rafael,
>
> For Type C ports, we have Type C connectors at /sys/class/typec in
> which we can add generic location information, as you suggested.
> However, since Type A ports do not have such connectors, I was curious
> what would be a good way to add a generic location, instead of
> exposing _PLD directly in the USB-A port's ACPI device.
>
> Now that I think about it again and look through sysfs, I think we can
> also add generic location to
> /sys/bus/usb/devices/.../<hub_interface>/port<X>, some of which
> represents Type A ports. Benson, do you think this could be a good way
> for Type A ports? Who would be a good person to get feedback on this?
>
> Heikki, I am also convinced by Rafael's feedback since userspace code
> would also be quite ACPI-specific to access _PLD fields from ACPI
> device sysfs. Would you agree? Regarding your concerns with DT, we can
> look for some ways to have similar location information on systems
> with DT. Would it sound okay to you to add generic location in Type C
> connectors? If it does, I can start working on it and send patches for
> review. If it does not, I would appreciate it if you can share your
> thoughts on possible alternative approach.
>
> Thank you very much all for the feedback!
> Won

Hi all,

As a follow up, I sent another patch with generic location added to
typec connector, instead of acpi. Please take a look and share some
feedback if you have time. Thanks!

Won

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

[Rafael J. Wysocki]

Adding Greg, who should be involved in this discussion IMO.

On Mon, Feb 14, 2022 at 11:59 PM Won Chung <wonchung@google.com> wrote:
>
> On Mon, Feb 14, 2022 at 12:30 PM Won Chung <wonchung@google.com> wrote:
> >
> > On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@google.com> wrote:
> > > >
> > > > When ACPI table includes _PLD fields for a device, create a new
> > > > directory (pld) in sysfs to share _PLD fields.
> > >
> > > This version of the patch loos better to me, but I'm not sure if it
> > > goes into the right direction overall.
> > >
> > > > Currently without PLD information, when there are multiple of same
> > > > devices, it is hard to distinguish which device corresponds to which
> > > > physical device in which location. For example, when there are two Type
> > > > C connectors, it is hard to find out which connector corresponds to the
> > > > Type C port on the left panel versus the Type C port on the right panel.
> > >
> > > So I think that this is your primary use case and I'm wondering if
> > > this is the best way to address it.
> > >
> > > Namely, by exposing _PLD information under the ACPI device object,
> > > you'll make user space wanting to use that information depend on this
> > > interface, but the problem is not ACPI-specific (inevitably, it will
> > > appear on systems using DT, sooner or later) and making the user space
> > > interface related to it depend on ACPI doesn't look like a perfect
> > > choice.
> > >
> > > IOW, why don't you create a proper ABI for this in the Type C
> > > subsystem and expose the information needed by user space in a generic
> > > way that can be based on the _PLD information on systems with ACPI?
> >
> > Hi Rafael,
> >
> > Thank you for the review.
> >
> > I was thinking that _PLD info is specific to ACPI since it is part of
> > the ACPI table. Could you explain a little bit more on why you think
> > exposing _PLD fields is not an ACPI-specific problem?

_PLD is an interface defined by ACPI, but its purpose is not ACPI-specific.

> Hi Rafael again,
>
> Sorry for the silly question here. I misunderstood your comment a bit,
> but I talked to Benson and Prashant for clarification. I understand
> now what you mean by it is not an ACPI-specific problem and exposing
> PLD would depend on ACPI.

Right.

> >
> > I gave an example of how _PLD fields can be used for specifying Type C
> > connectors, but it is not Type C specific. For Chrome OS, we plan to
> > initially add PLD to not only Type C connectors but also USB port
> > devices (including Type C and Type A). Also, PLD can be used in the
> > future for describing other types of ports too like HDMI. (Benson and
> > Prashant, please correct or add if I am wrong or missing some
> > information) Maybe my commit message was not detailed enough..
> >
> > I am also curious what Heikki thinks about this. Heikki, can you take
> > a look and share your thoughts?
>
> I am still curious what you and Heikki think about this since it may
> not be a Type C specific issue. We can start from adding generic
> location info to Type C subsystem first, as you suggested, then
> consider how to do the same for USB devices and Type A ports
> afterwards. I would appreciate sharing any thoughts or feedback. Thank
> you very much!

I don't really think that this is a Type C problem either.

It has existed for a long time in the USB world, for example, or
wherever there are user-accessible ports, but it looks like in the
Type C case it has become vitally important.

My point is that writing user space depending on accessing _PLD
information exposed under an ACPI device interface that only
corresponds to the device in question and in the ACPI-specific format
would be a mistake (Greg, please let me know if you disagree).  That's
because (a) it would depend on ACPI tables being present (so it
wouldn't work on systems without them) and (b) it would depend on the
format of data which covers information that isn't likely to be
relevant.

If this information is exposed by the kernel verbatim and user space
depending on this information is created, it will not be possible to
unexpose it even if it turns out that exposing it has been a mistake.

OTOH, if only the relevant pieces of information are exposed in a
generic way, it is always possible to expose more pieces of it in the
future as needed.

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

[Rafael J. Wysocki]

On Tue, Feb 15, 2022 at 3:04 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> Adding Greg, who should be involved in this discussion IMO.
>
> On Mon, Feb 14, 2022 at 11:59 PM Won Chung <wonchung@google.com> wrote:
> >
> > On Mon, Feb 14, 2022 at 12:30 PM Won Chung <wonchung@google.com> wrote:
> > >
> > > On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@google.com> wrote:
> > > > >
> > > > > When ACPI table includes _PLD fields for a device, create a new
> > > > > directory (pld) in sysfs to share _PLD fields.
> > > >
> > > > This version of the patch loos better to me, but I'm not sure if it
> > > > goes into the right direction overall.
> > > >
> > > > > Currently without PLD information, when there are multiple of same
> > > > > devices, it is hard to distinguish which device corresponds to which
> > > > > physical device in which location. For example, when there are two Type
> > > > > C connectors, it is hard to find out which connector corresponds to the
> > > > > Type C port on the left panel versus the Type C port on the right panel.
> > > >
> > > > So I think that this is your primary use case and I'm wondering if
> > > > this is the best way to address it.
> > > >
> > > > Namely, by exposing _PLD information under the ACPI device object,
> > > > you'll make user space wanting to use that information depend on this
> > > > interface, but the problem is not ACPI-specific (inevitably, it will
> > > > appear on systems using DT, sooner or later) and making the user space
> > > > interface related to it depend on ACPI doesn't look like a perfect
> > > > choice.
> > > >
> > > > IOW, why don't you create a proper ABI for this in the Type C
> > > > subsystem and expose the information needed by user space in a generic
> > > > way that can be based on the _PLD information on systems with ACPI?
> > >
> > > Hi Rafael,
> > >
> > > Thank you for the review.
> > >
> > > I was thinking that _PLD info is specific to ACPI since it is part of
> > > the ACPI table. Could you explain a little bit more on why you think
> > > exposing _PLD fields is not an ACPI-specific problem?
>
> _PLD is an interface defined by ACPI, but its purpose is not ACPI-specific.
>
> > Hi Rafael again,
> >
> > Sorry for the silly question here. I misunderstood your comment a bit,
> > but I talked to Benson and Prashant for clarification. I understand
> > now what you mean by it is not an ACPI-specific problem and exposing
> > PLD would depend on ACPI.
>
> Right.
>
> > >
> > > I gave an example of how _PLD fields can be used for specifying Type C
> > > connectors, but it is not Type C specific. For Chrome OS, we plan to
> > > initially add PLD to not only Type C connectors but also USB port
> > > devices (including Type C and Type A). Also, PLD can be used in the
> > > future for describing other types of ports too like HDMI. (Benson and
> > > Prashant, please correct or add if I am wrong or missing some
> > > information) Maybe my commit message was not detailed enough..
> > >
> > > I am also curious what Heikki thinks about this. Heikki, can you take
> > > a look and share your thoughts?
> >
> > I am still curious what you and Heikki think about this since it may
> > not be a Type C specific issue. We can start from adding generic
> > location info to Type C subsystem first, as you suggested, then
> > consider how to do the same for USB devices and Type A ports
> > afterwards. I would appreciate sharing any thoughts or feedback. Thank
> > you very much!
>
> I don't really think that this is a Type C problem either.
>
> It has existed for a long time in the USB world, for example, or
> wherever there are user-accessible ports, but it looks like in the
> Type C case it has become vitally important.
>
> My point is that writing user space depending on accessing _PLD
> information exposed under an ACPI device interface that only
> corresponds to the device in question and in the ACPI-specific format
> would be a mistake (Greg, please let me know if you disagree).  That's
> because (a) it would depend on ACPI tables being present (so it
> wouldn't work on systems without them) and (b) it would depend on the
> format of data which covers information that isn't likely to be
> relevant.

Also finding _PLD information for a given "real" device would not be
particularly straightforward as it would involve looking up an ACPI
device interface corresponding to it in the first place and then
retrieving the _PLD data from it.

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

[Greg Kroah-Hartman]

On Tue, Feb 15, 2022 at 03:04:54PM +0100, Rafael J. Wysocki wrote:
> Adding Greg, who should be involved in this discussion IMO.
> 
> On Mon, Feb 14, 2022 at 11:59 PM Won Chung <wonchung@google.com> wrote:
> >
> > On Mon, Feb 14, 2022 at 12:30 PM Won Chung <wonchung@google.com> wrote:
> > >
> > > On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@google.com> wrote:
> > > > >
> > > > > When ACPI table includes _PLD fields for a device, create a new
> > > > > directory (pld) in sysfs to share _PLD fields.
> > > >
> > > > This version of the patch loos better to me, but I'm not sure if it
> > > > goes into the right direction overall.
> > > >
> > > > > Currently without PLD information, when there are multiple of same
> > > > > devices, it is hard to distinguish which device corresponds to which
> > > > > physical device in which location. For example, when there are two Type
> > > > > C connectors, it is hard to find out which connector corresponds to the
> > > > > Type C port on the left panel versus the Type C port on the right panel.
> > > >
> > > > So I think that this is your primary use case and I'm wondering if
> > > > this is the best way to address it.
> > > >
> > > > Namely, by exposing _PLD information under the ACPI device object,
> > > > you'll make user space wanting to use that information depend on this
> > > > interface, but the problem is not ACPI-specific (inevitably, it will
> > > > appear on systems using DT, sooner or later) and making the user space
> > > > interface related to it depend on ACPI doesn't look like a perfect
> > > > choice.
> > > >
> > > > IOW, why don't you create a proper ABI for this in the Type C
> > > > subsystem and expose the information needed by user space in a generic
> > > > way that can be based on the _PLD information on systems with ACPI?
> > >
> > > Hi Rafael,
> > >
> > > Thank you for the review.
> > >
> > > I was thinking that _PLD info is specific to ACPI since it is part of
> > > the ACPI table. Could you explain a little bit more on why you think
> > > exposing _PLD fields is not an ACPI-specific problem?
> 
> _PLD is an interface defined by ACPI, but its purpose is not ACPI-specific.
> 
> > Hi Rafael again,
> >
> > Sorry for the silly question here. I misunderstood your comment a bit,
> > but I talked to Benson and Prashant for clarification. I understand
> > now what you mean by it is not an ACPI-specific problem and exposing
> > PLD would depend on ACPI.
> 
> Right.
> 
> > >
> > > I gave an example of how _PLD fields can be used for specifying Type C
> > > connectors, but it is not Type C specific. For Chrome OS, we plan to
> > > initially add PLD to not only Type C connectors but also USB port
> > > devices (including Type C and Type A). Also, PLD can be used in the
> > > future for describing other types of ports too like HDMI. (Benson and
> > > Prashant, please correct or add if I am wrong or missing some
> > > information) Maybe my commit message was not detailed enough..
> > >
> > > I am also curious what Heikki thinks about this. Heikki, can you take
> > > a look and share your thoughts?
> >
> > I am still curious what you and Heikki think about this since it may
> > not be a Type C specific issue. We can start from adding generic
> > location info to Type C subsystem first, as you suggested, then
> > consider how to do the same for USB devices and Type A ports
> > afterwards. I would appreciate sharing any thoughts or feedback. Thank
> > you very much!
> 
> I don't really think that this is a Type C problem either.
> 
> It has existed for a long time in the USB world, for example, or
> wherever there are user-accessible ports, but it looks like in the
> Type C case it has become vitally important.
> 
> My point is that writing user space depending on accessing _PLD
> information exposed under an ACPI device interface that only
> corresponds to the device in question and in the ACPI-specific format
> would be a mistake (Greg, please let me know if you disagree).  That's
> because (a) it would depend on ACPI tables being present (so it
> wouldn't work on systems without them) and (b) it would depend on the
> format of data which covers information that isn't likely to be
> relevant.
> 
> If this information is exposed by the kernel verbatim and user space
> depending on this information is created, it will not be possible to
> unexpose it even if it turns out that exposing it has been a mistake.
> 
> OTOH, if only the relevant pieces of information are exposed in a
> generic way, it is always possible to expose more pieces of it in the
> future as needed.

There are pending patches on the linux-usb mailing list from Heikki to
help expand on the typec information in sysfs in a generic way.  Won,
please work with the linux-usb developers on this change and do not do
anything that is ACPI-specific and only.  That way will not be good for
anyone involved.

Please see:
	https://lore.kernel.org/r/20220203144657.16527-1-heikki.krogerus@linux.intel.com

for the current discussion.

thanks,

greg k-h