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

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

[Enric Balletbo i Serra]

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

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Hi,

I sent the first patch of this three years ago [1], and then, due a lack
of time and a change of priorities I forget about it. Now, I completely
reworked that driver and keep only in this driver the part that is related
to export the sysfs attributes, making it a bit more easy to review,
hopefully I can get your feedback and I'll able to address it now to
finally land this patch.

These properties are used on some userspace tools available in the
ChromeOS userspace like the crash reporter. This driver was tested on a
Samus Chromebook with following data and checking that matches with the
data reported with the downstream driver.

Also installed/removed the driver several times.

 /sys/bus/acpi/devices/GGL0001:00/BINF.2 : 1
 /sys/bus/acpi/devices/GGL0001:00/FMAP : -2031616
 /sys/bus/acpi/devices/GGL0001:00/HWID : SAMUS E25-G7R-W35
 /sys/bus/acpi/devices/GGL0001:00/BINF.0 : 0
 /sys/bus/acpi/devices/GGL0001:00/GPIO.0/GPIO.2 : -1
 /sys/bus/acpi/devices/GGL0001:00/GPIO.0/GPIO.0 : 1
 /sys/bus/acpi/devices/GGL0001:00/GPIO.0/GPIO.3 : INT3437:00
 /sys/bus/acpi/devices/GGL0001:00/GPIO.0/GPIO.1 : 0
 /sys/bus/acpi/devices/GGL0001:00/FRID : Google_Samus.6300.102.0
 /sys/bus/acpi/devices/GGL0001:00/VBNV.0 : 38
 /sys/bus/acpi/devices/GGL0001:00/BINF.3 : 2
 /sys/bus/acpi/devices/GGL0001:00/BINF.1 : 1
 /sys/bus/acpi/devices/GGL0001:00/GPIO.1/GPIO.2 : 16
 /sys/bus/acpi/devices/GGL0001:00/GPIO.1/GPIO.0 : 3
 /sys/bus/acpi/devices/GGL0001:00/GPIO.1/GPIO.3 : INT3437:00
 /sys/bus/acpi/devices/GGL0001:00/GPIO.1/GPIO.1 : 1
 /sys/bus/acpi/devices/GGL0001:00/CHSW : 0
 /sys/bus/acpi/devices/GGL0001:00/FWID : Google_Samus.6300.330.0
 /sys/bus/acpi/devices/GGL0001:00/VBNV.1 : 16
 /sys/bus/acpi/devices/GGL0001:00/BINF.4 : 0

And for binary packages:

cat /sys/bus/acpi/devices/GGL0001:00/MECK | hexdump
 0000000 02fb 8e72 a025 0a73 0f13 095e 9e07 41e6
 0000010 f9e6 bb4e 76cc bef9 cca7 70e2 8f6d 863d
 0000020

cat /sys/bus/acpi/devices/GGL0001:00/VDAT | hexdump
 0000000 6256 4453 0002 0000 0448 0000 0000 0000
 0000010 0c00 0000 0000 0000 0850 0000 0000 0000
 0000020 7c54 0003 0000 0000 0420 0000 0000 0000
 0000030 0408 0000 0000 0000 0007 0000 0000 0000
 0000040 0003 0000 0000 0000 0448 0000 0000 0000
 0000050 0408 0000 0000 0000 9335 1f80 0000 0000
 0000060 69a8 21f3 0000 0000 1d02 21f9 0000 0000
 0000070 ba55 371b 0000 0000 0000 0000 0000 0000
 0000080 bcae 001d 0000 0000 0003 0001 0001 0003
 0000090 000c 0000 0003 0001 0003 0001 0001 0000
 00000a0 0001 0000 0000 0000 cc00 01da 0000 0000
 00000b0 0200 0000 0204 0000 0001 0000 0000 0000
 00000c0 0800 0000 0000 0000 0000 0001 0000 0000
 00000d0 0001 0001 1301 0000 0000 0000 0000 0000
 00000e0 0000 0000 0000 0000 0000 0000 0000 0000
 *

Thanks,
 Enric

[1] https://lkml.org/lkml/2017/7/31/378

Changes in v4:
- Add COMPILE_TEST to increase build coverage.
- Add sysfs ABI documentation.
- Rebased on top of 5.7-rc1 and solve conflicts.
- Cc ACPI maintainers.

Changes in v3:
- Use attribute groups instead of adding files "by hand".
- Do not use "raw" kobject to create directories.
- Do not abuse of the platform_device interface. Remove it.

Changes in v2:
- Note that this version is a total rework, with those major changes:
  - Use lists to track dinamically allocated attributes and groups.
  - Use sysfs binary attributes to store the ACPI contents.
  - Remove all the functionalities except the one that creates the sysfs files.

 .../ABI/testing/sysfs-driver-chromeos-acpi    | 133 +++++
 drivers/platform/x86/Kconfig                  |  11 +
 drivers/platform/x86/Makefile                 |   3 +
 drivers/platform/x86/chromeos_acpi.c          | 517 ++++++++++++++++++
 4 files changed, 664 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-chromeos-acpi
 create mode 100644 drivers/platform/x86/chromeos_acpi.c

diff --git a/Documentation/ABI/testing/sysfs-driver-chromeos-acpi b/Documentation/ABI/testing/sysfs-driver-chromeos-acpi
new file mode 100644
index 000000000000..8b22add9cf69
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-chromeos-acpi
@@ -0,0 +1,133 @@
+What:		/sys/bus/acpi/devices/GGL0001:00/BINF.{0,1,4}
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This file is reserved and doesn't shows useful information
+		for now.
+
+What:		/sys/bus/acpi/devices/GGL0001:00/BINF.2
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This file shows information about the current boot of
+		the active EC firmware.
+		  * 0 - Read only (recovery) firmware.
+		  * 1 - Rewritable firmware.
+
+What:		/sys/bus/acpi/devices/GGL0001:00/BINF.3
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This file shows information about the current boot of
+		the active main	firmware type.
+		  * 0 - Recovery.
+		  * 1 - Normal.
+		  * 2 - Developer.
+		  * 3 - Netboot (factory installation only).
+
+What:		/sys/bus/acpi/devices/GGL0001:00/CHSW
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This file shows the switch position for the Chrome OS specific
+		hardware switches.
+		  * 0   - No changes.
+		  * 2   - Recovery button was pressed when firmware booted.
+		  * 4   - Recovery button was pressed when EC firmware booted.
+		  * 32  - Developer switch was enabled when firmware booted.
+		  * 512 - Firmware write protect was disabled when firmware
+			  booted.
+
+What:		/sys/bus/acpi/devices/GGL0001:00/FMAP
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This file shows the physical memory address of the start of
+		the main processor firmware flashmap.
+
+What:		/sys/bus/acpi/devices/GGL0001:00/FRID
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This file shows the firmware version for the read-only portion
+		of the main processor firmware.
+
+What:		/sys/bus/acpi/devices/GGL0001:00/FWID
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This file shows the firmware version for the rewritable portion
+		of the main processor firmware.
+
+What:		/sys/bus/acpi/devices/GGL0001:00/GPIO.X/GPIO.0
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This file shows the type of the GPIO signal for the Chrome OS
+		specific GPIO assignments.
+		  * 1   - Recovery button.
+		  * 2   - Developer mode switch.
+		  * 3   - Firmware write protect switch.
+		  * 256 to 511 - Debug header GPIO 0 to GPIO 255.
+
+What:		/sys/bus/acpi/devices/GGL0001:00/GPIO.X/GPIO.1
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This file shows the signal attributes of the GPIO signal.
+		  * 0 - Signal is active low.
+		  * 1 - Signal is active high.
+
+What:		/sys/bus/acpi/devices/GGL0001:00/GPIO.X/GPIO.2
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This file shows the GPIO number on the specified GPIO
+		controller.
+
+What:		/sys/bus/acpi/devices/GGL0001:00/GPIO.X/GPIO.3
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This file shows the name of the GPIO controller.
+
+What:		/sys/bus/acpi/devices/GGL0001:00/HWID
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This file shows the hardware ID for the Chromebook.
+
+What:		/sys/bus/acpi/devices/GGL0001:00/MECK
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This binary file returns the SHA-1 or SHA-256 hash that is
+		read out of the Management Engine extend registers during
+		boot. The hash is exported vi ACPI so the OS can verify that
+		the Management Engine firmware has not changed. If Management
+		Engine is not present, or if the firmware was unable to read the
+		extend registers, this buffer can be zero.
+
+What:		/sys/bus/acpi/devices/GGL0001:00/VBNV.0
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This file shows the offset in CMOS bank 0 of the verified boot
+		non-volatile storage block, counting from the first writable
+		CMOS byte (that is, 'offset = 0' is the byte following the 14
+		bytes of clock data).
+
+What:		/sys/bus/acpi/devices/GGL0001:00/VBNV.1
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This file shows the size in bytes of the verified boot
+		non-volatile storage block.
+
+What:		/sys/bus/acpi/devices/GGL0001:00/VDAT
+Date:		April 2020
+KernelVersion:	5.8
+Description:
+		This binary file returns the verified boot data block shared
+		between the firmware verification step and the kernel
+		verification step.
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0ad7ad8cf8e1..c649f2a4ed9c 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -257,6 +257,17 @@ config ASUS_NB_WMI
 	  If you have an ACPI-WMI compatible Asus Notebook, say Y or M
 	  here.
 
+config ACPI_CHROMEOS
+	tristate "ChromeOS specific ACPI extensions"
+	depends on ACPI || COMPILE_TEST
+	help
+	  This driver provides the firmware interface for the services
+	  exported through the ChromeOS interfaces when using ChromeOS
+	  ACPI firmware.
+
+	  If you have an ACPI-compatible Chromebook, say Y or M
+	  here.
+
 config EEEPC_LAPTOP
 	tristate "Eee PC Hotkey Driver"
 	depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 53408d965874..a1846c80d988 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -32,6 +32,9 @@ obj-$(CONFIG_ASUS_NB_WMI)	+= asus-nb-wmi.o
 obj-$(CONFIG_EEEPC_LAPTOP)	+= eeepc-laptop.o
 obj-$(CONFIG_EEEPC_WMI)		+= eeepc-wmi.o
 
+# Chrome
+obj-$(CONFIG_ACPI_CHROMEOS)	+= chromeos_acpi.o
+
 # Dell
 obj-$(CONFIG_DCDBAS)			+= dcdbas.o
 obj-$(CONFIG_DELL_SMBIOS)		+= dell-smbios.o
diff --git a/drivers/platform/x86/chromeos_acpi.c b/drivers/platform/x86/chromeos_acpi.c
new file mode 100644
index 000000000000..dc8967281e70
--- /dev/null
+++ b/drivers/platform/x86/chromeos_acpi.c
@@ -0,0 +1,517 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ChromeOS specific ACPI extensions
+ *
+ * Copyright 2011 Google, Inc.
+ * Copyright 2020 Google LLC
+ *
+ * This file is a rework and part of the code is ported from
+ * drivers/platform/x86/chromeos_acpi.c of the chromeos-3.18 kernel and
+ * was originally written by Vadim Bendebury <vbendeb@chromium.org>.
+ *
+ * This driver attaches to the ChromeOS ACPI device and then exports the
+ * values reported by the ACPI in a sysfs directory. All values are
+ * presented in the string form (numbers as decimal values) and can be
+ * accessed as the contents of the appropriate read only files in the
+ * sysfs directory tree.
+ */
+
+#include <linux/acpi.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+
+/*
+ * ACPI method name for MLST; the response for this method is a package of
+ * strings listing the methods which should be reflected in sysfs.
+ */
+#define MLST "MLST"
+
+/*
+ * The default list of methods the ChromeOS ACPI device is supposed to export,
+ * if the MLST method is not present or is poorly formed.  The MLST method
+ * itself is included, to aid in debugging.
+ */
+static char *chromeos_acpi_default_methods[] = {
+	"CHSW", "HWID", "BINF", "GPIO", "CHNV", "FWID", "FRID", MLST
+};
+
+/*
+ * Representation of a single sysfs attribute. In addition to the standard
+ * bin_attribute structure has a list of these structures (to keep track for
+ * de-allocation when removing the driver) and a pointer to the actual
+ * attribute name and value, reported when accessing the appropriate sysfs
+ * file.
+ */
+struct chromeos_acpi_attribute {
+	struct bin_attribute bin_attr;
+	struct list_head list;
+	char *name;
+	char *data;
+};
+
+/*
+ * Representation of a sysfs attribute group (a sub directory in the device's
+ * sysfs directory). In addition to the standard structure has lists to allow
+ * to keep track of the allocated structures.
+ */
+struct chromeos_acpi_attribute_group {
+	struct attribute_group group;
+	struct list_head attribs;
+	struct list_head list;
+	char *name;
+};
+
+/*
+ * This is the main structure, we use it to store data and adds links pointing
+ * at lists of allocated attributes and attribute groups.
+ */
+struct chromeos_acpi_dev {
+	struct chromeos_acpi_attribute_group *root;
+	const struct attribute_group **dev_groups;
+	struct list_head groups;
+	unsigned int num_groups;
+	unsigned int num_attrs;
+};
+
+static struct chromeos_acpi_dev chromeos_acpi;
+
+static ssize_t chromeos_acpi_read_bin_attribute(struct file *filp,
+						struct kobject *kobj,
+						struct bin_attribute *bin_attr,
+						char *buffer, loff_t pos,
+						size_t count)
+{
+	struct chromeos_acpi_attribute *info = bin_attr->private;
+
+	return memory_read_from_buffer(buffer, count, &pos, info->data,
+				       info->bin_attr.size);
+}
+
+static char *chromeos_acpi_alloc_name(char *name, int count, int index)
+{
+	char *str;
+
+	if (count == 1)
+		str = kstrdup(name, GFP_KERNEL);
+	else
+		str = kasprintf(GFP_KERNEL, "%s.%d", name, index);
+
+	return str;
+}
+
+static int
+chromeos_acpi_add_attr(struct chromeos_acpi_attribute_group *aag,
+		       union acpi_object *element, char *name,
+		       int count, int index)
+{
+	struct chromeos_acpi_attribute *info;
+	char buffer[24]; /* enough to store a u64 and "\n\0" */
+	int length;
+	int ret;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->name = chromeos_acpi_alloc_name(name, count, index);
+	if (!info->name) {
+		ret = -ENOMEM;
+		goto free_attribute;
+	}
+
+	sysfs_bin_attr_init(&info->bin_attr);
+	info->bin_attr.attr.name = info->name;
+	info->bin_attr.attr.mode = 0444;
+
+	switch (element->type) {
+	case ACPI_TYPE_BUFFER:
+		length = element->buffer.length;
+		info->data = kmemdup(element->buffer.pointer,
+				     length, GFP_KERNEL);
+		break;
+	case ACPI_TYPE_INTEGER:
+		length = snprintf(buffer, sizeof(buffer), "%d",
+				  (int)element->integer.value);
+		info->data = kmemdup(buffer, length, GFP_KERNEL);
+		break;
+	case ACPI_TYPE_STRING:
+		length = element->string.length + 1;
+		info->data = kstrdup(element->string.pointer, GFP_KERNEL);
+		break;
+	default:
+		ret = -EINVAL;
+		goto free_attr_name;
+	}
+
+	if (!info->data) {
+		ret = -ENOMEM;
+		goto free_attr_name;
+	}
+
+	info->bin_attr.size = length;
+	info->bin_attr.read = chromeos_acpi_read_bin_attribute;
+	info->bin_attr.private = info;
+
+	INIT_LIST_HEAD(&info->list);
+
+	list_add(&info->list, &aag->attribs);
+
+	return 0;
+
+free_attr_name:
+	kfree(info->name);
+free_attribute:
+	kfree(info);
+	return ret;
+}
+
+static void
+chromeos_acpi_remove_attribs(struct chromeos_acpi_attribute_group *aag)
+{
+	struct chromeos_acpi_attribute *attr, *tmp_attr;
+
+	list_for_each_entry_safe(attr, tmp_attr, &aag->attribs, list) {
+		kfree(attr->name);
+		kfree(attr->data);
+		kfree(attr);
+	}
+}
+
+static int
+chromeos_acpi_add_attribs_to_group(struct chromeos_acpi_attribute_group *aag,
+				   unsigned int num_attrs)
+{
+	struct chromeos_acpi_attribute *attr;
+	int count = 0;
+
+	aag->group.bin_attrs = kcalloc(num_attrs + 1,
+				       sizeof(*aag->group.bin_attrs),
+				       GFP_KERNEL);
+	if (!aag->group.bin_attrs)
+		return -ENOMEM;
+
+	list_for_each_entry(attr, &aag->attribs, list) {
+		aag->group.bin_attrs[count] = &attr->bin_attr;
+		count++;
+	}
+
+	chromeos_acpi.num_groups++;
+	list_add(&aag->list, &chromeos_acpi.groups);
+
+	return 0;
+}
+
+/**
+ * chromeos_acpi_add_group() - Create a sysfs group including attributes
+ *			       representing a nested ACPI package.
+ *
+ * @adev: ACPI device.
+ * @obj: Package contents as returned by ACPI.
+ * @name: Name of the group.
+ * @num_attrs: Number of attributes of this package.
+ * @index: Index number of this particular group.
+ *
+ * The created group is called @name in case there is a single instance, or
+ * @name.@index otherwise.
+ *
+ * All group and attribute storage allocations are included in the lists for
+ * tracking of allocated memory.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+static int chromeos_acpi_add_group(struct acpi_device *adev,
+				   union acpi_object *obj, char *name,
+				   int num_attrs, int index)
+{
+	struct chromeos_acpi_attribute_group *aag;
+	union acpi_object *element;
+	int i, count, ret;
+
+	aag = kzalloc(sizeof(*aag), GFP_KERNEL);
+	if (!aag)
+		return -ENOMEM;
+
+	aag->name = chromeos_acpi_alloc_name(name, num_attrs, index);
+	if (!aag->name) {
+		ret = -ENOMEM;
+		goto free_group;
+	}
+
+	INIT_LIST_HEAD(&aag->attribs);
+	INIT_LIST_HEAD(&aag->list);
+
+	count = obj->package.count;
+	element = obj->package.elements;
+	for (i = 0; i < count; i++, element++) {
+		ret = chromeos_acpi_add_attr(aag, element, name, count, i);
+		if (ret)
+			goto free_group_attr;
+	}
+
+	aag->group.name = aag->name;
+
+	ret = chromeos_acpi_add_attribs_to_group(aag, count);
+	if (ret)
+		goto free_group_attr;
+
+	return 0;
+
+free_group_attr:
+	chromeos_acpi_remove_attribs(aag);
+	kfree(aag->name);
+free_group:
+	kfree(aag);
+	return ret;
+}
+
+static void chromeos_acpi_remove_groups(void)
+{
+	struct chromeos_acpi_attribute_group *aag, *tmp_aag;
+
+	list_for_each_entry_safe(aag, tmp_aag, &chromeos_acpi.groups, list) {
+		chromeos_acpi_remove_attribs(aag);
+		kfree(aag->group.bin_attrs);
+		kfree(aag->name);
+		kfree(aag);
+	}
+}
+
+/**
+ * chromeos_acpi_handle_package() - Create sysfs group including attributes
+ *				    representing an ACPI package.
+ *
+ * @adev: ACPI device.
+ * @obj: Package contents as returned by ACPI.
+ * @name: Name of the group.
+ *
+ * Scalar objects included in the package get sysfs attributes created for
+ * them. Nested packages are passed to a function creating a sysfs group per
+ * package.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+static int chromeos_acpi_handle_package(struct acpi_device *adev,
+					union acpi_object *obj, char *name)
+{
+	struct device *dev = &adev->dev;
+	int count = obj->package.count;
+	union acpi_object *element;
+	int i, ret;
+
+	element = obj->package.elements;
+	for (i = 0; i < count; i++, element++) {
+		if (element->type == ACPI_TYPE_BUFFER ||
+		    element->type == ACPI_TYPE_STRING ||
+		    element->type == ACPI_TYPE_INTEGER) {
+			/* Create a single attribute in the root directory */
+			ret = chromeos_acpi_add_attr(chromeos_acpi.root,
+						     element, name,
+						     count, i);
+			if (ret) {
+				dev_err(dev, "error adding attributes (%d)\n",
+					ret);
+				return ret;
+			}
+			chromeos_acpi.num_attrs++;
+		} else if (element->type == ACPI_TYPE_PACKAGE) {
+			/* Create a group of attributes */
+			ret = chromeos_acpi_add_group(adev, element, name,
+						      count, i);
+			if (ret) {
+				dev_err(dev, "error adding a group (%d)\n",
+					ret);
+				return ret;
+			}
+		} else {
+			if (ret) {
+				dev_err(dev, "error on element type (%d)\n",
+					ret);
+				return -EINVAL;
+			}
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * chromeos_acpi_add_method() - Evaluate an ACPI method and create sysfs
+ *				attributes.
+ *
+ * @adev: ACPI device
+ * @name: Name of the method to evaluate
+ *
+ * Return: 0 on success, non-zero on failure
+ */
+static int chromeos_acpi_add_method(struct acpi_device *adev, char *name)
+{
+	struct device *dev = &adev->dev;
+	struct acpi_buffer output;
+	union acpi_object *obj;
+	acpi_status status;
+	int ret = 0;
+
+	output.length = ACPI_ALLOCATE_BUFFER;
+
+	status = acpi_evaluate_object(adev->handle, name, NULL, &output);
+	if (ACPI_FAILURE(status)) {
+		dev_err(dev, "failed to retrieve %s (%d)\n", name, status);
+		return status;
+	}
+
+	obj = output.pointer;
+	if (obj->type == ACPI_TYPE_PACKAGE)
+		ret = chromeos_acpi_handle_package(adev, obj, name);
+
+	kfree(output.pointer);
+
+	return ret;
+}
+
+/**
+ * chromeos_acpi_process_mlst() - Evaluate the MLST method and add methods
+ *				  listed in the response.
+ *
+ * @adev: ACPI device
+ *
+ * Returns: 0 if successful, non-zero if error.
+ */
+static int chromeos_acpi_process_mlst(struct acpi_device *adev)
+{
+	struct chromeos_acpi_attribute_group *aag;
+	char name[ACPI_NAMESEG_SIZE + 1];
+	union acpi_object *element, *obj;
+	struct device *dev = &adev->dev;
+	struct acpi_buffer output;
+	acpi_status status;
+	int ret = 0;
+	int size;
+	int i;
+
+	output.length = ACPI_ALLOCATE_BUFFER;
+	status = acpi_evaluate_object(adev->handle, MLST, NULL,
+				      &output);
+	if (ACPI_FAILURE(status))
+		return status;
+
+	obj = output.pointer;
+	if (obj->type != ACPI_TYPE_PACKAGE) {
+		ret = -EINVAL;
+		goto free_acpi_buffer;
+	}
+
+	element = obj->package.elements;
+	for (i = 0; i < obj->package.count; i++, element++) {
+		if (element->type == ACPI_TYPE_STRING) {
+			size = min(element->string.length + 1,
+				   (u32)ACPI_NAMESEG_SIZE + 1);
+			strlcpy(name, element->string.pointer, size);
+			ret = chromeos_acpi_add_method(adev, name);
+			if (ret) {
+				chromeos_acpi_remove_groups();
+				break;
+			}
+		}
+	}
+
+	/* Add attributes to the main group */
+	ret = chromeos_acpi_add_attribs_to_group(chromeos_acpi.root,
+						 chromeos_acpi.num_attrs);
+	if (ret)
+		goto free_acpi_buffer;
+
+	chromeos_acpi.dev_groups = kcalloc(chromeos_acpi.num_groups + 1,
+					   sizeof(struct attribute_group),
+					   GFP_KERNEL);
+
+	i = 0;
+	list_for_each_entry(aag, &chromeos_acpi.groups, list) {
+		chromeos_acpi.dev_groups[i] = &aag->group;
+		i++;
+	}
+
+	ret = sysfs_create_groups(&dev->kobj, chromeos_acpi.dev_groups);
+	if (ret)
+		kfree(chromeos_acpi.dev_groups);
+
+free_acpi_buffer:
+	kfree(output.pointer);
+	return ret;
+}
+
+static int chromeos_acpi_device_add(struct acpi_device *adev)
+{
+	struct chromeos_acpi_attribute_group *aag = chromeos_acpi.root;
+	struct device *dev = &adev->dev;
+	int i, ret;
+
+	aag = kzalloc(sizeof(*aag), GFP_KERNEL);
+	if (!aag)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&aag->attribs);
+	INIT_LIST_HEAD(&aag->list);
+	INIT_LIST_HEAD(&chromeos_acpi.groups);
+
+	chromeos_acpi.root = aag;
+
+	/*
+	 * Attempt to add methods by querying the device's MLST method
+	 * for the list of methods.
+	 */
+	if (!chromeos_acpi_process_mlst(adev))
+		return 0;
+
+	dev_info(dev, "falling back to default list of methods\n");
+
+	for (i = 0; i < ARRAY_SIZE(chromeos_acpi_default_methods); i++) {
+		ret = chromeos_acpi_add_method(adev,
+					     chromeos_acpi_default_methods[i]);
+		if (ret) {
+			dev_err(dev, "failed to add default methods (%d)\n",
+				ret);
+			goto free_group_root;
+		}
+	}
+
+	return 0;
+
+free_group_root:
+	kfree(chromeos_acpi.root);
+	return ret;
+}
+
+static int chromeos_acpi_device_remove(struct acpi_device *adev)
+{
+	/* Remove sysfs groups */
+	sysfs_remove_groups(&adev->dev.kobj, chromeos_acpi.dev_groups);
+	kfree(chromeos_acpi.dev_groups);
+
+	/* Remove allocated chromeos acpi groups and attributes */
+	chromeos_acpi_remove_groups();
+	kfree(chromeos_acpi.root);
+
+	return 0;
+}
+
+static const struct acpi_device_id chromeos_device_ids[] = {
+	{ "GGL0001", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, chromeos_device_ids);
+
+static struct acpi_driver chromeos_acpi_driver = {
+	.name = "chromeos-acpi",
+	.class = "chromeos-acpi",
+	.ids = chromeos_device_ids,
+	.ops = {
+		.add = chromeos_acpi_device_add,
+		.remove = chromeos_acpi_device_remove,
+	},
+};
+module_acpi_driver(chromeos_acpi_driver);
+
+MODULE_AUTHOR("Enric Balletbo i Serra <enric.balletbo@collabora.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ChromeOS specific ACPI extensions");
-- 
2.25.1

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

[Greg Kroah-Hartman]

Meta-comment to the ACPI developers, shouldn't all of this happen
"automatically" with the existing ACPI entries in sysfs?  If not, is
this driver the proper way to do this?

Minor review comments below:


On Mon, Apr 13, 2020 at 03:46:11PM +0200, Enric Balletbo i Serra wrote:
> +What:		/sys/bus/acpi/devices/GGL0001:00/BINF.{0,1,4}
> +Date:		April 2020
> +KernelVersion:	5.8
> +Description:
> +		This file is reserved and doesn't shows useful information
> +		for now.

Then do not even have it present.  sysfs should never export files that
nothing can be done with them, userspace "knows" that if a file is not
present, it can not use it.  Bring it back when it is useful.

> +What:		/sys/bus/acpi/devices/GGL0001:00/MECK
> +Date:		April 2020
> +KernelVersion:	5.8
> +Description:
> +		This binary file returns the SHA-1 or SHA-256 hash that is
> +		read out of the Management Engine extend registers during
> +		boot. The hash is exported vi ACPI so the OS can verify that
> +		the Management Engine firmware has not changed. If Management
> +		Engine is not present, or if the firmware was unable to read the
> +		extend registers, this buffer can be zero.

The size is zero, or the contents are 0?

> +static char *chromeos_acpi_alloc_name(char *name, int count, int index)
> +{
> +	char *str;
> +
> +	if (count == 1)
> +		str = kstrdup(name, GFP_KERNEL);
> +	else
> +		str = kasprintf(GFP_KERNEL, "%s.%d", name, index);

That's crazy, make this more obvious that "count" affects the name so
much.  As it is, no one would know this unless they read the function
code, and not just the name.


> +/**
> + * chromeos_acpi_add_group() - Create a sysfs group including attributes
> + *			       representing a nested ACPI package.
> + *
> + * @adev: ACPI device.
> + * @obj: Package contents as returned by ACPI.
> + * @name: Name of the group.
> + * @num_attrs: Number of attributes of this package.
> + * @index: Index number of this particular group.
> + *
> + * The created group is called @name in case there is a single instance, or
> + * @name.@index otherwise.
> + *
> + * All group and attribute storage allocations are included in the lists for
> + * tracking of allocated memory.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */

Meta-comment, no need for kerneldoc on static functions.  It's nice to
see, but nothing is going to notice them...

> +static int chromeos_acpi_add_method(struct acpi_device *adev, char *name)
> +{
> +	struct device *dev = &adev->dev;
> +	struct acpi_buffer output;
> +	union acpi_object *obj;
> +	acpi_status status;
> +	int ret = 0;
> +
> +	output.length = ACPI_ALLOCATE_BUFFER;
> +
> +	status = acpi_evaluate_object(adev->handle, name, NULL, &output);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(dev, "failed to retrieve %s (%d)\n", name, status);
> +		return status;
> +	}
> +
> +	obj = output.pointer;
> +	if (obj->type == ACPI_TYPE_PACKAGE)
> +		ret = chromeos_acpi_handle_package(adev, obj, name);
> +
> +	kfree(output.pointer);

Why the need for 'obj' at all in this function?  Minor nit.

> +
> +	return ret;
> +}
> +
> +static int chromeos_acpi_device_add(struct acpi_device *adev)
> +{
> +	struct chromeos_acpi_attribute_group *aag = chromeos_acpi.root;
> +	struct device *dev = &adev->dev;
> +	int i, ret;
> +
> +	aag = kzalloc(sizeof(*aag), GFP_KERNEL);
> +	if (!aag)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&aag->attribs);
> +	INIT_LIST_HEAD(&aag->list);
> +	INIT_LIST_HEAD(&chromeos_acpi.groups);
> +
> +	chromeos_acpi.root = aag;
> +
> +	/*
> +	 * Attempt to add methods by querying the device's MLST method
> +	 * for the list of methods.
> +	 */
> +	if (!chromeos_acpi_process_mlst(adev))
> +		return 0;
> +
> +	dev_info(dev, "falling back to default list of methods\n");

Is this debugging code left over?  If not, make it an error, and what
would a user be able to do with it?

thanks,

greg k-h

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

[Rafael J. Wysocki]

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?

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

I guess that these data are to be consubed by user space?

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

[Enric Balletbo i Serra]

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.

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

[Enric Balletbo i Serra]

Hi Greg,

Thank you for your comments, some answers below.

On 13/4/20 16:12, Greg Kroah-Hartman wrote:
> Meta-comment to the ACPI developers, shouldn't all of this happen
> "automatically" with the existing ACPI entries in sysfs?  If not, is
> this driver the proper way to do this?
> 

This is something maintainers didn't answer yet, and I am not sure, to be hones,
but meanwhile, I'll rework this driver fixing the Greg comments and send a new
version.

> Minor review comments below:
> 
> 
> On Mon, Apr 13, 2020 at 03:46:11PM +0200, Enric Balletbo i Serra wrote:
>> +What:		/sys/bus/acpi/devices/GGL0001:00/BINF.{0,1,4}
>> +Date:		April 2020
>> +KernelVersion:	5.8
>> +Description:
>> +		This file is reserved and doesn't shows useful information
>> +		for now.
> 
> Then do not even have it present.  sysfs should never export files that
> nothing can be done with them, userspace "knows" that if a file is not
> present, it can not use it.  Bring it back when it is useful.
> 

Makes sense. I'll do in next version.

>> +What:		/sys/bus/acpi/devices/GGL0001:00/MECK
>> +Date:		April 2020
>> +KernelVersion:	5.8
>> +Description:
>> +		This binary file returns the SHA-1 or SHA-256 hash that is
>> +		read out of the Management Engine extend registers during
>> +		boot. The hash is exported vi ACPI so the OS can verify that
>> +		the Management Engine firmware has not changed. If Management
>> +		Engine is not present, or if the firmware was unable to read the
>> +		extend registers, this buffer can be zero.
> 
> The size is zero, or the contents are 0?
> 

The size. I'll reword in the description,

>> +static char *chromeos_acpi_alloc_name(char *name, int count, int index)
>> +{
>> +	char *str;
>> +
>> +	if (count == 1)
>> +		str = kstrdup(name, GFP_KERNEL);
>> +	else
>> +		str = kasprintf(GFP_KERNEL, "%s.%d", name, index);
> 
> That's crazy, make this more obvious that "count" affects the name so
> much.  As it is, no one would know this unless they read the function
> code, and not just the name.
> 

I see, let me think about this.

> 
>> +/**
>> + * chromeos_acpi_add_group() - Create a sysfs group including attributes
>> + *			       representing a nested ACPI package.
>> + *
>> + * @adev: ACPI device.
>> + * @obj: Package contents as returned by ACPI.
>> + * @name: Name of the group.
>> + * @num_attrs: Number of attributes of this package.
>> + * @index: Index number of this particular group.
>> + *
>> + * The created group is called @name in case there is a single instance, or
>> + * @name.@index otherwise.
>> + *
>> + * All group and attribute storage allocations are included in the lists for
>> + * tracking of allocated memory.
>> + *
>> + * Return: 0 on success, negative errno on failure.
>> + */
> 
> Meta-comment, no need for kerneldoc on static functions.  It's nice to
> see, but nothing is going to notice them...
> 
>> +static int chromeos_acpi_add_method(struct acpi_device *adev, char *name)
>> +{
>> +	struct device *dev = &adev->dev;
>> +	struct acpi_buffer output;
>> +	union acpi_object *obj;
>> +	acpi_status status;
>> +	int ret = 0;
>> +
>> +	output.length = ACPI_ALLOCATE_BUFFER;
>> +
>> +	status = acpi_evaluate_object(adev->handle, name, NULL, &output);
>> +	if (ACPI_FAILURE(status)) {
>> +		dev_err(dev, "failed to retrieve %s (%d)\n", name, status);
>> +		return status;
>> +	}
>> +
>> +	obj = output.pointer;
>> +	if (obj->type == ACPI_TYPE_PACKAGE)
>> +		ret = chromeos_acpi_handle_package(adev, obj, name);
>> +
>> +	kfree(output.pointer);
> 
> Why the need for 'obj' at all in this function?  Minor nit.
> 

Ok, I'll remove obj.

>> +
>> +	return ret;
>> +}
>> +
>> +static int chromeos_acpi_device_add(struct acpi_device *adev)
>> +{
>> +	struct chromeos_acpi_attribute_group *aag = chromeos_acpi.root;
>> +	struct device *dev = &adev->dev;
>> +	int i, ret;
>> +
>> +	aag = kzalloc(sizeof(*aag), GFP_KERNEL);
>> +	if (!aag)
>> +		return -ENOMEM;
>> +
>> +	INIT_LIST_HEAD(&aag->attribs);
>> +	INIT_LIST_HEAD(&aag->list);
>> +	INIT_LIST_HEAD(&chromeos_acpi.groups);
>> +
>> +	chromeos_acpi.root = aag;
>> +
>> +	/*
>> +	 * Attempt to add methods by querying the device's MLST method
>> +	 * for the list of methods.
>> +	 */
>> +	if (!chromeos_acpi_process_mlst(adev))
>> +		return 0;
>> +
>> +	dev_info(dev, "falling back to default list of methods\n");
> 
> Is this debugging code left over?  If not, make it an error, and what
> would a user be able to do with it?
> 

I think I can downgrade to debug level.

Thanks,
 Enric

> thanks,
> 
> greg k-h
> 

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

[Rafael J. Wysocki]

On Monday, April 13, 2020 4:12:59 PM CEST Greg Kroah-Hartman wrote:
> Meta-comment to the ACPI developers, shouldn't all of this happen
> "automatically" with the existing ACPI entries in sysfs?

I'm not quite sure what you mean by "all of this" here.

Can you please explain?

> If not, is this driver the proper way to do this?
> 
> Minor review comments below:
> 
> 
> On Mon, Apr 13, 2020 at 03:46:11PM +0200, Enric Balletbo i Serra wrote:
> > +What:		/sys/bus/acpi/devices/GGL0001:00/BINF.{0,1,4}
> > +Date:		April 2020
> > +KernelVersion:	5.8
> > +Description:
> > +		This file is reserved and doesn't shows useful information
> > +		for now.
> 
> Then do not even have it present.  sysfs should never export files that
> nothing can be done with them, userspace "knows" that if a file is not
> present, it can not use it.  Bring it back when it is useful.
> 
> > +What:		/sys/bus/acpi/devices/GGL0001:00/MECK
> > +Date:		April 2020
> > +KernelVersion:	5.8
> > +Description:
> > +		This binary file returns the SHA-1 or SHA-256 hash that is
> > +		read out of the Management Engine extend registers during
> > +		boot. The hash is exported vi ACPI so the OS can verify that
> > +		the Management Engine firmware has not changed. If Management
> > +		Engine is not present, or if the firmware was unable to read the
> > +		extend registers, this buffer can be zero.
> 
> The size is zero, or the contents are 0?
> 
> > +static char *chromeos_acpi_alloc_name(char *name, int count, int index)
> > +{
> > +	char *str;
> > +
> > +	if (count == 1)
> > +		str = kstrdup(name, GFP_KERNEL);
> > +	else
> > +		str = kasprintf(GFP_KERNEL, "%s.%d", name, index);
> 
> That's crazy, make this more obvious that "count" affects the name so
> much.  As it is, no one would know this unless they read the function
> code, and not just the name.
> 
> 
> > +/**
> > + * chromeos_acpi_add_group() - Create a sysfs group including attributes
> > + *			       representing a nested ACPI package.
> > + *
> > + * @adev: ACPI device.
> > + * @obj: Package contents as returned by ACPI.
> > + * @name: Name of the group.
> > + * @num_attrs: Number of attributes of this package.
> > + * @index: Index number of this particular group.
> > + *
> > + * The created group is called @name in case there is a single instance, or
> > + * @name.@index otherwise.
> > + *
> > + * All group and attribute storage allocations are included in the lists for
> > + * tracking of allocated memory.
> > + *
> > + * Return: 0 on success, negative errno on failure.
> > + */
> 
> Meta-comment, no need for kerneldoc on static functions.  It's nice to
> see, but nothing is going to notice them...
> 
> > +static int chromeos_acpi_add_method(struct acpi_device *adev, char *name)
> > +{
> > +	struct device *dev = &adev->dev;
> > +	struct acpi_buffer output;
> > +	union acpi_object *obj;
> > +	acpi_status status;
> > +	int ret = 0;
> > +
> > +	output.length = ACPI_ALLOCATE_BUFFER;
> > +
> > +	status = acpi_evaluate_object(adev->handle, name, NULL, &output);
> > +	if (ACPI_FAILURE(status)) {
> > +		dev_err(dev, "failed to retrieve %s (%d)\n", name, status);
> > +		return status;
> > +	}
> > +
> > +	obj = output.pointer;
> > +	if (obj->type == ACPI_TYPE_PACKAGE)
> > +		ret = chromeos_acpi_handle_package(adev, obj, name);
> > +
> > +	kfree(output.pointer);
> 
> Why the need for 'obj' at all in this function?  Minor nit.
> 
> > +
> > +	return ret;
> > +}
> > +
> > +static int chromeos_acpi_device_add(struct acpi_device *adev)
> > +{
> > +	struct chromeos_acpi_attribute_group *aag = chromeos_acpi.root;
> > +	struct device *dev = &adev->dev;
> > +	int i, ret;
> > +
> > +	aag = kzalloc(sizeof(*aag), GFP_KERNEL);
> > +	if (!aag)
> > +		return -ENOMEM;
> > +
> > +	INIT_LIST_HEAD(&aag->attribs);
> > +	INIT_LIST_HEAD(&aag->list);
> > +	INIT_LIST_HEAD(&chromeos_acpi.groups);
> > +
> > +	chromeos_acpi.root = aag;
> > +
> > +	/*
> > +	 * Attempt to add methods by querying the device's MLST method
> > +	 * for the list of methods.
> > +	 */
> > +	if (!chromeos_acpi_process_mlst(adev))
> > +		return 0;
> > +
> > +	dev_info(dev, "falling back to default list of methods\n");
> 
> Is this debugging code left over?  If not, make it an error, and what
> would a user be able to do with it?

Or use dev_dbg() to print it, possibly?

Cheers!

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

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!

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

[Dmitry Torokhov]

Hi Rafael,

On Fri, Jun 05, 2020 at 01:17:15PM +0200, Rafael J. Wysocki wrote:
> 
> 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).

This is not going to happen, as there are devices in the wild with such
firmware (i.e. Samus - Google Pixel 2 - was shipped in 2015). Even if
Google were to release updated firmware (which is quite unlikely), it
does not mean that users who are not using Chrome OS would apply updated
firmware.

> The other one would
> be to properly register the GGL prefix for Google and establish a process for
> allocating IDs with that prefix internally.

I think it depends on whether there are more instances of "GGL" prefix.
I thought we mostly used GOOG for everything.

Thanks.

-- 
Dmitry

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

[Enric Balletbo i Serra]

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.


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

> 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/heads/chromeos-2016.05/src/vendorcode/google/chromeos/acpi/chromeos.asl

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

[Mario.Limonciello]

> -----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

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

[Dmitry Torokhov]

On Wed, Jun 10, 2020 at 09:28:36PM +0000, Mario.Limonciello@dell.com wrote:
> > 
> > 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.

If we use this prefix for hatch EOL is ~7 years from now.

Thanks.

-- 
Dmitry

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

[Mario.Limonciello]

> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Sent: Wednesday, June 10, 2020 4:41 PM
> To: Limonciello, Mario
> Cc: enric.balletbo@collabora.com; rjw@rjwysocki.net; rafael@kernel.org;
> linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org; lenb@kernel.org;
> kernel@collabora.com; groeck@chromium.org; bleung@chromium.org;
> dtor@chromium.org; gwendal@chromium.org; vbendeb@chromium.org;
> andy@infradead.org; ayman.bagabas@gmail.com; benjamin.tissoires@redhat.com;
> blaz@mxxn.io; dvhart@infradead.org; gregkh@linuxfoundation.org;
> hdegoede@redhat.com; jeremy@system76.com; 2pi@mok.nu;
> mchehab+samsung@kernel.org; rajatja@google.com;
> srinivas.pandruvada@linux.intel.com; platform-driver-x86@vger.kernel.org
> Subject: Re: [PATCH v4] platform: x86: Add ACPI driver for ChromeOS
> 
> 
> [EXTERNAL EMAIL]
> 
> On Wed, Jun 10, 2020 at 09:28:36PM +0000, Mario.Limonciello@dell.com wrote:
> > >
> > > 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.
> 
> If we use this prefix for hatch EOL is ~7 years from now.
> 

Isn't the whole point of the ACPI registry and choosing an ID?  You know internally
if you need to change the interface that a new ID is needed and a new driver will
be needed that comprehends that ID change.  So if you can't guarantee that 0001 is
the same driver interface in every firmware implementation google used then that is
where this falls apart.

I know there is a long support lifecycle but you're talking about rebasing
to new LTS kernels a handful of times between now and then.  If the interface really
is stable the patch should be small and it shouldn't be a large amount of technical
debt to carry downstream until EOL.
 

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

[Dmitry Torokhov]

On Wed, Jun 10, 2020 at 09:52:12PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Sent: Wednesday, June 10, 2020 4:41 PM
> > To: Limonciello, Mario
> > Cc: enric.balletbo@collabora.com; rjw@rjwysocki.net; rafael@kernel.org;
> > linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org; lenb@kernel.org;
> > kernel@collabora.com; groeck@chromium.org; bleung@chromium.org;
> > dtor@chromium.org; gwendal@chromium.org; vbendeb@chromium.org;
> > andy@infradead.org; ayman.bagabas@gmail.com; benjamin.tissoires@redhat.com;
> > blaz@mxxn.io; dvhart@infradead.org; gregkh@linuxfoundation.org;
> > hdegoede@redhat.com; jeremy@system76.com; 2pi@mok.nu;
> > mchehab+samsung@kernel.org; rajatja@google.com;
> > srinivas.pandruvada@linux.intel.com; platform-driver-x86@vger.kernel.org
> > Subject: Re: [PATCH v4] platform: x86: Add ACPI driver for ChromeOS
> > 
> > 
> > [EXTERNAL EMAIL]
> > 
> > On Wed, Jun 10, 2020 at 09:28:36PM +0000, Mario.Limonciello@dell.com wrote:
> > > >
> > > > 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.
> > 
> > If we use this prefix for hatch EOL is ~7 years from now.
> > 
> 
> Isn't the whole point of the ACPI registry and choosing an ID?  You know internally
> if you need to change the interface that a new ID is needed and a new driver will
> be needed that comprehends that ID change.  So if you can't guarantee that 0001 is
> the same driver interface in every firmware implementation google used then that is
> where this falls apart.
> 
> I know there is a long support lifecycle but you're talking about rebasing
> to new LTS kernels a handful of times between now and then.  If the interface really
> is stable the patch should be small and it shouldn't be a large amount of technical
> debt to carry downstream until EOL.

I think we are talking about different things actually. Let's forget
about Chrome OS and downstream kernels. We have devices that have
already been shipped and in hands of users. Some of them are old, some
of them are new. We can't not enforce that firmware for these devices
will be either released or updated. Therefore, if we want expose this
device in mainline kernel, we need to have it handle "GGL0001" HID in
addition to whatever proper HID we may select for it.

We internally can fix it (HID) for next generation of devices.

Thanks.

-- 
Dmitry

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

[Enric Balletbo i Serra]

Hi,

On 11/6/20 0:43, Dmitry Torokhov wrote:
> On Wed, Jun 10, 2020 at 09:52:12PM +0000, Mario.Limonciello@dell.com wrote:
>>> -----Original Message-----
>>> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> Sent: Wednesday, June 10, 2020 4:41 PM
>>> To: Limonciello, Mario
>>> Cc: enric.balletbo@collabora.com; rjw@rjwysocki.net; rafael@kernel.org;
>>> linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org; lenb@kernel.org;
>>> kernel@collabora.com; groeck@chromium.org; bleung@chromium.org;
>>> dtor@chromium.org; gwendal@chromium.org; vbendeb@chromium.org;
>>> andy@infradead.org; ayman.bagabas@gmail.com; benjamin.tissoires@redhat.com;
>>> blaz@mxxn.io; dvhart@infradead.org; gregkh@linuxfoundation.org;
>>> hdegoede@redhat.com; jeremy@system76.com; 2pi@mok.nu;
>>> mchehab+samsung@kernel.org; rajatja@google.com;
>>> srinivas.pandruvada@linux.intel.com; platform-driver-x86@vger.kernel.org
>>> Subject: Re: [PATCH v4] platform: x86: Add ACPI driver for ChromeOS
>>>
>>>
>>> [EXTERNAL EMAIL]
>>>
>>> On Wed, Jun 10, 2020 at 09:28:36PM +0000, Mario.Limonciello@dell.com wrote:
>>>>>
>>>>> 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.
>>>
>>> If we use this prefix for hatch EOL is ~7 years from now.
>>>
>>
>> Isn't the whole point of the ACPI registry and choosing an ID?  You know internally
>> if you need to change the interface that a new ID is needed and a new driver will
>> be needed that comprehends that ID change.  So if you can't guarantee that 0001 is
>> the same driver interface in every firmware implementation google used then that is
>> where this falls apart.
>>

As far as I know GGL0001 has the same driver interface in every firmware
implementation Google used. But I'll ask to make sure.

>> I know there is a long support lifecycle but you're talking about rebasing
>> to new LTS kernels a handful of times between now and then.  If the interface really
>> is stable the patch should be small and it shouldn't be a large amount of technical
>> debt to carry downstream until EOL.
> 
> I think we are talking about different things actually. Let's forget
> about Chrome OS and downstream kernels. We have devices that have
> already been shipped and in hands of users. Some of them are old, some
> of them are new. We can't not enforce that firmware for these devices
> will be either released or updated. Therefore, if we want expose this
> device in mainline kernel, we need to have it handle "GGL0001" HID in
> addition to whatever proper HID we may select for it.
> 

FWIW, after investigate a bit more, although GGL is not in the ACPI ID list it
is in the PNP ID list [1]. So as far as I understand GGL0001 is valid ID. I know
that PNP ID is the legacy identifier but since this was here for long time and
will be here also for long time, I am wondering if we can take that as an
argument to have GGL0001 as a valid device to be exposed in the kernel.

Thanks,
 Enric

[1] https://uefi.org/pnp_id_list


> We internally can fix it (HID) for next generation of devices.
> 
> Thanks.
> 

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

[Enric Balletbo i Serra]

Hi Rafael,

On 6/6/20 20:04, Dmitry Torokhov wrote:
> Hi Rafael,
> 
> On Fri, Jun 05, 2020 at 01:17:15PM +0200, Rafael J. Wysocki wrote:
>>
>> 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
>>

True, this device ID is not in the ACPI id list, but it is in the legacy PNP id
list at https://uefi.org/pnp_id_list

Even is a legacy one, this device has been here a long time, just that Google
had an out-of-tree patch to support that we would like to upstream.

So, I'm wondering if PNP id's are still valid?

>> 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).
> 
> This is not going to happen, as there are devices in the wild with such
> firmware (i.e. Samus - Google Pixel 2 - was shipped in 2015). Even if
> Google were to release updated firmware (which is quite unlikely), it
> does not mean that users who are not using Chrome OS would apply updated
> firmware.
> 
>> The other one would
>> be to properly register the GGL prefix for Google and establish a process for
>> allocating IDs with that prefix internally.
> 
> I think it depends on whether there are more instances of "GGL" prefix.
> I thought we mostly used GOOG for everything.
> 

I only see one instance using GGL, GGL0001 which I think is present on all
ACPI-based Chromebooks, and I'd think that the PNP id GGL is a proper valid
prefix for Google. However is true that then Google mostly used GOOG.

[1]
https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/refs/heads/chromeos-2016.05/src/vendorcode/google/chromeos/acpi/chromeos.asl

Thanks,
 Enric

> Thanks.
> 

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

[Enric Balletbo i Serra]

Hi Rafael,

On 11/6/20 13:06, Enric Balletbo i Serra wrote:
> Hi,
> 
> On 11/6/20 0:43, Dmitry Torokhov wrote:
>> On Wed, Jun 10, 2020 at 09:52:12PM +0000, Mario.Limonciello@dell.com wrote:
>>>> -----Original Message-----
>>>> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>> Sent: Wednesday, June 10, 2020 4:41 PM
>>>> To: Limonciello, Mario
>>>> Cc: enric.balletbo@collabora.com; rjw@rjwysocki.net; rafael@kernel.org;
>>>> linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org; lenb@kernel.org;
>>>> kernel@collabora.com; groeck@chromium.org; bleung@chromium.org;
>>>> dtor@chromium.org; gwendal@chromium.org; vbendeb@chromium.org;
>>>> andy@infradead.org; ayman.bagabas@gmail.com; benjamin.tissoires@redhat.com;
>>>> blaz@mxxn.io; dvhart@infradead.org; gregkh@linuxfoundation.org;
>>>> hdegoede@redhat.com; jeremy@system76.com; 2pi@mok.nu;
>>>> mchehab+samsung@kernel.org; rajatja@google.com;
>>>> srinivas.pandruvada@linux.intel.com; platform-driver-x86@vger.kernel.org
>>>> Subject: Re: [PATCH v4] platform: x86: Add ACPI driver for ChromeOS
>>>>
>>>>
>>>> [EXTERNAL EMAIL]
>>>>
>>>> On Wed, Jun 10, 2020 at 09:28:36PM +0000, Mario.Limonciello@dell.com wrote:
>>>>>>
>>>>>> 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.
>>>>
>>>> If we use this prefix for hatch EOL is ~7 years from now.
>>>>
>>>
>>> Isn't the whole point of the ACPI registry and choosing an ID?  You know internally
>>> if you need to change the interface that a new ID is needed and a new driver will
>>> be needed that comprehends that ID change.  So if you can't guarantee that 0001 is
>>> the same driver interface in every firmware implementation google used then that is
>>> where this falls apart.
>>>
> 
> As far as I know GGL0001 has the same driver interface in every firmware
> implementation Google used. But I'll ask to make sure.
> 
>>> I know there is a long support lifecycle but you're talking about rebasing
>>> to new LTS kernels a handful of times between now and then.  If the interface really
>>> is stable the patch should be small and it shouldn't be a large amount of technical
>>> debt to carry downstream until EOL.
>>
>> I think we are talking about different things actually. Let's forget
>> about Chrome OS and downstream kernels. We have devices that have
>> already been shipped and in hands of users. Some of them are old, some
>> of them are new. We can't not enforce that firmware for these devices
>> will be either released or updated. Therefore, if we want expose this
>> device in mainline kernel, we need to have it handle "GGL0001" HID in
>> addition to whatever proper HID we may select for it.
>>
> 
> FWIW, after investigate a bit more, although GGL is not in the ACPI ID list it
> is in the PNP ID list [1]. So as far as I understand GGL0001 is valid ID. I know
> that PNP ID is the legacy identifier but since this was here for long time and
> will be here also for long time, I am wondering if we can take that as an
> argument to have GGL0001 as a valid device to be exposed in the kernel.
> 

So, as the GGL prefix is a valid ID in the PNP ID list, is this a valid argument
to take in consideration this patch and resolves your concern regarding the ID?

Thanks,
 Enric



> Thanks,
>  Enric
> 
> [1] https://uefi.org/pnp_id_list
> 
> 
>> We internally can fix it (HID) for next generation of devices.
>>
>> Thanks.
>>
> 

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

[Rafael J. Wysocki]

On Thu, Jul 9, 2020 at 11:31 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Rafael,
>
> On 11/6/20 13:06, Enric Balletbo i Serra wrote:
> > Hi,
> >
> > On 11/6/20 0:43, Dmitry Torokhov wrote:
> >> On Wed, Jun 10, 2020 at 09:52:12PM +0000, Mario.Limonciello@dell.com wrote:
> >>>> -----Original Message-----
> >>>> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>>> Sent: Wednesday, June 10, 2020 4:41 PM
> >>>> To: Limonciello, Mario
> >>>> Cc: enric.balletbo@collabora.com; rjw@rjwysocki.net; rafael@kernel.org;
> >>>> linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org; lenb@kernel.org;
> >>>> kernel@collabora.com; groeck@chromium.org; bleung@chromium.org;
> >>>> dtor@chromium.org; gwendal@chromium.org; vbendeb@chromium.org;
> >>>> andy@infradead.org; ayman.bagabas@gmail.com; benjamin.tissoires@redhat.com;
> >>>> blaz@mxxn.io; dvhart@infradead.org; gregkh@linuxfoundation.org;
> >>>> hdegoede@redhat.com; jeremy@system76.com; 2pi@mok.nu;
> >>>> mchehab+samsung@kernel.org; rajatja@google.com;
> >>>> srinivas.pandruvada@linux.intel.com; platform-driver-x86@vger.kernel.org
> >>>> Subject: Re: [PATCH v4] platform: x86: Add ACPI driver for ChromeOS
> >>>>
> >>>>
> >>>> [EXTERNAL EMAIL]
> >>>>
> >>>> On Wed, Jun 10, 2020 at 09:28:36PM +0000, Mario.Limonciello@dell.com wrote:
> >>>>>>
> >>>>>> 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.
> >>>>
> >>>> If we use this prefix for hatch EOL is ~7 years from now.
> >>>>
> >>>
> >>> Isn't the whole point of the ACPI registry and choosing an ID?  You know internally
> >>> if you need to change the interface that a new ID is needed and a new driver will
> >>> be needed that comprehends that ID change.  So if you can't guarantee that 0001 is
> >>> the same driver interface in every firmware implementation google used then that is
> >>> where this falls apart.
> >>>
> >
> > As far as I know GGL0001 has the same driver interface in every firmware
> > implementation Google used. But I'll ask to make sure.
> >
> >>> I know there is a long support lifecycle but you're talking about rebasing
> >>> to new LTS kernels a handful of times between now and then.  If the interface really
> >>> is stable the patch should be small and it shouldn't be a large amount of technical
> >>> debt to carry downstream until EOL.
> >>
> >> I think we are talking about different things actually. Let's forget
> >> about Chrome OS and downstream kernels. We have devices that have
> >> already been shipped and in hands of users. Some of them are old, some
> >> of them are new. We can't not enforce that firmware for these devices
> >> will be either released or updated. Therefore, if we want expose this
> >> device in mainline kernel, we need to have it handle "GGL0001" HID in
> >> addition to whatever proper HID we may select for it.
> >>
> >
> > FWIW, after investigate a bit more, although GGL is not in the ACPI ID list it
> > is in the PNP ID list [1]. So as far as I understand GGL0001 is valid ID. I know
> > that PNP ID is the legacy identifier but since this was here for long time and
> > will be here also for long time, I am wondering if we can take that as an
> > argument to have GGL0001 as a valid device to be exposed in the kernel.
> >
>
> So, as the GGL prefix is a valid ID in the PNP ID list, is this a valid argument
> to take in consideration this patch and resolves your concern regarding the ID?

Yes, it does, thanks!

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

[Rafael J. Wysocki]

On Wed, Jun 10, 2020 at 11:21 PM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> 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.

Please do.

> > 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

[cut]

> > 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?

I would like the firmware interface to be documented in
Documentation/firmware-guide/acpi/ in the first place, given the lack
of any existing documentation of it that can be pointed to.

Thanks!