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

[Rafael J. Wysocki]

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@xxxxxxxxxxxxx> 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.

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?

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?

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).  The other one would
be to properly register the GGL prefix for Google and establish a process for
allocating IDs with that prefix internally.

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?

Thanks!