[PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Enric Balletbo i Serra]

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@collabora.com>
---

 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@de.ibm.com>
 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@winischhofer.net>
 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);
+
+	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;
+	}
+
+	spin_lock(&chardev_lock);
+	list_del(&data->list);
+	spin_unlock(&chardev_lock);
+	misc_deregister(&data->misc);
+
+	return 0;
+}
+
+static struct platform_driver cros_ec_chardev_driver = {
+	.driver = {
+		.name = DRV_NAME,
+	},
+	.probe = cros_ec_chardev_probe,
+	.remove = cros_ec_chardev_remove,
+};
+
+module_platform_driver(cros_ec_chardev_driver);
+
+MODULE_ALIAS("platform:" DRV_NAME);
+MODULE_AUTHOR("Enric Balletbo i Serra <enric.balletbo@collabora.com>");
+MODULE_DESCRIPTION("ChromeOS EC Miscellaneous Character Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/cros_ec_dev.h b/include/uapi/linux/cros_ec_chardev.h
similarity index 70%
rename from drivers/mfd/cros_ec_dev.h
rename to include/uapi/linux/cros_ec_chardev.h
index 7a42c3ef50e4..c6dd2549a2a5 100644
--- a/drivers/mfd/cros_ec_dev.h
+++ b/include/uapi/linux/cros_ec_chardev.h
@@ -1,12 +1,12 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
 /*
- * cros_ec_dev - expose the Chrome OS Embedded Controller to userspace
+ * ChromeOS EC device interface.
  *
  * Copyright (C) 2014 Google, Inc.
  */
 
-#ifndef _CROS_EC_DEV_H_
-#define _CROS_EC_DEV_H_
+#ifndef _UAPI_LINUX_CROS_EC_DEV_H_
+#define _UAPI_LINUX_CROS_EC_DEV_H_
 
 #include <linux/ioctl.h>
 #include <linux/types.h>
@@ -20,16 +20,16 @@
  * @bytes: Number of bytes to read. Zero means "read a string" (including '\0')
  *         At most only EC_MEMMAP_SIZE bytes can be read.
  * @buffer: Where to store the result. The ioctl returns the number of bytes
- *         read or negative on error.
+ *          read or negative on error.
  */
 struct cros_ec_readmem {
-	uint32_t offset;
-	uint32_t bytes;
-	uint8_t buffer[EC_MEMMAP_SIZE];
+	__u32	offset;
+	__u32	bytes;
+	__u8	buffer[EC_MEMMAP_SIZE];
 };
 
 #define CROS_EC_DEV_IOC       0xEC
 #define CROS_EC_DEV_IOCXCMD   _IOWR(CROS_EC_DEV_IOC, 0, struct cros_ec_command)
 #define CROS_EC_DEV_IOCRDMEM  _IOWR(CROS_EC_DEV_IOC, 1, struct cros_ec_readmem)
 
-#endif /* _CROS_EC_DEV_H_ */
+#endif /* _UAPI_LINUX_CROS_EC_DEV_H_ */
-- 
2.20.1

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Greg Kroah-Hartman]

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@collabora.com>
> ---
> 
>  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@de.ibm.com>
>  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@winischhofer.net>
>  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.

> +
> +	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?

> +	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?

thanks,

greg k-h

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Ezequiel Garcia]

Hey Greg,

> > +	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.
> 

I sometimes wonder about this being noise or not, so I will slightly
hijack this thread for this discussion.

From a kernel developer point-of-view, or even from a platform
developer or user with a debugging hat point-of-view, having
a "device created" or "device registered" message is often very useful.

In fact, I wish people would do this more often, so I don't have to
deal with dynamic debug, or hack my way:

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 4589631798c9..473549b26bb2 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -603,7 +603,7 @@ static int ov5647_probe(struct i2c_client *client,
        if (ret < 0)
                goto error;
 
-       dev_dbg(dev, "OmniVision OV5647 camera driver probed\n");
+       dev_info(dev, "OmniVision OV5647 camera driver probed\n");
        return 0;
 error:
        media_entity_cleanup(&sd->entity);

In some subsystems, it's even a behavior I'm more or less relying on:

$ git grep v4l2_info.*registered drivers/media/ | wc -l
26

And on the downsides, I can't find much. It's just one little line,
that is not even noticed unless you have logging turned on.

Thanks,
Ezequiel

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Greg Kroah-Hartman]

On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> Hey Greg,
> 
> > > +	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.
> > 
> 
> I sometimes wonder about this being noise or not, so I will slightly
> hijack this thread for this discussion.
> 
> >From a kernel developer point-of-view, or even from a platform
> developer or user with a debugging hat point-of-view, having
> a "device created" or "device registered" message is often very useful.

For you, yes.  For someone with 30000 devices attached to their system,
it is not, and causes booting to take longer than it should be.

> In fact, I wish people would do this more often, so I don't have to
> deal with dynamic debug, or hack my way:
> 
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 4589631798c9..473549b26bb2 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -603,7 +603,7 @@ static int ov5647_probe(struct i2c_client *client,
>         if (ret < 0)
>                 goto error;
>  
> -       dev_dbg(dev, "OmniVision OV5647 camera driver probed\n");
> +       dev_info(dev, "OmniVision OV5647 camera driver probed\n");
>         return 0;
>  error:
>         media_entity_cleanup(&sd->entity);
> 
> In some subsystems, it's even a behavior I'm more or less relying on:
> 
> $ git grep v4l2_info.*registered drivers/media/ | wc -l
> 26
> 
> And on the downsides, I can't find much. It's just one little line,
> that is not even noticed unless you have logging turned on.

Its better to be quiet, which is why the "default driver registration"
macros do not have any printk messages in them.  When converting drivers
over to it, we made the boot process much more sane, don't try to go and
add messages for no good reason back in please.

dynamic debugging can be enabled on a module and line-by-line basis,
even from the boot command line.  So if you need debugging, you can
always ask someone to just reboot or unload/load the module and get the
message that way.

thanks,

greg k-h

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Guenter Roeck]

On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> > Hey Greg,
> >
> > > > + 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.
> > >
> >
> > I sometimes wonder about this being noise or not, so I will slightly
> > hijack this thread for this discussion.
> >
> > >From a kernel developer point-of-view, or even from a platform
> > developer or user with a debugging hat point-of-view, having
> > a "device created" or "device registered" message is often very useful.
>
> For you, yes.  For someone with 30000 devices attached to their system,
> it is not, and causes booting to take longer than it should be.
>
> > In fact, I wish people would do this more often, so I don't have to
> > deal with dynamic debug, or hack my way:
> >
> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > index 4589631798c9..473549b26bb2 100644
> > --- a/drivers/media/i2c/ov5647.c
> > +++ b/drivers/media/i2c/ov5647.c
> > @@ -603,7 +603,7 @@ static int ov5647_probe(struct i2c_client *client,
> >         if (ret < 0)
> >                 goto error;
> >
> > -       dev_dbg(dev, "OmniVision OV5647 camera driver probed\n");
> > +       dev_info(dev, "OmniVision OV5647 camera driver probed\n");
> >         return 0;
> >  error:
> >         media_entity_cleanup(&sd->entity);
> >
> > In some subsystems, it's even a behavior I'm more or less relying on:
> >
> > $ git grep v4l2_info.*registered drivers/media/ | wc -l
> > 26
> >
> > And on the downsides, I can't find much. It's just one little line,
> > that is not even noticed unless you have logging turned on.
>
> Its better to be quiet, which is why the "default driver registration"
> macros do not have any printk messages in them.  When converting drivers
> over to it, we made the boot process much more sane, don't try to go and
> add messages for no good reason back in please.
>
> dynamic debugging can be enabled on a module and line-by-line basis,
> even from the boot command line.  So if you need debugging, you can
> always ask someone to just reboot or unload/load the module and get the
> message that way.
>

Can we by any chance make this an official policy ? I am kind of tired
having to argue about this over and over again.

Thanks,
Guenter

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Greg Kroah-Hartman]

On Tue, Jun 04, 2019 at 11:39:21AM -0700, Guenter Roeck wrote:
> On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> > > Hey Greg,
> > >
> > > > > + 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.
> > > >
> > >
> > > I sometimes wonder about this being noise or not, so I will slightly
> > > hijack this thread for this discussion.
> > >
> > > >From a kernel developer point-of-view, or even from a platform
> > > developer or user with a debugging hat point-of-view, having
> > > a "device created" or "device registered" message is often very useful.
> >
> > For you, yes.  For someone with 30000 devices attached to their system,
> > it is not, and causes booting to take longer than it should be.
> >
> > > In fact, I wish people would do this more often, so I don't have to
> > > deal with dynamic debug, or hack my way:
> > >
> > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > index 4589631798c9..473549b26bb2 100644
> > > --- a/drivers/media/i2c/ov5647.c
> > > +++ b/drivers/media/i2c/ov5647.c
> > > @@ -603,7 +603,7 @@ static int ov5647_probe(struct i2c_client *client,
> > >         if (ret < 0)
> > >                 goto error;
> > >
> > > -       dev_dbg(dev, "OmniVision OV5647 camera driver probed\n");
> > > +       dev_info(dev, "OmniVision OV5647 camera driver probed\n");
> > >         return 0;
> > >  error:
> > >         media_entity_cleanup(&sd->entity);
> > >
> > > In some subsystems, it's even a behavior I'm more or less relying on:
> > >
> > > $ git grep v4l2_info.*registered drivers/media/ | wc -l
> > > 26
> > >
> > > And on the downsides, I can't find much. It's just one little line,
> > > that is not even noticed unless you have logging turned on.
> >
> > Its better to be quiet, which is why the "default driver registration"
> > macros do not have any printk messages in them.  When converting drivers
> > over to it, we made the boot process much more sane, don't try to go and
> > add messages for no good reason back in please.
> >
> > dynamic debugging can be enabled on a module and line-by-line basis,
> > even from the boot command line.  So if you need debugging, you can
> > always ask someone to just reboot or unload/load the module and get the
> > message that way.
> >
> 
> Can we by any chance make this an official policy ? I am kind of tired
> having to argue about this over and over again.

Sure, but how does anyone make any "official policy" in the kernel?  :)

I could just go through and delete all "look ma, a new driver/device!"
messages, but that might be annoying...

thanks,

greg k-h

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Lee Jones]

On Tue, 04 Jun 2019, Greg Kroah-Hartman wrote:
> On Tue, Jun 04, 2019 at 11:39:21AM -0700, Guenter Roeck wrote:
> > On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> > > > Hey Greg,
> > > >
> > > > > > + 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.
> > > > >
> > > >
> > > > I sometimes wonder about this being noise or not, so I will slightly
> > > > hijack this thread for this discussion.
> > > >
> > > > >From a kernel developer point-of-view, or even from a platform
> > > > developer or user with a debugging hat point-of-view, having
> > > > a "device created" or "device registered" message is often very useful.
> > >
> > > For you, yes.  For someone with 30000 devices attached to their system,
> > > it is not, and causes booting to take longer than it should be.

Who has 30,000 devices attached to their systems?  I would argue that
in these special corner-cases, they should knock the log-level *down*
a notch.  For the rest of us who run normal platforms, an extra second
of boot time renders a more forthcoming/useful system than if each of
our devices initialised silently.

Personally I like to know what devices I have on my system, and the
kernel log is the first place I look.  As far as I'm concerned, for
the most part, if it's not in the kernel log, I don't have it.

 "Oh wow, I didn't know I had XXX functionality on this platform."

In my real job, I am currently enabling some newly released AArch64
based laptops for booting with ACPI.  I must have wasted a day whilst
enabling some of the devices the system relies upon, just to find
out that 90% of them were actually probing semi-fine (at least probe()
was succeeding), just silently. *grumble*

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Greg Kroah-Hartman]

On Wed, Jun 05, 2019 at 07:48:39AM +0100, Lee Jones wrote:
> On Tue, 04 Jun 2019, Greg Kroah-Hartman wrote:
> > On Tue, Jun 04, 2019 at 11:39:21AM -0700, Guenter Roeck wrote:
> > > On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> > > > > Hey Greg,
> > > > >
> > > > > > > + 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.
> > > > > >
> > > > >
> > > > > I sometimes wonder about this being noise or not, so I will slightly
> > > > > hijack this thread for this discussion.
> > > > >
> > > > > >From a kernel developer point-of-view, or even from a platform
> > > > > developer or user with a debugging hat point-of-view, having
> > > > > a "device created" or "device registered" message is often very useful.
> > > >
> > > > For you, yes.  For someone with 30000 devices attached to their system,
> > > > it is not, and causes booting to take longer than it should be.
> 
> Who has 30,000 devices attached to their systems?

More than you might imagine.

> I would argue that
> in these special corner-cases, they should knock the log-level *down*
> a notch.  For the rest of us who run normal platforms, an extra second
> of boot time renders a more forthcoming/useful system than if each of
> our devices initialised silently.
> 
> Personally I like to know what devices I have on my system, and the
> kernel log is the first place I look.  As far as I'm concerned, for
> the most part, if it's not in the kernel log, I don't have it.

Then you "do not have" lots of devices, as we have been removing these
messages for a number of years now :)

>  "Oh wow, I didn't know I had XXX functionality on this platform."
> 
> In my real job, I am currently enabling some newly released AArch64
> based laptops for booting with ACPI.  I must have wasted a day whilst
> enabling some of the devices the system relies upon, just to find
> out that 90% of them were actually probing semi-fine (at least probe()
> was succeeding), just silently. *grumble*

Yup, that's normal.  If you want to see what devices are in the system,
look in /sys/devices/ as that is what it is for, not the kernel log.

thanks,

greg k-h

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Lee Jones]

On Wed, 05 Jun 2019, Greg Kroah-Hartman wrote:

> On Wed, Jun 05, 2019 at 07:48:39AM +0100, Lee Jones wrote:
> > On Tue, 04 Jun 2019, Greg Kroah-Hartman wrote:
> > > On Tue, Jun 04, 2019 at 11:39:21AM -0700, Guenter Roeck wrote:
> > > > On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> > > > > > Hey Greg,
> > > > > >
> > > > > > > > + 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.
> > > > > > >
> > > > > >
> > > > > > I sometimes wonder about this being noise or not, so I will slightly
> > > > > > hijack this thread for this discussion.
> > > > > >
> > > > > > >From a kernel developer point-of-view, or even from a platform
> > > > > > developer or user with a debugging hat point-of-view, having
> > > > > > a "device created" or "device registered" message is often very useful.
> > > > >
> > > > > For you, yes.  For someone with 30000 devices attached to their system,
> > > > > it is not, and causes booting to take longer than it should be.
> > 
> > Who has 30,000 devices attached to their systems?
> 
> More than you might imagine.
> 
> > I would argue that
> > in these special corner-cases, they should knock the log-level *down*
> > a notch.  For the rest of us who run normal platforms, an extra second
> > of boot time renders a more forthcoming/useful system than if each of
> > our devices initialised silently.
> > 
> > Personally I like to know what devices I have on my system, and the
> > kernel log is the first place I look.  As far as I'm concerned, for
> > the most part, if it's not in the kernel log, I don't have it.
> 
> Then you "do not have" lots of devices, as we have been removing these
> messages for a number of years now :)
> 
> >  "Oh wow, I didn't know I had XXX functionality on this platform."
> > 
> > In my real job, I am currently enabling some newly released AArch64
> > based laptops for booting with ACPI.  I must have wasted a day whilst
> > enabling some of the devices the system relies upon, just to find
> > out that 90% of them were actually probing semi-fine (at least probe()
> > was succeeding), just silently. *grumble*
> 
> Yup, that's normal.  If you want to see what devices are in the system,
> look in /sys/devices/ as that is what it is for, not the kernel log.

My guess is that less than 1% of Linux users use /sys/devices in this
way.  It's a very unfriendly interface.  Besides, when enabling a new
platform, access to sysfs comes too far down the line to be useful in
the majority of cases.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Greg Kroah-Hartman]

On Wed, Jun 05, 2019 at 09:40:02AM +0100, Lee Jones wrote:
> On Wed, 05 Jun 2019, Greg Kroah-Hartman wrote:
> 
> > On Wed, Jun 05, 2019 at 07:48:39AM +0100, Lee Jones wrote:
> > > On Tue, 04 Jun 2019, Greg Kroah-Hartman wrote:
> > > > On Tue, Jun 04, 2019 at 11:39:21AM -0700, Guenter Roeck wrote:
> > > > > On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> > > > > > > Hey Greg,
> > > > > > >
> > > > > > > > > + 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.
> > > > > > > >
> > > > > > >
> > > > > > > I sometimes wonder about this being noise or not, so I will slightly
> > > > > > > hijack this thread for this discussion.
> > > > > > >
> > > > > > > >From a kernel developer point-of-view, or even from a platform
> > > > > > > developer or user with a debugging hat point-of-view, having
> > > > > > > a "device created" or "device registered" message is often very useful.
> > > > > >
> > > > > > For you, yes.  For someone with 30000 devices attached to their system,
> > > > > > it is not, and causes booting to take longer than it should be.
> > > 
> > > Who has 30,000 devices attached to their systems?
> > 
> > More than you might imagine.
> > 
> > > I would argue that
> > > in these special corner-cases, they should knock the log-level *down*
> > > a notch.  For the rest of us who run normal platforms, an extra second
> > > of boot time renders a more forthcoming/useful system than if each of
> > > our devices initialised silently.
> > > 
> > > Personally I like to know what devices I have on my system, and the
> > > kernel log is the first place I look.  As far as I'm concerned, for
> > > the most part, if it's not in the kernel log, I don't have it.
> > 
> > Then you "do not have" lots of devices, as we have been removing these
> > messages for a number of years now :)
> > 
> > >  "Oh wow, I didn't know I had XXX functionality on this platform."
> > > 
> > > In my real job, I am currently enabling some newly released AArch64
> > > based laptops for booting with ACPI.  I must have wasted a day whilst
> > > enabling some of the devices the system relies upon, just to find
> > > out that 90% of them were actually probing semi-fine (at least probe()
> > > was succeeding), just silently. *grumble*
> > 
> > Yup, that's normal.  If you want to see what devices are in the system,
> > look in /sys/devices/ as that is what it is for, not the kernel log.
> 
> My guess is that less than 1% of Linux users use /sys/devices in this
> way.  It's a very unfriendly interface.  Besides, when enabling a new
> platform, access to sysfs comes too far down the line to be useful in
> the majority of cases.

`lshw` is your friend :)

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Lee Jones]

On Wed, 05 Jun 2019, Greg Kroah-Hartman wrote:

> On Wed, Jun 05, 2019 at 09:40:02AM +0100, Lee Jones wrote:
> > On Wed, 05 Jun 2019, Greg Kroah-Hartman wrote:
> > 
> > > On Wed, Jun 05, 2019 at 07:48:39AM +0100, Lee Jones wrote:
> > > > On Tue, 04 Jun 2019, Greg Kroah-Hartman wrote:
> > > > > On Tue, Jun 04, 2019 at 11:39:21AM -0700, Guenter Roeck wrote:
> > > > > > On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
> > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > >
> > > > > > > On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> > > > > > > > Hey Greg,
> > > > > > > >
> > > > > > > > > > + 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.
> > > > > > > > >
> > > > > > > >
> > > > > > > > I sometimes wonder about this being noise or not, so I will slightly
> > > > > > > > hijack this thread for this discussion.
> > > > > > > >
> > > > > > > > >From a kernel developer point-of-view, or even from a platform
> > > > > > > > developer or user with a debugging hat point-of-view, having
> > > > > > > > a "device created" or "device registered" message is often very useful.
> > > > > > >
> > > > > > > For you, yes.  For someone with 30000 devices attached to their system,
> > > > > > > it is not, and causes booting to take longer than it should be.
> > > > 
> > > > Who has 30,000 devices attached to their systems?
> > > 
> > > More than you might imagine.
> > > 
> > > > I would argue that
> > > > in these special corner-cases, they should knock the log-level *down*
> > > > a notch.  For the rest of us who run normal platforms, an extra second
> > > > of boot time renders a more forthcoming/useful system than if each of
> > > > our devices initialised silently.
> > > > 
> > > > Personally I like to know what devices I have on my system, and the
> > > > kernel log is the first place I look.  As far as I'm concerned, for
> > > > the most part, if it's not in the kernel log, I don't have it.
> > > 
> > > Then you "do not have" lots of devices, as we have been removing these
> > > messages for a number of years now :)
> > > 
> > > >  "Oh wow, I didn't know I had XXX functionality on this platform."
> > > > 
> > > > In my real job, I am currently enabling some newly released AArch64
> > > > based laptops for booting with ACPI.  I must have wasted a day whilst
> > > > enabling some of the devices the system relies upon, just to find
> > > > out that 90% of them were actually probing semi-fine (at least probe()
> > > > was succeeding), just silently. *grumble*
> > > 
> > > Yup, that's normal.  If you want to see what devices are in the system,
> > > look in /sys/devices/ as that is what it is for, not the kernel log.
> > 
> > My guess is that less than 1% of Linux users use /sys/devices in this
> > way.  It's a very unfriendly interface.  Besides, when enabling a new
> > platform, access to sysfs comes too far down the line to be useful in
> > the majority of cases.
> 
> `lshw` is your friend :)

Provided you have a command line (with `lshw` installed) and a
working keyboard.  ;)

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Ezequiel Garcia]

On Tue, 2019-06-04 at 20:59 +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 04, 2019 at 11:39:21AM -0700, Guenter Roeck wrote:
> > On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> > > > Hey Greg,
> > > > 
> > > > > > + 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.
> > > > > 
> > > > 
> > > > I sometimes wonder about this being noise or not, so I will slightly
> > > > hijack this thread for this discussion.
> > > > 
> > > > > From a kernel developer point-of-view, or even from a platform
> > > > developer or user with a debugging hat point-of-view, having
> > > > a "device created" or "device registered" message is often very useful.
> > > 
> > > For you, yes.  For someone with 30000 devices attached to their system,
> > > it is not, and causes booting to take longer than it should be.
> > > 
> > > > In fact, I wish people would do this more often, so I don't have to
> > > > deal with dynamic debug, or hack my way:
> > > > 
> > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > > index 4589631798c9..473549b26bb2 100644
> > > > --- a/drivers/media/i2c/ov5647.c
> > > > +++ b/drivers/media/i2c/ov5647.c
> > > > @@ -603,7 +603,7 @@ static int ov5647_probe(struct i2c_client *client,
> > > >         if (ret < 0)
> > > >                 goto error;
> > > > 
> > > > -       dev_dbg(dev, "OmniVision OV5647 camera driver probed\n");
> > > > +       dev_info(dev, "OmniVision OV5647 camera driver probed\n");
> > > >         return 0;
> > > >  error:
> > > >         media_entity_cleanup(&sd->entity);
> > > > 
> > > > In some subsystems, it's even a behavior I'm more or less relying on:
> > > > 
> > > > $ git grep v4l2_info.*registered drivers/media/ | wc -l
> > > > 26
> > > > 
> > > > And on the downsides, I can't find much. It's just one little line,
> > > > that is not even noticed unless you have logging turned on.
> > > 
> > > Its better to be quiet, which is why the "default driver registration"
> > > macros do not have any printk messages in them.  When converting drivers
> > > over to it, we made the boot process much more sane, don't try to go and
> > > add messages for no good reason back in please.
> > > 
> > > dynamic debugging can be enabled on a module and line-by-line basis,
> > > even from the boot command line.  So if you need debugging, you can
> > > always ask someone to just reboot or unload/load the module and get the
> > > message that way.
> > > 
> > 
> > Can we by any chance make this an official policy ? I am kind of tired
> > having to argue about this over and over again.
> 
> Sure, but how does anyone make any "official policy" in the kernel?  :)
> 
> I could just go through and delete all "look ma, a new driver/device!"
> messages, but that might be annoying...
> 

Well, I really need to task.

If it's not an official policy (and won't be anytime soon?), then
what's preventing Enric from pushing this print on this driver,
given he is the one maintaining the code?

Thanks,
Eze

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Greg Kroah-Hartman]

On Thu, Jun 06, 2019 at 11:01:17AM -0300, Ezequiel Garcia wrote:
> On Tue, 2019-06-04 at 20:59 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jun 04, 2019 at 11:39:21AM -0700, Guenter Roeck wrote:
> > > On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > > On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> > > > > Hey Greg,
> > > > > 
> > > > > > > + 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.
> > > > > > 
> > > > > 
> > > > > I sometimes wonder about this being noise or not, so I will slightly
> > > > > hijack this thread for this discussion.
> > > > > 
> > > > > > From a kernel developer point-of-view, or even from a platform
> > > > > developer or user with a debugging hat point-of-view, having
> > > > > a "device created" or "device registered" message is often very useful.
> > > > 
> > > > For you, yes.  For someone with 30000 devices attached to their system,
> > > > it is not, and causes booting to take longer than it should be.
> > > > 
> > > > > In fact, I wish people would do this more often, so I don't have to
> > > > > deal with dynamic debug, or hack my way:
> > > > > 
> > > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > > > index 4589631798c9..473549b26bb2 100644
> > > > > --- a/drivers/media/i2c/ov5647.c
> > > > > +++ b/drivers/media/i2c/ov5647.c
> > > > > @@ -603,7 +603,7 @@ static int ov5647_probe(struct i2c_client *client,
> > > > >         if (ret < 0)
> > > > >                 goto error;
> > > > > 
> > > > > -       dev_dbg(dev, "OmniVision OV5647 camera driver probed\n");
> > > > > +       dev_info(dev, "OmniVision OV5647 camera driver probed\n");
> > > > >         return 0;
> > > > >  error:
> > > > >         media_entity_cleanup(&sd->entity);
> > > > > 
> > > > > In some subsystems, it's even a behavior I'm more or less relying on:
> > > > > 
> > > > > $ git grep v4l2_info.*registered drivers/media/ | wc -l
> > > > > 26
> > > > > 
> > > > > And on the downsides, I can't find much. It's just one little line,
> > > > > that is not even noticed unless you have logging turned on.
> > > > 
> > > > Its better to be quiet, which is why the "default driver registration"
> > > > macros do not have any printk messages in them.  When converting drivers
> > > > over to it, we made the boot process much more sane, don't try to go and
> > > > add messages for no good reason back in please.
> > > > 
> > > > dynamic debugging can be enabled on a module and line-by-line basis,
> > > > even from the boot command line.  So if you need debugging, you can
> > > > always ask someone to just reboot or unload/load the module and get the
> > > > message that way.
> > > > 
> > > 
> > > Can we by any chance make this an official policy ? I am kind of tired
> > > having to argue about this over and over again.
> > 
> > Sure, but how does anyone make any "official policy" in the kernel?  :)
> > 
> > I could just go through and delete all "look ma, a new driver/device!"
> > messages, but that might be annoying...
> > 
> 
> Well, I really need to task.

???

> If it's not an official policy (and won't be anytime soon?),

The ":)" there was that we really have very few "official" policies,
only things that we all strongly encourage to happen.  And get grumpy if
we see them in code reviews.  Like I did here.

> then what's preventing Enric from pushing this print on this driver,
> given he is the one maintaining the code?

Given that he wants people to review his code, why would you tell him to
ignore what people are trying to tell him?

Again, don't be noisy, it's not hard, and is how things have been
trending for many years now.

thanks,

greg k-h

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Ezequiel Garcia]

On Thu, 2019-06-06 at 16:51 +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 06, 2019 at 11:01:17AM -0300, Ezequiel Garcia wrote:
> > On Tue, 2019-06-04 at 20:59 +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Jun 04, 2019 at 11:39:21AM -0700, Guenter Roeck wrote:
> > > > On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > > On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> > > > > > Hey Greg,
> > > > > > 
> > > > > > > > + 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.
> > > > > > > 
> > > > > > 
> > > > > > I sometimes wonder about this being noise or not, so I will slightly
> > > > > > hijack this thread for this discussion.
> > > > > > 
> > > > > > > From a kernel developer point-of-view, or even from a platform
> > > > > > developer or user with a debugging hat point-of-view, having
> > > > > > a "device created" or "device registered" message is often very useful.
> > > > > 
> > > > > For you, yes.  For someone with 30000 devices attached to their system,
> > > > > it is not, and causes booting to take longer than it should be.
> > > > > 
> > > > > > In fact, I wish people would do this more often, so I don't have to
> > > > > > deal with dynamic debug, or hack my way:
> > > > > > 
> > > > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > > > > index 4589631798c9..473549b26bb2 100644
> > > > > > --- a/drivers/media/i2c/ov5647.c
> > > > > > +++ b/drivers/media/i2c/ov5647.c
> > > > > > @@ -603,7 +603,7 @@ static int ov5647_probe(struct i2c_client *client,
> > > > > >         if (ret < 0)
> > > > > >                 goto error;
> > > > > > 
> > > > > > -       dev_dbg(dev, "OmniVision OV5647 camera driver probed\n");
> > > > > > +       dev_info(dev, "OmniVision OV5647 camera driver probed\n");
> > > > > >         return 0;
> > > > > >  error:
> > > > > >         media_entity_cleanup(&sd->entity);
> > > > > > 
> > > > > > In some subsystems, it's even a behavior I'm more or less relying on:
> > > > > > 
> > > > > > $ git grep v4l2_info.*registered drivers/media/ | wc -l
> > > > > > 26
> > > > > > 
> > > > > > And on the downsides, I can't find much. It's just one little line,
> > > > > > that is not even noticed unless you have logging turned on.
> > > > > 
> > > > > Its better to be quiet, which is why the "default driver registration"
> > > > > macros do not have any printk messages in them.  When converting drivers
> > > > > over to it, we made the boot process much more sane, don't try to go and
> > > > > add messages for no good reason back in please.
> > > > > 
> > > > > dynamic debugging can be enabled on a module and line-by-line basis,
> > > > > even from the boot command line.  So if you need debugging, you can
> > > > > always ask someone to just reboot or unload/load the module and get the
> > > > > message that way.
> > > > > 
> > > > 
> > > > Can we by any chance make this an official policy ? I am kind of tired
> > > > having to argue about this over and over again.
> > > 
> > > Sure, but how does anyone make any "official policy" in the kernel?  :)
> > > 
> > > I could just go through and delete all "look ma, a new driver/device!"
> > > messages, but that might be annoying...
> > > 
> > 
> > Well, I really need to task.
> 
> ???
> 

Oops, typo: s/task/ask :-)

> > If it's not an official policy (and won't be anytime soon?),
> 
> The ":)" there was that we really have very few "official" policies,
> only things that we all strongly encourage to happen.  And get grumpy if
> we see them in code reviews.  Like I did here.
> 

Well, not everyone gets grumpy. As I pointed out, we use this "registered"
messages (messages or noise, seems this lie in the eye of the beholder),
consistently across entire subsystems.

> > then what's preventing Enric from pushing this print on this driver,
> > given he is the one maintaining the code?
> 
> Given that he wants people to review his code, why would you tell him to
> ignore what people are trying to tell him?
> 

I'm not suggesting to ignore anyone, rather to consider all voices
involved in each review comment.

> Again, don't be noisy, it's not hard, and is how things have been
> trending for many years now.
> 

Thanks,
Eze

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Randy Dunlap]

On 6/6/19 8:12 AM, Ezequiel Garcia wrote:
> On Thu, 2019-06-06 at 16:51 +0200, Greg Kroah-Hartman wrote:
>> On Thu, Jun 06, 2019 at 11:01:17AM -0300, Ezequiel Garcia wrote:
>>> On Tue, 2019-06-04 at 20:59 +0200, Greg Kroah-Hartman wrote:
>>>> On Tue, Jun 04, 2019 at 11:39:21AM -0700, Guenter Roeck wrote:
>>>>> On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
>>>>> <gregkh@linuxfoundation.org> wrote:
>>>>>> On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
>>>>>>> Hey Greg,
>>>>>>>
>>>>>>>>> + 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.
>>>>>>>>
>>>>>>>
>>>>>>> I sometimes wonder about this being noise or not, so I will slightly
>>>>>>> hijack this thread for this discussion.
>>>>>>>
>>>>>>>> From a kernel developer point-of-view, or even from a platform
>>>>>>> developer or user with a debugging hat point-of-view, having
>>>>>>> a "device created" or "device registered" message is often very useful.
>>>>>>
>>>>>> For you, yes.  For someone with 30000 devices attached to their system,
>>>>>> it is not, and causes booting to take longer than it should be.
>>>>>>
>>>>>>> In fact, I wish people would do this more often, so I don't have to
>>>>>>> deal with dynamic debug, or hack my way:
>>>>>>>
>>>>>>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
>>>>>>> index 4589631798c9..473549b26bb2 100644
>>>>>>> --- a/drivers/media/i2c/ov5647.c
>>>>>>> +++ b/drivers/media/i2c/ov5647.c
>>>>>>> @@ -603,7 +603,7 @@ static int ov5647_probe(struct i2c_client *client,
>>>>>>>         if (ret < 0)
>>>>>>>                 goto error;
>>>>>>>
>>>>>>> -       dev_dbg(dev, "OmniVision OV5647 camera driver probed\n");
>>>>>>> +       dev_info(dev, "OmniVision OV5647 camera driver probed\n");
>>>>>>>         return 0;
>>>>>>>  error:
>>>>>>>         media_entity_cleanup(&sd->entity);
>>>>>>>
>>>>>>> In some subsystems, it's even a behavior I'm more or less relying on:
>>>>>>>
>>>>>>> $ git grep v4l2_info.*registered drivers/media/ | wc -l
>>>>>>> 26
>>>>>>>
>>>>>>> And on the downsides, I can't find much. It's just one little line,
>>>>>>> that is not even noticed unless you have logging turned on.
>>>>>>
>>>>>> Its better to be quiet, which is why the "default driver registration"
>>>>>> macros do not have any printk messages in them.  When converting drivers
>>>>>> over to it, we made the boot process much more sane, don't try to go and
>>>>>> add messages for no good reason back in please.
>>>>>>
>>>>>> dynamic debugging can be enabled on a module and line-by-line basis,
>>>>>> even from the boot command line.  So if you need debugging, you can
>>>>>> always ask someone to just reboot or unload/load the module and get the
>>>>>> message that way.
>>>>>>
>>>>>
>>>>> Can we by any chance make this an official policy ? I am kind of tired
>>>>> having to argue about this over and over again.
>>>>
>>>> Sure, but how does anyone make any "official policy" in the kernel?  :)
>>>>
>>>> I could just go through and delete all "look ma, a new driver/device!"
>>>> messages, but that might be annoying...
>>>>
>>>
>>> Well, I really need to task.
>>
>> ???
>>
> 
> Oops, typo: s/task/ask :-)
> 
>>> If it's not an official policy (and won't be anytime soon?),
>>
>> The ":)" there was that we really have very few "official" policies,
>> only things that we all strongly encourage to happen.  And get grumpy if
>> we see them in code reviews.  Like I did here.
>>
> 
> Well, not everyone gets grumpy. As I pointed out, we use this "registered"
> messages (messages or noise, seems this lie in the eye of the beholder),
> consistently across entire subsystems.

:(

>>> then what's preventing Enric from pushing this print on this driver,
>>> given he is the one maintaining the code?
>>
>> Given that he wants people to review his code, why would you tell him to
>> ignore what people are trying to tell him?
>>
> 
> I'm not suggesting to ignore anyone, rather to consider all voices
> involved in each review comment.
> 
>> Again, don't be noisy, it's not hard, and is how things have been
>> trending for many years now.

Ack that.


-- 
~Randy

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Lee Jones]

On Thu, 06 Jun 2019, Randy Dunlap wrote:
> > On Thu, 2019-06-06 at 16:51 +0200, Greg Kroah-Hartman wrote:
> >> Again, don't be noisy, it's not hard, and is how things have been
> >> trending for many years now.
> 
> Ack that.

Not to say that this particular print is acceptable, but there are
places where a low-level (dbg/info) print on successful probe is
helpful.  Driver initialisation is important!

There's a big difference between drivers 'being noisy', spilling all
sorts of information that may well be useful or interesting to a
driver developer, but has little value to anyone else, and providing a
single print to say that a device has been detected and successfully
initialised/probed.

I recently fell victim to a silent, but fully functional device.
Successful device initialisation should not be silent when debugging
has been set to the highest level IMHO.

And yes, of course turning on debugging for Driver Core works, but is
not practical for all cases and is certainly not the first port of
call when figuring out why initialisation seems to be failing for a
single particular device.

Truly surplus churn should absolutely be removed from the boot log, or
at the very least downgraded, leaving only truly useful information
such as highlighting a newly detected device for example.  If the user
wants an even more silent boot log, they should turn the log level
down a notch.  That is why we have log levels after all.  Simply
removing all useful prints regardless of log-level is not the way to
go IMHO.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Enric Balletbo Serra]

Hi Greg,

Many thanks for your comments,

Missatge de Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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@collabora.com>
> > ---
> >
> >  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@de.ibm.com>
> >  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@winischhofer.net>
> >  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