Re: [PATCH RESEND v2 2/7] mfd: cros_ec: Add char dev and virtual dev pointers

[Lee Jones <lee.jones@linaro.org>]

On Fri, 02 Jan 2015, Javier Martinez Canillas wrote:

> The ChromeOS Embedded Controller has to be accessed by applications.
> A virtual character device is used as an interface with user-space.
> 
> Extend the struct cros_ec_device with the fields needed by the driver
> of this virtual character device.
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
> 
> Changes since v1: None, new patch.
> 
>  include/linux/mfd/cros_ec.h | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 71675b1..324a346 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -16,6 +16,7 @@
>  #ifndef __LINUX_MFD_CROS_EC_H
>  #define __LINUX_MFD_CROS_EC_H
>  
> +#include <linux/cdev.h>
>  #include <linux/notifier.h>
>  #include <linux/mfd/cros_ec_commands.h>
>  #include <linux/mutex.h>
> @@ -59,9 +60,17 @@ struct cros_ec_command {
>   *
>   * @ec_name: name of EC device (e.g. 'chromeos-ec')
>   * @phys_name: name of physical comms layer (e.g. 'i2c-4')
> - * @dev: Device pointer
> + * @dev: Device pointer for physical comms device
> + * @vdev: Device pointer for virtual comms device
> + * @cdev: Character device structure for virtual comms device
>   * @was_wake_device: true if this device was set to wake the system from
>   * sleep at the last suspend
> + * @cmd_readmem: direct read of the EC memory-mapped region, if supported
> + *     @offset is within EC_LPC_ADDR_MEMMAP region.
> + *     @bytes: number of bytes to read. zero means "read a string" (including
> + *     the trailing '\0'). At most only EC_MEMMAP_SIZE bytes can be read.
> + *     Caller must ensure that the buffer is large enough for the result when
> + *     reading a string.
>   *
>   * @priv: Private data
>   * @irq: Interrupt to use
> @@ -90,8 +99,12 @@ struct cros_ec_device {
>  	const char *ec_name;
>  	const char *phys_name;
>  	struct device *dev;
> +	struct device *vdev;
> +	struct cdev cdev;
>  	bool was_wake_device;
>  	struct class *cros_class;
> +	int (*cmd_readmem)(struct cros_ec_device *ec, unsigned int offset,
> +			   unsigned int bytes, void *dest);

Is this safe?  Are you sure it's okay to provide an interface from
userspace to read (kernel?) memory?

>  	/* These are used to implement the platform-specific interface */
>  	void *priv;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog