Re: [PATCH 2/2] drivers/misc: add transfer ioctl for HPS

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Jan 27, 2022 at 07:35:45PM +1100, Sami Kyöstilä wrote:
> This patch adds an ioctl operation for sending and receiving data from
> the ChromeOS snooping protection sensor (a.k.a., HPS). This allows
> userspace programs to perform a combined read/write I2C transaction
> through a single syscall.
> 
> The I2C wire protocol for the device is documented at:
> 
> https://chromium.googlesource.com/chromiumos/platform/hps-firmware/+/
> refs/heads/main/docs/host_device_i2c_protocol.md
> 
> Signed-off-by: Sami Kyöstilä <skyostil@chromium.org>
> ---
> 
>  MAINTAINERS              |  1 +
>  drivers/misc/hps-i2c.c   | 81 ++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/hps.h | 20 ++++++++++
>  3 files changed, 102 insertions(+)
>  create mode 100644 include/uapi/linux/hps.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9dea4b8c2ab5..d5fc066fdbc2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8803,6 +8803,7 @@ M:	Sami Kyöstilä <skyostil@chromium.org>
>  R:	Evan Benn <evanbenn@chromium.org>
>  S:	Maintained
>  F:	drivers/misc/hps-i2c.c
> +F:	include/uapi/linux/hps.h
>  
>  HSI SUBSYSTEM
>  M:	Sebastian Reichel <sre@kernel.org>
> diff --git a/drivers/misc/hps-i2c.c b/drivers/misc/hps-i2c.c
> index fe9f073b0352..748ead49d678 100644
> --- a/drivers/misc/hps-i2c.c
> +++ b/drivers/misc/hps-i2c.c
> @@ -17,9 +17,11 @@
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> +#include <uapi/linux/hps.h>
>  
>  #define HPS_ACPI_ID		"GOOG0020"
>  #define HPS_MAX_DEVICES		1
> +#define HPS_MAX_MSG_SIZE	8192
>  
>  struct hps_drvdata {
>  	struct i2c_client *client;
> @@ -60,6 +62,8 @@ static int hps_open(struct inode *inode, struct file *file)
>  	ret = pm_runtime_get_sync(dev);
>  	if (ret < 0)
>  		goto pm_get_fail;
> +
> +	file->private_data = hps->client;
>  	return 0;
>  
>  pm_get_fail:
> @@ -84,10 +88,87 @@ static int hps_release(struct inode *inode, struct file *file)
>  	return ret;
>  }
>  
> +static int hps_do_ioctl_transfer(struct i2c_client *client,
> +				 struct hps_transfer_ioctl_data *args)
> +{
> +	int ret;
> +	int nmsg = 0;
> +	struct i2c_msg msgs[2] = {
> +		{
> +			.addr = client->addr,
> +			.flags = client->flags,
> +		},
> +		{
> +			.addr = client->addr,
> +			.flags = client->flags,
> +		},
> +	};
> +
> +	if (args->isize) {
> +		msgs[nmsg].len = args->isize;
> +		msgs[nmsg].buf = memdup_user(args->ibuf, args->isize);
> +		if (IS_ERR(msgs[nmsg].buf)) {
> +			ret = PTR_ERR(msgs[nmsg].buf);
> +			goto memdup_fail;
> +		}
> +		nmsg++;
> +	}
> +
> +	if (args->osize) {
> +		msgs[nmsg].len = args->osize;
> +		msgs[nmsg].buf = memdup_user(args->obuf, args->osize);
> +		msgs[nmsg].flags |= I2C_M_RD;
> +		if (IS_ERR(msgs[nmsg].buf)) {
> +			ret = PTR_ERR(msgs[nmsg].buf);
> +			goto memdup_fail;
> +		}
> +		nmsg++;
> +	}
> +
> +	ret = i2c_transfer(client->adapter, &msgs[0], nmsg);
> +	if (ret > 0 && args->osize) {
> +		if (copy_to_user(args->obuf, msgs[nmsg - 1].buf, ret))
> +			ret = -EFAULT;
> +	}

Why can't you just do normal i2c transfers to/from userspace instead of
requring a custom ioctl?

thanks,

greg k-h