[PATCH 0/2] Add a driver for the ChromeOS snooping protection sensor (HPS)

[PATCH 0/2] Add a driver for the ChromeOS snooping protection sensor (HPS)

[Sami Kyöstilä]

This series adds a driver for the ChromeOS snooping protection sensor
(a.k.a. HPS) device, attached on the I2C bus. The sensor is a
hardware-isolated module which reports a high-level presence signal,
e.g., whether there is person in front of the computer or not.

The driver exposes the device to userspace as a character device, which
can be used to send and receive messages to the sensor. The driver
automatically turns power off to the sensor unless a process is

More information about HPS, its firmware and communication protocol can
be found at
https://chromium.googlesource.com/chromiumos/platform/hps-firmware.


Sami Kyöstilä (2):
  drivers/misc: add a driver for HPS
  drivers/misc: add transfer ioctl for HPS

 MAINTAINERS              |   7 +
 drivers/misc/Kconfig     |  10 ++
 drivers/misc/Makefile    |   1 +
 drivers/misc/hps-i2c.c   | 304 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/hps.h |  20 +++
 5 files changed, 342 insertions(+)
 create mode 100644 drivers/misc/hps-i2c.c
 create mode 100644 include/uapi/linux/hps.h

-- 
2.35.0.rc0.227.g00780c9af4-goog

[PATCH 1/2] drivers/misc: add a driver for HPS

[Sami Kyöstilä]

This patch introduces a driver for the ChromeOS snooping protection
sensor (aka. HPS). The driver supports a sensor connected to the I2C bus
and identified as "GOOG0020" in the ACPI tables.

When loaded, the driver exports the sensor to userspace through a
character device. This initial version of the device only supports power
management, i.e., communicating with the sensor must be done through I2C
from userspace.

Power management is implemented by enabling the respective power GPIO
while at least one userspace process holds an open fd on the character
device. By default, the device is powered down if there are no active
clients.

Note that the driver makes no effort to preserve the state of the sensor
between power down and power up events. Userspace is responsible for
reinitializing any needed state once power has been restored.

The device firmware, I2C protocol and other documentation is available
at https://chromium.googlesource.com/chromiumos/platform/hps-firmware.

Signed-off-by: Sami Kyöstilä <skyostil@chromium.org>
---

 MAINTAINERS            |   6 ++
 drivers/misc/Kconfig   |  10 ++
 drivers/misc/Makefile  |   1 +
 drivers/misc/hps-i2c.c | 223 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 240 insertions(+)
 create mode 100644 drivers/misc/hps-i2c.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ea3e6c914384..9dea4b8c2ab5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8798,6 +8798,12 @@ S:	Maintained
 W:	http://artax.karlin.mff.cuni.cz/~mikulas/vyplody/hpfs/index-e.cgi
 F:	fs/hpfs/
 
+HPS (ChromeOS snooping protection sensor) DRIVER
+M:	Sami Kyöstilä <skyostil@chromium.org>
+R:	Evan Benn <evanbenn@chromium.org>
+S:	Maintained
+F:	drivers/misc/hps-i2c.c
+
 HSI SUBSYSTEM
 M:	Sebastian Reichel <sre@kernel.org>
 S:	Maintained
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 0f5a49fc7c9e..b48b7803f537 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -244,6 +244,16 @@ config HP_ILO
 	  To compile this driver as a module, choose M here: the
 	  module will be called hpilo.
 
+config HPS_I2C
+	tristate "ChromeOS HPS device support"
+	depends on HID && I2C && PM
+	help
+	  Say Y here if you want to enable support for the ChromeOS
+	  anti-snooping sensor (HPS), attached via I2C. The driver supports a
+	  sensor connected to the I2C bus and exposes it as a character device.
+	  To save power, the sensor is automatically powered down when no
+	  clients are accessing it.
+
 config QCOM_COINCELL
 	tristate "Qualcomm coincell charger support"
 	depends on MFD_SPMI_PMIC || COMPILE_TEST
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index a086197af544..162a7d530dab 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_SGI_GRU)		+= sgi-gru/
 obj-$(CONFIG_CS5535_MFGPT)	+= cs5535-mfgpt.o
 obj-$(CONFIG_GEHC_ACHC)		+= gehc-achc.o
 obj-$(CONFIG_HP_ILO)		+= hpilo.o
+obj-$(CONFIG_HPS_I2C)		+= hps-i2c.o
 obj-$(CONFIG_APDS9802ALS)	+= apds9802als.o
 obj-$(CONFIG_ISL29003)		+= isl29003.o
 obj-$(CONFIG_ISL29020)		+= isl29020.o
diff --git a/drivers/misc/hps-i2c.c b/drivers/misc/hps-i2c.c
new file mode 100644
index 000000000000..fe9f073b0352
--- /dev/null
+++ b/drivers/misc/hps-i2c.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for the ChromeOS anti-snooping sensor (HPS), attached via I2C.
+ *
+ * The driver exposes HPS as a character device, although currently no read or
+ * write operations are supported. Instead, the driver only controls the power
+ * state of the sensor, keeping it on only while userspace holds an open file
+ * descriptor to the HPS device.
+ *
+ * Copyright 2022 Google LLC.
+ */
+
+#include <linux/acpi.h>
+#include <linux/cdev.h>
+#include <linux/fs.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+
+#define HPS_ACPI_ID		"GOOG0020"
+#define HPS_MAX_DEVICES		1
+
+struct hps_drvdata {
+	struct i2c_client *client;
+
+	struct cdev cdev;
+	struct class *cdev_class;
+
+	struct gpio_desc *enable_gpio;
+};
+
+static int hps_dev_major;
+
+static void hps_power_on(struct hps_drvdata *hps)
+{
+	if (!IS_ERR_OR_NULL(hps->enable_gpio))
+		gpiod_set_value_cansleep(hps->enable_gpio, 1);
+}
+
+static void hps_power_off(struct hps_drvdata *hps)
+{
+	if (!IS_ERR_OR_NULL(hps->enable_gpio))
+		gpiod_set_value_cansleep(hps->enable_gpio, 0);
+}
+
+static void hps_unload(void *drv_data)
+{
+	struct hps_drvdata *hps = drv_data;
+
+	hps_power_on(hps);
+}
+
+static int hps_open(struct inode *inode, struct file *file)
+{
+	struct hps_drvdata *hps = container_of(inode->i_cdev, struct hps_drvdata, cdev);
+	struct device *dev = &hps->client->dev;
+	int ret;
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto pm_get_fail;
+	return 0;
+
+pm_get_fail:
+	pm_runtime_put(dev);
+	pm_runtime_disable(dev);
+	return ret;
+}
+
+static int hps_release(struct inode *inode, struct file *file)
+{
+	struct hps_drvdata *hps = container_of(inode->i_cdev, struct hps_drvdata, cdev);
+	struct device *dev = &hps->client->dev;
+	int ret;
+
+	ret = pm_runtime_put(dev);
+	if (ret < 0)
+		goto pm_put_fail;
+	return 0;
+
+pm_put_fail:
+	pm_runtime_disable(dev);
+	return ret;
+}
+
+const struct file_operations hps_fops = {
+	.owner = THIS_MODULE,
+	.open = hps_open,
+	.release = hps_release,
+};
+
+static int hps_i2c_probe(struct i2c_client *client)
+{
+	struct hps_drvdata *hps;
+	int ret = 0;
+	dev_t hps_dev;
+
+	hps = devm_kzalloc(&client->dev, sizeof(*hps), GFP_KERNEL);
+	if (!hps)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, hps);
+	hps->client = client;
+	hps->enable_gpio = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_HIGH);
+	if (IS_ERR(hps->enable_gpio)) {
+		ret = PTR_ERR(hps->enable_gpio);
+		dev_err(&client->dev, "failed to get enable gpio: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_add_action(&client->dev, &hps_unload, hps);
+	if (ret) {
+		dev_err(&client->dev,
+			"failed to install unload action: %d\n", ret);
+		return ret;
+	}
+
+	ret = alloc_chrdev_region(&hps_dev, 0, HPS_MAX_DEVICES, "hps");
+	if (ret) {
+		dev_err(&client->dev,
+			"failed to register char dev: %d\n", ret);
+		return ret;
+	}
+	hps_dev_major = MAJOR(hps_dev);
+	cdev_init(&hps->cdev, &hps_fops);
+	ret = cdev_add(&hps->cdev, hps_dev, 1);
+	if (ret) {
+		dev_err(&client->dev, "cdev_add() failed: %d\n", ret);
+		goto cdev_add_failed;
+	}
+
+	hps->cdev_class = class_create(THIS_MODULE, "hps");
+	if (IS_ERR(hps->cdev_class)) {
+		dev_err(&client->dev, "class_create() failed: %d\n", ret);
+		goto class_create_failed;
+	}
+	device_create(hps->cdev_class, NULL, hps_dev, NULL, "hps");
+
+	hps_power_off(hps);
+	pm_runtime_enable(&client->dev);
+	return ret;
+
+class_create_failed:
+	ret = PTR_ERR(hps->cdev_class);
+	hps->cdev_class = NULL;
+
+cdev_add_failed:
+	unregister_chrdev_region(MKDEV(hps_dev_major, 0), HPS_MAX_DEVICES);
+	hps_dev_major = 0;
+	return ret;
+}
+
+static int hps_i2c_remove(struct i2c_client *client)
+{
+	struct hps_drvdata *hps = i2c_get_clientdata(client);
+
+	pm_runtime_disable(&client->dev);
+	if (hps_dev_major) {
+		dev_t hps_dev = MKDEV(hps_dev_major, 0);
+
+		device_destroy(hps->cdev_class, hps_dev);
+		class_destroy(hps->cdev_class);
+		hps->cdev_class = NULL;
+
+		cdev_del(&hps->cdev);
+		unregister_chrdev_region(hps_dev, HPS_MAX_DEVICES);
+		hps_dev_major = 0;
+	}
+	return 0;
+}
+
+static int hps_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct hps_drvdata *hps = i2c_get_clientdata(client);
+
+	hps_power_off(hps);
+	return 0;
+}
+
+static int hps_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct hps_drvdata *hps = i2c_get_clientdata(client);
+
+	hps_power_on(hps);
+	return 0;
+}
+static UNIVERSAL_DEV_PM_OPS(hps_pm_ops, hps_suspend, hps_resume, NULL);
+
+static const struct i2c_device_id hps_i2c_id[] = {
+	{ "hps", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, hps_i2c_id);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id hps_acpi_id[] = {
+	{ HPS_ACPI_ID, 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, hps_acpi_id);
+#endif /* CONFIG_ACPI */
+
+static struct i2c_driver hps_i2c_driver = {
+	.probe_new = hps_i2c_probe,
+	.remove = hps_i2c_remove,
+	.id_table = hps_i2c_id,
+	.driver = {
+		.name = "hps",
+		.pm = &hps_pm_ops,
+#ifdef CONFIG_ACPI
+		.acpi_match_table = ACPI_PTR(hps_acpi_id),
+#endif
+	},
+};
+module_i2c_driver(hps_i2c_driver);
+
+MODULE_ALIAS("acpi:" HPS_ACPI_ID);
+MODULE_AUTHOR("Sami Kyöstilä <skyostil@chromium.org>");
+MODULE_DESCRIPTION("Driver for ChromeOS HPS");
+MODULE_LICENSE("GPL");
-- 
2.35.0.rc0.227.g00780c9af4-goog

Re: [PATCH 1/2] drivers/misc: add a driver for HPS

[Greg KH]

On Thu, Jan 27, 2022 at 07:35:44PM +1100, Sami Kyöstilä wrote:
> This patch introduces a driver for the ChromeOS snooping protection
> sensor (aka. HPS). The driver supports a sensor connected to the I2C bus
> and identified as "GOOG0020" in the ACPI tables.
> 
> When loaded, the driver exports the sensor to userspace through a
> character device. This initial version of the device only supports power
> management, i.e., communicating with the sensor must be done through I2C
> from userspace.
> 
> Power management is implemented by enabling the respective power GPIO
> while at least one userspace process holds an open fd on the character
> device. By default, the device is powered down if there are no active
> clients.
> 
> Note that the driver makes no effort to preserve the state of the sensor
> between power down and power up events. Userspace is responsible for
> reinitializing any needed state once power has been restored.
> 
> The device firmware, I2C protocol and other documentation is available
> at https://chromium.googlesource.com/chromiumos/platform/hps-firmware.

How about a userspace tool that interacts with this new ioctl interface
as well so that we can understand how the driver is supposed to work?


> 
> Signed-off-by: Sami Kyöstilä <skyostil@chromium.org>
> ---
> 
>  MAINTAINERS            |   6 ++
>  drivers/misc/Kconfig   |  10 ++
>  drivers/misc/Makefile  |   1 +
>  drivers/misc/hps-i2c.c | 223 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 240 insertions(+)
>  create mode 100644 drivers/misc/hps-i2c.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ea3e6c914384..9dea4b8c2ab5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8798,6 +8798,12 @@ S:	Maintained
>  W:	http://artax.karlin.mff.cuni.cz/~mikulas/vyplody/hpfs/index-e.cgi
>  F:	fs/hpfs/
>  
> +HPS (ChromeOS snooping protection sensor) DRIVER
> +M:	Sami Kyöstilä <skyostil@chromium.org>
> +R:	Evan Benn <evanbenn@chromium.org>
> +S:	Maintained
> +F:	drivers/misc/hps-i2c.c
> +
>  HSI SUBSYSTEM
>  M:	Sebastian Reichel <sre@kernel.org>
>  S:	Maintained
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 0f5a49fc7c9e..b48b7803f537 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -244,6 +244,16 @@ config HP_ILO
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called hpilo.
>  
> +config HPS_I2C
> +	tristate "ChromeOS HPS device support"
> +	depends on HID && I2C && PM
> +	help
> +	  Say Y here if you want to enable support for the ChromeOS
> +	  anti-snooping sensor (HPS), attached via I2C. The driver supports a
> +	  sensor connected to the I2C bus and exposes it as a character device.
> +	  To save power, the sensor is automatically powered down when no
> +	  clients are accessing it.
> +
>  config QCOM_COINCELL
>  	tristate "Qualcomm coincell charger support"
>  	depends on MFD_SPMI_PMIC || COMPILE_TEST
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index a086197af544..162a7d530dab 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_SGI_GRU)		+= sgi-gru/
>  obj-$(CONFIG_CS5535_MFGPT)	+= cs5535-mfgpt.o
>  obj-$(CONFIG_GEHC_ACHC)		+= gehc-achc.o
>  obj-$(CONFIG_HP_ILO)		+= hpilo.o
> +obj-$(CONFIG_HPS_I2C)		+= hps-i2c.o
>  obj-$(CONFIG_APDS9802ALS)	+= apds9802als.o
>  obj-$(CONFIG_ISL29003)		+= isl29003.o
>  obj-$(CONFIG_ISL29020)		+= isl29020.o
> diff --git a/drivers/misc/hps-i2c.c b/drivers/misc/hps-i2c.c
> new file mode 100644
> index 000000000000..fe9f073b0352
> --- /dev/null
> +++ b/drivers/misc/hps-i2c.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the ChromeOS anti-snooping sensor (HPS), attached via I2C.
> + *
> + * The driver exposes HPS as a character device, although currently no read or
> + * write operations are supported. Instead, the driver only controls the power
> + * state of the sensor, keeping it on only while userspace holds an open file
> + * descriptor to the HPS device.
> + *
> + * Copyright 2022 Google LLC.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/cdev.h>
> +#include <linux/fs.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +
> +#define HPS_ACPI_ID		"GOOG0020"
> +#define HPS_MAX_DEVICES		1
> +
> +struct hps_drvdata {
> +	struct i2c_client *client;
> +
> +	struct cdev cdev;
> +	struct class *cdev_class;

As you only have 1 device, please just use the miscdev interface, not
the chardev interface.  Makes your code much smaller and easier to
review and maintain over time and does not "burn" a whole major number
for this tiny driver.

thanks,

greg k-h

Re: [PATCH 1/2] drivers/misc: add a driver for HPS

[Sami Kyostila]

to 27. tammik. 2022 klo 20.40 Greg KH (gregkh@linuxfoundation.org) kirjoitti:
>
> On Thu, Jan 27, 2022 at 07:35:44PM +1100, Sami Kyöstilä wrote:
> > This patch introduces a driver for the ChromeOS snooping protection
> > sensor (aka. HPS). The driver supports a sensor connected to the I2C bus
> > and identified as "GOOG0020" in the ACPI tables.
> >
> > When loaded, the driver exports the sensor to userspace through a
> > character device. This initial version of the device only supports power
> > management, i.e., communicating with the sensor must be done through I2C
> > from userspace.
> >
> > Power management is implemented by enabling the respective power GPIO
> > while at least one userspace process holds an open fd on the character
> > device. By default, the device is powered down if there are no active
> > clients.
> >
> > Note that the driver makes no effort to preserve the state of the sensor
> > between power down and power up events. Userspace is responsible for
> > reinitializing any needed state once power has been restored.
> >
> > The device firmware, I2C protocol and other documentation is available
> > at https://chromium.googlesource.com/chromiumos/platform/hps-firmware.
>
> How about a userspace tool that interacts with this new ioctl interface
> as well so that we can understand how the driver is supposed to work?

Sure, here's a small example that shows how to read a magic register
value from the device:
https://gist.github.com/skyostil/13d60a288919d18d00b20e81dfe018cd

(Let me know if you'd prefer this to be checked into the tree somewhere.)

Here's also a patch that makes the production daemon use this
interface: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3353640

> >
> > Signed-off-by: Sami Kyöstilä <skyostil@chromium.org>
> > ---
> >
> >  MAINTAINERS            |   6 ++
> >  drivers/misc/Kconfig   |  10 ++
> >  drivers/misc/Makefile  |   1 +
> >  drivers/misc/hps-i2c.c | 223 +++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 240 insertions(+)
> >  create mode 100644 drivers/misc/hps-i2c.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ea3e6c914384..9dea4b8c2ab5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8798,6 +8798,12 @@ S:     Maintained
> >  W:   http://artax.karlin.mff.cuni.cz/~mikulas/vyplody/hpfs/index-e.cgi
> >  F:   fs/hpfs/
> >
> > +HPS (ChromeOS snooping protection sensor) DRIVER
> > +M:   Sami Kyöstilä <skyostil@chromium.org>
> > +R:   Evan Benn <evanbenn@chromium.org>
> > +S:   Maintained
> > +F:   drivers/misc/hps-i2c.c
> > +
> >  HSI SUBSYSTEM
> >  M:   Sebastian Reichel <sre@kernel.org>
> >  S:   Maintained
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index 0f5a49fc7c9e..b48b7803f537 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -244,6 +244,16 @@ config HP_ILO
> >         To compile this driver as a module, choose M here: the
> >         module will be called hpilo.
> >
> > +config HPS_I2C
> > +     tristate "ChromeOS HPS device support"
> > +     depends on HID && I2C && PM
> > +     help
> > +       Say Y here if you want to enable support for the ChromeOS
> > +       anti-snooping sensor (HPS), attached via I2C. The driver supports a
> > +       sensor connected to the I2C bus and exposes it as a character device.
> > +       To save power, the sensor is automatically powered down when no
> > +       clients are accessing it.
> > +
> >  config QCOM_COINCELL
> >       tristate "Qualcomm coincell charger support"
> >       depends on MFD_SPMI_PMIC || COMPILE_TEST
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > index a086197af544..162a7d530dab 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -26,6 +26,7 @@ obj-$(CONFIG_SGI_GRU)               += sgi-gru/
> >  obj-$(CONFIG_CS5535_MFGPT)   += cs5535-mfgpt.o
> >  obj-$(CONFIG_GEHC_ACHC)              += gehc-achc.o
> >  obj-$(CONFIG_HP_ILO)         += hpilo.o
> > +obj-$(CONFIG_HPS_I2C)                += hps-i2c.o
> >  obj-$(CONFIG_APDS9802ALS)    += apds9802als.o
> >  obj-$(CONFIG_ISL29003)               += isl29003.o
> >  obj-$(CONFIG_ISL29020)               += isl29020.o
> > diff --git a/drivers/misc/hps-i2c.c b/drivers/misc/hps-i2c.c
> > new file mode 100644
> > index 000000000000..fe9f073b0352
> > --- /dev/null
> > +++ b/drivers/misc/hps-i2c.c
> > @@ -0,0 +1,223 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for the ChromeOS anti-snooping sensor (HPS), attached via I2C.
> > + *
> > + * The driver exposes HPS as a character device, although currently no read or
> > + * write operations are supported. Instead, the driver only controls the power
> > + * state of the sensor, keeping it on only while userspace holds an open file
> > + * descriptor to the HPS device.
> > + *
> > + * Copyright 2022 Google LLC.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/cdev.h>
> > +#include <linux/fs.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> > +
> > +#define HPS_ACPI_ID          "GOOG0020"
> > +#define HPS_MAX_DEVICES              1
> > +
> > +struct hps_drvdata {
> > +     struct i2c_client *client;
> > +
> > +     struct cdev cdev;
> > +     struct class *cdev_class;
>
> As you only have 1 device, please just use the miscdev interface, not
> the chardev interface.  Makes your code much smaller and easier to
> review and maintain over time and does not "burn" a whole major number
> for this tiny driver.

Will do, thanks for the tip!

>
> thanks,
>
> greg k-h



- Sami

Re: [PATCH 1/2] drivers/misc: add a driver for HPS

[Greg KH]

On Fri, Jan 28, 2022 at 06:41:25PM +1100, Sami Kyostila wrote:
> to 27. tammik. 2022 klo 20.40 Greg KH (gregkh@linuxfoundation.org) kirjoitti:
> >
> > On Thu, Jan 27, 2022 at 07:35:44PM +1100, Sami Kyöstilä wrote:
> > > This patch introduces a driver for the ChromeOS snooping protection
> > > sensor (aka. HPS). The driver supports a sensor connected to the I2C bus
> > > and identified as "GOOG0020" in the ACPI tables.
> > >
> > > When loaded, the driver exports the sensor to userspace through a
> > > character device. This initial version of the device only supports power
> > > management, i.e., communicating with the sensor must be done through I2C
> > > from userspace.
> > >
> > > Power management is implemented by enabling the respective power GPIO
> > > while at least one userspace process holds an open fd on the character
> > > device. By default, the device is powered down if there are no active
> > > clients.
> > >
> > > Note that the driver makes no effort to preserve the state of the sensor
> > > between power down and power up events. Userspace is responsible for
> > > reinitializing any needed state once power has been restored.
> > >
> > > The device firmware, I2C protocol and other documentation is available
> > > at https://chromium.googlesource.com/chromiumos/platform/hps-firmware.
> >
> > How about a userspace tool that interacts with this new ioctl interface
> > as well so that we can understand how the driver is supposed to work?
> 
> Sure, here's a small example that shows how to read a magic register
> value from the device:
> https://gist.github.com/skyostil/13d60a288919d18d00b20e81dfe018cd
> 
> (Let me know if you'd prefer this to be checked into the tree somewhere.)

It should be somewhere so we know where to look and how this works and
who to complain to when it needs to be changed :)

thanks,

greg k-h

Re: [PATCH 1/2] drivers/misc: add a driver for HPS

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

Hi!

> +config HPS_I2C
> +	tristate "ChromeOS HPS device support"
> +	depends on HID && I2C && PM
> +	help
> +	  Say Y here if you want to enable support for the ChromeOS
> +	  anti-snooping sensor (HPS), attached via I2C. The driver supports a
> +	  sensor connected to the I2C bus and exposes it as a character device.
> +	  To save power, the sensor is automatically powered down when no
> +	  clients are accessing it.
> +

We don't need to know about power management here; but it would be
good to have explanation what anti-snooping sensor actually does. Is
it camera detecting user or what?

Best regards,
								Pavel
								
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Sami Kyöstilä]

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;
+	}
+
+memdup_fail:
+	while (nmsg > 0)
+		kfree(msgs[--nmsg].buf);
+	return ret;
+}
+
+static long hps_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct i2c_client *client = file->private_data;
+
+	switch (cmd) {
+	case HPS_IOC_TRANSFER: {
+		struct hps_transfer_ioctl_data args;
+
+		if (copy_from_user(&args,
+				   (struct hps_transfer_ioctl_data __user *)arg,
+				   sizeof(args))) {
+			return -EFAULT;
+		}
+
+		if (!args.isize && !args.osize)
+			return -EINVAL;
+		if (args.isize > HPS_MAX_MSG_SIZE || args.osize > HPS_MAX_MSG_SIZE)
+			return -EINVAL;
+
+		return hps_do_ioctl_transfer(client, &args);
+	}
+	default:
+		return -EFAULT;
+	}
+}
+
+
 const struct file_operations hps_fops = {
 	.owner = THIS_MODULE,
 	.open = hps_open,
 	.release = hps_release,
+	.unlocked_ioctl = hps_ioctl,
 };
 
 static int hps_i2c_probe(struct i2c_client *client)
diff --git a/include/uapi/linux/hps.h b/include/uapi/linux/hps.h
new file mode 100644
index 000000000000..2c1bd174cd02
--- /dev/null
+++ b/include/uapi/linux/hps.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+/*
+ *  Copyright 2022 Google LLC.
+ */
+
+#ifndef _UAPI_HPS_H
+#define _UAPI_HPS_H
+
+#include <linux/types.h>
+
+#define HPS_IOC_TRANSFER	_IOWR('h', 0x01, struct hps_transfer_ioctl_data)
+
+struct hps_transfer_ioctl_data {
+	__u32 isize;			/* Number of bytes to send */
+	unsigned char __user *ibuf;	/* Input buffer */
+	__u32 osize;			/* Number of bytes to receive */
+	unsigned char __user *obuf;	/* Output buffer */
+};
+
+#endif /* _UAPI_HPS_H */
-- 
2.35.0.rc0.227.g00780c9af4-goog

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

[Greg KH]

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

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

[Sami Kyostila]

to 27. tammik. 2022 klo 20.41 Greg KH (gregkh@linuxfoundation.org) kirjoitti:
>
> 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?

The main reason is security: without this driver, in order to talk to
HPS the userspace daemon needs read/write access to the entire I2C
controller (e.g., /dev/i2c-0). This means the daemon can also control
any other I2C device which happens to be on the same bus. With a
separate ioctl we can limit access to just HPS.

As far as I can tell, there's no way to restrict access on a
per-device level with normal i2c, but I'd be happy to be proven wrong
:)

>
> thanks,
>
> greg k-h


- Sami

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

[Greg KH]

On Fri, Jan 28, 2022 at 06:42:08PM +1100, Sami Kyostila wrote:
> to 27. tammik. 2022 klo 20.41 Greg KH (gregkh@linuxfoundation.org) kirjoitti:
> >
> > 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?
> 
> The main reason is security: without this driver, in order to talk to
> HPS the userspace daemon needs read/write access to the entire I2C
> controller (e.g., /dev/i2c-0). This means the daemon can also control
> any other I2C device which happens to be on the same bus. With a
> separate ioctl we can limit access to just HPS.

Then use seccomp for this?

> As far as I can tell, there's no way to restrict access on a
> per-device level with normal i2c, but I'd be happy to be proven wrong
> :)

Selinux rules?

What else is on this bus for the device that you care about?

thanks,

greg k-h

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

[Sami Kyostila]

pe 28. tammik. 2022 klo 20.36 Greg KH (gregkh@linuxfoundation.org) kirjoitti:
>
> On Fri, Jan 28, 2022 at 06:42:08PM +1100, Sami Kyostila wrote:
> > to 27. tammik. 2022 klo 20.41 Greg KH (gregkh@linuxfoundation.org) kirjoitti:
> > >
> > > 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?
> >
> > The main reason is security: without this driver, in order to talk to
> > HPS the userspace daemon needs read/write access to the entire I2C
> > controller (e.g., /dev/i2c-0). This means the daemon can also control
> > any other I2C device which happens to be on the same bus. With a
> > separate ioctl we can limit access to just HP
> Then use seccomp for this?

As far as I can tell, seccomp can't do this because it can't read the
device address field[1] which is behind two userspace pointer hops in
the I2C_RDWR ioctl[2].

[1] https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/i2c.h#L74
[2] https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/i2c-dev.h#L51

>
> > As far as I can tell, there's no way to restrict access on a
> > per-device level with normal i2c, but I'd be happy to be proven wrong
> > :)
>
> Selinux rules?

I guess we could add an LSM hook for I2C transfers, but that would
require baking device addresses into the SELinux policy which seems a
little unfortunate.

I think that leaves the options suggested by Arnd (thanks!):

a) Add a generic way to expose device nodes for individual I2C devices
(something like /dev/i2c/by-id/NN?).

b) Make the ioctl interface more fully featured instead of just exposing I2C.

I think I'm leaning toward (a) since it's not yet totally clear what
the right high level abstraction for this type of device is (e.g.,
should it be HID, in which case the protocol should probably become
HID-I2C).

If this sounds okay, then I suggest we merge just the power control
patch from this series (with comments addressed) and I'll follow-up
with splitting out the i2c devices.

> What else is on this bus for the device that you care about?

That's really up to the device manufacturer; it could be anything from
touchpads to cameras or fingerprint scanners.

> thanks,
>
> greg k-h

- Sami

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

[Arnd Bergmann]

On Mon, Jan 31, 2022 at 9:23 AM Sami Kyostila <skyostil@chromium.org> wrote:
>
> I guess we could add an LSM hook for I2C transfers, but that would
> require baking device addresses into the SELinux policy which seems a
> little unfortunate.
>
> I think that leaves the options suggested by Arnd (thanks!):
>
> a) Add a generic way to expose device nodes for individual I2C devices
> (something like /dev/i2c/by-id/NN?).
>
> b) Make the ioctl interface more fully featured instead of just exposing I2C.
>
> I think I'm leaning toward (a) since it's not yet totally clear what
> the right high level abstraction for this type of device is (e.g.,
> should it be HID, in which case the protocol should probably become
> HID-I2C).

(adding i2c list to cc)

I think the implementation of the character device should be really
straightforward, it can probably just use the exact same ioctls
as the normal device, or a subset of them, and filter out any access
that has the wrong address with ioctl(fd, I2C_SLAVE, ...) or other
commands.

The tricky part is coming up with a sensible way of creating those
character device nodes, as there generic method of knowing what
is attached to the bus. I suppose this could be done either
automatically based on the nodes in DT, or it could be done
with user interaction like a new ioctl command on the normal
device node or some sysfs interface to create the chardev for
a particular slave device.

           Arnd

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

[Arnd Bergmann]

On Fri, Jan 28, 2022 at 8:42 AM Sami Kyostila <skyostil@chromium.org> wrote:
> to 27. tammik. 2022 klo 20.41 Greg KH (gregkh@linuxfoundation.org) kirjoitti:

> >
> > Why can't you just do normal i2c transfers to/from userspace instead of
> > requring a custom ioctl?
>
> The main reason is security: without this driver, in order to talk to
> HPS the userspace daemon needs read/write access to the entire I2C
> controller (e.g., /dev/i2c-0). This means the daemon can also control
> any other I2C device which happens to be on the same bus. With a
> separate ioctl we can limit access to just HPS.
>
> As far as I can tell, there's no way to restrict access on a
> per-device level with normal i2c, but I'd be happy to be proven wrong
> :)

I don't know if is correct, but I trust your research on that. However,
I would still argue that instead of adding a custom low-level interface to
give user access to a single raw i2c device, we would be better off doing
one of two other approaches:

a) add the generic interface you are missing -- it seems unlikely that you
are the only user that has come across this issue, and adding a generic
per-device interface to complement the per-controller one would likely
help avoid others coming up with the same thing elsewhere, using yet
another set of custom ioctls.

b) move part of your application into the kernel, and provide a high-level
abstraction for your device in place of the low-level i2c access.

          Arnd

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

[Randy Dunlap]

On 1/27/22 00:35, 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
> 

Hi--

If your next patch version continues to use an ioctl, its magic "number"
('h') should be documented in Documentation/userspace-api/ioctl/ioctl-number.rst.

thanks.

> diff --git a/include/uapi/linux/hps.h b/include/uapi/linux/hps.h
> new file mode 100644
> index 000000000000..2c1bd174cd02
> --- /dev/null
> +++ b/include/uapi/linux/hps.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +/*
> + *  Copyright 2022 Google LLC.
> + */
> +
> +#ifndef _UAPI_HPS_H
> +#define _UAPI_HPS_H
> +
> +#include <linux/types.h>
> +
> +#define HPS_IOC_TRANSFER	_IOWR('h', 0x01, struct hps_transfer_ioctl_data)
> +
> +struct hps_transfer_ioctl_data {
> +	__u32 isize;			/* Number of bytes to send */
> +	unsigned char __user *ibuf;	/* Input buffer */
> +	__u32 osize;			/* Number of bytes to receive */
> +	unsigned char __user *obuf;	/* Output buffer */
> +};
> +
> +#endif /* _UAPI_HPS_H */

-- 
~Randy

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

[Sami Kyostila]

pe 28. tammik. 2022 klo 9.39 Randy Dunlap (rdunlap@infradead.org) kirjoitti:
>
>
>
> On 1/27/22 00:35, 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
> >
>
> Hi--
>
> If your next patch version continues to use an ioctl, its magic "number"
> ('h') should be documented in Documentation/userspace-api/ioctl/ioctl-number.rst.

Ah, thanks for the pointer. Will do.


- Sami

>
> thanks.
>
> > diff --git a/include/uapi/linux/hps.h b/include/uapi/linux/hps.h
> > new file mode 100644
> > index 000000000000..2c1bd174cd02
> > --- /dev/null
> > +++ b/include/uapi/linux/hps.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> > +/*
> > + *  Copyright 2022 Google LLC.
> > + */
> > +
> > +#ifndef _UAPI_HPS_H
> > +#define _UAPI_HPS_H
> > +
> > +#include <linux/types.h>
> > +
> > +#define HPS_IOC_TRANSFER     _IOWR('h', 0x01, struct hps_transfer_ioctl_data)
> > +
> > +struct hps_transfer_ioctl_data {
> > +     __u32 isize;                    /* Number of bytes to send */
> > +     unsigned char __user *ibuf;     /* Input buffer */
> > +     __u32 osize;                    /* Number of bytes to receive */
> > +     unsigned char __user *obuf;     /* Output buffer */
> > +};
> > +
> > +#endif /* _UAPI_HPS_H */
>
> --
> ~Randy