Hi Greg, Many thanks for your comments, Missatge de Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> del dia dt., 4 de juny 2019 a les 17:53: > > On Tue, Jun 04, 2019 at 05:20:12PM +0200, Enric Balletbo i Serra wrote: > > That's a driver to talk with the ChromeOS Embedded Controller via a > > miscellaneous character device, it creates an entry in /dev for every > > instance and implements basic file operations for communicating with the > > Embedded Controller with an userspace application. The API is moved to > > the uapi folder, which is supposed to contain the user space API of the > > kernel. > > > > Note that this will replace current character device interface > > implemented in the cros-ec-dev driver in the MFD subsystem. The idea is > > to move all the functionality that extends the bounds of what MFD was > > designed to platform/chrome subsystem. > > > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx> > > --- > > > > Documentation/ioctl/ioctl-number.txt | 2 +- > > drivers/mfd/cros_ec_dev.c | 2 +- > > drivers/platform/chrome/Kconfig | 11 + > > drivers/platform/chrome/Makefile | 1 + > > drivers/platform/chrome/cros_ec_chardev.c | 279 ++++++++++++++++++ > > .../uapi/linux/cros_ec_chardev.h | 18 +- > > 6 files changed, 302 insertions(+), 11 deletions(-) > > create mode 100644 drivers/platform/chrome/cros_ec_chardev.c > > rename drivers/mfd/cros_ec_dev.h => include/uapi/linux/cros_ec_chardev.h (70%) > > > > diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt > > index c9558146ac58..8bd7907ee36d 100644 > > --- a/Documentation/ioctl/ioctl-number.txt > > +++ b/Documentation/ioctl/ioctl-number.txt > > @@ -340,7 +340,7 @@ Code Seq#(hex) Include File Comments > > 0xDD 00-3F ZFCP device driver see drivers/s390/scsi/ > > <mailto:aherrman@xxxxxxxxxx> > > 0xE5 00-3F linux/fuse.h > > -0xEC 00-01 drivers/platform/chrome/cros_ec_dev.h ChromeOS EC driver > > +0xEC 00-01 include/uapi/linux/cros_ec_chardev.h ChromeOS EC driver > > 0xF3 00-3F drivers/usb/misc/sisusbvga/sisusb.h sisfb (in development) > > <mailto:thomas@xxxxxxxxxxxxxxxx> > > 0xF4 00-1F video/mbxfb.h mbxfb > > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c > > index 607383b67cf1..11b791c28f84 100644 > > --- a/drivers/mfd/cros_ec_dev.c > > +++ b/drivers/mfd/cros_ec_dev.c > > @@ -15,7 +15,7 @@ > > #include <linux/slab.h> > > #include <linux/uaccess.h> > > > > -#include "cros_ec_dev.h" > > +#include <uapi/linux/cros_ec_chardev.h> > > > > #define DRV_NAME "cros-ec-dev" > > > > diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig > > index 9417b982ad92..3a9ad001838a 100644 > > --- a/drivers/platform/chrome/Kconfig > > +++ b/drivers/platform/chrome/Kconfig > > @@ -147,6 +147,17 @@ config CROS_KBD_LED_BACKLIGHT > > To compile this driver as a module, choose M here: the > > module will be called cros_kbd_led_backlight. > > > > +config CROS_EC_CHARDEV > > + tristate "ChromeOS EC miscdevice" > > + depends on MFD_CROS_EC_CHARDEV > > + default MFD_CROS_EC_CHARDEV > > + help > > + This driver adds file operations support to talk with the > > + ChromeOS EC from userspace via a character device. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called cros_ec_chardev. > > + > > config CROS_EC_LIGHTBAR > > tristate "Chromebook Pixel's lightbar support" > > depends on MFD_CROS_EC_CHARDEV > > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile > > index ebb57e21923b..d47a7e1097ee 100644 > > --- a/drivers/platform/chrome/Makefile > > +++ b/drivers/platform/chrome/Makefile > > @@ -16,6 +16,7 @@ cros_ec_lpcs-$(CONFIG_CROS_EC_LPC_MEC) += cros_ec_lpc_mec.o > > obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o > > obj-$(CONFIG_CROS_EC_PROTO) += cros_ec_proto.o cros_ec_trace.o > > obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o > > +obj-$(CONFIG_CROS_EC_CHARDEV) += cros_ec_chardev.o > > obj-$(CONFIG_CROS_EC_LIGHTBAR) += cros_ec_lightbar.o > > obj-$(CONFIG_CROS_EC_VBC) += cros_ec_vbc.o > > obj-$(CONFIG_CROS_EC_DEBUGFS) += cros_ec_debugfs.o > > diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c > > new file mode 100644 > > index 000000000000..1a0a27080026 > > --- /dev/null > > +++ b/drivers/platform/chrome/cros_ec_chardev.c > > @@ -0,0 +1,279 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Miscellaneous character driver for ChromeOS Embedded Controller > > + * > > + * Copyright 2019 Google LLC > > + */ > > + > > +#include <linux/init.h> > > +#include <linux/device.h> > > +#include <linux/fs.h> > > +#include <linux/list.h> > > +#include <linux/mfd/cros_ec.h> > > +#include <linux/mfd/cros_ec_commands.h> > > +#include <linux/miscdevice.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/slab.h> > > +#include <linux/spinlock.h> > > +#include <linux/types.h> > > +#include <linux/uaccess.h> > > + > > +#include <uapi/linux/cros_ec_chardev.h> > > + > > +#define DRV_NAME "cros-ec-chardev" > > + > > +static LIST_HEAD(chardev_devices); > > +static DEFINE_SPINLOCK(chardev_lock); > > + > > +struct chardev_data { > > + struct list_head list; > > + struct cros_ec_dev *ec_dev; > > + struct miscdevice misc; > > +}; > > + > > +static int ec_get_version(struct cros_ec_dev *ec, char *str, int maxlen) > > +{ > > + static const char * const current_image_name[] = { > > + "unknown", "read-only", "read-write", "invalid", > > + }; > > + struct ec_response_get_version *resp; > > + struct cros_ec_command *msg; > > + int ret; > > + > > + msg = kzalloc(sizeof(*msg) + sizeof(*resp), GFP_KERNEL); > > + if (!msg) > > + return -ENOMEM; > > + > > + msg->command = EC_CMD_GET_VERSION + ec->cmd_offset; > > + msg->insize = sizeof(*resp); > > + > > + ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg); > > + if (ret < 0) { > > + snprintf(str, maxlen, > > + "Unknown EC version, returned error: %d\n", > > + msg->result); > > + goto exit; > > + } > > + > > + resp = (struct ec_response_get_version *)msg->data; > > + if (resp->current_image >= ARRAY_SIZE(current_image_name)) > > + resp->current_image = 3; /* invalid */ > > + > > + snprintf(str, maxlen, "%s\n%s\n%s\n", > > + resp->version_string_ro, > > + resp->version_string_rw, > > + current_image_name[resp->current_image]); > > + > > + ret = 0; > > +exit: > > + kfree(msg); > > + return ret; > > +} > > + > > +/* > > + * Device file ops > > + */ > > +static int cros_ec_chardev_open(struct inode *inode, struct file *filp) > > +{ > > + struct miscdevice *mdev = filp->private_data; > > + struct cros_ec_dev *ec_dev = dev_get_drvdata(mdev->parent); > > + > > + filp->private_data = ec_dev; > > + nonseekable_open(inode, filp); > > + > > + return 0; > > +} > > + > > +static ssize_t cros_ec_chardev_read(struct file *filp, char __user *buffer, > > + size_t length, loff_t *offset) > > +{ > > + char msg[sizeof(struct ec_response_get_version) + > > + sizeof(CROS_EC_DEV_VERSION)]; > > + struct cros_ec_dev *ec = filp->private_data; > > + size_t count; > > + int ret; > > + > > + if (*offset != 0) > > + return 0; > > + > > + ret = ec_get_version(ec, msg, sizeof(msg)); > > + if (ret) > > + return ret; > > + > > + count = min(length, strlen(msg)); > > + > > + if (copy_to_user(buffer, msg, count)) > > + return -EFAULT; > > + > > + *offset = count; > > + return count; > > +} > > + > > +/* > > + * Ioctls > > + */ > > +static long cros_ec_chardev_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg) > > +{ > > + struct cros_ec_command *s_cmd; > > + struct cros_ec_command u_cmd; > > + long ret; > > + > > + if (copy_from_user(&u_cmd, arg, sizeof(u_cmd))) > > + return -EFAULT; > > + > > + if (u_cmd.outsize > EC_MAX_MSG_BYTES || > > + u_cmd.insize > EC_MAX_MSG_BYTES) > > + return -EINVAL; > > + > > + s_cmd = kmalloc(sizeof(*s_cmd) + max(u_cmd.outsize, u_cmd.insize), > > + GFP_KERNEL); > > + if (!s_cmd) > > + return -ENOMEM; > > + > > + if (copy_from_user(s_cmd, arg, sizeof(*s_cmd) + u_cmd.outsize)) { > > + ret = -EFAULT; > > + goto exit; > > + } > > + > > + if (u_cmd.outsize != s_cmd->outsize || > > + u_cmd.insize != s_cmd->insize) { > > + ret = -EINVAL; > > + goto exit; > > + } > > + > > + s_cmd->command += ec->cmd_offset; > > + ret = cros_ec_cmd_xfer(ec->ec_dev, s_cmd); > > + /* Only copy data to userland if data was received. */ > > + if (ret < 0) > > + goto exit; > > + > > + if (copy_to_user(arg, s_cmd, sizeof(*s_cmd) + s_cmd->insize)) > > + ret = -EFAULT; > > +exit: > > + kfree(s_cmd); > > + return ret; > > +} > > + > > +static long cros_ec_chardev_ioctl_readmem(struct cros_ec_dev *ec, > > + void __user *arg) > > +{ > > + struct cros_ec_device *ec_dev = ec->ec_dev; > > + struct cros_ec_readmem s_mem = { }; > > + long num; > > + > > + /* Not every platform supports direct reads */ > > + if (!ec_dev->cmd_readmem) > > + return -ENOTTY; > > + > > + if (copy_from_user(&s_mem, arg, sizeof(s_mem))) > > + return -EFAULT; > > + > > + num = ec_dev->cmd_readmem(ec_dev, s_mem.offset, s_mem.bytes, > > + s_mem.buffer); > > + if (num <= 0) > > + return num; > > + > > + if (copy_to_user((void __user *)arg, &s_mem, sizeof(s_mem))) > > + return -EFAULT; > > + > > + return num; > > +} > > + > > +static long cros_ec_chardev_ioctl(struct file *filp, unsigned int cmd, > > + unsigned long arg) > > +{ > > + struct cros_ec_dev *ec = filp->private_data; > > + > > + if (_IOC_TYPE(cmd) != CROS_EC_DEV_IOC) > > + return -ENOTTY; > > + > > + switch (cmd) { > > + case CROS_EC_DEV_IOCXCMD: > > + return cros_ec_chardev_ioctl_xcmd(ec, (void __user *)arg); > > + case CROS_EC_DEV_IOCRDMEM: > > + return cros_ec_chardev_ioctl_readmem(ec, (void __user *)arg); > > + } > > + > > + return -ENOTTY; > > +} > > + > > +static const struct file_operations chardev_fops = { > > + .open = cros_ec_chardev_open, > > + .read = cros_ec_chardev_read, > > + .unlocked_ioctl = cros_ec_chardev_ioctl, > > +#ifdef CONFIG_COMPAT > > + .compat_ioctl = cros_ec_chardev_ioctl, > > +#endif > > +}; > > + > > +static int cros_ec_chardev_probe(struct platform_device *pdev) > > +{ > > + struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent); > > + struct cros_ec_platform *ec_platform = dev_get_platdata(ec_dev->dev); > > + struct chardev_data *data; > > + int ret; > > + > > + /* Create a char device: we want to create it anew */ > > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + data->ec_dev = ec_dev; > > + data->misc.minor = MISC_DYNAMIC_MINOR; > > + data->misc.fops = &chardev_fops; > > + data->misc.name = ec_platform->ec_name; > > + data->misc.parent = pdev->dev.parent; > > + > > + ret = misc_register(&data->misc); > > + if (ret) > > + return ret; > > + > > + spin_lock(&chardev_lock); > > + list_add(&data->list, &chardev_devices); > > + spin_unlock(&chardev_lock); > > + > > + dev_info(&pdev->dev, "Created misc device /dev/%s\n", > > + data->misc.name); > > No need to be noisy, if all goes well, your code should be quiet. Ok, I'll remove on next version. > > + > > + return 0; > > +} > > + > > +static int cros_ec_chardev_remove(struct platform_device *pdev) > > +{ > > + struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent); > > + struct chardev_data *data; > > + > > + list_for_each_entry(data, &chardev_devices, list) > > + if (data->ec_dev == ec_dev) > > + break; > > + > > + if (data->ec_dev != ec_dev) { > > + dev_err(&pdev->dev, > > + "remove called but miscdevice %s not found\n", > > + data->misc.name); > > + return -ENODEV; > > + } > > Why do you have this separate list of devices? You don't seem to need > it, you only iterate over it, why is it needed? > Right, my bad, I was carrying some code from an old version that is not needed at all now. I'll remove all the related code. > > + spin_lock(&chardev_lock); > > + list_del(&data->list); > > + spin_unlock(&chardev_lock); > > + misc_deregister(&data->misc); > > + > > + return 0; > > +} > > You also iterate over the list without the lock, so why even have the > lock? Are you sure the list, and the lock, is even needed? > Right, not needed. Removed on next version. Thanks, Enric > thanks, > > greg k-h
