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

[Enric Balletbo i Serra <enric.balletbo@collabora.com>]

Hi Nick,

Many thanks for the patch, some few comments below.

On 8/5/19 23:38, 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
> 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. 32 bytes of data are expected for arguments, and 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, ...(29 more 0s)]
> to the device node. An ASCII string of the build date is
> returned.
> 
> Signed-off-by: Nick Crews <ncrews@chromium.org>
> ---
> 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 | 439 +++++++++++++++++++
>  include/linux/platform_data/wilco-ec.h       |   2 +
>  6 files changed, 464 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 29b734137786..5a244da3b6c3 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
>  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 d060d3aa5bae..4cb05d80e5af 100644
> --- a/drivers/platform/chrome/wilco_ec/core.c
> +++ b/drivers/platform/chrome/wilco_ec/core.c
> @@ -87,8 +87,20 @@ static int wilco_ec_probe(struct platform_device *pdev)
>  		goto unregister_debugfs;
>  	}
>  
> +	/* 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 unregister_rtc;
> +	}
> +
>  	return 0;
>  
> +unregister_rtc:
> +	platform_device_unregister(ec->rtc_pdev);
>  unregister_debugfs:
>  	if (ec->debugfs_pdev)
>  		platform_device_unregister(ec->debugfs_pdev);
> @@ -100,6 +112,7 @@ static int wilco_ec_remove(struct platform_device *pdev)
>  {
>  	struct wilco_ec_device *ec = platform_get_drvdata(pdev);
>  
> +	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..f9f9e11fea71
> --- /dev/null
> +++ b/drivers/platform/chrome/wilco_ec/telemetry.c
> @@ -0,0 +1,439 @@
> +// 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. 32 bytes of data are expected for arguments, and 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, ...(29 more 0s)]
> + * 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

Do you expect to have more than one telemetry character device handled by this
driver?

If the answer is no, I think that the driver could be simplified by using a
miscellaneous character device (drivers/char/misc.c) instead.

> +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;
> +} __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;
> +
> +static const char TELEM_ARGS_ZERO[TELEM_ARGS_SIZE_MAX] = {0};
> +
> +/**
> + * check_args_length() - Ensure that un-needed argument bytes are 0.
> + * @rq:	Request to be validated.
> + * @arg_size: Number of bytes of rq->args that are allowed to be non-zero.
> + */
> +static int check_args_length(struct wilco_ec_telem_request *rq, size_t arg_size)
> +{
> +	if (memcmp(rq->args + arg_size, TELEM_ARGS_ZERO,
> +		   TELEM_ARGS_SIZE_MAX - arg_size) != 0)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/*
> + * 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 arguments only contain the necessary data. Each command takes a
> + *    different format of arguments, and only these arguments can be non-zero.
> + *    For instance, if the request uses command WILCO_EC_TELEM_GET_LOG, then
> + *    the command uses the argument format of telem_args_get_log, which
> + *    contains 2 bytes of arguments. Thus, everything in the args besides
> + *    these two bytes had better be zero.
> + */
> +static int check_telem_request(struct wilco_ec_telem_request *rq)
> +{
> +	if (rq->reserved)
> +		return -EINVAL;
> +
> +	switch (rq->command) {
> +	case WILCO_EC_TELEM_GET_LOG:
> +		return check_args_length(rq,
> +				sizeof(struct telem_args_get_log));
> +	case WILCO_EC_TELEM_GET_VERSION:
> +		return check_args_length(rq,
> +				sizeof(struct telem_args_get_version));
> +	case WILCO_EC_TELEM_GET_FAN_INFO:
> +		return check_args_length(rq,
> +				sizeof(struct telem_args_get_fan_info));
> +	case WILCO_EC_TELEM_GET_DIAG_INFO:
> +		return check_args_length(rq,
> +				sizeof(struct telem_args_get_diag_info));
> +	case WILCO_EC_TELEM_GET_TEMP_INFO:
> +		return check_args_length(rq,
> +				sizeof(struct telem_args_get_temp_info));
> +	case WILCO_EC_TELEM_GET_TEMP_READ:
> +		return check_args_length(rq,
> +				sizeof(struct telem_args_get_temp_read));
> +	case WILCO_EC_TELEM_GET_BATT_EXT_INFO:
> +		return check_args_length(rq,
> +				sizeof(struct telem_args_get_batt_ext_info));
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +/**
> + * 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;
> +
> +	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 -EINVAL;
> +	if (copy_from_user(&sess_data->request, buf, count))
> +		return -EFAULT;
> +	ret = check_telem_request(&sess_data->request);
> +	if (ret < 0)
> +		return ret;
> +
> +	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);
> +	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_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;
> +	dev_t dev_num;
> +	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 = devm_kzalloc(&pdev->dev, sizeof(*dev_data), GFP_KERNEL);
> +	if (!dev_data) {
> +		ida_simple_remove(&telem_ida, minor);
> +		return -ENOMEM;
> +	}

You can move that to the top and you will get rid of the ida_simple_remove call.

I'm not sure is right use devm_kzalloc here. I think that you should allocate
the data here via kzalloc and free in class release callback. See
Documentation/kobject.txt

> +
> +	/* 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_num = MKDEV(telem_major, minor);
> +	dev_data->dev.devt = dev_num;

dev_num is only used here, you can remove the local variable and do

dev_data.devt = MKDEV(telem_major, minor);

> +	dev_data->dev.class = &telem_class;
> +	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) {

I think that you should call

	put_device(&dev_data->dev);

to release the reference from device_initialize()?


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

Same here, 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_warn(DRV_NAME ": Failed registering class: %d", ret);

pr_err?

> +		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_warn(DRV_NAME ": Failed allocating dev numbers: %d", ret);

pr_err?

> +		goto destroy_class;
> +	}
> +	telem_major = MAJOR(dev_num);
> +
> +	ret = platform_driver_register(&telem_driver);
> +	if (ret < 0) {
> +		pr_warn(DRV_NAME ": Failed registering driver: %d\n", ret);

pr_err?

> +		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@chromium.org>");
> +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 33f137078737..67cceb7ba577 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;
>  };
>  
>  /**
>