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@xxxxxxxxxx> wrote:
>
> On Mon, Feb 14, 2022 at 12:30 PM Won Chung <wonchung@xxxxxxxxxx> wrote:
> >
> > On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@xxxxxxxxxx> 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.