2019-07-05 16:44:13

by Geert Uytterhoeven

Subject: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

GPIO controllers are exported to userspace using /dev/gpiochip*
character devices. Access control to these devices is provided by
standard UNIX file system permissions, on an all-or-nothing basis:
either a GPIO controller is accessible for a user, or it is not.
Currently no mechanism exists to control access to individual GPIOs.

Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
and expose them as a new gpiochip. This is useful for implementing
access control, and assigning a set of GPIOs to a specific user.
Furthermore, it would simplify and harden exporting GPIOs to a virtual
machine, as the VM can just grab the full virtual GPIO controller, and
no longer needs to care about which GPIOs to grab and which not,
reducing the attack surface.

Virtual GPIO controllers are instantiated by writing to the "new_device"
attribute file in sysfs:

$ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
"[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
> /sys/bus/platform/drivers/gpio-virt-agg/new_device

Likewise, virtual GPIO controllers can be destroyed after use:

$ echo gpio-virt-agg.<N> \
> /sys/bus/platform/drivers/gpio-virt-agg/delete_device

Signed-off-by: Geert Uytterhoeven <[email protected]>
---
Aggregating GPIOs and exposing them as a new gpiochip was suggested in
response to my proof-of-concept for GPIO virtualization with QEMU[1][2].

Sample session on r8a7791/koelsch:

- Disable the leds node in arch/arm/boot/dts/r8a7791-koelsch.dts

- Create virtual aggregators:

$ echo "e6052000.gpio 19 20" \
> /sys/bus/platform/drivers/gpio-virt-agg/new_device

gpio-virt-agg gpio-virt-agg.0: GPIO 0 => e6052000.gpio/19
gpio-virt-agg gpio-virt-agg.0: GPIO 1 => e6052000.gpio/20
gpiochip_find_base: found new base at 778
gpio gpiochip8: (gpio-virt-agg.0): added GPIO chardev (254:8)
gpiochip_setup_dev: registered GPIOs 778 to 779 on device: gpiochip8 (gpio-virt-agg.0)

$ echo "e6052000.gpio 21, e6050000.gpio 20 21 22" \
> /sys/bus/platform/drivers/gpio-virt-agg/new_device

gpio-virt-agg gpio-virt-agg.1: GPIO 0 => e6052000.gpio/21
gpio-virt-agg gpio-virt-agg.1: GPIO 1 => e6050000.gpio/20
gpio-virt-agg gpio-virt-agg.1: GPIO 2 => e6050000.gpio/21
gpio-virt-agg gpio-virt-agg.1: GPIO 3 => e6050000.gpio/22
gpiochip_find_base: found new base at 774
gpio gpiochip9: (gpio-virt-agg.1): added GPIO chardev (254:9)
gpiochip_setup_dev: registered GPIOs 774 to 777 on device: gpiochip9 (gpio-virt-agg.1)

- Adjust permissions on /dev/gpiochip[89] (optional)

- Control LEDs:

$ gpioset gpiochip8 0=0 1=1 # LED6 OFF, LED7 ON
$ gpioset gpiochip8 0=1 1=0 # LED6 ON, LED7 OFF
$ gpioset gpiochip9 0=0 # LED8 OFF
$ gpioset gpiochip9 0=1 # LED8 ON

- Destroy virtual aggregators:

$ echo gpio-virt-agg.0 \
> /sys/bus/platform/drivers/gpio-virt-agg/delete_device
$ echo gpio-virt-agg.1 \
> /sys/bus/platform/drivers/gpio-virt-agg/delete_device

Thanks for your comments!

References:
- [1] "[PATCH QEMU POC] Add a GPIO backend"
(https://lore.kernel.org/linux-renesas-soc/[email protected]/)
- [2] "Getting To Blinky: Virt Edition / Making device pass-through
work on embedded ARM"
(https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)
---
drivers/gpio/Kconfig | 9 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-virt-agg.c | 390 +++++++++++++++++++++++++++++++++++
3 files changed, 400 insertions(+)
create mode 100644 drivers/gpio/gpio-virt-agg.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index f1f02dac324e52b6..8aff4d9626dee110 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1475,3 +1475,12 @@ config GPIO_MOCKUP
it.

endif
+
+config GPIO_VIRT_AGG
+ tristate "GPIO Virtual Aggregator"
+ depends on GPIOLIB
+ help
+ This enabled the GPIO Virtual Aggregator, which provides a way to
+ aggregate existing GPIOs into a new virtual GPIO device.
+ This is useful for assigning a collection of GPIOs to a user, or
+ exported them to a virtual machine.
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 0a494052c1e845ee..32e885b7f3aa4eee 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -152,6 +152,7 @@ obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o
obj-$(CONFIG_GPIO_UNIPHIER) += gpio-uniphier.o
obj-$(CONFIG_GPIO_VF610) += gpio-vf610.o
obj-$(CONFIG_GPIO_VIPERBOARD) += gpio-viperboard.o
+obj-$(CONFIG_GPIO_VIRT_AGG) += gpio-virt-agg.o
obj-$(CONFIG_GPIO_VR41XX) += gpio-vr41xx.o
obj-$(CONFIG_GPIO_VX855) += gpio-vx855.o
obj-$(CONFIG_GPIO_WHISKEY_COVE) += gpio-wcove.o
diff --git a/drivers/gpio/gpio-virt-agg.c b/drivers/gpio/gpio-virt-agg.c
new file mode 100644
index 0000000000000000..20e5f22beed9d385
--- /dev/null
+++ b/drivers/gpio/gpio-virt-agg.c
@@ -0,0 +1,390 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// GPIO Virtual Aggregator
+//
+// Copyright (C) 2019 Glider bvba
+
+#include <linux/gpio/driver.h>
+#include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+
+#include "gpiolib.h"
+
+#define DRV_NAME "gpio-virt-agg"
+#define MAX_GPIOS 32
+
+struct gpio_virt_agg_entry {
+ struct platform_device *pdev;
+};
+
+struct gpio_virt_agg_priv {
+ struct gpio_chip chip;
+ struct gpio_desc *desc[MAX_GPIOS];
+};
+
+static DEFINE_MUTEX(gpio_virt_agg_lock); /* protects idr */
+static DEFINE_IDR(gpio_virt_agg_idr);
+
+static int gpio_virt_agg_get_direction(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
+
+ return gpiod_get_direction(priv->desc[offset]);
+}
+
+static int gpio_virt_agg_direction_input(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
+
+ return gpiod_direction_input(priv->desc[offset]);
+}
+
+static int gpio_virt_agg_direction_output(struct gpio_chip *chip,
+ unsigned int offset, int value)
+{
+ struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
+
+ return gpiod_direction_output(priv->desc[offset], value);
+}
+
+static int gpio_virt_agg_get(struct gpio_chip *chip, unsigned int offset)
+{
+ struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
+
+ return gpiod_get_value(priv->desc[offset]);
+}
+
+static int gpio_virt_agg_get_multiple(struct gpio_chip *chip,
+ unsigned long *mask, unsigned long *bits)
+{
+ struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
+ DECLARE_BITMAP(values, MAX_GPIOS) = { 0 };
+ struct gpio_desc *desc[MAX_GPIOS];
+ unsigned int i, j = 0;
+ int ret;
+
+ for_each_set_bit(i, mask, priv->chip.ngpio)
+ desc[j++] = priv->desc[i];
+
+ ret = gpiod_get_array_value(j, desc, NULL, values);
+ if (ret)
+ return ret;
+
+ for_each_set_bit(i, mask, priv->chip.ngpio)
+ __assign_bit(i, bits, test_bit(j++, values));
+
+ return 0;
+}
+
+static void gpio_virt_agg_set(struct gpio_chip *chip, unsigned int offset,
+ int value)
+{
+ struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
+
+ gpiod_set_value(priv->desc[offset], value);
+}
+
+static void gpio_virt_agg_set_multiple(struct gpio_chip *chip,
+ unsigned long *mask,
+ unsigned long *bits)
+{
+ struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
+ DECLARE_BITMAP(values, MAX_GPIOS);
+ struct gpio_desc *desc[MAX_GPIOS];
+ unsigned int i, j = 0;
+
+ for_each_set_bit(i, mask, priv->chip.ngpio) {
+ __assign_bit(j, values, test_bit(i, bits));
+ desc[j++] = priv->desc[i];
+ }
+
+ gpiod_set_array_value(j, desc, NULL, values);
+}
+
+static int gpio_virt_agg_set_config(struct gpio_chip *chip,
+ unsigned int offset, unsigned long config)
+{
+ struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
+
+ chip = priv->desc[offset]->gdev->chip;
+ if (chip->set_config)
+ return chip->set_config(chip, offset, config);
+
+ // FIXME gpiod_set_transitory() expects success if not implemented
+ return -ENOTSUPP;
+}
+
+static int gpio_virt_agg_init_valid_mask(struct gpio_chip *chip)
+{
+ struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
+ unsigned int i;
+
+ for (i = 0; i < priv->chip.ngpio; i++) {
+ if (gpiochip_line_is_valid(priv->desc[i]->gdev->chip,
+ gpio_chip_hwgpio(priv->desc[i])))
+ set_bit(i, chip->valid_mask);
+ }
+
+ return 0;
+}
+
+static int gpiochip_match_label(struct gpio_chip *chip, void *data)
+{
+ return !strcmp(chip->label, data);
+}
+
+static struct gpio_chip *gpiochip_find_by_label(const char *label)
+{
+ return gpiochip_find((void *)label, gpiochip_match_label);
+}
+
+static ssize_t new_device_store(struct device_driver *driver, const char *buf,
+ size_t count)
+{
+ struct gpio_virt_agg_entry *gva;
+ struct platform_device *pdev;
+ int res, id;
+
+ gva = kzalloc(sizeof(*gva), GFP_KERNEL);
+ if (!gva)
+ return -ENOMEM;
+
+ mutex_lock(&gpio_virt_agg_lock);
+ id = idr_alloc(&gpio_virt_agg_idr, gva, 0, 0, GFP_KERNEL);
+ mutex_unlock(&gpio_virt_agg_lock);
+
+ if (id < 0) {
+ res = id;
+ goto free_gva;
+ }
+
+ /* kernfs guarantees string termination, so count + 1 is safe */
+ pdev = platform_device_register_data(NULL, DRV_NAME, id, buf,
+ count + 1);
+ if (IS_ERR(pdev)) {
+ res = PTR_ERR(pdev);
+ goto remove_idr;
+ }
+
+ gva->pdev = pdev;
+ return count;
+
+remove_idr:
+ mutex_lock(&gpio_virt_agg_lock);
+ idr_remove(&gpio_virt_agg_idr, id);
+ mutex_unlock(&gpio_virt_agg_lock);
+free_gva:
+ kfree(gva);
+ return res;
+}
+
+static DRIVER_ATTR_WO(new_device);
+
+static ssize_t delete_device_store(struct device_driver *driver,
+ const char *buf, size_t count)
+{
+ struct gpio_virt_agg_entry *gva;
+ int id;
+
+ if (strncmp(buf, DRV_NAME ".", strlen(DRV_NAME ".")))
+ return -EINVAL;
+
+ id = simple_strtoul(buf + strlen(DRV_NAME "."), NULL, 10);
+
+ mutex_lock(&gpio_virt_agg_lock);
+ gva = idr_remove(&gpio_virt_agg_idr, id);
+ mutex_unlock(&gpio_virt_agg_lock);
+
+ if (!gva) {
+ pr_info("Cannot find %s.%d\n", DRV_NAME, id);
+ return -ENOENT;
+ }
+
+ platform_device_unregister(gva->pdev);
+ kfree(gva);
+ return count;
+}
+static DRIVER_ATTR_WO(delete_device);
+
+static struct attribute *gpio_virt_agg_attrs[] = {
+ &driver_attr_new_device.attr,
+ &driver_attr_delete_device.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(gpio_virt_agg);
+
+static int gpio_virt_agg_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ const char *param = dev_get_platdata(dev);
+ struct gpio_virt_agg_priv *priv;
+ const char *label = NULL;
+ struct gpio_chip *chip;
+ struct gpio_desc *desc;
+ unsigned int offset;
+ int error, i;
+ char *s;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv) {
+ error = -ENOMEM;
+ goto fail;
+ }
+
+ for (i = 0; i < MAX_GPIOS; i++) {
+ if (*param == '\0' || *param == '\n')
+ break;
+
+ if (*param == ',') {
+ if (label) {
+ devm_kfree(dev, label);
+ label = NULL;
+ }
+ for (param++; *param == ' '; param++) ;
+ }
+
+ if (!label) {
+ s = strchr(param, ' ');
+ if (!s) {
+ dev_info(dev, "Missing gpiochip\n");
+ error = -EINVAL;
+ goto fail;
+ }
+ label = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
+ (int)(s - param), param);
+ if (!label) {
+ error = -ENOMEM;
+ goto fail;
+ }
+
+ chip = gpiochip_find_by_label(label);
+ if (!chip) {
+ dev_info(dev, "Cannot find gpiochip %s\n",
+ label);
+ error = -ENODEV;
+ goto fail;
+ }
+
+ for (param = s + 1; *param == ' '; param++) ;
+ }
+
+ offset = simple_strtoul(param, &s, 10);
+
+ desc = gpiochip_get_desc(chip, offset);
+ if (IS_ERR(desc)) {
+ error = PTR_ERR(desc);
+ dev_info(dev, "Cannot get GPIO %s/%u: %d\n", label,
+ offset, error);
+ goto fail;
+ }
+
+ error = gpiod_request(desc, dev_name(dev));
+ if (error) {
+ dev_info(dev, "Cannot request GPIO %s/%u: %d\n", label,
+ offset, error);
+ goto fail;
+ }
+
+ dev_dbg(dev, "GPIO %u => %s/%u\n", i, label, offset);
+ priv->desc[i] = desc;
+
+ if (gpiod_cansleep(desc))
+ priv->chip.can_sleep = true;
+ if (desc->gdev->chip->set_config)
+ priv->chip.set_config = gpio_virt_agg_set_config;
+ if (desc->gdev->chip->need_valid_mask) {
+ priv->chip.need_valid_mask = true;
+ priv->chip.init_valid_mask =
+ gpio_virt_agg_init_valid_mask;
+ }
+
+ for (param = s; *param == ' '; param++) ;
+ }
+ if (i == MAX_GPIOS)
+ dev_warn(&pdev->dev,
+ "Too many gpios specified, truncating to %u\n",
+ MAX_GPIOS);
+
+ priv->chip.label = dev_name(dev);
+ priv->chip.parent = dev;
+ priv->chip.owner = THIS_MODULE;
+ priv->chip.get_direction = gpio_virt_agg_get_direction;
+ priv->chip.direction_input = gpio_virt_agg_direction_input;
+ priv->chip.direction_output = gpio_virt_agg_direction_output;
+ priv->chip.get = gpio_virt_agg_get;
+ priv->chip.get_multiple = gpio_virt_agg_get_multiple;
+ priv->chip.set = gpio_virt_agg_set;
+ priv->chip.set_multiple = gpio_virt_agg_set_multiple;
+ priv->chip.base = -1;
+ priv->chip.ngpio = i;
+ platform_set_drvdata(pdev, priv);
+
+ error = gpiochip_add_data(&priv->chip, priv);
+ if (error)
+ goto fail;
+
+ return 0;
+
+fail:
+ while (i-- > 0)
+ gpiod_free(priv->desc[i]);
+
+ return error;
+}
+
+static int gpio_virt_agg_remove(struct platform_device *pdev)
+{
+ struct gpio_virt_agg_priv *priv = platform_get_drvdata(pdev);
+ unsigned int i;
+
+ gpiochip_remove(&priv->chip);
+
+ for (i = 0; i < priv->chip.ngpio; i++)
+ gpiod_free(priv->desc[i]);
+
+ return 0;
+}
+
+static struct platform_driver gpio_virt_agg_driver = {
+ .probe = gpio_virt_agg_probe,
+ .remove = gpio_virt_agg_remove,
+ .driver = {
+ .name = DRV_NAME,
+ .groups = gpio_virt_agg_groups,
+ },
+};
+
+static int __init gpio_virt_agg_init(void)
+{
+ return platform_driver_register(&gpio_virt_agg_driver);
+}
+module_init(gpio_virt_agg_init);
+
+static int __exit gpio_virt_agg_idr_remove(int id, void *p, void *data)
+{
+ struct gpio_virt_agg_entry *gva = p;
+
+ platform_device_unregister(gva->pdev);
+ kfree(gva);
+ return 0;
+}
+
+static void __exit gpio_virt_agg_exit(void)
+{
+ mutex_lock(&gpio_virt_agg_lock);
+ idr_for_each(&gpio_virt_agg_idr, gpio_virt_agg_idr_remove, NULL);
+ idr_destroy(&gpio_virt_agg_idr);
+ mutex_unlock(&gpio_virt_agg_lock);
+
+ platform_driver_unregister(&gpio_virt_agg_driver);
+}
+module_exit(gpio_virt_agg_exit);
+
+MODULE_AUTHOR("Geert Uytterhoeven <[email protected]>");
+MODULE_DESCRIPTION("GPIO Virtual Aggregator");
+MODULE_LICENSE("GPL v2");
--
2.17.1

2019-07-08 14:07:56

by Bartosz Golaszewski

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

pt., 5 lip 2019 o 18:05 Geert Uytterhoeven <[email protected]> napisał(a):
>
> GPIO controllers are exported to userspace using /dev/gpiochip*
> character devices. Access control to these devices is provided by
> standard UNIX file system permissions, on an all-or-nothing basis:
> either a GPIO controller is accessible for a user, or it is not.
> Currently no mechanism exists to control access to individual GPIOs.
>
> Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> and expose them as a new gpiochip. This is useful for implementing
> access control, and assigning a set of GPIOs to a specific user.
> Furthermore, it would simplify and harden exporting GPIOs to a virtual
> machine, as the VM can just grab the full virtual GPIO controller, and
> no longer needs to care about which GPIOs to grab and which not,
> reducing the attack surface.
>
> Virtual GPIO controllers are instantiated by writing to the "new_device"
> attribute file in sysfs:
>
> $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
> "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>
> Likewise, virtual GPIO controllers can be destroyed after use:
>
> $ echo gpio-virt-agg.<N> \
> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> Aggregating GPIOs and exposing them as a new gpiochip was suggested in
> response to my proof-of-concept for GPIO virtualization with QEMU[1][2].
>
> Sample session on r8a7791/koelsch:
>
> - Disable the leds node in arch/arm/boot/dts/r8a7791-koelsch.dts
>
> - Create virtual aggregators:
>
> $ echo "e6052000.gpio 19 20" \
> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>
> gpio-virt-agg gpio-virt-agg.0: GPIO 0 => e6052000.gpio/19
> gpio-virt-agg gpio-virt-agg.0: GPIO 1 => e6052000.gpio/20
> gpiochip_find_base: found new base at 778
> gpio gpiochip8: (gpio-virt-agg.0): added GPIO chardev (254:8)
> gpiochip_setup_dev: registered GPIOs 778 to 779 on device: gpiochip8 (gpio-virt-agg.0)
>
> $ echo "e6052000.gpio 21, e6050000.gpio 20 21 22" \
> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>
> gpio-virt-agg gpio-virt-agg.1: GPIO 0 => e6052000.gpio/21
> gpio-virt-agg gpio-virt-agg.1: GPIO 1 => e6050000.gpio/20
> gpio-virt-agg gpio-virt-agg.1: GPIO 2 => e6050000.gpio/21
> gpio-virt-agg gpio-virt-agg.1: GPIO 3 => e6050000.gpio/22
> gpiochip_find_base: found new base at 774
> gpio gpiochip9: (gpio-virt-agg.1): added GPIO chardev (254:9)
> gpiochip_setup_dev: registered GPIOs 774 to 777 on device: gpiochip9 (gpio-virt-agg.1)
>
> - Adjust permissions on /dev/gpiochip[89] (optional)
>
> - Control LEDs:
>
> $ gpioset gpiochip8 0=0 1=1 # LED6 OFF, LED7 ON
> $ gpioset gpiochip8 0=1 1=0 # LED6 ON, LED7 OFF
> $ gpioset gpiochip9 0=0 # LED8 OFF
> $ gpioset gpiochip9 0=1 # LED8 ON
>
> - Destroy virtual aggregators:
>
> $ echo gpio-virt-agg.0 \
> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> $ echo gpio-virt-agg.1 \
> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
>
> Thanks for your comments!
>

Hi Geert,

I like the general idea and the interface looks mostly fine. Since
this is new ABI I think it needs to be documented as well.

I'm having trouble building this module:

CALL scripts/atomic/check-atomics.sh
CALL scripts/checksyscalls.sh
CHK include/generated/compile.h
Kernel: arch/arm/boot/Image is ready
Building modules, stage 2.
MODPOST 235 modules
ERROR: "gpiod_request" [drivers/gpio/gpio-virt-agg.ko] undefined!
ERROR: "gpiochip_get_desc" [drivers/gpio/gpio-virt-agg.ko] undefined!
ERROR: "gpiod_free" [drivers/gpio/gpio-virt-agg.ko] undefined!
scripts/Makefile.modpost:91: recipe for target '__modpost' failed
make[1]: *** [__modpost] Error 1
Makefile:1287: recipe for target 'modules' failed
make: *** [modules] Error 2
make: *** Waiting for unfinished jobs....

I'm not sure what the problem is.

> References:
> - [1] "[PATCH QEMU POC] Add a GPIO backend"
> (https://lore.kernel.org/linux-renesas-soc/[email protected]/)
> - [2] "Getting To Blinky: Virt Edition / Making device pass-through
> work on embedded ARM"
> (https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)
> ---
> drivers/gpio/Kconfig | 9 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-virt-agg.c | 390 +++++++++++++++++++++++++++++++++++
> 3 files changed, 400 insertions(+)
> create mode 100644 drivers/gpio/gpio-virt-agg.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index f1f02dac324e52b6..8aff4d9626dee110 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1475,3 +1475,12 @@ config GPIO_MOCKUP
> it.
>
> endif
> +
> +config GPIO_VIRT_AGG
> + tristate "GPIO Virtual Aggregator"
> + depends on GPIOLIB
> + help
> + This enabled the GPIO Virtual Aggregator, which provides a way to
> + aggregate existing GPIOs into a new virtual GPIO device.
> + This is useful for assigning a collection of GPIOs to a user, or
> + exported them to a virtual machine.
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 0a494052c1e845ee..32e885b7f3aa4eee 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -152,6 +152,7 @@ obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o
> obj-$(CONFIG_GPIO_UNIPHIER) += gpio-uniphier.o
> obj-$(CONFIG_GPIO_VF610) += gpio-vf610.o
> obj-$(CONFIG_GPIO_VIPERBOARD) += gpio-viperboard.o
> +obj-$(CONFIG_GPIO_VIRT_AGG) += gpio-virt-agg.o
> obj-$(CONFIG_GPIO_VR41XX) += gpio-vr41xx.o
> obj-$(CONFIG_GPIO_VX855) += gpio-vx855.o
> obj-$(CONFIG_GPIO_WHISKEY_COVE) += gpio-wcove.o
> diff --git a/drivers/gpio/gpio-virt-agg.c b/drivers/gpio/gpio-virt-agg.c
> new file mode 100644
> index 0000000000000000..20e5f22beed9d385
> --- /dev/null
> +++ b/drivers/gpio/gpio-virt-agg.c
> @@ -0,0 +1,390 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// GPIO Virtual Aggregator
> +//
> +// Copyright (C) 2019 Glider bvba
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +
> +#include "gpiolib.h"
> +
> +#define DRV_NAME "gpio-virt-agg"
> +#define MAX_GPIOS 32

Do we really need this limit? I see it simplifies the code, but maybe
we can allocate the relevant arrays dynamically and not limit users?

> +
> +struct gpio_virt_agg_entry {
> + struct platform_device *pdev;
> +};
> +
> +struct gpio_virt_agg_priv {
> + struct gpio_chip chip;
> + struct gpio_desc *desc[MAX_GPIOS];
> +};
> +
> +static DEFINE_MUTEX(gpio_virt_agg_lock); /* protects idr */
> +static DEFINE_IDR(gpio_virt_agg_idr);
> +
> +static int gpio_virt_agg_get_direction(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + return gpiod_get_direction(priv->desc[offset]);
> +}
> +
> +static int gpio_virt_agg_direction_input(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + return gpiod_direction_input(priv->desc[offset]);
> +}
> +
> +static int gpio_virt_agg_direction_output(struct gpio_chip *chip,
> + unsigned int offset, int value)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + return gpiod_direction_output(priv->desc[offset], value);
> +}
> +
> +static int gpio_virt_agg_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + return gpiod_get_value(priv->desc[offset]);
> +}
> +
> +static int gpio_virt_agg_get_multiple(struct gpio_chip *chip,
> + unsigned long *mask, unsigned long *bits)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> + DECLARE_BITMAP(values, MAX_GPIOS) = { 0 };
> + struct gpio_desc *desc[MAX_GPIOS];
> + unsigned int i, j = 0;
> + int ret;
> +
> + for_each_set_bit(i, mask, priv->chip.ngpio)
> + desc[j++] = priv->desc[i];
> +
> + ret = gpiod_get_array_value(j, desc, NULL, values);
> + if (ret)
> + return ret;
> +
> + for_each_set_bit(i, mask, priv->chip.ngpio)
> + __assign_bit(i, bits, test_bit(j++, values));
> +
> + return 0;
> +}
> +
> +static void gpio_virt_agg_set(struct gpio_chip *chip, unsigned int offset,
> + int value)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + gpiod_set_value(priv->desc[offset], value);
> +}
> +
> +static void gpio_virt_agg_set_multiple(struct gpio_chip *chip,
> + unsigned long *mask,
> + unsigned long *bits)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> + DECLARE_BITMAP(values, MAX_GPIOS);
> + struct gpio_desc *desc[MAX_GPIOS];
> + unsigned int i, j = 0;
> +
> + for_each_set_bit(i, mask, priv->chip.ngpio) {
> + __assign_bit(j, values, test_bit(i, bits));
> + desc[j++] = priv->desc[i];
> + }
> +
> + gpiod_set_array_value(j, desc, NULL, values);
> +}
> +
> +static int gpio_virt_agg_set_config(struct gpio_chip *chip,
> + unsigned int offset, unsigned long config)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + chip = priv->desc[offset]->gdev->chip;
> + if (chip->set_config)
> + return chip->set_config(chip, offset, config);
> +
> + // FIXME gpiod_set_transitory() expects success if not implemented
> + return -ENOTSUPP;
> +}
> +
> +static int gpio_virt_agg_init_valid_mask(struct gpio_chip *chip)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> + unsigned int i;
> +
> + for (i = 0; i < priv->chip.ngpio; i++) {
> + if (gpiochip_line_is_valid(priv->desc[i]->gdev->chip,
> + gpio_chip_hwgpio(priv->desc[i])))
> + set_bit(i, chip->valid_mask);
> + }
> +
> + return 0;
> +}
> +
> +static int gpiochip_match_label(struct gpio_chip *chip, void *data)
> +{
> + return !strcmp(chip->label, data);
> +}
> +
> +static struct gpio_chip *gpiochip_find_by_label(const char *label)
> +{
> + return gpiochip_find((void *)label, gpiochip_match_label);
> +}
> +
> +static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> + size_t count)
> +{
> + struct gpio_virt_agg_entry *gva;
> + struct platform_device *pdev;
> + int res, id;
> +
> + gva = kzalloc(sizeof(*gva), GFP_KERNEL);
> + if (!gva)
> + return -ENOMEM;
> +
> + mutex_lock(&gpio_virt_agg_lock);
> + id = idr_alloc(&gpio_virt_agg_idr, gva, 0, 0, GFP_KERNEL);
> + mutex_unlock(&gpio_virt_agg_lock);
> +
> + if (id < 0) {
> + res = id;
> + goto free_gva;
> + }
> +
> + /* kernfs guarantees string termination, so count + 1 is safe */
> + pdev = platform_device_register_data(NULL, DRV_NAME, id, buf,
> + count + 1);
> + if (IS_ERR(pdev)) {
> + res = PTR_ERR(pdev);
> + goto remove_idr;
> + }
> +
> + gva->pdev = pdev;
> + return count;
> +
> +remove_idr:
> + mutex_lock(&gpio_virt_agg_lock);
> + idr_remove(&gpio_virt_agg_idr, id);
> + mutex_unlock(&gpio_virt_agg_lock);
> +free_gva:
> + kfree(gva);
> + return res;
> +}
> +
> +static DRIVER_ATTR_WO(new_device);
> +
> +static ssize_t delete_device_store(struct device_driver *driver,
> + const char *buf, size_t count)
> +{
> + struct gpio_virt_agg_entry *gva;
> + int id;
> +
> + if (strncmp(buf, DRV_NAME ".", strlen(DRV_NAME ".")))
> + return -EINVAL;
> +
> + id = simple_strtoul(buf + strlen(DRV_NAME "."), NULL, 10);
> +
> + mutex_lock(&gpio_virt_agg_lock);
> + gva = idr_remove(&gpio_virt_agg_idr, id);
> + mutex_unlock(&gpio_virt_agg_lock);
> +
> + if (!gva) {
> + pr_info("Cannot find %s.%d\n", DRV_NAME, id);
> + return -ENOENT;
> + }
> +
> + platform_device_unregister(gva->pdev);
> + kfree(gva);
> + return count;
> +}
> +static DRIVER_ATTR_WO(delete_device);
> +
> +static struct attribute *gpio_virt_agg_attrs[] = {
> + &driver_attr_new_device.attr,
> + &driver_attr_delete_device.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(gpio_virt_agg);
> +
> +static int gpio_virt_agg_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + const char *param = dev_get_platdata(dev);
> + struct gpio_virt_agg_priv *priv;
> + const char *label = NULL;
> + struct gpio_chip *chip;
> + struct gpio_desc *desc;
> + unsigned int offset;
> + int error, i;

This 'i' here is reported as possibly not initialized:

drivers/gpio/gpio-virt-agg.c: In function ‘gpio_virt_agg_probe’:
drivers/gpio/gpio-virt-agg.c:230:13: warning: ‘i’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
int error, i;
^

> + char *s;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + error = -ENOMEM;
> + goto fail;
> + }
> +
> + for (i = 0; i < MAX_GPIOS; i++) {
> + if (*param == '\0' || *param == '\n')
> + break;
> +
> + if (*param == ',') {
> + if (label) {
> + devm_kfree(dev, label);
> + label = NULL;
> + }
> + for (param++; *param == ' '; param++) ;
> + }
> +
> + if (!label) {
> + s = strchr(param, ' ');
> + if (!s) {
> + dev_info(dev, "Missing gpiochip\n");
> + error = -EINVAL;
> + goto fail;
> + }
> + label = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
> + (int)(s - param), param);
> + if (!label) {
> + error = -ENOMEM;
> + goto fail;
> + }
> +
> + chip = gpiochip_find_by_label(label);
> + if (!chip) {
> + dev_info(dev, "Cannot find gpiochip %s\n",
> + label);
> + error = -ENODEV;
> + goto fail;
> + }
> +
> + for (param = s + 1; *param == ' '; param++) ;
> + }
> +
> + offset = simple_strtoul(param, &s, 10);
> +
> + desc = gpiochip_get_desc(chip, offset);
> + if (IS_ERR(desc)) {
> + error = PTR_ERR(desc);
> + dev_info(dev, "Cannot get GPIO %s/%u: %d\n", label,
> + offset, error);
> + goto fail;
> + }
> +
> + error = gpiod_request(desc, dev_name(dev));
> + if (error) {
> + dev_info(dev, "Cannot request GPIO %s/%u: %d\n", label,
> + offset, error);
> + goto fail;
> + }
> +
> + dev_dbg(dev, "GPIO %u => %s/%u\n", i, label, offset);
> + priv->desc[i] = desc;
> +
> + if (gpiod_cansleep(desc))
> + priv->chip.can_sleep = true;
> + if (desc->gdev->chip->set_config)
> + priv->chip.set_config = gpio_virt_agg_set_config;
> + if (desc->gdev->chip->need_valid_mask) {
> + priv->chip.need_valid_mask = true;
> + priv->chip.init_valid_mask =
> + gpio_virt_agg_init_valid_mask;
> + }
> +
> + for (param = s; *param == ' '; param++) ;
> + }
> + if (i == MAX_GPIOS)
> + dev_warn(&pdev->dev,
> + "Too many gpios specified, truncating to %u\n",
> + MAX_GPIOS);
> +
> + priv->chip.label = dev_name(dev);
> + priv->chip.parent = dev;
> + priv->chip.owner = THIS_MODULE;
> + priv->chip.get_direction = gpio_virt_agg_get_direction;
> + priv->chip.direction_input = gpio_virt_agg_direction_input;
> + priv->chip.direction_output = gpio_virt_agg_direction_output;
> + priv->chip.get = gpio_virt_agg_get;
> + priv->chip.get_multiple = gpio_virt_agg_get_multiple;
> + priv->chip.set = gpio_virt_agg_set;
> + priv->chip.set_multiple = gpio_virt_agg_set_multiple;
> + priv->chip.base = -1;
> + priv->chip.ngpio = i;
> + platform_set_drvdata(pdev, priv);
> +
> + error = gpiochip_add_data(&priv->chip, priv);
> + if (error)
> + goto fail;
> +
> + return 0;
> +
> +fail:
> + while (i-- > 0)
> + gpiod_free(priv->desc[i]);
> +
> + return error;
> +}
> +
> +static int gpio_virt_agg_remove(struct platform_device *pdev)
> +{
> + struct gpio_virt_agg_priv *priv = platform_get_drvdata(pdev);
> + unsigned int i;
> +
> + gpiochip_remove(&priv->chip);
> +
> + for (i = 0; i < priv->chip.ngpio; i++)
> + gpiod_free(priv->desc[i]);
> +
> + return 0;
> +}

You shouldn't need this function at all. It's up to users to free descriptors.

Best regards,
Bartosz Golaszewski

> +
> +static struct platform_driver gpio_virt_agg_driver = {
> + .probe = gpio_virt_agg_probe,
> + .remove = gpio_virt_agg_remove,
> + .driver = {
> + .name = DRV_NAME,
> + .groups = gpio_virt_agg_groups,
> + },
> +};
> +
> +static int __init gpio_virt_agg_init(void)
> +{
> + return platform_driver_register(&gpio_virt_agg_driver);
> +}
> +module_init(gpio_virt_agg_init);
> +
> +static int __exit gpio_virt_agg_idr_remove(int id, void *p, void *data)
> +{
> + struct gpio_virt_agg_entry *gva = p;
> +
> + platform_device_unregister(gva->pdev);
> + kfree(gva);
> + return 0;
> +}
> +
> +static void __exit gpio_virt_agg_exit(void)
> +{
> + mutex_lock(&gpio_virt_agg_lock);
> + idr_for_each(&gpio_virt_agg_idr, gpio_virt_agg_idr_remove, NULL);
> + idr_destroy(&gpio_virt_agg_idr);
> + mutex_unlock(&gpio_virt_agg_lock);
> +
> + platform_driver_unregister(&gpio_virt_agg_driver);
> +}
> +module_exit(gpio_virt_agg_exit);
> +
> +MODULE_AUTHOR("Geert Uytterhoeven <[email protected]>");
> +MODULE_DESCRIPTION("GPIO Virtual Aggregator");
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1
>

2019-07-08 14:41:37

by Geert Uytterhoeven

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

Hi Bartosz,

On Mon, Jul 8, 2019 at 11:45 AM Bartosz Golaszewski
<[email protected]> wrote:
> pt., 5 lip 2019 o 18:05 Geert Uytterhoeven <[email protected]> napisał(a):
> > GPIO controllers are exported to userspace using /dev/gpiochip*
> > character devices. Access control to these devices is provided by
> > standard UNIX file system permissions, on an all-or-nothing basis:
> > either a GPIO controller is accessible for a user, or it is not.
> > Currently no mechanism exists to control access to individual GPIOs.
> >
> > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> > and expose them as a new gpiochip. This is useful for implementing
> > access control, and assigning a set of GPIOs to a specific user.
> > Furthermore, it would simplify and harden exporting GPIOs to a virtual
> > machine, as the VM can just grab the full virtual GPIO controller, and
> > no longer needs to care about which GPIOs to grab and which not,
> > reducing the attack surface.
> >
> > Virtual GPIO controllers are instantiated by writing to the "new_device"
> > attribute file in sysfs:
> >
> > $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
> > "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
> > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> >
> > Likewise, virtual GPIO controllers can be destroyed after use:
> >
> > $ echo gpio-virt-agg.<N> \
> > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> >
> > Signed-off-by: Geert Uytterhoeven <[email protected]>

> I like the general idea and the interface looks mostly fine. Since
> this is new ABI I think it needs to be documented as well.

Sure.

> I'm having trouble building this module:
>
> CALL scripts/atomic/check-atomics.sh
> CALL scripts/checksyscalls.sh
> CHK include/generated/compile.h
> Kernel: arch/arm/boot/Image is ready
> Building modules, stage 2.
> MODPOST 235 modules
> ERROR: "gpiod_request" [drivers/gpio/gpio-virt-agg.ko] undefined!
> ERROR: "gpiochip_get_desc" [drivers/gpio/gpio-virt-agg.ko] undefined!
> ERROR: "gpiod_free" [drivers/gpio/gpio-virt-agg.ko] undefined!
> scripts/Makefile.modpost:91: recipe for target '__modpost' failed
> make[1]: *** [__modpost] Error 1
> Makefile:1287: recipe for target 'modules' failed
> make: *** [modules] Error 2
> make: *** Waiting for unfinished jobs....
>
> I'm not sure what the problem is.

Oops. As this is an RFC, I didn't bother trying to build this driver as
a module, only builtin. Apparently the 3 symbols above are not yet
exported using EXPORT_SYMBOL_GPL().

> > --- /dev/null
> > +++ b/drivers/gpio/gpio-virt-agg.c
> > @@ -0,0 +1,390 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +//
> > +// GPIO Virtual Aggregator
> > +//
> > +// Copyright (C) 2019 Glider bvba
> > +
> > +#include <linux/gpio/driver.h>
> > +#include <linux/idr.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "gpiolib.h"
> > +
> > +#define DRV_NAME "gpio-virt-agg"
> > +#define MAX_GPIOS 32
>
> Do we really need this limit? I see it simplifies the code, but maybe
> we can allocate the relevant arrays dynamically and not limit users?

Sure. That limit can be lifted.

> > +static int gpio_virt_agg_set_config(struct gpio_chip *chip,
> > + unsigned int offset, unsigned long config)
> > +{
> > + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> > +
> > + chip = priv->desc[offset]->gdev->chip;
> > + if (chip->set_config)
> > + return chip->set_config(chip, offset, config);
> > +
> > + // FIXME gpiod_set_transitory() expects success if not implemented

BTW, do you have a comment about this FIXME?

> > + return -ENOTSUPP;
> > +}

> > +static int gpio_virt_agg_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + const char *param = dev_get_platdata(dev);
> > + struct gpio_virt_agg_priv *priv;
> > + const char *label = NULL;
> > + struct gpio_chip *chip;
> > + struct gpio_desc *desc;
> > + unsigned int offset;
> > + int error, i;
>
> This 'i' here is reported as possibly not initialized:
>
> drivers/gpio/gpio-virt-agg.c: In function ‘gpio_virt_agg_probe’:
> drivers/gpio/gpio-virt-agg.c:230:13: warning: ‘i’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> int error, i;
> ^

Oops, should be preinitialized to zero. WIll fix.

> > +static int gpio_virt_agg_remove(struct platform_device *pdev)
> > +{
> > + struct gpio_virt_agg_priv *priv = platform_get_drvdata(pdev);
> > + unsigned int i;
> > +
> > + gpiochip_remove(&priv->chip);
> > +
> > + for (i = 0; i < priv->chip.ngpio; i++)
> > + gpiod_free(priv->desc[i]);

Perhaps I should use gpiod_put() instead, which is exported to modules?

> > +
> > + return 0;
> > +}
>
> You shouldn't need this function at all. It's up to users to free descriptors.

This frees the upstream descriptors, not the descriptors used by users
of the virtual gpiochip. Shouldn't they be freed, as they are no longer
in use?

Note that .probe() doesn't use devm_gpiochip_add_data(), as the upstream
descriptors need to be freed after the call to gpiochip_remove().

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2019-07-09 14:59:37

by Bartosz Golaszewski

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

pon., 8 lip 2019 o 12:24 Geert Uytterhoeven <[email protected]> napisał(a):
>
> Hi Bartosz,
>
> On Mon, Jul 8, 2019 at 11:45 AM Bartosz Golaszewski
> <[email protected]> wrote:
> > pt., 5 lip 2019 o 18:05 Geert Uytterhoeven <[email protected]> napisał(a):
> > > GPIO controllers are exported to userspace using /dev/gpiochip*
> > > character devices. Access control to these devices is provided by
> > > standard UNIX file system permissions, on an all-or-nothing basis:
> > > either a GPIO controller is accessible for a user, or it is not.
> > > Currently no mechanism exists to control access to individual GPIOs.
> > >
> > > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> > > and expose them as a new gpiochip. This is useful for implementing
> > > access control, and assigning a set of GPIOs to a specific user.
> > > Furthermore, it would simplify and harden exporting GPIOs to a virtual
> > > machine, as the VM can just grab the full virtual GPIO controller, and
> > > no longer needs to care about which GPIOs to grab and which not,
> > > reducing the attack surface.
> > >
> > > Virtual GPIO controllers are instantiated by writing to the "new_device"
> > > attribute file in sysfs:
> > >
> > > $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
> > > "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
> > > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> > >
> > > Likewise, virtual GPIO controllers can be destroyed after use:
> > >
> > > $ echo gpio-virt-agg.<N> \
> > > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> > >
> > > Signed-off-by: Geert Uytterhoeven <[email protected]>
>
> > I like the general idea and the interface looks mostly fine. Since
> > this is new ABI I think it needs to be documented as well.
>
> Sure.
>
> > I'm having trouble building this module:
> >
> > CALL scripts/atomic/check-atomics.sh
> > CALL scripts/checksyscalls.sh
> > CHK include/generated/compile.h
> > Kernel: arch/arm/boot/Image is ready
> > Building modules, stage 2.
> > MODPOST 235 modules
> > ERROR: "gpiod_request" [drivers/gpio/gpio-virt-agg.ko] undefined!
> > ERROR: "gpiochip_get_desc" [drivers/gpio/gpio-virt-agg.ko] undefined!
> > ERROR: "gpiod_free" [drivers/gpio/gpio-virt-agg.ko] undefined!
> > scripts/Makefile.modpost:91: recipe for target '__modpost' failed
> > make[1]: *** [__modpost] Error 1
> > Makefile:1287: recipe for target 'modules' failed
> > make: *** [modules] Error 2
> > make: *** Waiting for unfinished jobs....
> >
> > I'm not sure what the problem is.
>
> Oops. As this is an RFC, I didn't bother trying to build this driver as
> a module, only builtin. Apparently the 3 symbols above are not yet
> exported using EXPORT_SYMBOL_GPL().
>

Am I doing it right? I'm trying to create a device and am only getting this:

# echo gpiochip2 23 > new_device
[ 707.507039] gpio-virt-agg gpio-virt-agg.0: Cannot find gpiochip gpiochip2

gpiochip2 *does* exist in the system.

> > > --- /dev/null
> > > +++ b/drivers/gpio/gpio-virt-agg.c
> > > @@ -0,0 +1,390 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +//
> > > +// GPIO Virtual Aggregator
> > > +//
> > > +// Copyright (C) 2019 Glider bvba
> > > +
> > > +#include <linux/gpio/driver.h>
> > > +#include <linux/idr.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#include "gpiolib.h"
> > > +
> > > +#define DRV_NAME "gpio-virt-agg"
> > > +#define MAX_GPIOS 32
> >
> > Do we really need this limit? I see it simplifies the code, but maybe
> > we can allocate the relevant arrays dynamically and not limit users?
>
> Sure. That limit can be lifted.
>
> > > +static int gpio_virt_agg_set_config(struct gpio_chip *chip,
> > > + unsigned int offset, unsigned long config)
> > > +{
> > > + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> > > +
> > > + chip = priv->desc[offset]->gdev->chip;
> > > + if (chip->set_config)
> > > + return chip->set_config(chip, offset, config);
> > > +
> > > + // FIXME gpiod_set_transitory() expects success if not implemented
>
> BTW, do you have a comment about this FIXME?
>

Ha! Interesting. I'll give it a thought and respond elsewhere as it's
a different subject.

> > > + return -ENOTSUPP;
> > > +}
>
> > > +static int gpio_virt_agg_probe(struct platform_device *pdev)
> > > +{
> > > + struct device *dev = &pdev->dev;
> > > + const char *param = dev_get_platdata(dev);
> > > + struct gpio_virt_agg_priv *priv;
> > > + const char *label = NULL;
> > > + struct gpio_chip *chip;
> > > + struct gpio_desc *desc;
> > > + unsigned int offset;
> > > + int error, i;
> >
> > This 'i' here is reported as possibly not initialized:
> >
> > drivers/gpio/gpio-virt-agg.c: In function ‘gpio_virt_agg_probe’:
> > drivers/gpio/gpio-virt-agg.c:230:13: warning: ‘i’ may be used
> > uninitialized in this function [-Wmaybe-uninitialized]
> > int error, i;
> > ^
>
> Oops, should be preinitialized to zero. WIll fix.
>
> > > +static int gpio_virt_agg_remove(struct platform_device *pdev)
> > > +{
> > > + struct gpio_virt_agg_priv *priv = platform_get_drvdata(pdev);
> > > + unsigned int i;
> > > +
> > > + gpiochip_remove(&priv->chip);
> > > +
> > > + for (i = 0; i < priv->chip.ngpio; i++)
> > > + gpiod_free(priv->desc[i]);
>
> Perhaps I should use gpiod_put() instead, which is exported to modules?
>
> > > +
> > > + return 0;
> > > +}
> >
> > You shouldn't need this function at all. It's up to users to free descriptors.
>
> This frees the upstream descriptors, not the descriptors used by users
> of the virtual gpiochip. Shouldn't they be freed, as they are no longer
> in use?
>
> Note that .probe() doesn't use devm_gpiochip_add_data(), as the upstream
> descriptors need to be freed after the call to gpiochip_remove().
>
> Thanks!

I see. I'll try to review it more thoroughly once I get to play with
it. So far I'm stuck on creating the virtual chip.

Bart

>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds

2019-07-09 16:00:33

by Geert Uytterhoeven

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

Hi Bartosz,

On Tue, Jul 9, 2019 at 4:59 PM Bartosz Golaszewski
<[email protected]> wrote:
> pon., 8 lip 2019 o 12:24 Geert Uytterhoeven <[email protected]> napisał(a):
> > On Mon, Jul 8, 2019 at 11:45 AM Bartosz Golaszewski
> > <[email protected]> wrote:
> > > pt., 5 lip 2019 o 18:05 Geert Uytterhoeven <[email protected]> napisał(a):
> > > > GPIO controllers are exported to userspace using /dev/gpiochip*
> > > > character devices. Access control to these devices is provided by
> > > > standard UNIX file system permissions, on an all-or-nothing basis:
> > > > either a GPIO controller is accessible for a user, or it is not.
> > > > Currently no mechanism exists to control access to individual GPIOs.
> > > >
> > > > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> > > > and expose them as a new gpiochip. This is useful for implementing
> > > > access control, and assigning a set of GPIOs to a specific user.
> > > > Furthermore, it would simplify and harden exporting GPIOs to a virtual
> > > > machine, as the VM can just grab the full virtual GPIO controller, and
> > > > no longer needs to care about which GPIOs to grab and which not,
> > > > reducing the attack surface.
> > > >
> > > > Virtual GPIO controllers are instantiated by writing to the "new_device"
> > > > attribute file in sysfs:
> > > >
> > > > $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
> > > > "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
> > > > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> > > >
> > > > Likewise, virtual GPIO controllers can be destroyed after use:
> > > >
> > > > $ echo gpio-virt-agg.<N> \
> > > > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device

> Am I doing it right? I'm trying to create a device and am only getting this:
>
> # echo gpiochip2 23 > new_device
> [ 707.507039] gpio-virt-agg gpio-virt-agg.0: Cannot find gpiochip gpiochip2
>
> gpiochip2 *does* exist in the system.

Please try the name of the platform device instead.
I.e. for my koelsch (R-Car M2-W), it needs "e6052000.gpio" instead
of "gpiochip2".

Probably the driver should match on both.

> I see. I'll try to review it more thoroughly once I get to play with
> it. So far I'm stuck on creating the virtual chip.

Thanks, good luck!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2019-07-10 02:11:21

by Phil Reid

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

G'day Geert,

On 6/07/2019 00:05, Geert Uytterhoeven wrote:
> GPIO controllers are exported to userspace using /dev/gpiochip*
> character devices. Access control to these devices is provided by
> standard UNIX file system permissions, on an all-or-nothing basis:
> either a GPIO controller is accessible for a user, or it is not.
> Currently no mechanism exists to control access to individual GPIOs.
>
> Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> and expose them as a new gpiochip. This is useful for implementing
> access control, and assigning a set of GPIOs to a specific user.
> Furthermore, it would simplify and harden exporting GPIOs to a virtual
> machine, as the VM can just grab the full virtual GPIO controller, and
> no longer needs to care about which GPIOs to grab and which not,
> reducing the attack surface.
>
> Virtual GPIO controllers are instantiated by writing to the "new_device"
> attribute file in sysfs:
>
> $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
> "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>
> Likewise, virtual GPIO controllers can be destroyed after use:
>
> $ echo gpio-virt-agg.<N> \
> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
>

Nice.
This provides similar functionality to the "gpio inverter" driver currently on the list.
Other than being just a buffer.

Would it be possible to do the lookup via line names?



> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> Aggregating GPIOs and exposing them as a new gpiochip was suggested in
> response to my proof-of-concept for GPIO virtualization with QEMU[1][2].
>
> Sample session on r8a7791/koelsch:
>
> - Disable the leds node in arch/arm/boot/dts/r8a7791-koelsch.dts
>
> - Create virtual aggregators:
>
> $ echo "e6052000.gpio 19 20" \
> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>
> gpio-virt-agg gpio-virt-agg.0: GPIO 0 => e6052000.gpio/19
> gpio-virt-agg gpio-virt-agg.0: GPIO 1 => e6052000.gpio/20
> gpiochip_find_base: found new base at 778
> gpio gpiochip8: (gpio-virt-agg.0): added GPIO chardev (254:8)
> gpiochip_setup_dev: registered GPIOs 778 to 779 on device: gpiochip8 (gpio-virt-agg.0)
>
> $ echo "e6052000.gpio 21, e6050000.gpio 20 21 22" \
> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>
> gpio-virt-agg gpio-virt-agg.1: GPIO 0 => e6052000.gpio/21
> gpio-virt-agg gpio-virt-agg.1: GPIO 1 => e6050000.gpio/20
> gpio-virt-agg gpio-virt-agg.1: GPIO 2 => e6050000.gpio/21
> gpio-virt-agg gpio-virt-agg.1: GPIO 3 => e6050000.gpio/22
> gpiochip_find_base: found new base at 774
> gpio gpiochip9: (gpio-virt-agg.1): added GPIO chardev (254:9)
> gpiochip_setup_dev: registered GPIOs 774 to 777 on device: gpiochip9 (gpio-virt-agg.1)
>
> - Adjust permissions on /dev/gpiochip[89] (optional)
>
> - Control LEDs:
>
> $ gpioset gpiochip8 0=0 1=1 # LED6 OFF, LED7 ON
> $ gpioset gpiochip8 0=1 1=0 # LED6 ON, LED7 OFF
> $ gpioset gpiochip9 0=0 # LED8 OFF
> $ gpioset gpiochip9 0=1 # LED8 ON
>
> - Destroy virtual aggregators:
>
> $ echo gpio-virt-agg.0 \
> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> $ echo gpio-virt-agg.1 \
> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
>
> Thanks for your comments!
>
> References:
> - [1] "[PATCH QEMU POC] Add a GPIO backend"
> (https://lore.kernel.org/linux-renesas-soc/[email protected]/)
> - [2] "Getting To Blinky: Virt Edition / Making device pass-through
> work on embedded ARM"
> (https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)
> ---
> drivers/gpio/Kconfig | 9 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-virt-agg.c | 390 +++++++++++++++++++++++++++++++++++
> 3 files changed, 400 insertions(+)
> create mode 100644 drivers/gpio/gpio-virt-agg.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index f1f02dac324e52b6..8aff4d9626dee110 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1475,3 +1475,12 @@ config GPIO_MOCKUP
> it.
>
> endif
> +
> +config GPIO_VIRT_AGG
> + tristate "GPIO Virtual Aggregator"
> + depends on GPIOLIB
> + help
> + This enabled the GPIO Virtual Aggregator, which provides a way to
> + aggregate existing GPIOs into a new virtual GPIO device.
> + This is useful for assigning a collection of GPIOs to a user, or
> + exported them to a virtual machine.
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 0a494052c1e845ee..32e885b7f3aa4eee 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -152,6 +152,7 @@ obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o
> obj-$(CONFIG_GPIO_UNIPHIER) += gpio-uniphier.o
> obj-$(CONFIG_GPIO_VF610) += gpio-vf610.o
> obj-$(CONFIG_GPIO_VIPERBOARD) += gpio-viperboard.o
> +obj-$(CONFIG_GPIO_VIRT_AGG) += gpio-virt-agg.o
> obj-$(CONFIG_GPIO_VR41XX) += gpio-vr41xx.o
> obj-$(CONFIG_GPIO_VX855) += gpio-vx855.o
> obj-$(CONFIG_GPIO_WHISKEY_COVE) += gpio-wcove.o
> diff --git a/drivers/gpio/gpio-virt-agg.c b/drivers/gpio/gpio-virt-agg.c
> new file mode 100644
> index 0000000000000000..20e5f22beed9d385
> --- /dev/null
> +++ b/drivers/gpio/gpio-virt-agg.c
> @@ -0,0 +1,390 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// GPIO Virtual Aggregator
> +//
> +// Copyright (C) 2019 Glider bvba
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +
> +#include "gpiolib.h"
> +
> +#define DRV_NAME "gpio-virt-agg"
> +#define MAX_GPIOS 32
> +
> +struct gpio_virt_agg_entry {
> + struct platform_device *pdev;
> +};
> +
> +struct gpio_virt_agg_priv {
> + struct gpio_chip chip;
> + struct gpio_desc *desc[MAX_GPIOS];
> +};
> +
> +static DEFINE_MUTEX(gpio_virt_agg_lock); /* protects idr */
> +static DEFINE_IDR(gpio_virt_agg_idr);
> +
> +static int gpio_virt_agg_get_direction(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + return gpiod_get_direction(priv->desc[offset]);
> +}
> +
> +static int gpio_virt_agg_direction_input(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + return gpiod_direction_input(priv->desc[offset]);
> +}
> +
> +static int gpio_virt_agg_direction_output(struct gpio_chip *chip,
> + unsigned int offset, int value)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + return gpiod_direction_output(priv->desc[offset], value);
> +}
> +
> +static int gpio_virt_agg_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + return gpiod_get_value(priv->desc[offset]);
> +}
> +
> +static int gpio_virt_agg_get_multiple(struct gpio_chip *chip,
> + unsigned long *mask, unsigned long *bits)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> + DECLARE_BITMAP(values, MAX_GPIOS) = { 0 };
> + struct gpio_desc *desc[MAX_GPIOS];
> + unsigned int i, j = 0;
> + int ret;
> +
> + for_each_set_bit(i, mask, priv->chip.ngpio)
> + desc[j++] = priv->desc[i];
> +
> + ret = gpiod_get_array_value(j, desc, NULL, values);
> + if (ret)
> + return ret;
> +
> + for_each_set_bit(i, mask, priv->chip.ngpio)
> + __assign_bit(i, bits, test_bit(j++, values));
> +
> + return 0;
> +}
> +
> +static void gpio_virt_agg_set(struct gpio_chip *chip, unsigned int offset,
> + int value)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + gpiod_set_value(priv->desc[offset], value);
> +}
> +
> +static void gpio_virt_agg_set_multiple(struct gpio_chip *chip,
> + unsigned long *mask,
> + unsigned long *bits)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> + DECLARE_BITMAP(values, MAX_GPIOS);
> + struct gpio_desc *desc[MAX_GPIOS];
> + unsigned int i, j = 0;
> +
> + for_each_set_bit(i, mask, priv->chip.ngpio) {
> + __assign_bit(j, values, test_bit(i, bits));
> + desc[j++] = priv->desc[i];
> + }
> +
> + gpiod_set_array_value(j, desc, NULL, values);
> +}
> +
> +static int gpio_virt_agg_set_config(struct gpio_chip *chip,
> + unsigned int offset, unsigned long config)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + chip = priv->desc[offset]->gdev->chip;
> + if (chip->set_config)
> + return chip->set_config(chip, offset, config);
> +
> + // FIXME gpiod_set_transitory() expects success if not implemented
> + return -ENOTSUPP;
> +}
> +
> +static int gpio_virt_agg_init_valid_mask(struct gpio_chip *chip)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> + unsigned int i;
> +
> + for (i = 0; i < priv->chip.ngpio; i++) {
> + if (gpiochip_line_is_valid(priv->desc[i]->gdev->chip,
> + gpio_chip_hwgpio(priv->desc[i])))
> + set_bit(i, chip->valid_mask);
> + }
> +
> + return 0;
> +}
> +
> +static int gpiochip_match_label(struct gpio_chip *chip, void *data)
> +{
> + return !strcmp(chip->label, data);
> +}
> +
> +static struct gpio_chip *gpiochip_find_by_label(const char *label)
> +{
> + return gpiochip_find((void *)label, gpiochip_match_label);
> +}
> +
> +static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> + size_t count)
> +{
> + struct gpio_virt_agg_entry *gva;
> + struct platform_device *pdev;
> + int res, id;
> +
> + gva = kzalloc(sizeof(*gva), GFP_KERNEL);
> + if (!gva)
> + return -ENOMEM;
> +
> + mutex_lock(&gpio_virt_agg_lock);
> + id = idr_alloc(&gpio_virt_agg_idr, gva, 0, 0, GFP_KERNEL);
> + mutex_unlock(&gpio_virt_agg_lock);
> +
> + if (id < 0) {
> + res = id;
> + goto free_gva;
> + }
> +
> + /* kernfs guarantees string termination, so count + 1 is safe */
> + pdev = platform_device_register_data(NULL, DRV_NAME, id, buf,
> + count + 1);
> + if (IS_ERR(pdev)) {
> + res = PTR_ERR(pdev);
> + goto remove_idr;
> + }
> +
> + gva->pdev = pdev;
> + return count;
> +
> +remove_idr:
> + mutex_lock(&gpio_virt_agg_lock);
> + idr_remove(&gpio_virt_agg_idr, id);
> + mutex_unlock(&gpio_virt_agg_lock);
> +free_gva:
> + kfree(gva);
> + return res;
> +}
> +
> +static DRIVER_ATTR_WO(new_device);
> +
> +static ssize_t delete_device_store(struct device_driver *driver,
> + const char *buf, size_t count)
> +{
> + struct gpio_virt_agg_entry *gva;
> + int id;
> +
> + if (strncmp(buf, DRV_NAME ".", strlen(DRV_NAME ".")))
> + return -EINVAL;
> +
> + id = simple_strtoul(buf + strlen(DRV_NAME "."), NULL, 10);
> +
> + mutex_lock(&gpio_virt_agg_lock);
> + gva = idr_remove(&gpio_virt_agg_idr, id);
> + mutex_unlock(&gpio_virt_agg_lock);
> +
> + if (!gva) {
> + pr_info("Cannot find %s.%d\n", DRV_NAME, id);
> + return -ENOENT;
> + }
> +
> + platform_device_unregister(gva->pdev);
> + kfree(gva);
> + return count;
> +}
> +static DRIVER_ATTR_WO(delete_device);
> +
> +static struct attribute *gpio_virt_agg_attrs[] = {
> + &driver_attr_new_device.attr,
> + &driver_attr_delete_device.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(gpio_virt_agg);
> +
> +static int gpio_virt_agg_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + const char *param = dev_get_platdata(dev);
> + struct gpio_virt_agg_priv *priv;
> + const char *label = NULL;
> + struct gpio_chip *chip;
> + struct gpio_desc *desc;
> + unsigned int offset;
> + int error, i;
> + char *s;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + error = -ENOMEM;
> + goto fail;
> + }
> +
> + for (i = 0; i < MAX_GPIOS; i++) {
> + if (*param == '\0' || *param == '\n')
> + break;
> +
> + if (*param == ',') {
> + if (label) {
> + devm_kfree(dev, label);
> + label = NULL;
> + }
> + for (param++; *param == ' '; param++) ;
> + }
> +
> + if (!label) {
> + s = strchr(param, ' ');
> + if (!s) {
> + dev_info(dev, "Missing gpiochip\n");
> + error = -EINVAL;
> + goto fail;
> + }
> + label = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
> + (int)(s - param), param);
> + if (!label) {
> + error = -ENOMEM;
> + goto fail;
> + }
> +
> + chip = gpiochip_find_by_label(label);
> + if (!chip) {
> + dev_info(dev, "Cannot find gpiochip %s\n",
> + label);
> + error = -ENODEV;
> + goto fail;
> + }
> +
> + for (param = s + 1; *param == ' '; param++) ;
> + }
> +
> + offset = simple_strtoul(param, &s, 10);
> +
> + desc = gpiochip_get_desc(chip, offset);
> + if (IS_ERR(desc)) {
> + error = PTR_ERR(desc);
> + dev_info(dev, "Cannot get GPIO %s/%u: %d\n", label,
> + offset, error);
> + goto fail;
> + }
> +
> + error = gpiod_request(desc, dev_name(dev));
> + if (error) {
> + dev_info(dev, "Cannot request GPIO %s/%u: %d\n", label,
> + offset, error);
> + goto fail;
> + }
> +
> + dev_dbg(dev, "GPIO %u => %s/%u\n", i, label, offset);
> + priv->desc[i] = desc;
> +
> + if (gpiod_cansleep(desc))
> + priv->chip.can_sleep = true;
> + if (desc->gdev->chip->set_config)
> + priv->chip.set_config = gpio_virt_agg_set_config;
> + if (desc->gdev->chip->need_valid_mask) {
> + priv->chip.need_valid_mask = true;
> + priv->chip.init_valid_mask =
> + gpio_virt_agg_init_valid_mask;
> + }
> +
> + for (param = s; *param == ' '; param++) ;
> + }
> + if (i == MAX_GPIOS)
> + dev_warn(&pdev->dev,
> + "Too many gpios specified, truncating to %u\n",
> + MAX_GPIOS);
> +
> + priv->chip.label = dev_name(dev);
> + priv->chip.parent = dev;
> + priv->chip.owner = THIS_MODULE;
> + priv->chip.get_direction = gpio_virt_agg_get_direction;
> + priv->chip.direction_input = gpio_virt_agg_direction_input;
> + priv->chip.direction_output = gpio_virt_agg_direction_output;
> + priv->chip.get = gpio_virt_agg_get;
> + priv->chip.get_multiple = gpio_virt_agg_get_multiple;
> + priv->chip.set = gpio_virt_agg_set;
> + priv->chip.set_multiple = gpio_virt_agg_set_multiple;
> + priv->chip.base = -1;
> + priv->chip.ngpio = i;
> + platform_set_drvdata(pdev, priv);
> +
> + error = gpiochip_add_data(&priv->chip, priv);
> + if (error)
> + goto fail;
> +
> + return 0;
> +
> +fail:
> + while (i-- > 0)
> + gpiod_free(priv->desc[i]);
> +
> + return error;
> +}
> +
> +static int gpio_virt_agg_remove(struct platform_device *pdev)
> +{
> + struct gpio_virt_agg_priv *priv = platform_get_drvdata(pdev);
> + unsigned int i;
> +
> + gpiochip_remove(&priv->chip);
> +
> + for (i = 0; i < priv->chip.ngpio; i++)
> + gpiod_free(priv->desc[i]);
> +
> + return 0;
> +}
> +
> +static struct platform_driver gpio_virt_agg_driver = {
> + .probe = gpio_virt_agg_probe,
> + .remove = gpio_virt_agg_remove,
> + .driver = {
> + .name = DRV_NAME,
> + .groups = gpio_virt_agg_groups,
> + },
> +};
> +
> +static int __init gpio_virt_agg_init(void)
> +{
> + return platform_driver_register(&gpio_virt_agg_driver);
> +}
> +module_init(gpio_virt_agg_init);
> +
> +static int __exit gpio_virt_agg_idr_remove(int id, void *p, void *data)
> +{
> + struct gpio_virt_agg_entry *gva = p;
> +
> + platform_device_unregister(gva->pdev);
> + kfree(gva);
> + return 0;
> +}
> +
> +static void __exit gpio_virt_agg_exit(void)
> +{
> + mutex_lock(&gpio_virt_agg_lock);
> + idr_for_each(&gpio_virt_agg_idr, gpio_virt_agg_idr_remove, NULL);
> + idr_destroy(&gpio_virt_agg_idr);
> + mutex_unlock(&gpio_virt_agg_lock);
> +
> + platform_driver_unregister(&gpio_virt_agg_driver);
> +}
> +module_exit(gpio_virt_agg_exit);
> +
> +MODULE_AUTHOR("Geert Uytterhoeven <[email protected]>");
> +MODULE_DESCRIPTION("GPIO Virtual Aggregator");
> +MODULE_LICENSE("GPL v2");
>


--
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
http://www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: [email protected]

2019-07-10 10:23:26

by Geert Uytterhoeven

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

Hi Phil,

On Wed, Jul 10, 2019 at 4:00 AM Phil Reid <[email protected]> wrote:
> On 6/07/2019 00:05, Geert Uytterhoeven wrote:
> > GPIO controllers are exported to userspace using /dev/gpiochip*
> > character devices. Access control to these devices is provided by
> > standard UNIX file system permissions, on an all-or-nothing basis:
> > either a GPIO controller is accessible for a user, or it is not.
> > Currently no mechanism exists to control access to individual GPIOs.
> >
> > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> > and expose them as a new gpiochip. This is useful for implementing
> > access control, and assigning a set of GPIOs to a specific user.
> > Furthermore, it would simplify and harden exporting GPIOs to a virtual
> > machine, as the VM can just grab the full virtual GPIO controller, and
> > no longer needs to care about which GPIOs to grab and which not,
> > reducing the attack surface.
> >
> > Virtual GPIO controllers are instantiated by writing to the "new_device"
> > attribute file in sysfs:
> >
> > $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
> > "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
> > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> >
> > Likewise, virtual GPIO controllers can be destroyed after use:
> >
> > $ echo gpio-virt-agg.<N> \
> > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> >
>
> Nice.
> This provides similar functionality to the "gpio inverter" driver currently on the list.
> Other than being just a buffer.

Indeed, both drivers forward GPIO calls, but the gpio inverter modifies
some parameters passed.

The way the drivers obtain references to GPIOs is different, though: the
inverter driver obtains a fixed description from DT, while the virtual
aggregator receives the description at runtime, from sysfs.

But perhaps both drivers could share some code?

> Would it be possible to do the lookup via line names?

Doesn't the fact that a GPIO has a line name means that it is in use, and
thus cannot be aggregated and exported to another user?

Thanks!


Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2019-07-11 09:27:02

by Phil Reid

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

On 10/07/2019 18:21, Geert Uytterhoeven wrote:
> Hi Phil,
>
> On Wed, Jul 10, 2019 at 4:00 AM Phil Reid <[email protected]> wrote:
>> On 6/07/2019 00:05, Geert Uytterhoeven wrote:
>>> GPIO controllers are exported to userspace using /dev/gpiochip*
>>> character devices. Access control to these devices is provided by
>>> standard UNIX file system permissions, on an all-or-nothing basis:
>>> either a GPIO controller is accessible for a user, or it is not.
>>> Currently no mechanism exists to control access to individual GPIOs.
>>>
>>> Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
>>> and expose them as a new gpiochip. This is useful for implementing
>>> access control, and assigning a set of GPIOs to a specific user.
>>> Furthermore, it would simplify and harden exporting GPIOs to a virtual
>>> machine, as the VM can just grab the full virtual GPIO controller, and
>>> no longer needs to care about which GPIOs to grab and which not,
>>> reducing the attack surface.
>>>
>>> Virtual GPIO controllers are instantiated by writing to the "new_device"
>>> attribute file in sysfs:
>>>
>>> $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
>>> "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
>>> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>>>
>>> Likewise, virtual GPIO controllers can be destroyed after use:
>>>
>>> $ echo gpio-virt-agg.<N> \
>>> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
>>>
>>
>> Nice.
>> This provides similar functionality to the "gpio inverter" driver currently on the list.
>> Other than being just a buffer.
>
> Indeed, both drivers forward GPIO calls, but the gpio inverter modifies
> some parameters passed.
>
> The way the drivers obtain references to GPIOs is different, though: the
> inverter driver obtains a fixed description from DT, while the virtual
> aggregator receives the description at runtime, from sysfs.
>
> But perhaps both drivers could share some code?
Other than probing they're almost the same, except the inversion.
This one's more complete for set / get multiple etc.

>
>> Would it be possible to do the lookup via line names?
>
> Doesn't the fact that a GPIO has a line name means that it is in use, and
> thus cannot be aggregated and exported to another user?
>

They can be given line names via the dt property gpio-line-names.
Which can be used by user space to find a gpio. Not sure if there's an equivalent api inkerenl.
But it looks like we can find the info via struct gpiochip_info / gpioline_info linfo and work
out the chip name and line offsets. So probably not required.

Find the right gpio always seems tricky.
We have systems with multiple i2c gpio behind muxes that may or may not be present.
So i2c bus numbers are never consistent. And then different board revisions move the
same gpio line to a different pin (or cahnge the gpio chip type completely) to make routing easier etc.




--
Regards
Phil Reid

2019-07-11 09:56:28

by Geert Uytterhoeven

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

Hi Phil,

On Thu, Jul 11, 2019 at 11:24 AM Phil Reid <[email protected]> wrote:
> On 10/07/2019 18:21, Geert Uytterhoeven wrote:
> > On Wed, Jul 10, 2019 at 4:00 AM Phil Reid <[email protected]> wrote:
> >> On 6/07/2019 00:05, Geert Uytterhoeven wrote:
> >>> GPIO controllers are exported to userspace using /dev/gpiochip*
> >>> character devices. Access control to these devices is provided by
> >>> standard UNIX file system permissions, on an all-or-nothing basis:
> >>> either a GPIO controller is accessible for a user, or it is not.
> >>> Currently no mechanism exists to control access to individual GPIOs.
> >>>
> >>> Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> >>> and expose them as a new gpiochip. This is useful for implementing
> >>> access control, and assigning a set of GPIOs to a specific user.
> >>> Furthermore, it would simplify and harden exporting GPIOs to a virtual
> >>> machine, as the VM can just grab the full virtual GPIO controller, and
> >>> no longer needs to care about which GPIOs to grab and which not,
> >>> reducing the attack surface.
> >>>
> >>> Virtual GPIO controllers are instantiated by writing to the "new_device"
> >>> attribute file in sysfs:
> >>>
> >>> $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
> >>> "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
> >>> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> >>>
> >>> Likewise, virtual GPIO controllers can be destroyed after use:
> >>>
> >>> $ echo gpio-virt-agg.<N> \
> >>> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> >>>

> >> Would it be possible to do the lookup via line names?
> >
> > Doesn't the fact that a GPIO has a line name means that it is in use, and
> > thus cannot be aggregated and exported to another user?
>
> They can be given line names via the dt property gpio-line-names.
> Which can be used by user space to find a gpio. Not sure if there's an equivalent api inkerenl.
> But it looks like we can find the info via struct gpiochip_info / gpioline_info linfo and work
> out the chip name and line offsets. So probably not required.
>
> Find the right gpio always seems tricky.
> We have systems with multiple i2c gpio behind muxes that may or may not be present.
> So i2c bus numbers are never consistent. And then different board revisions move the
> same gpio line to a different pin (or cahnge the gpio chip type completely) to make routing easier etc.

OK, so extending lookup to line names makes sense.
This requires making gpio_name_to_desc() public.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2019-07-12 09:28:28

by Bartosz Golaszewski

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

wt., 9 lip 2019 o 17:59 Geert Uytterhoeven <[email protected]> napisał(a):
>
> Hi Bartosz,
>
> On Tue, Jul 9, 2019 at 4:59 PM Bartosz Golaszewski
> <[email protected]> wrote:
> > pon., 8 lip 2019 o 12:24 Geert Uytterhoeven <[email protected]> napisał(a):
> > > On Mon, Jul 8, 2019 at 11:45 AM Bartosz Golaszewski
> > > <[email protected]> wrote:
> > > > pt., 5 lip 2019 o 18:05 Geert Uytterhoeven <[email protected]> napisał(a):
> > > > > GPIO controllers are exported to userspace using /dev/gpiochip*
> > > > > character devices. Access control to these devices is provided by
> > > > > standard UNIX file system permissions, on an all-or-nothing basis:
> > > > > either a GPIO controller is accessible for a user, or it is not.
> > > > > Currently no mechanism exists to control access to individual GPIOs.
> > > > >
> > > > > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> > > > > and expose them as a new gpiochip. This is useful for implementing
> > > > > access control, and assigning a set of GPIOs to a specific user.
> > > > > Furthermore, it would simplify and harden exporting GPIOs to a virtual
> > > > > machine, as the VM can just grab the full virtual GPIO controller, and
> > > > > no longer needs to care about which GPIOs to grab and which not,
> > > > > reducing the attack surface.
> > > > >
> > > > > Virtual GPIO controllers are instantiated by writing to the "new_device"
> > > > > attribute file in sysfs:
> > > > >
> > > > > $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
> > > > > "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
> > > > > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> > > > >
> > > > > Likewise, virtual GPIO controllers can be destroyed after use:
> > > > >
> > > > > $ echo gpio-virt-agg.<N> \
> > > > > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
>
> > Am I doing it right? I'm trying to create a device and am only getting this:
> >
> > # echo gpiochip2 23 > new_device
> > [ 707.507039] gpio-virt-agg gpio-virt-agg.0: Cannot find gpiochip gpiochip2
> >
> > gpiochip2 *does* exist in the system.
>
> Please try the name of the platform device instead.
> I.e. for my koelsch (R-Car M2-W), it needs "e6052000.gpio" instead
> of "gpiochip2".
>
> Probably the driver should match on both.
>
> > I see. I'll try to review it more thoroughly once I get to play with
> > it. So far I'm stuck on creating the virtual chip.
>
> Thanks, good luck!
>

This is not a show-stopper but one thing that's bothering me in this
is that lines used by the aggregator are considered 'used' in regard
to the original chip. I'm wondering how much effort would it take to
have them be 'muxed' into two (real and virtual) chips at once.

Other than that - seems to works pretty nice other than the matching
by chip name and by line names.

Bart

> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds

2019-08-01 08:45:07

by Linus Walleij

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

Hi Geert!

Thanks for this very interesting patch!

On Fri, Jul 5, 2019 at 6:05 PM Geert Uytterhoeven
<[email protected]> wrote:

> GPIO controllers are exported to userspace using /dev/gpiochip*
> character devices. Access control to these devices is provided by
> standard UNIX file system permissions, on an all-or-nothing basis:
> either a GPIO controller is accessible for a user, or it is not.
> Currently no mechanism exists to control access to individual GPIOs.

Yes, I did that decision deliberately, as the chip is one device
and the base system control is usually on a per-device granularity.
At one point some people were asking for individual GPIO line
permissions in the character device and my argument was something
like why can't I have individual control over the access rights on a block
device or the pixels on a graphics device then.

Jokes aside, filesystems do provide access control over individual
blocks on a block device in a way. So it is further up the stack.

The same goes for this: something above the GPIO chip provide
more granular access control, and as such it fits the need very well.

> Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> and expose them as a new gpiochip. This is useful for implementing
> access control, and assigning a set of GPIOs to a specific user.
> Furthermore, it would simplify and harden exporting GPIOs to a virtual
> machine, as the VM can just grab the full virtual GPIO controller, and
> no longer needs to care about which GPIOs to grab and which not,
> reducing the attack surface.

Excellent approach.

I would even go so far as to call it "gpio-virtualization" or
"gpio-virtualized" rather than "gpio-virtual" so it is clear what the
intended usecase is. We have a bit of confusion in the kernel
because people misuse the word "virtual" left and right, like for
"virtual IRQ number" (Linux IRQ numbers) which are just some
random number space, but not really "virtual", it's a semantic
disease similar to the confusion of using the word "manual" in
computer code.

Here it is however used correctly! (Maybe for the first time.)

> Virtual GPIO controllers are instantiated by writing to the "new_device"
> attribute file in sysfs:
>
> $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
> "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>
> Likewise, virtual GPIO controllers can be destroyed after use:
>
> $ echo gpio-virt-agg.<N> \
> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device

I suppose this is the right way to use sysfs.

I would check with some virtualization people (paged Marc Zyngier
and Christoffer Dall) so they can say whether this is the way any
virtual machine wants to populate its local GPIO chip for
use with a certain machine.

If QEMU can deal in a simple and straight-forward way with this
I see it quickly becoming a very useful tool for industrial automation
where you want to run each control system in isolation and just
respawn the virtual machine if something goes wrong.

Since this might be very popular we need some scrutiny but the
concept as a whole is very appetizing!

Yours,
Linus Walleij

2019-08-05 10:23:00

by Marc Zyngier

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

On 01/08/2019 09:41, Linus Walleij wrote:
> Hi Geert!
>
> Thanks for this very interesting patch!
>
> On Fri, Jul 5, 2019 at 6:05 PM Geert Uytterhoeven
> <[email protected]> wrote:
>
>> GPIO controllers are exported to userspace using /dev/gpiochip*
>> character devices. Access control to these devices is provided by
>> standard UNIX file system permissions, on an all-or-nothing basis:
>> either a GPIO controller is accessible for a user, or it is not.
>> Currently no mechanism exists to control access to individual GPIOs.
>
> Yes, I did that decision deliberately, as the chip is one device
> and the base system control is usually on a per-device granularity.
> At one point some people were asking for individual GPIO line
> permissions in the character device and my argument was something
> like why can't I have individual control over the access rights on a block
> device or the pixels on a graphics device then.
>
> Jokes aside, filesystems do provide access control over individual
> blocks on a block device in a way. So it is further up the stack.
>
> The same goes for this: something above the GPIO chip provide
> more granular access control, and as such it fits the need very well.
>
>> Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
>> and expose them as a new gpiochip. This is useful for implementing
>> access control, and assigning a set of GPIOs to a specific user.
>> Furthermore, it would simplify and harden exporting GPIOs to a virtual
>> machine, as the VM can just grab the full virtual GPIO controller, and
>> no longer needs to care about which GPIOs to grab and which not,
>> reducing the attack surface.
>
> Excellent approach.
>
> I would even go so far as to call it "gpio-virtualization" or
> "gpio-virtualized" rather than "gpio-virtual" so it is clear what the
> intended usecase is. We have a bit of confusion in the kernel
> because people misuse the word "virtual" left and right, like for
> "virtual IRQ number" (Linux IRQ numbers) which are just some
> random number space, but not really "virtual", it's a semantic
> disease similar to the confusion of using the word "manual" in
> computer code.

I'd drop the notion of "virtual" altogether. Nothing is virtual in this
thing at all (the GPIOs are very real, from what I gather). Instead (and
assuming I got it right, which is a long shot), what you have is a
"synthetic" GPIO controller, made from the GPIOs belonging to other
controllers. I'd call it "GPIO aggregator".

>
> Here it is however used correctly! (Maybe for the first time.)
>
>> Virtual GPIO controllers are instantiated by writing to the "new_device"
>> attribute file in sysfs:
>>
>> $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
>> "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
>> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>>
>> Likewise, virtual GPIO controllers can be destroyed after use:
>>
>> $ echo gpio-virt-agg.<N> \
>> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
>
> I suppose this is the right way to use sysfs.
>
> I would check with some virtualization people (paged Marc Zyngier
> and Christoffer Dall) so they can say whether this is the way any
> virtual machine wants to populate its local GPIO chip for
> use with a certain machine.
>
> If QEMU can deal in a simple and straight-forward way with this
> I see it quickly becoming a very useful tool for industrial automation
> where you want to run each control system in isolation and just
> respawn the virtual machine if something goes wrong.

What the VMM (QEMU, kvmtool) would need to do is to present this as a
"standard" GPIO IP, and use the backend aggregator to emulate it.
Certainly doable. The nice part is that all the work is in userspace,
and thus completely off my plate! ;-)

You also may want to look into not emulating a standard IP, but use
something virtio-based instead. The ACRN project seems to have something
like this in progress, but I know nothing about it.

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2019-08-05 10:59:51

by Linus Walleij

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

On Mon, Aug 5, 2019 at 12:21 PM Marc Zyngier <[email protected]> wrote:
> On 01/08/2019 09:41, Linus Walleij wrote:

> > I would even go so far as to call it "gpio-virtualization" or
> > "gpio-virtualized" rather than "gpio-virtual" so it is clear what the
> > intended usecase is. We have a bit of confusion in the kernel
> > because people misuse the word "virtual" left and right, like for
> > "virtual IRQ number" (Linux IRQ numbers) which are just some
> > random number space, but not really "virtual", it's a semantic
> > disease similar to the confusion of using the word "manual" in
> > computer code.
>
> I'd drop the notion of "virtual" altogether. Nothing is virtual in this
> thing at all (the GPIOs are very real, from what I gather). Instead (and
> assuming I got it right, which is a long shot), what you have is a
> "synthetic" GPIO controller, made from the GPIOs belonging to other
> controllers. I'd call it "GPIO aggregator".

+1 on this.

Next thing that will predictably follow is a userspace ABI to
create those aggregators and have them go away if the
process creating it dies. Something to think of...

> > If QEMU can deal in a simple and straight-forward way with this
> > I see it quickly becoming a very useful tool for industrial automation
> > where you want to run each control system in isolation and just
> > respawn the virtual machine if something goes wrong.
>
> What the VMM (QEMU, kvmtool) would need to do is to present this as a
> "standard" GPIO IP, and use the backend aggregator to emulate it.
> Certainly doable. The nice part is that all the work is in userspace,
> and thus completely off my plate! ;-)

Yeah there is not really any "standard" GPIO, but if the user is running
e.g. a Versatile Express model, that has a PL061 GPIO, and then
a user would create an "aggregator GPIO" with say 8 lines and
QEMU would pick that up as /dev/gpiochipN and bind it inside
of QEMU to the lines of the virttualized PL061 GPIO controller,
so the machine thinks it is using a PL061 but in reality it is
just 8 GPIO lines on the host computer.

> You also may want to look into not emulating a standard IP, but use
> something virtio-based instead. The ACRN project seems to have something
> like this in progress, but I know nothing about it.

That sounds quite interesting as well.

Then the virtualized machine can just pick this "virtio GPIO" and
some command line switches or config file on the host can
set up and route a GPIO aggregator.

Yours,
Linus Walleij

2019-09-06 15:03:38

by Geert Uytterhoeven

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

On Tue, Jul 9, 2019 at 4:59 PM Bartosz Golaszewski
<[email protected]> wrote:
> pon., 8 lip 2019 o 12:24 Geert Uytterhoeven <[email protected]> napisał(a):
> > On Mon, Jul 8, 2019 at 11:45 AM Bartosz Golaszewski
> > <[email protected]> wrote:
> > > pt., 5 lip 2019 o 18:05 Geert Uytterhoeven <[email protected]> napisał(a):
> > > > +static int gpio_virt_agg_set_config(struct gpio_chip *chip,
> > > > + unsigned int offset, unsigned long config)
> > > > +{
> > > > + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> > > > +
> > > > + chip = priv->desc[offset]->gdev->chip;
> > > > + if (chip->set_config)
> > > > + return chip->set_config(chip, offset, config);
> > > > +
> > > > + // FIXME gpiod_set_transitory() expects success if not implemented
> >
> > BTW, do you have a comment about this FIXME?
>
> Ha! Interesting. I'll give it a thought and respond elsewhere as it's
> a different subject.
>
> > > > + return -ENOTSUPP;

Upon closer look, this turns out to be a red herring: gpiod_set_transitory()
converts -ENOTSUPP to zero, so there is no issue.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2019-09-06 17:02:54

by Geert Uytterhoeven

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

Hi Bartosz,

On Fri, Jul 12, 2019 at 11:27 AM Bartosz Golaszewski
<[email protected]> wrote:
> wt., 9 lip 2019 o 17:59 Geert Uytterhoeven <[email protected]> napisał(a):
> > On Tue, Jul 9, 2019 at 4:59 PM Bartosz Golaszewski
> > <[email protected]> wrote:
> > > pon., 8 lip 2019 o 12:24 Geert Uytterhoeven <[email protected]> napisał(a):
> > > > On Mon, Jul 8, 2019 at 11:45 AM Bartosz Golaszewski
> > > > <[email protected]> wrote:
> > > > > pt., 5 lip 2019 o 18:05 Geert Uytterhoeven <[email protected]> napisał(a):
> > > > > > GPIO controllers are exported to userspace using /dev/gpiochip*
> > > > > > character devices. Access control to these devices is provided by
> > > > > > standard UNIX file system permissions, on an all-or-nothing basis:
> > > > > > either a GPIO controller is accessible for a user, or it is not.
> > > > > > Currently no mechanism exists to control access to individual GPIOs.
> > > > > >
> > > > > > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> > > > > > and expose them as a new gpiochip. This is useful for implementing
> > > > > > access control, and assigning a set of GPIOs to a specific user.
> > > > > > Furthermore, it would simplify and harden exporting GPIOs to a virtual
> > > > > > machine, as the VM can just grab the full virtual GPIO controller, and
> > > > > > no longer needs to care about which GPIOs to grab and which not,
> > > > > > reducing the attack surface.
> > > > > >
> > > > > > Virtual GPIO controllers are instantiated by writing to the "new_device"
> > > > > > attribute file in sysfs:
> > > > > >
> > > > > > $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
> > > > > > "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
> > > > > > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> > > > > >
> > > > > > Likewise, virtual GPIO controllers can be destroyed after use:
> > > > > >
> > > > > > $ echo gpio-virt-agg.<N> \
> > > > > > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> >
> > > Am I doing it right? I'm trying to create a device and am only getting this:
> > >
> > > # echo gpiochip2 23 > new_device
> > > [ 707.507039] gpio-virt-agg gpio-virt-agg.0: Cannot find gpiochip gpiochip2
> > >
> > > gpiochip2 *does* exist in the system.
> >
> > Please try the name of the platform device instead.
> > I.e. for my koelsch (R-Car M2-W), it needs "e6052000.gpio" instead
> > of "gpiochip2".
> >
> > Probably the driver should match on both.
> >
> > > I see. I'll try to review it more thoroughly once I get to play with
> > > it. So far I'm stuck on creating the virtual chip.
>
> This is not a show-stopper but one thing that's bothering me in this
> is that lines used by the aggregator are considered 'used' in regard
> to the original chip. I'm wondering how much effort would it take to
> have them be 'muxed' into two (real and virtual) chips at once.

Is that really what you want?
If a GPIO is aggregated with othrs, it's intended to be used only through
the aggregator, isn't it?

> Other than that - seems to works pretty nice other than the matching
> by chip name and by line names.

Thanks, working on that...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2019-09-12 08:59:22

by Linus Walleij

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

On Fri, Jul 5, 2019 at 5:05 PM Geert Uytterhoeven
<[email protected]> wrote:

> GPIO controllers are exported to userspace using /dev/gpiochip*
> character devices. Access control to these devices is provided by
> standard UNIX file system permissions, on an all-or-nothing basis:
> either a GPIO controller is accessible for a user, or it is not.
> Currently no mechanism exists to control access to individual GPIOs.
>
> Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> and expose them as a new gpiochip. This is useful for implementing
> access control, and assigning a set of GPIOs to a specific user.
> Furthermore, it would simplify and harden exporting GPIOs to a virtual
> machine, as the VM can just grab the full virtual GPIO controller, and
> no longer needs to care about which GPIOs to grab and which not,
> reducing the attack surface.
>
> Virtual GPIO controllers are instantiated by writing to the "new_device"
> attribute file in sysfs:
>
> $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
> "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>
> Likewise, virtual GPIO controllers can be destroyed after use:
>
> $ echo gpio-virt-agg.<N> \
> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> Aggregating GPIOs and exposing them as a new gpiochip was suggested in
> response to my proof-of-concept for GPIO virtualization with QEMU[1][2].
>
> Sample session on r8a7791/koelsch:
>
> - Disable the leds node in arch/arm/boot/dts/r8a7791-koelsch.dts
>
> - Create virtual aggregators:
>
> $ echo "e6052000.gpio 19 20" \
> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>
> gpio-virt-agg gpio-virt-agg.0: GPIO 0 => e6052000.gpio/19
> gpio-virt-agg gpio-virt-agg.0: GPIO 1 => e6052000.gpio/20
> gpiochip_find_base: found new base at 778
> gpio gpiochip8: (gpio-virt-agg.0): added GPIO chardev (254:8)
> gpiochip_setup_dev: registered GPIOs 778 to 779 on device: gpiochip8 (gpio-virt-agg.0)
>
> $ echo "e6052000.gpio 21, e6050000.gpio 20 21 22" \
> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>
> gpio-virt-agg gpio-virt-agg.1: GPIO 0 => e6052000.gpio/21
> gpio-virt-agg gpio-virt-agg.1: GPIO 1 => e6050000.gpio/20
> gpio-virt-agg gpio-virt-agg.1: GPIO 2 => e6050000.gpio/21
> gpio-virt-agg gpio-virt-agg.1: GPIO 3 => e6050000.gpio/22
> gpiochip_find_base: found new base at 774
> gpio gpiochip9: (gpio-virt-agg.1): added GPIO chardev (254:9)
> gpiochip_setup_dev: registered GPIOs 774 to 777 on device: gpiochip9 (gpio-virt-agg.1)
>
> - Adjust permissions on /dev/gpiochip[89] (optional)
>
> - Control LEDs:
>
> $ gpioset gpiochip8 0=0 1=1 # LED6 OFF, LED7 ON
> $ gpioset gpiochip8 0=1 1=0 # LED6 ON, LED7 OFF
> $ gpioset gpiochip9 0=0 # LED8 OFF
> $ gpioset gpiochip9 0=1 # LED8 ON
>
> - Destroy virtual aggregators:
>
> $ echo gpio-virt-agg.0 \
> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> $ echo gpio-virt-agg.1 \
> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
>
> Thanks for your comments!
>
> References:
> - [1] "[PATCH QEMU POC] Add a GPIO backend"
> (https://lore.kernel.org/linux-renesas-soc/[email protected]/)
> - [2] "Getting To Blinky: Virt Edition / Making device pass-through
> work on embedded ARM"
> (https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)

I'm looping in my friends at Google for this discussion.

They need a virtualized gpio_chip for their Android emulator,
and their current approach for other devices has been around
using virtio in most cases and an emulated AC97 for the
audio case as far as I remember.

It would be great to have their input on this so we can create a
virtualization/aggregate that works for all.

Please include [email protected] on future postings of this!

Yours,
Linus Walleij

2019-09-12 09:01:26

by Geert Uytterhoeven

Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

Hi Linus,

On Thu, Sep 12, 2019 at 10:56 AM Linus Walleij <[email protected]> wrote:
> On Fri, Jul 5, 2019 at 5:05 PM Geert Uytterhoeven
> <[email protected]> wrote:
> > GPIO controllers are exported to userspace using /dev/gpiochip*
> > character devices. Access control to these devices is provided by
> > standard UNIX file system permissions, on an all-or-nothing basis:
> > either a GPIO controller is accessible for a user, or it is not.
> > Currently no mechanism exists to control access to individual GPIOs.
> >
> > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> > and expose them as a new gpiochip. This is useful for implementing
> > access control, and assigning a set of GPIOs to a specific user.
> > Furthermore, it would simplify and harden exporting GPIOs to a virtual
> > machine, as the VM can just grab the full virtual GPIO controller, and
> > no longer needs to care about which GPIOs to grab and which not,
> > reducing the attack surface.
> >
> > Virtual GPIO controllers are instantiated by writing to the "new_device"
> > attribute file in sysfs:
> >
> > $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
> > "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
> > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> >
> > Likewise, virtual GPIO controllers can be destroyed after use:
> >
> > $ echo gpio-virt-agg.<N> \
> > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> >
> > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > ---
> > Aggregating GPIOs and exposing them as a new gpiochip was suggested in
> > response to my proof-of-concept for GPIO virtualization with QEMU[1][2].
> >
> > Sample session on r8a7791/koelsch:
> >
> > - Disable the leds node in arch/arm/boot/dts/r8a7791-koelsch.dts
> >
> > - Create virtual aggregators:
> >
> > $ echo "e6052000.gpio 19 20" \
> > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> >
> > gpio-virt-agg gpio-virt-agg.0: GPIO 0 => e6052000.gpio/19
> > gpio-virt-agg gpio-virt-agg.0: GPIO 1 => e6052000.gpio/20
> > gpiochip_find_base: found new base at 778
> > gpio gpiochip8: (gpio-virt-agg.0): added GPIO chardev (254:8)
> > gpiochip_setup_dev: registered GPIOs 778 to 779 on device: gpiochip8 (gpio-virt-agg.0)
> >
> > $ echo "e6052000.gpio 21, e6050000.gpio 20 21 22" \
> > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> >
> > gpio-virt-agg gpio-virt-agg.1: GPIO 0 => e6052000.gpio/21
> > gpio-virt-agg gpio-virt-agg.1: GPIO 1 => e6050000.gpio/20
> > gpio-virt-agg gpio-virt-agg.1: GPIO 2 => e6050000.gpio/21
> > gpio-virt-agg gpio-virt-agg.1: GPIO 3 => e6050000.gpio/22
> > gpiochip_find_base: found new base at 774
> > gpio gpiochip9: (gpio-virt-agg.1): added GPIO chardev (254:9)
> > gpiochip_setup_dev: registered GPIOs 774 to 777 on device: gpiochip9 (gpio-virt-agg.1)
> >
> > - Adjust permissions on /dev/gpiochip[89] (optional)
> >
> > - Control LEDs:
> >
> > $ gpioset gpiochip8 0=0 1=1 # LED6 OFF, LED7 ON
> > $ gpioset gpiochip8 0=1 1=0 # LED6 ON, LED7 OFF
> > $ gpioset gpiochip9 0=0 # LED8 OFF
> > $ gpioset gpiochip9 0=1 # LED8 ON
> >
> > - Destroy virtual aggregators:
> >
> > $ echo gpio-virt-agg.0 \
> > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> > $ echo gpio-virt-agg.1 \
> > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> >
> > Thanks for your comments!
> >
> > References:
> > - [1] "[PATCH QEMU POC] Add a GPIO backend"
> > (https://lore.kernel.org/linux-renesas-soc/[email protected]/)
> > - [2] "Getting To Blinky: Virt Edition / Making device pass-through
> > work on embedded ARM"
> > (https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)
>
> I'm looping in my friends at Google for this discussion.
>
> They need a virtualized gpio_chip for their Android emulator,
> and their current approach for other devices has been around
> using virtio in most cases and an emulated AC97 for the
> audio case as far as I remember.
>
> It would be great to have their input on this so we can create a
> virtualization/aggregate that works for all.
>
> Please include [email protected] on future postings of this!

I've sent v2 yesterday:
https://lore.kernel.org/lkml/[email protected]/

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds