Re: [PATCH v8] platform: x86: Add ChromeOS ACPI device driver

[Barnabás Pőcze]

Hi


2022. április 15., péntek 19:08 keltezéssel, Muhammad Usama Anjum <usama.anjum@xxxxxxxxxxxxx> írta

> From: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>
> The x86 Chromebooks have ChromeOS ACPI device. This driver attaches to
> the ChromeOS ACPI device and exports the values reported by ACPI in a
> sysfs directory. This data isn't present in ACPI tables when read
> through ACPI tools, hence a driver is needed to do it. The driver gets
> data from firmware using ACPI component of the kernel. The ACPI values
> are presented in string form (numbers as decimal values) or binary
> blobs, and can be accessed as the contents of the appropriate read only
> files in the standard ACPI device's sysfs directory tree. This data is
> consumed by the ChromeOS user space.
>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@xxxxxxxxxxxxx>
> ---
> Changes in v8:
> - Change struct platform_driver name from chromeos_acpi_driver to
>   chromeos_acpi_device_driver
>
> Changes in v7:
> - Rename acpi_chromeos Kconfig option to chromeos_acpi
> - Change this acpi driver to platform driver
> - Minor cosmetic changes
>
> There were the following concerns on v4 which have been delt with in
> v5/v6:
> - Remove BINF.{0,1,4} from sysfs as they are reserved and not used
>   anymore
> - Reword the description of MECK
> - Change function name from chromeos_acpi_alloc_name() to
>   chromeos_acpi_gen_file_name()
> - Remove local variable obj in chromeos_acpi_add_method()
> - Replace usage of dev_info() to dev_dbg()
> - Improve the description of the patch
> - Add the firmware interface document which serves as primary
>   documentation and garantees that this interface will not change
> - GGL0001 is valid PNP ID of the Google. PNP ID can be used with the
>   ACPI devices. Consensus was developed on it in discussion of v4.
>
> Changes in v6:
> - Correct authorship and path email's From
> - Add changelog between v4 and v5 in detail
> - Add copywrite year 2022
> - Improve the description and add concerns from V4 which have been fixed
>
> Changes in v5:
> - Improve the description of the patch
> - Document firmware interface
> - Update sysfs interface documentation
> - Remove binf{0,1,4} as they have been deprecated
> - Update some cleanup logic in case of error
>   - Remove freeing of chromeos_acpi.root explicitely in
>     chromeos_acpi_device_remove() as it'll be automatically freed by
>     chromeos_acpi_remove_groups()
>   - If sysfs_create_groups() fails in chromeos_acpi_process_mlst(),
>     cleanup all groups
> - Cosmetic changes
>
> Changes in v4:
> https://lore.kernel.org/lkml/20200413134611.478441-1-enric.balletbo@xxxxxxxxxxxxx/t/
> - Add COMPILE_TEST to increase build coverage.
> - Add sysfs ABI documentation.
> - Rebased on top of 5.7-rc1 and solve conflicts.
> - Cc ACPI maintainers.
>
> Changes in v3:
> - Use attribute groups instead of adding files "by hand".
> - Do not use "raw" kobject to create directories.
> - Do not abuse of the platform_device interface. Remove it.
>
> Changes in v2:
> - Note that this version is a total rework, with those major changes:
> - Use lists to track dinamically allocated attributes and groups.
> - Use sysfs binary attributes to store the ACPI contents.
> - Remove all the functionalities except the one that creates the sysfs files.
> ---
>  .../ABI/testing/sysfs-driver-chromeos-acpi    | 126 +++++
>  .../acpi/chromeos-acpi-device.rst             | 363 +++++++++++++
>  Documentation/firmware-guide/acpi/index.rst   |   1 +
>  drivers/platform/x86/Kconfig                  |  11 +
>  drivers/platform/x86/Makefile                 |   3 +
>  drivers/platform/x86/chromeos_acpi.c          | 513 ++++++++++++++++++
>  6 files changed, 1017 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-chromeos-acpi
>  create mode 100644 Documentation/firmware-guide/acpi/chromeos-acpi-device.rst
>  create mode 100644 drivers/platform/x86/chromeos_acpi.c
>
> [...]
> diff --git a/drivers/platform/x86/chromeos_acpi.c b/drivers/platform/x86/chromeos_acpi.c
> new file mode 100644
> index 0000000000000..de86119a446b8
> --- /dev/null
> +++ b/drivers/platform/x86/chromeos_acpi.c
> @@ -0,0 +1,513 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ChromeOS specific ACPI extensions
> + *
> + * Copyright 2011 Google, Inc.
> + * Copyright 2020-2022 Google LLC
> + *
> + * This file is a rework and part of the code is ported from chromeos-3.18
> + * kernel and was originally written by Vadim Bendebury <vbendeb@xxxxxxxxxxxx>.
> + *
> + * This driver attaches to the ChromeOS ACPI device and then exports the
> + * values reported by the ACPI in a sysfs directory. All values are
> + * presented in the string form (numbers as decimal values) and can be
> + * accessed as the contents of the appropriate read only files in the
> + * sysfs directory tree.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +
> +/*
> + * ACPI method name for MLST; the response for this method is a package of
> + * strings listing the methods which should be reflected in sysfs.
> + */
> +#define MLST "MLST"
> +
> +/*
> + * The default list of methods the ChromeOS ACPI device is supposed to export,
> + * if the MLST method is not present or is poorly formed.  The MLST method
> + * itself is included, to aid in debugging.
> + */
> +static char *chromeos_acpi_default_methods[] = {
> +	"CHSW", "HWID", "BINF", "GPIO", "CHNV", "FWID", "FRID", MLST
> +};
> +
> +/*
> + * Representation of a single sysfs attribute. In addition to the standard
> + * bin_attribute structure has a list of these structures (to keep track for
> + * de-allocation when removing the driver) and a pointer to the actual
> + * attribute name and value, reported when accessing the appropriate sysfs
> + * file.
> + */
> +struct chromeos_acpi_attribute {
> +	struct bin_attribute bin_attr;
> +	struct list_head list;
> +	char *name;
> +	char *data;
> +};
> +
> +/*
> + * Representation of a sysfs attribute group (a sub directory in the device's
> + * sysfs directory). In addition to the standard structure has lists to allow
> + * to keep track of the allocated structures.
> + */
> +struct chromeos_acpi_attribute_group {
> +	struct attribute_group group;
> +	struct list_head attribs;
> +	struct list_head list;
> +	char *name;
> +};
> +
> +/*
> + * This is the main structure, we use it to store data and adds links pointing
> + * at lists of allocated attributes and attribute groups.
> + */
> +struct chromeos_acpi_dev {
> +	struct chromeos_acpi_attribute_group *root;
> +	const struct attribute_group **dev_groups;
> +	struct list_head groups;
> +	unsigned int num_groups;
> +	unsigned int num_attrs;
> +};
> +
> +static struct chromeos_acpi_dev chromeos_acpi;
> +
> +static ssize_t chromeos_acpi_read_bin_attribute(struct file *filp,
> +						struct kobject *kobj,
> +						struct bin_attribute *bin_attr,
> +						char *buffer, loff_t pos,
> +						size_t count)
> +{
> +	struct chromeos_acpi_attribute *info = bin_attr->private;
> +
> +	return memory_read_from_buffer(buffer, count, &pos, info->data,
> +				       info->bin_attr.size);
> +}
> +
> +static char *chromeos_acpi_gen_file_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 null character */
> +	int length;
> +	int ret;
> +
> +	/* Files BINF.{0,1,4} are historical and no longer used. */
> +	if (!strcmp(name, "BINF") && (index == 0 || index == 1 || index == 4))
> +		return 0;
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->name = chromeos_acpi_gen_file_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);

You can use `kasprintf()` here, no?


> +		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);

(technically you only need to initialize the list head explicitly, so this is not
 strictly needed; there are other examples of such `INIT_LIST_HEAD()` calls
 in the code)


> +
> +	list_add(&info->list, &aag->attribs);
> +	return 0;
> +
> +free_attr_name:
> +	kfree(info->name);
> +free_attribute:
> +	kfree(info);
> +	return ret;
> +}
> +
> +static void
> +chromeos_acpi_remove_attribs(struct chromeos_acpi_attribute_group *aag)
> +{
> +	struct chromeos_acpi_attribute *attr, *tmp_attr;
> +
> +	list_for_each_entry_safe(attr, tmp_attr, &aag->attribs, list) {
> +		kfree(attr->name);
> +		kfree(attr->data);
> +		kfree(attr);
> +	}
> +}
> +
> +static int
> +chromeos_acpi_add_attribs_to_group(struct chromeos_acpi_attribute_group *aag,
> +				   unsigned int num_attrs)
> +{
> +	struct chromeos_acpi_attribute *attr;
> +	int count = 0;
> +
> +	aag->group.bin_attrs = kcalloc(num_attrs + 1,
> +				       sizeof(*aag->group.bin_attrs),
> +				       GFP_KERNEL);
> +	if (!aag->group.bin_attrs)
> +		return -ENOMEM;
> +
> +	list_for_each_entry(attr, &aag->attribs, list) {
> +		aag->group.bin_attrs[count] = &attr->bin_attr;
> +		count++;
> +	}
> +
> +	chromeos_acpi.num_groups++;
> +	list_add(&aag->list, &chromeos_acpi.groups);
> +
> +	return 0;
> +}
> +
> +/**
> + * chromeos_acpi_add_group() - Create a sysfs group including attributes
> + *			       representing a nested ACPI package
> + *
> + * @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 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_gen_file_name(name, num_attrs, index);
> +	if (!aag->name) {
> +		ret = -ENOMEM;
> +		goto free_group;
> +	}
> +
> +	INIT_LIST_HEAD(&aag->attribs);
> +	INIT_LIST_HEAD(&aag->list);
> +
> +	count = obj->package.count;
> +	element = obj->package.elements;
> +	for (i = 0; i < count; i++, element++) {
> +		ret = chromeos_acpi_add_attr(aag, element, name, count, i);
> +		if (ret)
> +			goto free_group_attr;
> +	}
> +
> +	aag->group.name = aag->name;
> +
> +	ret = chromeos_acpi_add_attribs_to_group(aag, count);
> +	if (ret)
> +		goto free_group_attr;
> +
> +	return 0;
> +
> +free_group_attr:
> +	chromeos_acpi_remove_attribs(aag);
> +	kfree(aag->name);
> +free_group:
> +	kfree(aag);
> +	return ret;
> +}
> +
> +static void chromeos_acpi_remove_groups(void)
> +{
> +	struct chromeos_acpi_attribute_group *aag, *tmp_aag;
> +
> +	list_for_each_entry_safe(aag, tmp_aag, &chromeos_acpi.groups, list) {
> +		chromeos_acpi_remove_attribs(aag);
> +		kfree(aag->group.bin_attrs);
> +		kfree(aag->name);
> +		kfree(aag);
> +	}
> +}
> +
> +/**
> + * chromeos_acpi_handle_package() - Create sysfs group including attributes
> + *				    representing an ACPI package
> + *
> + * @pdev: Platform device
> + * @obj: Package contents as returned by ACPI
> + * @name: Name of the group
> + *
> + * Scalar objects included in the package get sysfs attributes created for
> + * them. Nested packages are passed to a function creating a sysfs group per
> + * package.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +static int chromeos_acpi_handle_package(struct platform_device *pdev,
> +					union acpi_object *obj, char *name)
> +{
> +	struct device *dev = &pdev->dev;
> +	int count = obj->package.count;
> +	union acpi_object *element;
> +	int i, ret;
> +
> +	element = obj->package.elements;
> +	for (i = 0; i < count; i++, element++) {
> +		if (element->type == ACPI_TYPE_BUFFER ||
> +		    element->type == ACPI_TYPE_STRING ||
> +		    element->type == ACPI_TYPE_INTEGER) {
> +			/* Create a single attribute in the root directory */
> +			ret = chromeos_acpi_add_attr(chromeos_acpi.root,
> +						     element, name,
> +						     count, i);
> +			if (ret) {
> +				dev_err(dev, "error adding attributes (%d)\n",
> +					ret);
> +				return ret;
> +			}
> +			chromeos_acpi.num_attrs++;
> +		} else if (element->type == ACPI_TYPE_PACKAGE) {
> +			/* Create a group of attributes */
> +			ret = chromeos_acpi_add_group(element, name, count, i);
> +			if (ret) {
> +				dev_err(dev, "error adding a group (%d)\n",
> +					ret);
> +				return ret;
> +			}
> +		} else {
> +			if (ret) {

`ret` can be potentially uninitialized here, no?


> +				dev_err(dev, "error on element type (%d)\n",
> +					ret);
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * chromeos_acpi_add_method() - Evaluate an ACPI method and create sysfs
> + *				attributes
> + *
> + * @pdev: Platform device
> + * @name: Name of the method to evaluate
> + *
> + * Return: 0 on success, non-zero on failure
> + */
> +static int chromeos_acpi_add_method(struct platform_device *pdev, char *name)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +	acpi_status status;
> +	int ret = 0;
> +
> +	status = acpi_evaluate_object(ACPI_COMPANION(&pdev->dev)->handle, name, NULL, &output);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(dev, "failed to retrieve %s (%d)\n", name, status);

(maybe `acpi_format_exception(status)` would be more meaningful than the numeric value)


> +		return status;

This return value is potentially propagated to become the return value of
the probe function. The problem is that it is not a negative errno that the probe
method should return but rather an ACPI status code.


> +	}
> +
> +	if (((union acpi_object *)output.pointer)->type == ACPI_TYPE_PACKAGE)
> +		ret = chromeos_acpi_handle_package(pdev, output.pointer, name);
> +
> +	kfree(output.pointer);
> +	return ret;
> +}
> +
> +/**
> + * chromeos_acpi_process_mlst() - Evaluate the MLST method and add methods
> + *				  listed in the response
> + *
> + * @pdev: Platform device
> + *
> + * Returns: 0 if successful, non-zero if error.
> + */
> +static int chromeos_acpi_process_mlst(struct platform_device *pdev)
> +{
> +	struct chromeos_acpi_attribute_group *aag;
> +	char name[ACPI_NAMESEG_SIZE + 1];
> +	union acpi_object *element, *obj;
> +	struct device *dev = &pdev->dev;
> +	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +	acpi_status status;
> +	int ret = 0;
> +	int size;
> +	int i;
> +
> +	status = acpi_evaluate_object(ACPI_COMPANION(&pdev->dev)->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);

Is truncation a real possibility? Shouldn't it abort/etc. in that case?
And `min()` "returns" a u32 here but `size` is an `int`.


> +			strscpy(name, element->string.pointer, size);
> +			ret = chromeos_acpi_add_method(pdev, name);
> +			if (ret) {
> +				chromeos_acpi_remove_groups();
> +				break;

Is just a `break` is enough here to handle the error? If this is not fatal,
then why is a `dev_warn()` not sufficient? If this is fatal, why continue
with the rest of the function?


> +			}
> +		}
> +	}
> +
> +	/* Add root attributes to the main group */
> +	ret = chromeos_acpi_add_attribs_to_group(chromeos_acpi.root,
> +						 chromeos_acpi.num_attrs);
> +	if (ret)
> +		goto free_acpi_buffer;
> +
> +	chromeos_acpi.dev_groups = kcalloc(chromeos_acpi.num_groups + 1,
> +					   sizeof(struct attribute_group),
> +					   GFP_KERNEL);
> +
> +	i = 0;
> +	list_for_each_entry(aag, &chromeos_acpi.groups, list) {
> +		chromeos_acpi.dev_groups[i] = &aag->group;
> +		i++;
> +	}
> +
> +	ret = sysfs_create_groups(&dev->kobj, chromeos_acpi.dev_groups);
> +	if (ret) {
> +		kfree(chromeos_acpi.dev_groups);
> +
> +		/* Remove allocated chromeos acpi groups and attributes */
> +		chromeos_acpi_remove_groups();
> +	}
> +
> +free_acpi_buffer:
> +	kfree(output.pointer);
> +	return ret;
> +}
> +
> +static int chromeos_acpi_device_probe(struct platform_device *pdev)
> +{
> +	struct chromeos_acpi_attribute_group *aag;
> +	struct device *dev = &pdev->dev;
> +	int i, ret;
> +
> +	aag = kzalloc(sizeof(*aag), GFP_KERNEL);
> +	if (!aag)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&aag->attribs);
> +	INIT_LIST_HEAD(&aag->list);
> +	INIT_LIST_HEAD(&chromeos_acpi.groups);
> +
> +	chromeos_acpi.root = aag;
> +
> +	/*
> +	 * Attempt to add methods by querying the device's MLST method
> +	 * for the list of methods.
> +	 */
> +	if (!chromeos_acpi_process_mlst(pdev))
> +		return 0;
> +
> +	dev_dbg(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(pdev,
> +					       chromeos_acpi_default_methods[i]);
> +		if (ret) {
> +			dev_err(dev, "failed to add default methods (%d)\n",
> +				ret);
> +			goto free_group_root;
> +		}
> +	}
> +
> +	return 0;
> +
> +free_group_root:
> +	kfree(chromeos_acpi.root);
> +	return ret;
> +}
> +
> +static int chromeos_acpi_device_remove(struct platform_device *pdev)
> +{
> +	/* Remove sysfs groups */
> +	sysfs_remove_groups(&pdev->dev.kobj, chromeos_acpi.dev_groups);
> +	kfree(chromeos_acpi.dev_groups);
> +
> +	/* Remove allocated chromeos acpi groups and attributes */
> +	chromeos_acpi_remove_groups();
> +
> +	return 0;
> +}
> +
> +/* GGL is valid PNP ID of Google. PNP ID can be used with the ACPI devices. */
> +static const struct acpi_device_id chromeos_device_ids[] = {
> +	{ "GGL0001", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, chromeos_device_ids);
> +
> +static struct platform_driver chromeos_acpi_device_driver = {
> +	.probe = chromeos_acpi_device_probe,
> +	.remove = chromeos_acpi_device_remove,
> +	.driver = {
> +		.name   = "chromeos-acpi",
> +		.acpi_match_table = ACPI_PTR(chromeos_device_ids)
> +	}
> +};
> +
> +module_platform_driver(chromeos_acpi_device_driver);
> +
> +MODULE_AUTHOR("Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("ChromeOS specific ACPI extensions");
> --
> 2.30.2
>
>

Excuse me if I have missed previous discussions about it, but I am confused by
the design. Why is a global variable needed here? The global struct's members
are overwritten in the probe method in any case.

And checkpatch reports that no MAINTAINERS entry has been added for the new file.
(And it appears to be right if I have not missed anything.)


Regards,
Barnabás Pőcze