Re: [PATCH v5] platform/chrome: wilco_ec: Add telemetry char device interface

[Enric Balletbo i Serra]

Hi Nick,

I'm mostly fine with it but ...

On 21/5/19 21:20, Nick Crews wrote:
> The Wilco Embedded Controller is able to send telemetry data
> which is useful for enterprise applications. A daemon running on
> the OS sends a command to the EC via a write() to a char device,
> and can read the response with a read(). The write() request is
> verified by the driver to ensure that it is performing only one
> of the whitelisted commands, and that no extraneous data is
> being transmitted to the EC. The response is passed directly
> back to the reader with no modification.
> 
> The character device will appear as /dev/wilco_telemN, where N
> is some small non-negative integer, starting with 0. Only one

Still remains my question in the previous version.

We will really have more than one /dev/wilco_telemN devices handled by this
driver? Why not use a Miscellaneous Character Device Driver that will simplify
the code?

Thanks,
 Enric


> process may have the file descriptor open at a time. The calling
> userspace program needs to keep the device file descriptor open
> between the calls to write() and read() in order to preserve the
> response. Up to 32 bytes will be available for reading.
> 
> For testing purposes, try requesting the EC's firmware build
> date, by sending the WILCO_EC_TELEM_GET_VERSION command with
> argument index=3. i.e. write [0x38, 0x00, 0x03]
> to the device node. An ASCII string of the build date is
> returned.
> 
> Signed-off-by: Nick Crews <ncrews@xxxxxxxxxxxx>
> ---
> v5 changes:
> - Free device data in callback so that it isn't freed while
>   the character device is open.
> - Remove under-used dev_num variable in probe()
> - Change pr_warn to pr_err in module_init()
> - Allow for telemetry requests smaller than 32 bytes,
>   and improve the validation on the requests.
> - Return -EMSGSIZE instead of -EINVAL in for too-long requests.
> - Zero out response buffer before making a request, so the
>   response is more clearly padded.
> v3 changes:
> - Change WILCO_EC_CMD_* commands to WILCO_EC_TELEM_* in
>   order to differentiate from the 0xF0 commannds.
> - Use kernel-doc style for wilco_ec_telem_request.
> - Change "GPL v2" to "GPL" in MODULE_LICENSE.
> - Fix formatting in check_args_length().
> v2 changes:
> - Add verification of userspace requests, so that only
>   whitelisted commands and args can get sent to the EC
> - Use EC firmware build date request as example/test
> - Pass the wilco_ec_device to the child driver better,
>   instead of the child driver needing to access the parent
>   devices' data.
> 
>  drivers/platform/chrome/wilco_ec/Kconfig     |   7 +
>  drivers/platform/chrome/wilco_ec/Makefile    |   2 +
>  drivers/platform/chrome/wilco_ec/core.c      |  13 +
>  drivers/platform/chrome/wilco_ec/debugfs.c   |   2 +-
>  drivers/platform/chrome/wilco_ec/telemetry.c | 450 +++++++++++++++++++
>  include/linux/platform_data/wilco-ec.h       |   2 +
>  6 files changed, 475 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/platform/chrome/wilco_ec/telemetry.c
> 
> diff --git a/drivers/platform/chrome/wilco_ec/Kconfig b/drivers/platform/chrome/wilco_ec/Kconfig
> index e09e4cebe9b4..2fc03aa624cf 100644
> --- a/drivers/platform/chrome/wilco_ec/Kconfig
> +++ b/drivers/platform/chrome/wilco_ec/Kconfig
> @@ -18,3 +18,10 @@ config WILCO_EC_DEBUGFS
>  	  manipulation and allow for testing arbitrary commands.  This
>  	  interface is intended for debug only and will not be present
>  	  on production devices.
> +
> +config WILCO_EC_TELEMETRY
> +	tristate "Enable querying telemetry data from EC"
> +	depends on WILCO_EC
> +	help
> +	  If you say Y here, you get support to query EC telemetry data from
> +	  /dev/wilco_telem0 using write() and then read().
> diff --git a/drivers/platform/chrome/wilco_ec/Makefile b/drivers/platform/chrome/wilco_ec/Makefile
> index 72df9b5e1983..72a99f854241 100644
> --- a/drivers/platform/chrome/wilco_ec/Makefile
> +++ b/drivers/platform/chrome/wilco_ec/Makefile
> @@ -4,3 +4,5 @@ wilco_ec-objs				:= core.o mailbox.o properties.o sysfs.o
>  obj-$(CONFIG_WILCO_EC)			+= wilco_ec.o
>  wilco_ec_debugfs-objs			:= debugfs.o
>  obj-$(CONFIG_WILCO_EC_DEBUGFS)		+= wilco_ec_debugfs.o
> +wilco_ec_telem-objs			:= telemetry.o
> +obj-$(CONFIG_WILCO_EC_TELEMETRY)	+= wilco_ec_telem.o
> diff --git a/drivers/platform/chrome/wilco_ec/core.c b/drivers/platform/chrome/wilco_ec/core.c
> index 45cf3a5ed062..3724bf4b77c6 100644
> --- a/drivers/platform/chrome/wilco_ec/core.c
> +++ b/drivers/platform/chrome/wilco_ec/core.c
> @@ -93,8 +93,20 @@ static int wilco_ec_probe(struct platform_device *pdev)
>  		goto unregister_rtc;
>  	}
>  
> +	/* Register child device that will be found by the telemetry driver. */
> +	ec->telem_pdev = platform_device_register_data(dev, "wilco_telem",
> +						       PLATFORM_DEVID_AUTO,
> +						       ec, sizeof(*ec));
> +	if (IS_ERR(ec->telem_pdev)) {
> +		dev_err(dev, "Failed to create telemetry platform device\n");
> +		ret = PTR_ERR(ec->telem_pdev);
> +		goto remove_sysfs;
> +	}
> +
>  	return 0;
>  
> +remove_sysfs:
> +	wilco_ec_remove_sysfs(ec);
>  unregister_rtc:
>  	platform_device_unregister(ec->rtc_pdev);
>  unregister_debugfs:
> @@ -109,6 +121,7 @@ static int wilco_ec_remove(struct platform_device *pdev)
>  	struct wilco_ec_device *ec = platform_get_drvdata(pdev);
>  
>  	wilco_ec_remove_sysfs(ec);
> +	platform_device_unregister(ec->telem_pdev);
>  	platform_device_unregister(ec->rtc_pdev);
>  	if (ec->debugfs_pdev)
>  		platform_device_unregister(ec->debugfs_pdev);
> diff --git a/drivers/platform/chrome/wilco_ec/debugfs.c b/drivers/platform/chrome/wilco_ec/debugfs.c
> index 281ec595e8e0..8d65a1e2f1a3 100644
> --- a/drivers/platform/chrome/wilco_ec/debugfs.c
> +++ b/drivers/platform/chrome/wilco_ec/debugfs.c
> @@ -16,7 +16,7 @@
>  
>  #define DRV_NAME "wilco-ec-debugfs"
>  
> -/* The 256 raw bytes will take up more space when represented as a hex string */
> +/* The raw bytes will take up more space when represented as a hex string */
>  #define FORMATTED_BUFFER_SIZE (EC_MAILBOX_DATA_SIZE * 4)
>  
>  struct wilco_ec_debugfs {
> diff --git a/drivers/platform/chrome/wilco_ec/telemetry.c b/drivers/platform/chrome/wilco_ec/telemetry.c
> new file mode 100644
> index 000000000000..94cdc166c840
> --- /dev/null
> +++ b/drivers/platform/chrome/wilco_ec/telemetry.c
> @@ -0,0 +1,450 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Telemetry communication for Wilco EC
> + *
> + * Copyright 2019 Google LLC
> + *
> + * The Wilco Embedded Controller is able to send telemetry data
> + * which is useful for enterprise applications. A daemon running on
> + * the OS sends a command to the EC via a write() to a char device,
> + * and can read the response with a read(). The write() request is
> + * verified by the driver to ensure that it is performing only one
> + * of the whitelisted commands, and that no extraneous data is
> + * being transmitted to the EC. The response is passed directly
> + * back to the reader with no modification.
> + *
> + * The character device will appear as /dev/wilco_telemN, where N
> + * is some small non-negative integer, starting with 0. Only one
> + * process may have the file descriptor open at a time. The calling
> + * userspace program needs to keep the device file descriptor open
> + * between the calls to write() and read() in order to preserve the
> + * response. Up to 32 bytes will be available for reading.
> + *
> + * For testing purposes, try requesting the EC's firmware build
> + * date, by sending the WILCO_EC_TELEM_GET_VERSION command with
> + * argument index=3. i.e. write [0x38, 0x00, 0x03]
> + * to the device node. An ASCII string of the build date is
> + * returned.
> + */
> +
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/wilco-ec.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +
> +#define TELEM_DEV_NAME		"wilco_telem"
> +#define TELEM_CLASS_NAME	TELEM_DEV_NAME
> +#define DRV_NAME		TELEM_DEV_NAME
> +#define TELEM_DEV_NAME_FMT	(TELEM_DEV_NAME "%d")
> +static struct class telem_class = {
> +	.owner	= THIS_MODULE,
> +	.name	= TELEM_CLASS_NAME,
> +};
> +
> +/* Keep track of all the device numbers used. */
> +#define TELEM_MAX_DEV 128
> +static int telem_major;
> +static DEFINE_IDA(telem_ida);
> +
> +/* EC telemetry command codes */
> +#define WILCO_EC_TELEM_GET_LOG			0x99
> +#define WILCO_EC_TELEM_GET_VERSION		0x38
> +#define WILCO_EC_TELEM_GET_FAN_INFO		0x2E
> +#define WILCO_EC_TELEM_GET_DIAG_INFO		0xFA
> +#define WILCO_EC_TELEM_GET_TEMP_INFO		0x95
> +#define WILCO_EC_TELEM_GET_TEMP_READ		0x2C
> +#define WILCO_EC_TELEM_GET_BATT_EXT_INFO	0x07
> +
> +#define TELEM_ARGS_SIZE_MAX	30
> +
> +/**
> + * struct wilco_ec_telem_request - Telemetry command and arguments sent to EC.
> + * @command: One of WILCO_EC_TELEM_GET_* command codes.
> + * @reserved: Must be 0.
> + * @args: The first N bytes are one of telem_args_get_* structs, the rest is 0.
> + */
> +struct wilco_ec_telem_request {
> +	u8 command;
> +	u8 reserved;
> +	u8 args[TELEM_ARGS_SIZE_MAX];
> +} __packed;
> +
> +/*
> + * The following telem_args_get_* structs are embedded within the |args| field
> + * of wilco_ec_telem_request.
> + */
> +
> +struct telem_args_get_log {
> +	u8 log_type;
> +	u8 log_index;
> +} __packed;
> +
> +/*
> + * Get a piece of info about the EC firmware version:
> + * 0 = label
> + * 1 = svn_rev
> + * 2 = model_no
> + * 3 = build_date
> + * 4 = frio_version
> + */
> +struct telem_args_get_version {
> +	u8 index;
> +} __packed;
> +
> +struct telem_args_get_fan_info {
> +	u8 command;
> +	u8 fan_number;
> +	u8 arg;
> +} __packed;
> +
> +struct telem_args_get_diag_info {
> +	u8 type;
> +	u8 sub_type;
> +} __packed;
> +
> +struct telem_args_get_temp_info {
> +	u8 command;
> +	u8 index;
> +	u8 field;
> +	u8 zone;
> +} __packed;
> +
> +struct telem_args_get_temp_read {
> +	u8 sensor_index;
> +} __packed;
> +
> +struct telem_args_get_batt_ext_info {
> +	u8 var_args[5];
> +} __packed;
> +
> +/**
> + * check_telem_request() - Ensure that a request from userspace is valid.
> + * @rq: Request buffer copied from userspace.
> + * @size: Number of bytes copied from userspace.
> + *
> + * Return: 0 if valid, -EINVAL if bad command or reserved byte is non-zero,
> + *         -EMSGSIZE if the request is too long.
> + *
> + * We do not want to allow userspace to send arbitrary telemetry commands to
> + * the EC. Therefore we check to ensure that
> + * 1. The request follows the format of struct wilco_ec_telem_request.
> + * 2. The supplied command code is one of the whitelisted commands.
> + * 3. The request only contains the necessary data for the header and arguments.
> + */
> +static int check_telem_request(struct wilco_ec_telem_request *rq,
> +			       size_t size)
> +{
> +	size_t max_size = offsetof(struct wilco_ec_telem_request, args);
> +
> +	if (rq->reserved)
> +		return -EINVAL;
> +
> +	switch (rq->command) {
> +	case WILCO_EC_TELEM_GET_LOG:
> +		max_size += sizeof(struct telem_args_get_log);
> +		break;
> +	case WILCO_EC_TELEM_GET_VERSION:
> +		max_size += sizeof(struct telem_args_get_version);
> +		break;
> +	case WILCO_EC_TELEM_GET_FAN_INFO:
> +		max_size += sizeof(struct telem_args_get_fan_info);
> +		break;
> +	case WILCO_EC_TELEM_GET_DIAG_INFO:
> +		max_size += sizeof(struct telem_args_get_diag_info);
> +		break;
> +	case WILCO_EC_TELEM_GET_TEMP_INFO:
> +		max_size += sizeof(struct telem_args_get_temp_info);
> +		break;
> +	case WILCO_EC_TELEM_GET_TEMP_READ:
> +		max_size += sizeof(struct telem_args_get_temp_read);
> +		break;
> +	case WILCO_EC_TELEM_GET_BATT_EXT_INFO:
> +		max_size += sizeof(struct telem_args_get_batt_ext_info);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return (size <= max_size) ? 0 : -EMSGSIZE;
> +}
> +
> +/**
> + * struct telem_device_data - Data for a Wilco EC device that queries telemetry.
> + * @cdev: Char dev that userspace reads and polls from.
> + * @dev: Device associated with the %cdev.
> + * @ec: Wilco EC that we will be communicating with using the mailbox interface.
> + * @available: Boolean of if the device can be opened.
> + */
> +struct telem_device_data {
> +	struct device dev;
> +	struct cdev cdev;
> +	struct wilco_ec_device *ec;
> +	atomic_t available;
> +};
> +
> +#define TELEM_RESPONSE_SIZE	EC_MAILBOX_DATA_SIZE
> +
> +/**
> + * struct telem_session_data - Data that exists between open() and release().
> + * @dev_data: Pointer to get back to the device data and EC.
> + * @request: Command and arguments sent to EC.
> + * @response: Response buffer of data from EC.
> + * @has_msg: Is there data available to read from a previous write?
> + */
> +struct telem_session_data {
> +	struct telem_device_data *dev_data;
> +	struct wilco_ec_telem_request request;
> +	u8 response[TELEM_RESPONSE_SIZE];
> +	bool has_msg;
> +};
> +
> +/**
> + * telem_open() - Callback for when the device node is opened.
> + * @inode: inode for this char device node.
> + * @filp: file for this char device node.
> + *
> + * We need to ensure that after writing a command to the device,
> + * the same userspace process reads the corresponding result.
> + * Therefore, we increment a refcount on opening the device, so that
> + * only one process can communicate with the EC at a time.
> + *
> + * Return: 0 on success, or negative error code on failure.
> + */
> +static int telem_open(struct inode *inode, struct file *filp)
> +{
> +	struct telem_device_data *dev_data;
> +	struct telem_session_data *sess_data;
> +
> +	/* Ensure device isn't already open */
> +	dev_data = container_of(inode->i_cdev, struct telem_device_data, cdev);
> +	if (atomic_cmpxchg(&dev_data->available, 1, 0) == 0)
> +		return -EBUSY;
> +
> +	get_device(&dev_data->dev);
> +
> +	sess_data = kzalloc(sizeof(*sess_data), GFP_KERNEL);
> +	if (!sess_data) {
> +		atomic_set(&dev_data->available, 1);
> +		return -ENOMEM;
> +	}
> +	sess_data->dev_data = dev_data;
> +	sess_data->has_msg = false;
> +
> +	nonseekable_open(inode, filp);
> +	filp->private_data = sess_data;
> +
> +	return 0;
> +}
> +
> +static ssize_t telem_write(struct file *filp, const char __user *buf,
> +			   size_t count, loff_t *pos)
> +{
> +	struct telem_session_data *sess_data = filp->private_data;
> +	struct wilco_ec_message msg = {};
> +	int ret;
> +
> +	if (count > sizeof(sess_data->request))
> +		return -EMSGSIZE;
> +	if (copy_from_user(&sess_data->request, buf, count))
> +		return -EFAULT;
> +	ret = check_telem_request(&sess_data->request, count);
> +	if (ret < 0)
> +		return ret;
> +
> +	memset(sess_data->response, 0, sizeof(sess_data->response));
> +	msg.type = WILCO_EC_MSG_TELEMETRY;
> +	msg.request_data = &sess_data->request;
> +	msg.request_size = sizeof(sess_data->request);
> +	msg.response_data = sess_data->response;
> +	msg.response_size = sizeof(sess_data->response);
> +
> +	ret = wilco_ec_mailbox(sess_data->dev_data->ec, &msg);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(sess_data->response))
> +		return -EMSGSIZE;
> +
> +	sess_data->has_msg = true;
> +
> +	return count;
> +}
> +
> +static ssize_t telem_read(struct file *filp, char __user *buf, size_t count,
> +			  loff_t *pos)
> +{
> +	struct telem_session_data *sess_data = filp->private_data;
> +
> +	if (!sess_data->has_msg)
> +		return -ENODATA;
> +	if (count > sizeof(sess_data->response))
> +		return -EINVAL;
> +
> +	if (copy_to_user(buf, sess_data->response, count))
> +		return -EFAULT;
> +
> +	sess_data->has_msg = false;
> +
> +	return count;
> +}
> +
> +static int telem_release(struct inode *inode, struct file *filp)
> +{
> +	struct telem_session_data *sess_data = filp->private_data;
> +
> +	atomic_set(&sess_data->dev_data->available, 1);
> +	put_device(&sess_data->dev_data->dev);
> +	kfree(sess_data);
> +
> +	return 0;
> +}
> +
> +static const struct file_operations telem_fops = {
> +	.open = telem_open,
> +	.write = telem_write,
> +	.read = telem_read,
> +	.release = telem_release,
> +	.llseek = no_llseek,
> +	.owner = THIS_MODULE,
> +};
> +
> +/**
> + * telem_device_free() - Callback to free the telem_device_data structure.
> + * @d: The device embedded in our device data, which we have been ref counting.
> + *
> + * Once all open file descriptors are closed and the device has been removed,
> + * the refcount of the device will fall to 0 and this will be called.
> + */
> +static void telem_device_free(struct device *d)
> +{
> +	struct telem_device_data *dev_data;
> +
> +	dev_data = container_of(d, struct telem_device_data, dev);
> +	kfree(dev_data);
> +}
> +
> +/**
> + * telem_device_probe() - Callback when creating a new device.
> + * @pdev: platform device that we will be receiving telems from.
> + *
> + * This finds a free minor number for the device, allocates and initializes
> + * some device data, and creates a new device and char dev node.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +static int telem_device_probe(struct platform_device *pdev)
> +{
> +	struct telem_device_data *dev_data;
> +	int error, minor;
> +
> +	/* Get the next available device number */
> +	minor = ida_alloc_max(&telem_ida, TELEM_MAX_DEV-1, GFP_KERNEL);
> +	if (minor < 0) {
> +		error = minor;
> +		dev_err(&pdev->dev, "Failed to find minor number: %d", error);
> +		return error;
> +	}
> +
> +	dev_data = kzalloc(sizeof(*dev_data), GFP_KERNEL);
> +	if (!dev_data) {
> +		ida_simple_remove(&telem_ida, minor);
> +		return -ENOMEM;
> +	}
> +
> +	/* Initialize the device data */
> +	dev_data->ec = dev_get_platdata(&pdev->dev);
> +	atomic_set(&dev_data->available, 1);
> +	platform_set_drvdata(pdev, dev_data);
> +
> +	/* Initialize the device */
> +	dev_data->dev.devt = MKDEV(telem_major, minor);
> +	dev_data->dev.class = &telem_class;
> +	dev_data->dev.release = telem_device_free;
> +	dev_set_name(&dev_data->dev, TELEM_DEV_NAME_FMT, minor);
> +	device_initialize(&dev_data->dev);
> +
> +	/* Initialize the character device and add it to userspace */;
> +	cdev_init(&dev_data->cdev, &telem_fops);
> +	error = cdev_device_add(&dev_data->cdev, &dev_data->dev);
> +	if (error) {
> +		put_device(&dev_data->dev);
> +		ida_simple_remove(&telem_ida, minor);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static int telem_device_remove(struct platform_device *pdev)
> +{
> +	struct telem_device_data *dev_data = platform_get_drvdata(pdev);
> +
> +	cdev_device_del(&dev_data->cdev, &dev_data->dev);
> +	put_device(&dev_data->dev);
> +	ida_simple_remove(&telem_ida, MINOR(dev_data->dev.devt));
> +
> +	return 0;
> +}
> +
> +static struct platform_driver telem_driver = {
> +	.probe = telem_device_probe,
> +	.remove = telem_device_remove,
> +	.driver = {
> +		.name = DRV_NAME,
> +	},
> +};
> +
> +static int __init telem_module_init(void)
> +{
> +	dev_t dev_num = 0;
> +	int ret;
> +
> +	ret = class_register(&telem_class);
> +	if (ret) {
> +		pr_err(DRV_NAME ": Failed registering class: %d", ret);
> +		return ret;
> +	}
> +
> +	/* Request the kernel for device numbers, starting with minor=0 */
> +	ret = alloc_chrdev_region(&dev_num, 0, TELEM_MAX_DEV, TELEM_DEV_NAME);
> +	if (ret) {
> +		pr_err(DRV_NAME ": Failed allocating dev numbers: %d", ret);
> +		goto destroy_class;
> +	}
> +	telem_major = MAJOR(dev_num);
> +
> +	ret = platform_driver_register(&telem_driver);
> +	if (ret < 0) {
> +		pr_err(DRV_NAME ": Failed registering driver: %d\n", ret);
> +		goto unregister_region;
> +	}
> +
> +	return 0;
> +
> +unregister_region:
> +	unregister_chrdev_region(MKDEV(telem_major, 0), TELEM_MAX_DEV);
> +destroy_class:
> +	class_unregister(&telem_class);
> +	ida_destroy(&telem_ida);
> +	return ret;
> +}
> +
> +static void __exit telem_module_exit(void)
> +{
> +	platform_driver_unregister(&telem_driver);
> +	unregister_chrdev_region(MKDEV(telem_major, 0), TELEM_MAX_DEV);
> +	class_unregister(&telem_class);
> +	ida_destroy(&telem_ida);
> +}
> +
> +module_init(telem_module_init);
> +module_exit(telem_module_exit);
> +
> +MODULE_AUTHOR("Nick Crews <ncrews@xxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Wilco EC telemetry driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);
> diff --git a/include/linux/platform_data/wilco-ec.h b/include/linux/platform_data/wilco-ec.h
> index e3ce9ce49b11..ad03b586a095 100644
> --- a/include/linux/platform_data/wilco-ec.h
> +++ b/include/linux/platform_data/wilco-ec.h
> @@ -29,6 +29,7 @@
>   * @data_size: Size of the data buffer used for EC communication.
>   * @debugfs_pdev: The child platform_device used by the debugfs sub-driver.
>   * @rtc_pdev: The child platform_device used by the RTC sub-driver.
> + * @telem_pdev: The child platform_device used by the telemetry sub-driver.
>   */
>  struct wilco_ec_device {
>  	struct device *dev;
> @@ -40,6 +41,7 @@ struct wilco_ec_device {
>  	size_t data_size;
>  	struct platform_device *debugfs_pdev;
>  	struct platform_device *rtc_pdev;
> +	struct platform_device *telem_pdev;
>  };
>  
>  /**
>