Message-ID: <CAOvb9yhGMtA2+jzQ5KxBRDDtASQfA3BPxnHhCrgd_8E4umtiig@mail.gmail.com>
Date:   Thu, 17 Feb 2022 17:15:23 -0800
From:   Won Chung <wonchung@...gle.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Len Brown <lenb@...nel.org>,
        Benson Leung <bleung@...omium.org>,
        Prashant Malani <pmalani@...omium.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <greg@...ah.com>
Subject: Re: [PATCH v6] ACPI: device_sysfs: Add sysfs support for _PLD

On Wed, Feb 16, 2022 at 8:39 AM Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> On Wed, Feb 16, 2022 at 12:34 PM Heikki Krogerus
> <heikki.krogerus@...ux.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@...ux.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@...gle.com> wrote:
> > > > > >
> > > > > > On Mon, Feb 14, 2022 at 11:12 AM Rafael J. Wysocki <rafael@...nel.org> wrote:
> > > > > > >
> > > > > > > On Fri, Feb 11, 2022 at 3:30 AM Won Chung <wonchung@...gle.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

Powered by blists - more mailing lists