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

[PATCH v2] 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. 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 sysfs
directory tree originating in /sys/devices/platform/chromeos_acpi.

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, no problems observed
and the allocated resouces are freeid.

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

And for binary packages:

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

cat /sys/devices/platform/chromeos_acpi/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 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.

 drivers/platform/x86/Kconfig         |  12 +
 drivers/platform/x86/Makefile        |   1 +
 drivers/platform/x86/chromeos_acpi.c | 489 +++++++++++++++++++++++++++
 3 files changed, 502 insertions(+)
 create mode 100644 drivers/platform/x86/chromeos_acpi.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 587403c44598..917a1c1a0758 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -72,6 +72,18 @@ config ACERHDF
 	  If you have an Acer Aspire One netbook, say Y or M
 	  here.
 
+config ACPI_CHROMEOS
+	tristate "ChromeOS specific ACPI extensions"
+	depends on ACPI
+	depends on CHROME_PLATFORMS
+	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 ALIENWARE_WMI
 	tristate "Alienware Special feature control"
 	depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 3747b1f07cf1..222e2e88ccb8 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -83,6 +83,7 @@ obj-$(CONFIG_SAMSUNG_Q10)	+= samsung-q10.o
 obj-$(CONFIG_APPLE_GMUX)	+= apple-gmux.o
 obj-$(CONFIG_INTEL_RST)		+= intel-rst.o
 obj-$(CONFIG_INTEL_SMARTCONNECT)	+= intel-smartconnect.o
+obj-$(CONFIG_ACPI_CHROMEOS)	+= chromeos_acpi.o
 
 obj-$(CONFIG_ALIENWARE_WMI)	+= alienware-wmi.o
 obj-$(CONFIG_INTEL_PMC_IPC)	+= intel_pmc_ipc.o
diff --git a/drivers/platform/x86/chromeos_acpi.c b/drivers/platform/x86/chromeos_acpi.c
new file mode 100644
index 000000000000..4d9addee2473
--- /dev/null
+++ b/drivers/platform/x86/chromeos_acpi.c
@@ -0,0 +1,489 @@
+// 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 originating in /sys/devices/platform/chromeos_acpi.
+ */
+
+#include <linux/acpi.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/platform_device.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 list_head attribs;
+	struct list_head list;
+	struct kobject *kobj;	/* chromeos_acpi/name directory */
+	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 platform_device *pdev;
+
+	struct chromeos_acpi_attribute_group root;
+	struct list_head groups;
+};
+
+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);
+
+	ret = sysfs_create_bin_file(aag->kobj, &info->bin_attr);
+	if (ret)
+		goto free_attr_data;
+
+	list_add(&info->list, &aag->attribs);
+
+	return 0;
+
+free_attr_data:
+	kfree(info->data);
+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) {
+		sysfs_remove_bin_file(aag->kobj, &attr->bin_attr);
+		kfree(attr->name);
+		kfree(attr->data);
+		kfree(attr);
+	}
+}
+
+/**
+ * chromeos_acpi_add_group() - Create a sysfs group including attributes
+ *			       representing a nested ACPI package.
+ *
+ * @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(union acpi_object *obj, char *name,
+				   int num_attrs, int index)
+{
+	struct device *dev = &chromeos_acpi.pdev->dev;
+	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;
+	}
+
+	aag->kobj = kobject_create_and_add(aag->name, &dev->kobj);
+	if (!aag->kobj) {
+		ret = -EINVAL;
+		goto free_group_name;
+	}
+
+	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;
+	}
+
+	list_add(&aag->list, &chromeos_acpi.groups);
+
+	return 0;
+
+free_group_attr:
+	chromeos_acpi_remove_attribs(aag);
+	kobject_put(aag->kobj);
+free_group_name:
+	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->name);
+		kobject_put(aag->kobj);
+		kfree(aag);
+	}
+}
+
+/**
+ * chromeos_acpi_handle_package() - Create sysfs group including attributes
+ *				    representing an ACPI package.
+ *
+ * @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(union acpi_object *obj, char *name)
+{
+	struct device *dev = &chromeos_acpi.pdev->dev;
+	int count = obj->package.count;
+	union acpi_object *element;
+	int i, ret = 0;
+
+	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);
+		else if (element->type == ACPI_TYPE_PACKAGE)
+			/* Create a group of attributes */
+			ret = chromeos_acpi_add_group(element, name,
+						      count, i);
+		else
+			ret = -EINVAL;
+		if (ret)
+			dev_err(dev,
+				"failed to create group attributes (%d)\n",
+				ret);
+	}
+
+	return ret;
+}
+
+/**
+ * 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 = &chromeos_acpi.pdev->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(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)
+{
+	char name[ACPI_NAMESEG_SIZE + 1];
+	union acpi_object *element, *obj;
+	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;
+			}
+		}
+	}
+
+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 = &chromeos_acpi.pdev->dev;
+	int i, ret;
+
+	INIT_LIST_HEAD(&aag->attribs);
+	INIT_LIST_HEAD(&aag->list);
+
+	aag->kobj = &dev->kobj;
+
+	/*
+	 * 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);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int chromeos_acpi_device_remove(struct acpi_device *adev)
+{
+	/* Remove dinamically allocated sysfs groups and attributes */
+	chromeos_acpi_remove_groups();
+	/* Remove attributes from the root group */
+	chromeos_acpi_remove_attribs(&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 driver",
+	.class = "chromeos-acpi",
+	.ids = chromeos_device_ids,
+	.ops = {
+		.add = chromeos_acpi_device_add,
+		.remove = chromeos_acpi_device_remove,
+	},
+	.owner = THIS_MODULE,
+};
+
+static int __init chromeos_acpi_init(void)
+{
+	int ret;
+
+	chromeos_acpi.pdev = platform_device_register_simple("chromeos_acpi",
+						PLATFORM_DEVID_NONE, NULL, 0);
+	if (IS_ERR(chromeos_acpi.pdev)) {
+		pr_err("unable to register chromeos_acpi platform device\n");
+		return PTR_ERR(chromeos_acpi.pdev);
+	}
+
+	INIT_LIST_HEAD(&chromeos_acpi.groups);
+
+	ret = acpi_bus_register_driver(&chromeos_acpi_driver);
+	if (ret < 0) {
+		pr_err("failed to register chromeos_acpi driver (%d)\n", ret);
+		platform_device_unregister(chromeos_acpi.pdev);
+		chromeos_acpi.pdev = NULL;
+		return ret;
+	}
+
+	return 0;
+}
+
+static void __exit chromeos_acpi_exit(void)
+{
+	acpi_bus_unregister_driver(&chromeos_acpi_driver);
+	platform_device_unregister(chromeos_acpi.pdev);
+}
+
+module_init(chromeos_acpi_init);
+module_exit(chromeos_acpi_exit);
+
+MODULE_AUTHOR("Enric Balletbo i Serra <enric.balletbo@collabora.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ChromeOS specific ACPI extensions");
-- 
2.25.1

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

[Greg Kroah-Hartman]

On Sun, Mar 22, 2020 at 10:43:34AM +0100, Enric Balletbo i Serra wrote:
> This driver attaches to the ChromeOS ACPI device and then exports the values
> reported by the ACPI in a sysfs directory. 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 sysfs
> directory tree originating in /sys/devices/platform/chromeos_acpi.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

What is wrong with the "default" ACPI sysfs access?  Why do you need a
special driver just for this specific ACPI firmware?

Also, you forgot to add Documentation/ABI/ entries for your new files :(

> +config ACPI_CHROMEOS
> +	tristate "ChromeOS specific ACPI extensions"
> +	depends on ACPI
> +	depends on CHROME_PLATFORMS

No BUILD_TEST?


> +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) {
> +		sysfs_remove_bin_file(aag->kobj, &attr->bin_attr);

Attribute groups are your friend, do not do this "by hand".

> +		kfree(attr->name);
> +		kfree(attr->data);
> +		kfree(attr);
> +	}
> +}
> +
> +/**
> + * chromeos_acpi_add_group() - Create a sysfs group including attributes
> + *			       representing a nested ACPI package.
> + *
> + * @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(union acpi_object *obj, char *name,
> +				   int num_attrs, int index)
> +{
> +	struct device *dev = &chromeos_acpi.pdev->dev;
> +	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;
> +	}
> +
> +	aag->kobj = kobject_create_and_add(aag->name, &dev->kobj);

By using "raw" kobjects, you just now prevented any userspace tool from
seeing these attributes (like udev).  Not nice :(

Why, if you really really have to do this, are you not just using
"normal" struct device attributes instead?

> +static int __init chromeos_acpi_init(void)
> +{
> +	int ret;
> +
> +	chromeos_acpi.pdev = platform_device_register_simple("chromeos_acpi",
> +						PLATFORM_DEVID_NONE, NULL, 0);
> +	if (IS_ERR(chromeos_acpi.pdev)) {
> +		pr_err("unable to register chromeos_acpi platform device\n");
> +		return PTR_ERR(chromeos_acpi.pdev);
> +	}

Only use platform devices and drivers for things that are actually
platform devices and drivers.  That's not what this is, it is an ACPI
device and driver.  Don't abuse the platform interface please.

thanks,

greg k-h

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

[Srinivas Pandruvada]

On Sun, 2020-03-22 at 10:43 +0100, Enric Balletbo i Serra wrote:
> This driver attaches to the ChromeOS ACPI device and then exports the
> values
> reported by the ACPI in a sysfs directory. 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
> sysfs
> directory tree originating in /sys/devices/platform/chromeos_acpi.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 

[...]

>  /sys/devices/platform/chromeos_acpi/BINF.2 : 1
Just wondering why don't you create additional attributes under ACPI
device itself, like in your case:
/sys/bus/acpi/devices/GGL0001/*

Additional attributes are added for ACPI device objects at such
location in other cases also for some PNP* and INT3* objects.

This will make your driver simple with one attr group.

Thanks,
Srinivas 


>  /sys/devices/platform/chromeos_acpi/FMAP : -2031616
>  /sys/devices/platform/chromeos_acpi/HWID : SAMUS E25-G7R-W35
>  /sys/devices/platform/chromeos_acpi/BINF.0 : 0
>  /sys/devices/platform/chromeos_acpi/GPIO.0/GPIO.2 : -1
>  /sys/devices/platform/chromeos_acpi/GPIO.0/GPIO.0 : 1
>  /sys/devices/platform/chromeos_acpi/GPIO.0/GPIO.3 : INT3437:00
>  /sys/devices/platform/chromeos_acpi/GPIO.0/GPIO.1 : 0
>  /sys/devices/platform/chromeos_acpi/FRID : Google_Samus.6300.102.0
>  /sys/devices/platform/chromeos_acpi/VBNV.0 : 38
>  /sys/devices/platform/chromeos_acpi/BINF.3 : 2
>  /sys/devices/platform/chromeos_acpi/BINF.1 : 1
>  /sys/devices/platform/chromeos_acpi/GPIO.1/GPIO.2 : 16
>  /sys/devices/platform/chromeos_acpi/GPIO.1/GPIO.0 : 3
>  /sys/devices/platform/chromeos_acpi/GPIO.1/GPIO.3 : INT3437:00
>  /sys/devices/platform/chromeos_acpi/GPIO.1/GPIO.1 : 1
>  /sys/devices/platform/chromeos_acpi/CHSW : 0
>  /sys/devices/platform/chromeos_acpi/FWID : Google_Samus.6300.330.0
>  /sys/devices/platform/chromeos_acpi/VBNV.1 : 16
>  /sys/devices/platform/chromeos_acpi/BINF.4 : 0
> 
> And for binary packages:
> 
> cat /sys/devices/platform/chromeos_acpi/MECK | hexdump
>  0000000 02fb 8e72 a025 0a73 0f13 095e 9e07 41e6
>  0000010 f9e6 bb4e 76cc bef9 cca7 70e2 8f6d 863d
>  0000020
> 
> cat /sys/devices/platform/chromeos_acpi/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 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.
> 
>  drivers/platform/x86/Kconfig         |  12 +
>  drivers/platform/x86/Makefile        |   1 +
>  drivers/platform/x86/chromeos_acpi.c | 489
> +++++++++++++++++++++++++++
>  3 files changed, 502 insertions(+)
>  create mode 100644 drivers/platform/x86/chromeos_acpi.c
> 
> diff --git a/drivers/platform/x86/Kconfig
> b/drivers/platform/x86/Kconfig
> index 587403c44598..917a1c1a0758 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -72,6 +72,18 @@ config ACERHDF
>  	  If you have an Acer Aspire One netbook, say Y or M
>  	  here.
>  
> +config ACPI_CHROMEOS
> +	tristate "ChromeOS specific ACPI extensions"
> +	depends on ACPI
> +	depends on CHROME_PLATFORMS
> +	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 ALIENWARE_WMI
>  	tristate "Alienware Special feature control"
>  	depends on ACPI
> diff --git a/drivers/platform/x86/Makefile
> b/drivers/platform/x86/Makefile
> index 3747b1f07cf1..222e2e88ccb8 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -83,6 +83,7 @@ obj-$(CONFIG_SAMSUNG_Q10)	+= samsung-q10.o
>  obj-$(CONFIG_APPLE_GMUX)	+= apple-gmux.o
>  obj-$(CONFIG_INTEL_RST)		+= intel-rst.o
>  obj-$(CONFIG_INTEL_SMARTCONNECT)	+= intel-smartconnect.o
> +obj-$(CONFIG_ACPI_CHROMEOS)	+= chromeos_acpi.o
>  
>  obj-$(CONFIG_ALIENWARE_WMI)	+= alienware-wmi.o
>  obj-$(CONFIG_INTEL_PMC_IPC)	+= intel_pmc_ipc.o
> diff --git a/drivers/platform/x86/chromeos_acpi.c
> b/drivers/platform/x86/chromeos_acpi.c
> new file mode 100644
> index 000000000000..4d9addee2473
> --- /dev/null
> +++ b/drivers/platform/x86/chromeos_acpi.c
> @@ -0,0 +1,489 @@
> +// 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 originating in
> /sys/devices/platform/chromeos_acpi.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.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 list_head attribs;
> +	struct list_head list;
> +	struct kobject *kobj;	/* chromeos_acpi/name directory */
> +	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 platform_device *pdev;
> +
> +	struct chromeos_acpi_attribute_group root;
> +	struct list_head groups;
> +};
> +
> +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);
> +
> +	ret = sysfs_create_bin_file(aag->kobj, &info->bin_attr);
> +	if (ret)
> +		goto free_attr_data;
> +
> +	list_add(&info->list, &aag->attribs);
> +
> +	return 0;
> +
> +free_attr_data:
> +	kfree(info->data);
> +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) {
> +		sysfs_remove_bin_file(aag->kobj, &attr->bin_attr);
> +		kfree(attr->name);
> +		kfree(attr->data);
> +		kfree(attr);
> +	}
> +}
> +
> +/**
> + * chromeos_acpi_add_group() - Create a sysfs group including
> attributes
> + *			       representing a nested ACPI package.
> + *
> + * @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(union acpi_object *obj, char
> *name,
> +				   int num_attrs, int index)
> +{
> +	struct device *dev = &chromeos_acpi.pdev->dev;
> +	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;
> +	}
> +
> +	aag->kobj = kobject_create_and_add(aag->name, &dev->kobj);
> +	if (!aag->kobj) {
> +		ret = -EINVAL;
> +		goto free_group_name;
> +	}
> +
> +	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;
> +	}
> +
> +	list_add(&aag->list, &chromeos_acpi.groups);
> +
> +	return 0;
> +
> +free_group_attr:
> +	chromeos_acpi_remove_attribs(aag);
> +	kobject_put(aag->kobj);
> +free_group_name:
> +	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->name);
> +		kobject_put(aag->kobj);
> +		kfree(aag);
> +	}
> +}
> +
> +/**
> + * chromeos_acpi_handle_package() - Create sysfs group including
> attributes
> + *				    representing an ACPI package.
> + *
> + * @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(union acpi_object *obj, char
> *name)
> +{
> +	struct device *dev = &chromeos_acpi.pdev->dev;
> +	int count = obj->package.count;
> +	union acpi_object *element;
> +	int i, ret = 0;
> +
> +	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);
> +		else if (element->type == ACPI_TYPE_PACKAGE)
> +			/* Create a group of attributes */
> +			ret = chromeos_acpi_add_group(element, name,
> +						      count, i);
> +		else
> +			ret = -EINVAL;
> +		if (ret)
> +			dev_err(dev,
> +				"failed to create group attributes
> (%d)\n",
> +				ret);
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * 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 = &chromeos_acpi.pdev->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(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)
> +{
> +	char name[ACPI_NAMESEG_SIZE + 1];
> +	union acpi_object *element, *obj;
> +	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;
> +			}
> +		}
> +	}
> +
> +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 = &chromeos_acpi.pdev->dev;
> +	int i, ret;
> +
> +	INIT_LIST_HEAD(&aag->attribs);
> +	INIT_LIST_HEAD(&aag->list);
> +
> +	aag->kobj = &dev->kobj;
> +
> +	/*
> +	 * 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_meth
> ods[i]);
> +		if (ret) {
> +			dev_err(dev, "failed to add default methods
> (%d)\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int chromeos_acpi_device_remove(struct acpi_device *adev)
> +{
> +	/* Remove dinamically allocated sysfs groups and attributes */
> +	chromeos_acpi_remove_groups();
> +	/* Remove attributes from the root group */
> +	chromeos_acpi_remove_attribs(&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 driver",
> +	.class = "chromeos-acpi",
> +	.ids = chromeos_device_ids,
> +	.ops = {
> +		.add = chromeos_acpi_device_add,
> +		.remove = chromeos_acpi_device_remove,
> +	},
> +	.owner = THIS_MODULE,
> +};
> +
> +static int __init chromeos_acpi_init(void)
> +{
> +	int ret;
> +
> +	chromeos_acpi.pdev =
> platform_device_register_simple("chromeos_acpi",
> +						PLATFORM_DEVID_NONE,
> NULL, 0);
> +	if (IS_ERR(chromeos_acpi.pdev)) {
> +		pr_err("unable to register chromeos_acpi platform
> device\n");
> +		return PTR_ERR(chromeos_acpi.pdev);
> +	}
> +
> +	INIT_LIST_HEAD(&chromeos_acpi.groups);
> +
> +	ret = acpi_bus_register_driver(&chromeos_acpi_driver);
> +	if (ret < 0) {
> +		pr_err("failed to register chromeos_acpi driver
> (%d)\n", ret);
> +		platform_device_unregister(chromeos_acpi.pdev);
> +		chromeos_acpi.pdev = NULL;
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit chromeos_acpi_exit(void)
> +{
> +	acpi_bus_unregister_driver(&chromeos_acpi_driver);
> +	platform_device_unregister(chromeos_acpi.pdev);
> +}
> +
> +module_init(chromeos_acpi_init);
> +module_exit(chromeos_acpi_exit);
> +
> +MODULE_AUTHOR("Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("ChromeOS specific ACPI extensions");

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

[Enric Balletbo i Serra]

Hi Greg,

Many thanks for your quick answer, some comments below.

On 22/3/20 12:10, Greg Kroah-Hartman wrote:
> On Sun, Mar 22, 2020 at 10:43:34AM +0100, Enric Balletbo i Serra wrote:
>> This driver attaches to the ChromeOS ACPI device and then exports the values
>> reported by the ACPI in a sysfs directory. 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 sysfs
>> directory tree originating in /sys/devices/platform/chromeos_acpi.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> 
> What is wrong with the "default" ACPI sysfs access?  Why do you need a
> special driver just for this specific ACPI firmware?
> 

Please correct me if I am wrong, as I'm not an ACPI expert and I probably have
some ACPI leaks and misunderstandings.

What is exporting this driver is the attributes for the non-default Chromebook
specific MLST ACPI method. Hence, I assumed we needed a special driver to expose
these values that can't be done using "default" ACPI sysfs. Note that these
attributes are dynamically created and are different between Chromebooks so need
some parsing.

I didn't find a "standard" way to expose these attributes to userspace, so,
please kindly point me to one if there is one.

> Also, you forgot to add Documentation/ABI/ entries for your new files :(
> 

Right, my bad. Not all Chromebooks have the same values. I can document the ones
that are created on the devices I have but I'll probably miss some of them. I'll
do some firmware research regarding this.


>> +config ACPI_CHROMEOS
>> +	tristate "ChromeOS specific ACPI extensions"
>> +	depends on ACPI
>> +	depends on CHROME_PLATFORMS
> 
> No BUILD_TEST?
> 

Will add in the next version.

> 
>> +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) {
>> +		sysfs_remove_bin_file(aag->kobj, &attr->bin_attr);
> 
> Attribute groups are your friend, do not do this "by hand".
> 

I thought that the code is more readable doing it attribute by attribute, and
the reason is that apart from remove the bin file I should also free the name,
the data and the specific struct to store the attribute itself as all are
dynamically allocated.

Using attribute groups I should do two steps:

sysfs_remove_group()
list_for_each_entry_safe(attr, tmp_attr, &aag->attribs, list)  {
   kfree(attr->name);
   kfree(attr->data);
   kfree(attr);
}

Ok, will do that in next version.

>> +		kfree(attr->name);
>> +		kfree(attr->data);
>> +		kfree(attr);
>> +	}
>> +}
>> +
>> +/**
>> + * chromeos_acpi_add_group() - Create a sysfs group including attributes
>> + *			       representing a nested ACPI package.
>> + *
>> + * @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(union acpi_object *obj, char *name,
>> +				   int num_attrs, int index)
>> +{
>> +	struct device *dev = &chromeos_acpi.pdev->dev;
>> +	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;
>> +	}
>> +
>> +	aag->kobj = kobject_create_and_add(aag->name, &dev->kobj);
> 
> By using "raw" kobjects, you just now prevented any userspace tool from
> seeing these attributes (like udev).  Not nice :(
> 
> Why, if you really really have to do this, are you not just using
> "normal" struct device attributes instead?
> 

Ok.

>> +static int __init chromeos_acpi_init(void)
>> +{
>> +	int ret;
>> +
>> +	chromeos_acpi.pdev = platform_device_register_simple("chromeos_acpi",
>> +						PLATFORM_DEVID_NONE, NULL, 0);
>> +	if (IS_ERR(chromeos_acpi.pdev)) {
>> +		pr_err("unable to register chromeos_acpi platform device\n");
>> +		return PTR_ERR(chromeos_acpi.pdev);
>> +	}
> 
> Only use platform devices and drivers for things that are actually
> platform devices and drivers.  That's not what this is, it is an ACPI
> device and driver.  Don't abuse the platform interface please.
> 

Ok. The purpose was to not break ChromeOS userspace since is looking for the
attributes inside /sys/devices/platform/chromeos_acpi. Not a good reason, I
know, and I assume we will need to change userspace instead, and convert this to
a ACPI device and driver only, right?

I'll investigate the different places in userspace where this is used and see
how difficult it is to do the changes.

Thanks,

~Enric

> thanks,
> 
> greg k-h
> 

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

[Enric Balletbo i Serra]

Hi Srinivas,

Thanks for your quick answer.

On 22/3/20 16:21, Srinivas Pandruvada wrote:
> On Sun, 2020-03-22 at 10:43 +0100, Enric Balletbo i Serra wrote:
>> This driver attaches to the ChromeOS ACPI device and then exports the
>> values
>> reported by the ACPI in a sysfs directory. 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
>> sysfs
>> directory tree originating in /sys/devices/platform/chromeos_acpi.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
> 
> [...]
> 
>>  /sys/devices/platform/chromeos_acpi/BINF.2 : 1
> Just wondering why don't you create additional attributes under ACPI
> device itself, like in your case:
> /sys/bus/acpi/devices/GGL0001/*
> 

As I commented in my previous email, it was to no not break current ChromeOS
userspace. But I assume we will need to break it, so I'll investigate how hard
is to do that change on the userspace side.

Thanks,
Enric B.S.


> Additional attributes are added for ACPI device objects at such
> location in other cases also for some PNP* and INT3* objects.
> 
> This will make your driver simple with one attr group.
> 
> Thanks,
> Srinivas 
> 
> 
>>  /sys/devices/platform/chromeos_acpi/FMAP : -2031616
>>  /sys/devices/platform/chromeos_acpi/HWID : SAMUS E25-G7R-W35
>>  /sys/devices/platform/chromeos_acpi/BINF.0 : 0
>>  /sys/devices/platform/chromeos_acpi/GPIO.0/GPIO.2 : -1
>>  /sys/devices/platform/chromeos_acpi/GPIO.0/GPIO.0 : 1
>>  /sys/devices/platform/chromeos_acpi/GPIO.0/GPIO.3 : INT3437:00
>>  /sys/devices/platform/chromeos_acpi/GPIO.0/GPIO.1 : 0
>>  /sys/devices/platform/chromeos_acpi/FRID : Google_Samus.6300.102.0
>>  /sys/devices/platform/chromeos_acpi/VBNV.0 : 38
>>  /sys/devices/platform/chromeos_acpi/BINF.3 : 2
>>  /sys/devices/platform/chromeos_acpi/BINF.1 : 1
>>  /sys/devices/platform/chromeos_acpi/GPIO.1/GPIO.2 : 16
>>  /sys/devices/platform/chromeos_acpi/GPIO.1/GPIO.0 : 3
>>  /sys/devices/platform/chromeos_acpi/GPIO.1/GPIO.3 : INT3437:00
>>  /sys/devices/platform/chromeos_acpi/GPIO.1/GPIO.1 : 1
>>  /sys/devices/platform/chromeos_acpi/CHSW : 0
>>  /sys/devices/platform/chromeos_acpi/FWID : Google_Samus.6300.330.0
>>  /sys/devices/platform/chromeos_acpi/VBNV.1 : 16
>>  /sys/devices/platform/chromeos_acpi/BINF.4 : 0
>>
>> And for binary packages:
>>
>> cat /sys/devices/platform/chromeos_acpi/MECK | hexdump
>>  0000000 02fb 8e72 a025 0a73 0f13 095e 9e07 41e6
>>  0000010 f9e6 bb4e 76cc bef9 cca7 70e2 8f6d 863d
>>  0000020
>>
>> cat /sys/devices/platform/chromeos_acpi/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 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.
>>
>>  drivers/platform/x86/Kconfig         |  12 +
>>  drivers/platform/x86/Makefile        |   1 +
>>  drivers/platform/x86/chromeos_acpi.c | 489
>> +++++++++++++++++++++++++++
>>  3 files changed, 502 insertions(+)
>>  create mode 100644 drivers/platform/x86/chromeos_acpi.c
>>
>> diff --git a/drivers/platform/x86/Kconfig
>> b/drivers/platform/x86/Kconfig
>> index 587403c44598..917a1c1a0758 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -72,6 +72,18 @@ config ACERHDF
>>  	  If you have an Acer Aspire One netbook, say Y or M
>>  	  here.
>>  
>> +config ACPI_CHROMEOS
>> +	tristate "ChromeOS specific ACPI extensions"
>> +	depends on ACPI
>> +	depends on CHROME_PLATFORMS
>> +	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 ALIENWARE_WMI
>>  	tristate "Alienware Special feature control"
>>  	depends on ACPI
>> diff --git a/drivers/platform/x86/Makefile
>> b/drivers/platform/x86/Makefile
>> index 3747b1f07cf1..222e2e88ccb8 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -83,6 +83,7 @@ obj-$(CONFIG_SAMSUNG_Q10)	+= samsung-q10.o
>>  obj-$(CONFIG_APPLE_GMUX)	+= apple-gmux.o
>>  obj-$(CONFIG_INTEL_RST)		+= intel-rst.o
>>  obj-$(CONFIG_INTEL_SMARTCONNECT)	+= intel-smartconnect.o
>> +obj-$(CONFIG_ACPI_CHROMEOS)	+= chromeos_acpi.o
>>  
>>  obj-$(CONFIG_ALIENWARE_WMI)	+= alienware-wmi.o
>>  obj-$(CONFIG_INTEL_PMC_IPC)	+= intel_pmc_ipc.o
>> diff --git a/drivers/platform/x86/chromeos_acpi.c
>> b/drivers/platform/x86/chromeos_acpi.c
>> new file mode 100644
>> index 000000000000..4d9addee2473
>> --- /dev/null
>> +++ b/drivers/platform/x86/chromeos_acpi.c
>> @@ -0,0 +1,489 @@
>> +// 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 originating in
>> /sys/devices/platform/chromeos_acpi.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/kernel.h>
>> +#include <linux/list.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.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 list_head attribs;
>> +	struct list_head list;
>> +	struct kobject *kobj;	/* chromeos_acpi/name directory */
>> +	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 platform_device *pdev;
>> +
>> +	struct chromeos_acpi_attribute_group root;
>> +	struct list_head groups;
>> +};
>> +
>> +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);
>> +
>> +	ret = sysfs_create_bin_file(aag->kobj, &info->bin_attr);
>> +	if (ret)
>> +		goto free_attr_data;
>> +
>> +	list_add(&info->list, &aag->attribs);
>> +
>> +	return 0;
>> +
>> +free_attr_data:
>> +	kfree(info->data);
>> +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) {
>> +		sysfs_remove_bin_file(aag->kobj, &attr->bin_attr);
>> +		kfree(attr->name);
>> +		kfree(attr->data);
>> +		kfree(attr);
>> +	}
>> +}
>> +
>> +/**
>> + * chromeos_acpi_add_group() - Create a sysfs group including
>> attributes
>> + *			       representing a nested ACPI package.
>> + *
>> + * @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(union acpi_object *obj, char
>> *name,
>> +				   int num_attrs, int index)
>> +{
>> +	struct device *dev = &chromeos_acpi.pdev->dev;
>> +	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;
>> +	}
>> +
>> +	aag->kobj = kobject_create_and_add(aag->name, &dev->kobj);
>> +	if (!aag->kobj) {
>> +		ret = -EINVAL;
>> +		goto free_group_name;
>> +	}
>> +
>> +	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;
>> +	}
>> +
>> +	list_add(&aag->list, &chromeos_acpi.groups);
>> +
>> +	return 0;
>> +
>> +free_group_attr:
>> +	chromeos_acpi_remove_attribs(aag);
>> +	kobject_put(aag->kobj);
>> +free_group_name:
>> +	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->name);
>> +		kobject_put(aag->kobj);
>> +		kfree(aag);
>> +	}
>> +}
>> +
>> +/**
>> + * chromeos_acpi_handle_package() - Create sysfs group including
>> attributes
>> + *				    representing an ACPI package.
>> + *
>> + * @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(union acpi_object *obj, char
>> *name)
>> +{
>> +	struct device *dev = &chromeos_acpi.pdev->dev;
>> +	int count = obj->package.count;
>> +	union acpi_object *element;
>> +	int i, ret = 0;
>> +
>> +	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);
>> +		else if (element->type == ACPI_TYPE_PACKAGE)
>> +			/* Create a group of attributes */
>> +			ret = chromeos_acpi_add_group(element, name,
>> +						      count, i);
>> +		else
>> +			ret = -EINVAL;
>> +		if (ret)
>> +			dev_err(dev,
>> +				"failed to create group attributes
>> (%d)\n",
>> +				ret);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * 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 = &chromeos_acpi.pdev->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(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)
>> +{
>> +	char name[ACPI_NAMESEG_SIZE + 1];
>> +	union acpi_object *element, *obj;
>> +	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;
>> +			}
>> +		}
>> +	}
>> +
>> +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 = &chromeos_acpi.pdev->dev;
>> +	int i, ret;
>> +
>> +	INIT_LIST_HEAD(&aag->attribs);
>> +	INIT_LIST_HEAD(&aag->list);
>> +
>> +	aag->kobj = &dev->kobj;
>> +
>> +	/*
>> +	 * 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_meth
>> ods[i]);
>> +		if (ret) {
>> +			dev_err(dev, "failed to add default methods
>> (%d)\n",
>> +				ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int chromeos_acpi_device_remove(struct acpi_device *adev)
>> +{
>> +	/* Remove dinamically allocated sysfs groups and attributes */
>> +	chromeos_acpi_remove_groups();
>> +	/* Remove attributes from the root group */
>> +	chromeos_acpi_remove_attribs(&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 driver",
>> +	.class = "chromeos-acpi",
>> +	.ids = chromeos_device_ids,
>> +	.ops = {
>> +		.add = chromeos_acpi_device_add,
>> +		.remove = chromeos_acpi_device_remove,
>> +	},
>> +	.owner = THIS_MODULE,
>> +};
>> +
>> +static int __init chromeos_acpi_init(void)
>> +{
>> +	int ret;
>> +
>> +	chromeos_acpi.pdev =
>> platform_device_register_simple("chromeos_acpi",
>> +						PLATFORM_DEVID_NONE,
>> NULL, 0);
>> +	if (IS_ERR(chromeos_acpi.pdev)) {
>> +		pr_err("unable to register chromeos_acpi platform
>> device\n");
>> +		return PTR_ERR(chromeos_acpi.pdev);
>> +	}
>> +
>> +	INIT_LIST_HEAD(&chromeos_acpi.groups);
>> +
>> +	ret = acpi_bus_register_driver(&chromeos_acpi_driver);
>> +	if (ret < 0) {
>> +		pr_err("failed to register chromeos_acpi driver
>> (%d)\n", ret);
>> +		platform_device_unregister(chromeos_acpi.pdev);
>> +		chromeos_acpi.pdev = NULL;
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void __exit chromeos_acpi_exit(void)
>> +{
>> +	acpi_bus_unregister_driver(&chromeos_acpi_driver);
>> +	platform_device_unregister(chromeos_acpi.pdev);
>> +}
>> +
>> +module_init(chromeos_acpi_init);
>> +module_exit(chromeos_acpi_exit);
>> +
>> +MODULE_AUTHOR("Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ");
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("ChromeOS specific ACPI extensions");
> 
> 

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

[Greg Kroah-Hartman]

On Tue, Mar 24, 2020 at 05:31:10PM +0100, Enric Balletbo i Serra wrote:
> Hi Greg,
> 
> Many thanks for your quick answer, some comments below.
> 
> On 22/3/20 12:10, Greg Kroah-Hartman wrote:
> > On Sun, Mar 22, 2020 at 10:43:34AM +0100, Enric Balletbo i Serra wrote:
> >> This driver attaches to the ChromeOS ACPI device and then exports the values
> >> reported by the ACPI in a sysfs directory. 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 sysfs
> >> directory tree originating in /sys/devices/platform/chromeos_acpi.
> >>
> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > 
> > What is wrong with the "default" ACPI sysfs access?  Why do you need a
> > special driver just for this specific ACPI firmware?
> > 
> 
> Please correct me if I am wrong, as I'm not an ACPI expert and I probably have
> some ACPI leaks and misunderstandings.
> 
> What is exporting this driver is the attributes for the non-default Chromebook
> specific MLST ACPI method. Hence, I assumed we needed a special driver to expose
> these values that can't be done using "default" ACPI sysfs. Note that these
> attributes are dynamically created and are different between Chromebooks so need
> some parsing.
> 
> I didn't find a "standard" way to expose these attributes to userspace, so,
> please kindly point me to one if there is one.

Are you sure they aren't already there under /sys/firmware/acpi/?  I
thought all tables and methods were exported there with no need to do
anything special.

What makes these attributes "special" from any other ACPI method?

> >> +static int __init chromeos_acpi_init(void)
> >> +{
> >> +	int ret;
> >> +
> >> +	chromeos_acpi.pdev = platform_device_register_simple("chromeos_acpi",
> >> +						PLATFORM_DEVID_NONE, NULL, 0);
> >> +	if (IS_ERR(chromeos_acpi.pdev)) {
> >> +		pr_err("unable to register chromeos_acpi platform device\n");
> >> +		return PTR_ERR(chromeos_acpi.pdev);
> >> +	}
> > 
> > Only use platform devices and drivers for things that are actually
> > platform devices and drivers.  That's not what this is, it is an ACPI
> > device and driver.  Don't abuse the platform interface please.
> > 
> 
> Ok. The purpose was to not break ChromeOS userspace since is looking for the
> attributes inside /sys/devices/platform/chromeos_acpi. Not a good reason, I
> know, and I assume we will need to change userspace instead, and convert this to
> a ACPI device and driver only, right?

How can any userspace be looking for anything that hasn't been submitted
before?  That's nothing to worry about, we don't have to support things
like that :)

> I'll investigate the different places in userspace where this is used and see
> how difficult it is to do the changes.

Look at /sys/firmware/acpi/ first please.

thanks,

greg k-h

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

[Enric Balletbo i Serra]

Hi Greg,

On 24/3/20 17:49, Greg Kroah-Hartman wrote:
> On Tue, Mar 24, 2020 at 05:31:10PM +0100, Enric Balletbo i Serra wrote:
>> Hi Greg,
>>
>> Many thanks for your quick answer, some comments below.
>>
>> On 22/3/20 12:10, Greg Kroah-Hartman wrote:
>>> On Sun, Mar 22, 2020 at 10:43:34AM +0100, Enric Balletbo i Serra wrote:
>>>> This driver attaches to the ChromeOS ACPI device and then exports the values
>>>> reported by the ACPI in a sysfs directory. 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 sysfs
>>>> directory tree originating in /sys/devices/platform/chromeos_acpi.
>>>>
>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>
>>> What is wrong with the "default" ACPI sysfs access?  Why do you need a
>>> special driver just for this specific ACPI firmware?
>>>
>>
>> Please correct me if I am wrong, as I'm not an ACPI expert and I probably have
>> some ACPI leaks and misunderstandings.
>>
>> What is exporting this driver is the attributes for the non-default Chromebook
>> specific MLST ACPI method. Hence, I assumed we needed a special driver to expose
>> these values that can't be done using "default" ACPI sysfs. Note that these
>> attributes are dynamically created and are different between Chromebooks so need
>> some parsing.
>>
>> I didn't find a "standard" way to expose these attributes to userspace, so,
>> please kindly point me to one if there is one.
> 
> Are you sure they aren't already there under /sys/firmware/acpi/?  I
> thought all tables and methods were exported there with no need to do
> anything special.
> 

That's the first I did when I started to forward port this patch from chromeos
kernel to mainline.

On my system I get:

/sys/firmware/acpi/tables#
APIC  DSDT  FACP  FACS  HPET  MCFG  SSDT  data  dynamic

(data and dynamic are empty directories)

I quickly concluded (maybe wrong) that as there is no a MLST entry it was not
exported, but maybe one of those already contains the info? Or, should I expect
a MLST entry here?

> What makes these attributes "special" from any other ACPI method?
> 

I can't answer this question right now. I need to investigate more I guess ;-)

Thanks again for your answer,
Enric

>>>> +static int __init chromeos_acpi_init(void)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	chromeos_acpi.pdev = platform_device_register_simple("chromeos_acpi",
>>>> +						PLATFORM_DEVID_NONE, NULL, 0);
>>>> +	if (IS_ERR(chromeos_acpi.pdev)) {
>>>> +		pr_err("unable to register chromeos_acpi platform device\n");
>>>> +		return PTR_ERR(chromeos_acpi.pdev);
>>>> +	}
>>>
>>> Only use platform devices and drivers for things that are actually
>>> platform devices and drivers.  That's not what this is, it is an ACPI
>>> device and driver.  Don't abuse the platform interface please.
>>>
>>
>> Ok. The purpose was to not break ChromeOS userspace since is looking for the
>> attributes inside /sys/devices/platform/chromeos_acpi. Not a good reason, I
>> know, and I assume we will need to change userspace instead, and convert this to
>> a ACPI device and driver only, right?
> 
> How can any userspace be looking for anything that hasn't been submitted
> before?  That's nothing to worry about, we don't have to support things
> like that :)
> 
>> I'll investigate the different places in userspace where this is used and see
>> how difficult it is to do the changes.
> 
> Look at /sys/firmware/acpi/ first please.
> 
> thanks,
> 
> greg k-h
> 

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

[Greg Kroah-Hartman]

On Tue, Mar 24, 2020 at 06:08:03PM +0100, Enric Balletbo i Serra wrote:
> Hi Greg,
> 
> On 24/3/20 17:49, Greg Kroah-Hartman wrote:
> > On Tue, Mar 24, 2020 at 05:31:10PM +0100, Enric Balletbo i Serra wrote:
> >> Hi Greg,
> >>
> >> Many thanks for your quick answer, some comments below.
> >>
> >> On 22/3/20 12:10, Greg Kroah-Hartman wrote:
> >>> On Sun, Mar 22, 2020 at 10:43:34AM +0100, Enric Balletbo i Serra wrote:
> >>>> This driver attaches to the ChromeOS ACPI device and then exports the values
> >>>> reported by the ACPI in a sysfs directory. 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 sysfs
> >>>> directory tree originating in /sys/devices/platform/chromeos_acpi.
> >>>>
> >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >>>
> >>> What is wrong with the "default" ACPI sysfs access?  Why do you need a
> >>> special driver just for this specific ACPI firmware?
> >>>
> >>
> >> Please correct me if I am wrong, as I'm not an ACPI expert and I probably have
> >> some ACPI leaks and misunderstandings.
> >>
> >> What is exporting this driver is the attributes for the non-default Chromebook
> >> specific MLST ACPI method. Hence, I assumed we needed a special driver to expose
> >> these values that can't be done using "default" ACPI sysfs. Note that these
> >> attributes are dynamically created and are different between Chromebooks so need
> >> some parsing.
> >>
> >> I didn't find a "standard" way to expose these attributes to userspace, so,
> >> please kindly point me to one if there is one.
> > 
> > Are you sure they aren't already there under /sys/firmware/acpi/?  I
> > thought all tables and methods were exported there with no need to do
> > anything special.
> > 
> 
> That's the first I did when I started to forward port this patch from chromeos
> kernel to mainline.
> 
> On my system I get:
> 
> /sys/firmware/acpi/tables#
> APIC  DSDT  FACP  FACS  HPET  MCFG  SSDT  data  dynamic
> 
> (data and dynamic are empty directories)
> 
> I quickly concluded (maybe wrong) that as there is no a MLST entry it was not
> exported, but maybe one of those already contains the info? Or, should I expect
> a MLST entry here?
> 
> > What makes these attributes "special" from any other ACPI method?
> > 
> 
> I can't answer this question right now. I need to investigate more I guess ;-)

You can always ask the acpi developers as well, you need to get their
review for your driver anyway :)

good luck!

greg k-h

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

[Srinivas Pandruvada]

On Tue, 2020-03-24 at 18:08 +0100, Enric Balletbo i Serra wrote:
> Hi Greg,
> 
> On 24/3/20 17:49, Greg Kroah-Hartman wrote:
> > On Tue, Mar 24, 2020 at 05:31:10PM +0100, Enric Balletbo i Serra
> > wrote:
> > > Hi Greg,
> > > 
> > > Many thanks for your quick answer, some comments below.
> > > 
[...]

> > Are you sure they aren't already there under
> > /sys/firmware/acpi/?  I
> > thought all tables and methods were exported there with no need to
> > do
> > anything special.
> > 
> 
> That's the first I did when I started to forward port this patch from
> chromeos
> kernel to mainline.
> 
> On my system I get:
> 
> /sys/firmware/acpi/tables#
> APIC  DSDT  FACP  FACS  HPET  MCFG  SSDT  data  dynamic
> 
> (data and dynamic are empty directories)
> 
> I quickly concluded (maybe wrong) that as there is no a MLST entry it
> was not
> exported, but maybe one of those already contains the info? Or,
> should I expect
> a MLST entry here?
> 
If the data you are reading doesn't depend on any runtime variable in
ACPI tables then you can read from firmware tables as is.

You can download acpica tools and run your method on acpi dump using
acpiexec tool. Once you can take dump, you can run on any Linux system.

If you can get what you need from running on the dump, then you can do
by directly reading from /sys/firmware/acpi/tables/ from user space
without kernel change. Sometimes it is enough as lots of config data
tend to be static.

Thanks,
Srinivas






> > What makes these attributes "special" from any other ACPI method?
> > 
> 
> I can't answer this question right now. I need to investigate more I
> guess ;-)
> 
> Thanks again for your answer,
> Enric
> 
> > > > > +static int __init chromeos_acpi_init(void)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	chromeos_acpi.pdev =
> > > > > platform_device_register_simple("chromeos_acpi",
> > > > > +						PLATFORM_DEVID_
> > > > > NONE, NULL, 0);
> > > > > +	if (IS_ERR(chromeos_acpi.pdev)) {
> > > > > +		pr_err("unable to register chromeos_acpi
> > > > > platform device\n");
> > > > > +		return PTR_ERR(chromeos_acpi.pdev);
> > > > > +	}
> > > > 
> > > > Only use platform devices and drivers for things that are
> > > > actually
> > > > platform devices and drivers.  That's not what this is, it is
> > > > an ACPI
> > > > device and driver.  Don't abuse the platform interface please.
> > > > 
> > > 
> > > Ok. The purpose was to not break ChromeOS userspace since is
> > > looking for the
> > > attributes inside /sys/devices/platform/chromeos_acpi. Not a good
> > > reason, I
> > > know, and I assume we will need to change userspace instead, and
> > > convert this to
> > > a ACPI device and driver only, right?
> > 
> > How can any userspace be looking for anything that hasn't been
> > submitted
> > before?  That's nothing to worry about, we don't have to support
> > things
> > like that :)
> > 
> > > I'll investigate the different places in userspace where this is
> > > used and see
> > > how difficult it is to do the changes.
> > 
> > Look at /sys/firmware/acpi/ first please.
> > 
> > thanks,
> > 
> > greg k-h
> > 

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

[Enric Balletbo i Serra]

Hi Srinivas,

On 24/3/20 18:20, Srinivas Pandruvada wrote:
> On Tue, 2020-03-24 at 18:08 +0100, Enric Balletbo i Serra wrote:
>> Hi Greg,
>>
>> On 24/3/20 17:49, Greg Kroah-Hartman wrote:
>>> On Tue, Mar 24, 2020 at 05:31:10PM +0100, Enric Balletbo i Serra
>>> wrote:
>>>> Hi Greg,
>>>>
>>>> Many thanks for your quick answer, some comments below.
>>>>
> [...]
> 
>>> Are you sure they aren't already there under
>>> /sys/firmware/acpi/?  I
>>> thought all tables and methods were exported there with no need to
>>> do
>>> anything special.
>>>
>>
>> That's the first I did when I started to forward port this patch from
>> chromeos
>> kernel to mainline.
>>
>> On my system I get:
>>
>> /sys/firmware/acpi/tables#
>> APIC  DSDT  FACP  FACS  HPET  MCFG  SSDT  data  dynamic
>>
>> (data and dynamic are empty directories)
>>
>> I quickly concluded (maybe wrong) that as there is no a MLST entry it
>> was not
>> exported, but maybe one of those already contains the info? Or,
>> should I expect
>> a MLST entry here?
>>
> If the data you are reading doesn't depend on any runtime variable in
> ACPI tables then you can read from firmware tables as is.
> 
> You can download acpica tools and run your method on acpi dump using
> acpiexec tool. Once you can take dump, you can run on any Linux system.
> 
> If you can get what you need from running on the dump, then you can do
> by directly reading from /sys/firmware/acpi/tables/ from user space
> without kernel change. Sometimes it is enough as lots of config data
> tend to be static.
> 

As I said I'm not an ACPI expert, so thanks in advance for your help.

I am trying to look if I can get from userspace the value of the HWID entry
exported from the driver.

$ cat /sys/devices/platform/chromeos_acpi/HWID
SAMUS E25-G7R-W35

Using acpiexec I get the element list of the MLST method, but I don't know how
to get the HWID value.

- evaluate crhw.mlst
Evaluating \CRHW.MLST
Evaluation of \CRHW.MLST returned object 0x55f17a7aed60, external buffer length 158
  [Package] Contains 10 Elements:
    [String] Length 04 = "CHSW"
    [String] Length 04 = "FWID"
    [String] Length 04 = "HWID"
    [String] Length 04 = "FRID"
    [String] Length 04 = "BINF"
    [String] Length 04 = "GPIO"
    [String] Length 04 = "VBNV"
    [String] Length 04 = "VDAT"
    [String] Length 04 = "FMAP"
    [String] Length 04 = "MECK"

Any clue?

Thanks in advance,
Enric


> Thanks,
> Srinivas
> 
> 
> 
> 
> 
> 
>>> What makes these attributes "special" from any other ACPI method?
>>>
>>
>> I can't answer this question right now. I need to investigate more I
>> guess ;-)
>>
>> Thanks again for your answer,
>> Enric
>>
>>>>>> +static int __init chromeos_acpi_init(void)
>>>>>> +{
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	chromeos_acpi.pdev =
>>>>>> platform_device_register_simple("chromeos_acpi",
>>>>>> +						PLATFORM_DEVID_
>>>>>> NONE, NULL, 0);
>>>>>> +	if (IS_ERR(chromeos_acpi.pdev)) {
>>>>>> +		pr_err("unable to register chromeos_acpi
>>>>>> platform device\n");
>>>>>> +		return PTR_ERR(chromeos_acpi.pdev);
>>>>>> +	}
>>>>>
>>>>> Only use platform devices and drivers for things that are
>>>>> actually
>>>>> platform devices and drivers.  That's not what this is, it is
>>>>> an ACPI
>>>>> device and driver.  Don't abuse the platform interface please.
>>>>>
>>>>
>>>> Ok. The purpose was to not break ChromeOS userspace since is
>>>> looking for the
>>>> attributes inside /sys/devices/platform/chromeos_acpi. Not a good
>>>> reason, I
>>>> know, and I assume we will need to change userspace instead, and
>>>> convert this to
>>>> a ACPI device and driver only, right?
>>>
>>> How can any userspace be looking for anything that hasn't been
>>> submitted
>>> before?  That's nothing to worry about, we don't have to support
>>> things
>>> like that :)
>>>
>>>> I'll investigate the different places in userspace where this is
>>>> used and see
>>>> how difficult it is to do the changes.
>>>
>>> Look at /sys/firmware/acpi/ first please.
>>>
>>> thanks,
>>>
>>> greg k-h
>>>
> 
> 

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

[Srinivas Pandruvada]

Hi Enric,

On Wed, 2020-03-25 at 21:34 +0100, Enric Balletbo i Serra wrote:
> Hi Srinivas,
> 
> On 24/3/20 18:20, Srinivas Pandruvada wrote:
> > On Tue, 2020-03-24 at 18:08 +0100, Enric Balletbo i Serra wrote:
> > > Hi Greg,
> > > 
> > > On 24/3/20 17:49, Greg Kroah-Hartman wrote:
> > > > On Tue, Mar 24, 2020 at 05:31:10PM +0100, Enric Balletbo i
> > > > Serra
> > > > wrote:
> > > > > Hi Greg,
> > > > > 
> > > > > Many thanks for your quick answer, some comments below.
> > > > > 
> > [...]
> > 
> > > > Are you sure they aren't already there under
> > > > /sys/firmware/acpi/?  I
> > > > thought all tables and methods were exported there with no need
> > > > to
> > > > do
> > > > anything special.
> > > > 
> > > 
> > > That's the first I did when I started to forward port this patch
> > > from
> > > chromeos
> > > kernel to mainline.
> > > 
> > > On my system I get:
> > > 
> > > /sys/firmware/acpi/tables#
> > > APIC  DSDT  FACP  FACS  HPET  MCFG  SSDT  data  dynamic
> > > 
> > > (data and dynamic are empty directories)
> > > 
> > > I quickly concluded (maybe wrong) that as there is no a MLST
> > > entry it
> > > was not
> > > exported, but maybe one of those already contains the info? Or,
> > > should I expect
> > > a MLST entry here?
> > > 
> > If the data you are reading doesn't depend on any runtime variable
> > in
> > ACPI tables then you can read from firmware tables as is.
> > 
> > You can download acpica tools and run your method on acpi dump
> > using
> > acpiexec tool. Once you can take dump, you can run on any Linux
> > system.
> > 
> > If you can get what you need from running on the dump, then you can
> > do
> > by directly reading from /sys/firmware/acpi/tables/ from user space
> > without kernel change. Sometimes it is enough as lots of config
> > data
> > tend to be static.
> > 
> 
> As I said I'm not an ACPI expert, so thanks in advance for your help.
> 
> I am trying to look if I can get from userspace the value of the HWID
> entry
> exported from the driver.
> 
> $ cat /sys/devices/platform/chromeos_acpi/HWID
> SAMUS E25-G7R-W35
> 
> Using acpiexec I get the element list of the MLST method, but I don't
> know how
> to get the HWID value.
> 
> - evaluate crhw.mlst
> Evaluating \CRHW.MLST
> Evaluation of \CRHW.MLST returned object 0x55f17a7aed60, external
> buffer length 158
>   [Package] Contains 10 Elements:
>     [String] Length 04 = "CHSW"
>     [String] Length 04 = "FWID"
>     [String] Length 04 = "HWID"
>     [String] Length 04 = "FRID"
>     [String] Length 04 = "BINF"
>     [String] Length 04 = "GPIO"
>     [String] Length 04 = "VBNV"
>     [String] Length 04 = "VDAT"
>     [String] Length 04 = "FMAP"
>     [String] Length 04 = "MECK"
> 
> Any clue?
> 
So I guess your  mlst method gives list of methods you can call.
So here your can directly evaluate

- evaluate crhw.HWID


Thanks,
Srinivas



> Thanks in advance,
> Enric
> 
> 
> > Thanks,
> > Srinivas
> > 
> > 
> > 
> > 
> > 
> > 
> > > > What makes these attributes "special" from any other ACPI
> > > > method?
> > > > 
> > > 
> > > I can't answer this question right now. I need to investigate
> > > more I
> > > guess ;-)
> > > 
> > > Thanks again for your answer,
> > > Enric
> > > 
> > > > > > > +static int __init chromeos_acpi_init(void)
> > > > > > > +{
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	chromeos_acpi.pdev =
> > > > > > > platform_device_register_simple("chromeos_acpi",
> > > > > > > +						PLATFORM_DEVID_
> > > > > > > NONE, NULL, 0);
> > > > > > > +	if (IS_ERR(chromeos_acpi.pdev)) {
> > > > > > > +		pr_err("unable to register chromeos_acpi
> > > > > > > platform device\n");
> > > > > > > +		return PTR_ERR(chromeos_acpi.pdev);
> > > > > > > +	}
> > > > > > 
> > > > > > Only use platform devices and drivers for things that are
> > > > > > actually
> > > > > > platform devices and drivers.  That's not what this is, it
> > > > > > is
> > > > > > an ACPI
> > > > > > device and driver.  Don't abuse the platform interface
> > > > > > please.
> > > > > > 
> > > > > 
> > > > > Ok. The purpose was to not break ChromeOS userspace since is
> > > > > looking for the
> > > > > attributes inside /sys/devices/platform/chromeos_acpi. Not a
> > > > > good
> > > > > reason, I
> > > > > know, and I assume we will need to change userspace instead,
> > > > > and
> > > > > convert this to
> > > > > a ACPI device and driver only, right?
> > > > 
> > > > How can any userspace be looking for anything that hasn't been
> > > > submitted
> > > > before?  That's nothing to worry about, we don't have to
> > > > support
> > > > things
> > > > like that :)
> > > > 
> > > > > I'll investigate the different places in userspace where this
> > > > > is
> > > > > used and see
> > > > > how difficult it is to do the changes.
> > > > 
> > > > Look at /sys/firmware/acpi/ first please.
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > > 

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

[Enric Balletbo i Serra]

Hi Srinivas,

On 25/3/20 22:41, Srinivas Pandruvada wrote:
> Hi Enric,
> 
> On Wed, 2020-03-25 at 21:34 +0100, Enric Balletbo i Serra wrote:
>> Hi Srinivas,
>>
>> On 24/3/20 18:20, Srinivas Pandruvada wrote:
>>> On Tue, 2020-03-24 at 18:08 +0100, Enric Balletbo i Serra wrote:
>>>> Hi Greg,
>>>>
>>>> On 24/3/20 17:49, Greg Kroah-Hartman wrote:
>>>>> On Tue, Mar 24, 2020 at 05:31:10PM +0100, Enric Balletbo i
>>>>> Serra
>>>>> wrote:
>>>>>> Hi Greg,
>>>>>>
>>>>>> Many thanks for your quick answer, some comments below.
>>>>>>
>>> [...]
>>>
>>>>> Are you sure they aren't already there under
>>>>> /sys/firmware/acpi/?  I
>>>>> thought all tables and methods were exported there with no need
>>>>> to
>>>>> do
>>>>> anything special.
>>>>>
>>>>
>>>> That's the first I did when I started to forward port this patch
>>>> from
>>>> chromeos
>>>> kernel to mainline.
>>>>
>>>> On my system I get:
>>>>
>>>> /sys/firmware/acpi/tables#
>>>> APIC  DSDT  FACP  FACS  HPET  MCFG  SSDT  data  dynamic
>>>>
>>>> (data and dynamic are empty directories)
>>>>
>>>> I quickly concluded (maybe wrong) that as there is no a MLST
>>>> entry it
>>>> was not
>>>> exported, but maybe one of those already contains the info? Or,
>>>> should I expect
>>>> a MLST entry here?
>>>>
>>> If the data you are reading doesn't depend on any runtime variable
>>> in
>>> ACPI tables then you can read from firmware tables as is.
>>>
>>> You can download acpica tools and run your method on acpi dump
>>> using
>>> acpiexec tool. Once you can take dump, you can run on any Linux
>>> system.
>>>
>>> If you can get what you need from running on the dump, then you can
>>> do
>>> by directly reading from /sys/firmware/acpi/tables/ from user space
>>> without kernel change. Sometimes it is enough as lots of config
>>> data
>>> tend to be static.
>>>
>>
>> As I said I'm not an ACPI expert, so thanks in advance for your help.
>>
>> I am trying to look if I can get from userspace the value of the HWID
>> entry
>> exported from the driver.
>>
>> $ cat /sys/devices/platform/chromeos_acpi/HWID
>> SAMUS E25-G7R-W35
>>
>> Using acpiexec I get the element list of the MLST method, but I don't
>> know how
>> to get the HWID value.
>>
>> - evaluate crhw.mlst
>> Evaluating \CRHW.MLST
>> Evaluation of \CRHW.MLST returned object 0x55f17a7aed60, external
>> buffer length 158
>>   [Package] Contains 10 Elements:
>>     [String] Length 04 = "CHSW"
>>     [String] Length 04 = "FWID"
>>     [String] Length 04 = "HWID"
>>     [String] Length 04 = "FRID"
>>     [String] Length 04 = "BINF"
>>     [String] Length 04 = "GPIO"
>>     [String] Length 04 = "VBNV"
>>     [String] Length 04 = "VDAT"
>>     [String] Length 04 = "FMAP"
>>     [String] Length 04 = "MECK"
>>
>> Any clue?
>>
> So I guess your  mlst method gives list of methods you can call.
> So here your can directly evaluate
> 
> - evaluate crhw.HWID
> 

Right, I tried, but that doesn't gives the result I want, for example:

Evaluating \CRHW.HWID
0x1 Outstanding allocations after evaluation of \CRHW.HWID
Evaluation of \CRHW.HWID returned object 0x55b9e373fd60, external buffer length 38
  [Package] Contains 1 Elements:
    [String] Length 00 = ""


I found that the VBTx are addresses setup in the following file.

src/vendorcode/google/chromeos/acpi/gnvs.asl:

VBT0,   32,     // 0x000 - Boot Reason
VBT1,   32,     // 0x004 - Active Main Firmware
VBT2,   32,     // 0x008 - Active EC Firmware
VBT3,   16,     // 0x00c - CHSW
VBT4, 2048,     // 0x00e - HWID
VBT5,  512,     // 0x10e - FWID
VBT6,  512,     // 0x14e - FRID
VBT7,   32,     // 0x18e - active main firmware type
VBT8,   32,     // 0x192 - Recovery Reason
VBT9,   32,     // 0x196 - FMAP base address
CHVD, 24576,    // 0x19a - VDAT space filled by verified boot
VBTA,   32,     // 0xd9a - pointer to smbios FWID
MEHH,  256,     // 0xd9e - Management Engine Hash
RMOB,   32,     // 0xdbe - RAM oops base address
RMOL,   32,     // 0xdc2 - RAM oops length
ROVP,   32,     // 0xdc6 - pointer to RO_VPD
ROVL,   32,     // 0xdca - size of RO_VPD
RWVP,   32,     // 0xdce - pointer to RW_VPD
RWVL,   32,     // 0xdd2 - size of RW_VPD
                // 0xdd6

Can I assume that the info I want is only accessible in runtime and is not
exported via the static tables?

Thanks,
 Enric


> 
> Thanks,
> Srinivas
> 
> 
> 
>> Thanks in advance,
>> Enric
>>
>>
>>> Thanks,
>>> Srinivas
>>>
>>>
>>>
>>>
>>>
>>>
>>>>> What makes these attributes "special" from any other ACPI
>>>>> method?
>>>>>
>>>>
>>>> I can't answer this question right now. I need to investigate
>>>> more I
>>>> guess ;-)
>>>>
>>>> Thanks again for your answer,
>>>> Enric
>>>>
>>>>>>>> +static int __init chromeos_acpi_init(void)
>>>>>>>> +{
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	chromeos_acpi.pdev =
>>>>>>>> platform_device_register_simple("chromeos_acpi",
>>>>>>>> +						PLATFORM_DEVID_
>>>>>>>> NONE, NULL, 0);
>>>>>>>> +	if (IS_ERR(chromeos_acpi.pdev)) {
>>>>>>>> +		pr_err("unable to register chromeos_acpi
>>>>>>>> platform device\n");
>>>>>>>> +		return PTR_ERR(chromeos_acpi.pdev);
>>>>>>>> +	}
>>>>>>>
>>>>>>> Only use platform devices and drivers for things that are
>>>>>>> actually
>>>>>>> platform devices and drivers.  That's not what this is, it
>>>>>>> is
>>>>>>> an ACPI
>>>>>>> device and driver.  Don't abuse the platform interface
>>>>>>> please.
>>>>>>>
>>>>>>
>>>>>> Ok. The purpose was to not break ChromeOS userspace since is
>>>>>> looking for the
>>>>>> attributes inside /sys/devices/platform/chromeos_acpi. Not a
>>>>>> good
>>>>>> reason, I
>>>>>> know, and I assume we will need to change userspace instead,
>>>>>> and
>>>>>> convert this to
>>>>>> a ACPI device and driver only, right?
>>>>>
>>>>> How can any userspace be looking for anything that hasn't been
>>>>> submitted
>>>>> before?  That's nothing to worry about, we don't have to
>>>>> support
>>>>> things
>>>>> like that :)
>>>>>
>>>>>> I'll investigate the different places in userspace where this
>>>>>> is
>>>>>> used and see
>>>>>> how difficult it is to do the changes.
>>>>>
>>>>> Look at /sys/firmware/acpi/ first please.
>>>>>
>>>>> thanks,
>>>>>
>>>>> greg k-h
>>>>>
> 

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

[Srinivas Pandruvada]

On Wed, 2020-03-25 at 22:54 +0100, Enric Balletbo i Serra wrote:
> Hi Srinivas,
> 

[cut]

> Evaluating \CRHW.HWID
> 0x1 Outstanding allocations after evaluation of \CRHW.HWID
> Evaluation of \CRHW.HWID returned object 0x55b9e373fd60, external
> buffer length 38
>   [Package] Contains 1 Elements:
>     [String] Length 00 = ""
> 
> 
> I found that the VBTx are addresses setup in the following file.
> 
> src/vendorcode/google/chromeos/acpi/gnvs.asl:
> 
> VBT0,   32,     // 0x000 - Boot Reason
> VBT1,   32,     // 0x004 - Active Main Firmware
> VBT2,   32,     // 0x008 - Active EC Firmware
> VBT3,   16,     // 0x00c - CHSW
> VBT4, 2048,     // 0x00e - HWID
> VBT5,  512,     // 0x10e - FWID
> VBT6,  512,     // 0x14e - FRID
> VBT7,   32,     // 0x18e - active main firmware type
> VBT8,   32,     // 0x192 - Recovery Reason
> VBT9,   32,     // 0x196 - FMAP base address
> CHVD, 24576,    // 0x19a - VDAT space filled by verified boot
> VBTA,   32,     // 0xd9a - pointer to smbios FWID
> MEHH,  256,     // 0xd9e - Management Engine Hash
> RMOB,   32,     // 0xdbe - RAM oops base address
> RMOL,   32,     // 0xdc2 - RAM oops length
> ROVP,   32,     // 0xdc6 - pointer to RO_VPD
> ROVL,   32,     // 0xdca - size of RO_VPD
> RWVP,   32,     // 0xdce - pointer to RW_VPD
> RWVL,   32,     // 0xdd2 - size of RW_VPD
>                 // 0xdd6
> 
> Can I assume that the info I want is only accessible in runtime and
> is not
> exported via the static tables?
Yes. Basically these are pointing to a memory address. From user space
you can't read this memory. We are planing to do something for this,
but it will take sometime.

Thanks,
Srinivas



> 
> Thanks,
>  Enric