RE: [PATCH v4] platform: x86: Add ACPI driver for ChromeOS

[<Mario.Limonciello@dell.com>]

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> owner@vger.kernel.org> On Behalf Of Enric Balletbo i Serra
> Sent: Wednesday, June 10, 2020 4:22 PM
> To: Rafael J. Wysocki
> Cc: Rafael J. Wysocki; Linux Kernel Mailing List; ACPI Devel Maling List;
> Len Brown; Collabora Kernel ML; Guenter Roeck; Benson Leung; Dmitry
> Torokhov; Gwendal Grignou; vbendeb@chromium.org; Andy Shevchenko; Ayman
> Bagabas; Benjamin Tissoires; Blaž Hrastnik; Darren Hart; Dmitry Torokhov;
> Greg Kroah-Hartman; Hans de Goede; Jeremy Soller; Mattias Jacobsson; Mauro
> Carvalho Chehab; Rajat Jain; Srinivas Pandruvada; platform-driver-
> x86@vger.kernel.org
> Subject: Re: [PATCH v4] platform: x86: Add ACPI driver for ChromeOS
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi Rafael,
> 
> Many thanks for your feedback. See my answers inline.
> 
> On 5/6/20 13:17, Rafael J. Wysocki wrote:
> > On Tuesday, April 14, 2020 4:35:38 PM CEST Enric Balletbo i Serra wrote:
> >> Hi Rafael,
> >>
> >> On 13/4/20 22:41, Rafael J. Wysocki wrote:
> >>> On Mon, Apr 13, 2020 at 3:46 PM Enric Balletbo i Serra
> >>> <enric.balletbo@collabora.com> wrote:
> >>>>
> >>>> This driver attaches to the ChromeOS ACPI device and then exports the
> values
> >>>> reported by the ACPI in a sysfs directory. These values are not
> exported
> >>>> via the standard ACPI tables, hence a specific driver is needed to do
> >>>> it.
> >>>
> >>> So how exactly are they exported?
> >>>
> >>
> >> They are exported through sysfs.
> >>
> >>>> The ACPI values are presented in the string form (numbers as decimal
> >>>> values) or binary blobs, and can be accessed as the contents of the
> >>>> appropriate read only files in the standard ACPI devices sysfs
> directory tree.
> >>>
> >>> My understanding based on a cursory look at the patch is that there is
> >>> an ACPI device with _HID equal to "GGL0001"  and one or more special
> >>> methods under it that return values which you want to export over
> >>> sysfs as binary attributes.  They appear to be read-only.
> >>>
> >>
> >> Exactly, there is an ACPI device equal to "GGL0001" and one special
> method
> >> called MLST that returns a list of the other control methods supported
> by the
> >> Chrome OS hardware device. The driver calls the special MLST method and
> goes
> >> through the list.
> >>
> >>> I guess that these data are to be consubed by user space?
> >>>
> >>
> >> Yes, this is used by user space, to be more specific ChromeOS userspace
> uses it.
> >
> > Well, let me start over.
> >
> > The subject and changelog of this patch are not precise enough IMO and
> there is
> > not enough information in the latter.
> >
> 
> Ok, I can improve that.
> 
> > It is not clear what "ACPI driver for ChromeOS" means.  There may be many
> ACPI
> > drivers in a Linux-based system as a rule.
> >
> > It is unclear what the ChromeOS ACPI device is and why it is there.  Is
> there
> > any documentation of it you can point me to?
> >
> 
> I'm afraid, I don't think there is any documentation, I'll ask around.
> 
> > It is unclear what you mean by "These values are not exported via the
> standard
> > ACPI tables".
> >
> > It looks like (but it is not actually documented in any way) the idea is
> to
> > get to the ACPI device object with _HID returning "GGL0001", evaluate the
> > MLST method under it and then evaluate the methods listed by it and
> export the
> > data returned by them via sysfs, under the "GGL0001" device on the "acpi"
> bus.
> > Is this correct?
> >
> 
> Yes, this is correct.
> 
> > If so, there is a couple of issues here.
> >
> > First off, GGL0001 is not a valid ACPI device ID, because the GGL prefix
> is not
> > present in the list at https://uefi.org/acpi_id_list
> >
> > There are two ways to address that.  One would be to take the GOOG prefix
> > (present in the list above), append a proper unique number (if I were to
> > guess, I would say that 0001 had been reserved already) to it and then
> > put the resulting device ID into the firmware, to be returned _HID for
> the
> > device in question (you can add a _CID returning "GGL0001" so it can be
> > found by the old invalid ID at least from the kernel).
> 
> As Dmitry said, this is not going to happen.

I think it's probably worth grouping "existing" platforms and new platforms
separately.  More below.

> 
> 
> > The other one would
> > be to properly register the GGL prefix for Google and establish a process
> for
> > allocating IDs with that prefix internally.
> >
> 
> IIUC I think this is the option we should go, although I am not really sure
> how
> to do it (I will investigate or ask).
> 
> To give you some references, if I'm not wrong, this prefix is used in all
> or
> almost all Intel Chromebook devices (auron, cyan, eve, fizz, hatch,
> octopus,
> poppy, strago ...) The ACPI source for this device can be found here [1],
> and,
> if not all, almost all Intel based Chromebooks are shipped with the
> firmware
> that supports this.

You can potentially carry a small patch in your downstream kernel for the
legacy stuff until it reaches EOL.  At least for the new stuff you could
enact a process that properly reserves unique numbers and changes the driver
when the interface provided by the ACPI device has changed.

> 
> > Next, device attributes in sysfs are part of the kernel ABI and once
> defined,
> > they cannot change (exceptions happen, but rarely), so you must guarantee
> > that whatever appears in there, will always be present for devices with
> the
> > given device ID in the future in the same format.
> >
> > Can you actually guarantee that?  If so, what is that guarantee based on?
> >
> 
> Although is not really documented, can we say that this is a standard "de
> facto"
> assuming that there are lots of devices in the field and for a long time
> with
> that? Can this be a guarantee?
> 
> > Thanks!
> >
> >
> >
> 
> Thanks!
> 
> [1]
> https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/refs/he
> ads/chromeos-2016.05/src/vendorcode/google/chromeos/acpi/chromeos.asl