[v3,5/7] gpio: Add GPIO Aggregator/Repeater 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 GPIO driver to aggregate existing GPIOs, and expose them as
a new gpiochip.

This supports the following use cases:
  1. Aggregating GPIOs using Sysfs
     This is useful for implementing access control, and assigning a set
     of GPIOs to a specific user or virtual machine.

  2. GPIO Repeater in Device Tree
     This supports modelling e.g. GPIO inverters in DT.

  3. Generic GPIO Driver
     This provides userspace access to a simple GPIO-operated device
     described in DT, cfr. e.g. spidev for SPI-operated devices.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - Absorb GPIO forwarder,
  - Integrate GPIO Repeater and Generic GPIO driver functionality,
  - Use the aggregator parameters to create a GPIO lookup table instead
    of an array of GPIO descriptors, which allows to simplify the code:
      1. This removes the need for calling gpio_name_to_desc(),
         gpiochip_find(), gpiochip_get_desc(), and gpiod_request(),
      2. This allows the platform device to always use
         devm_gpiod_get_index(), regardless of the origin of the GPIOs,
  - Move parameter parsing from platform device probe to sysfs attribute
    store, removing the need for platform data passing,
  - Use more devm_*() functions to simplify cleanup,
  - Add pr_fmt(),
  - General refactoring.

v2:
  - Add missing initialization of i in gpio_virt_agg_probe(),
  - Update for removed .need_valid_mask field and changed
    .init_valid_mask() signature,
  - Drop "virtual", rename to gpio-aggregator,
  - Drop bogus FIXME related to gpiod_set_transitory() expectations,
  - Use new GPIO Forwarder Helper,
  - Lift limit on the maximum number of GPIOs,
  - Improve parsing:
      - add support for specifying GPIOs by line name,
      - add support for specifying GPIO chips by ID,
      - add support for GPIO offset ranges,
      - names and offset specifiers must be separated by whitespace,
      - GPIO offsets must separated by spaces,
  - Use str_has_prefix() and kstrtouint().
---
 drivers/gpio/Kconfig           |  13 +
 drivers/gpio/Makefile          |   1 +
 drivers/gpio/gpio-aggregator.c | 587 +++++++++++++++++++++++++++++++++
 3 files changed, 601 insertions(+)
 create mode 100644 drivers/gpio/gpio-aggregator.c

Hi Geert,

Many thanks for the series upgrade.
A few static-analysis findings below (could be false positives).

On Wed, Nov 27, 2019 at 09:42:51AM +0100, Geert Uytterhoeven wrote:

[..]

> +static bool isrange(const char *s)
> +{
> +	size_t n = strlen(s);

Cppcheck 1.40-18521-ge6d692d96058:
drivers/gpio/gpio-aggregator.c:69:11: style: Variable 'n' is assigned a value that is never used. [unreadVariable]

Smatch v0.5.0-6150-gc1ed13e4ee7b:
drivers/gpio/gpio-aggregator.c:69 isrange() warn: unused return: n = strlen()

[..]

> +	aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
> +	if (!aggr->lookups) {
> +		res = -ENOMEM;
> +		goto remove_idr;
> +	}

s/aggr->lookups/aggr->lookups->dev_id/ ?

[..]

> +static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> +				 unsigned long *bits)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +	unsigned long *values, flags;

gcc 9.2.1:
warning: ‘flags’ may be used uninitialized in this function [-Wmaybe-uninitialized]

[..]

> +static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> +				  unsigned long *bits)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +	unsigned long *values, flags;

gcc 9.2.1, same as above:
warning: ‘flags’ may be used uninitialized in this function [-Wmaybe-uninitialized]

Should these be silenced like in 2bf593f101f3ca ("xilinx_uartps.c:
suppress "may be used uninitialised" warning") ?

I plan to do some runtime testing soon.

Hi Eugeniu,

On Wed, Nov 27, 2019 at 3:15 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> On Wed, Nov 27, 2019 at 09:42:51AM +0100, Geert Uytterhoeven wrote:
> > +static bool isrange(const char *s)
> > +{
> > +     size_t n = strlen(s);
>
> Cppcheck 1.40-18521-ge6d692d96058:
> drivers/gpio/gpio-aggregator.c:69:11: style: Variable 'n' is assigned a value that is never used. [unreadVariable]
>
> Smatch v0.5.0-6150-gc1ed13e4ee7b:
> drivers/gpio/gpio-aggregator.c:69 isrange() warn: unused return: n = strlen()

Correct, this is a remainder of code present temporarily during development.
Will drop.

(where are the days gcc itself warned about that?)

> > +     aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
> > +     if (!aggr->lookups) {
> > +             res = -ENOMEM;
> > +             goto remove_idr;
> > +     }
>
> s/aggr->lookups/aggr->lookups->dev_id/ ?

Thanks, will fix.

> > +static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> > +                              unsigned long *bits)
> > +{
> > +     struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> > +     unsigned long *values, flags;
>
> gcc 9.2.1:
> warning: ‘flags’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>
> [..]
>
> > +static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> > +                               unsigned long *bits)
> > +{
> > +     struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> > +     unsigned long *values, flags;
>
> gcc 9.2.1, same as above:
> warning: ‘flags’ may be used uninitialized in this function [-Wmaybe-uninitialized]

So newer gcc is (again) no longer smart enough to notice the check is
the same for initializer and user...

> Should these be silenced like in 2bf593f101f3ca ("xilinx_uartps.c:
> suppress "may be used uninitialised" warning") ?

TBH, I'm not a big fan of silencing false positives.
But if people like to see flags preinitialized to zero, that can be done...

> I plan to do some runtime testing soon.

Thanks, looking forward to the results!

Gr{oetje,eeting}s,

                        Geert

Thank you for this series!

> On November 27, 2019 at 9:42 AM Geert Uytterhoeven <geert+renesas@glider.be> 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 GPIO driver to aggregate existing GPIOs, and expose them as
> a new gpiochip.
> 
> This supports the following use cases:
>   1. Aggregating GPIOs using Sysfs
>      This is useful for implementing access control, and assigning a set
>      of GPIOs to a specific user or virtual machine.
> 
>   2. GPIO Repeater in Device Tree
>      This supports modelling e.g. GPIO inverters in DT.
> 
>   3. Generic GPIO Driver
>      This provides userspace access to a simple GPIO-operated device
>      described in DT, cfr. e.g. spidev for SPI-operated devices.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - Absorb GPIO forwarder,
>   - Integrate GPIO Repeater and Generic GPIO driver functionality,
>   - Use the aggregator parameters to create a GPIO lookup table instead
>     of an array of GPIO descriptors, which allows to simplify the code:
>       1. This removes the need for calling gpio_name_to_desc(),
>          gpiochip_find(), gpiochip_get_desc(), and gpiod_request(),
>       2. This allows the platform device to always use
>          devm_gpiod_get_index(), regardless of the origin of the GPIOs,
>   - Move parameter parsing from platform device probe to sysfs attribute
>     store, removing the need for platform data passing,
>   - Use more devm_*() functions to simplify cleanup,
>   - Add pr_fmt(),
>   - General refactoring.
> 
> v2:
>   - Add missing initialization of i in gpio_virt_agg_probe(),
>   - Update for removed .need_valid_mask field and changed
>     .init_valid_mask() signature,
>   - Drop "virtual", rename to gpio-aggregator,
>   - Drop bogus FIXME related to gpiod_set_transitory() expectations,
>   - Use new GPIO Forwarder Helper,
>   - Lift limit on the maximum number of GPIOs,
>   - Improve parsing:
>       - add support for specifying GPIOs by line name,
>       - add support for specifying GPIO chips by ID,
>       - add support for GPIO offset ranges,
>       - names and offset specifiers must be separated by whitespace,
>       - GPIO offsets must separated by spaces,
>   - Use str_has_prefix() and kstrtouint().
> ---
>  drivers/gpio/Kconfig           |  13 +
>  drivers/gpio/Makefile          |   1 +
>  drivers/gpio/gpio-aggregator.c | 587 +++++++++++++++++++++++++++++++++
>  3 files changed, 601 insertions(+)
>  create mode 100644 drivers/gpio/gpio-aggregator.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 8adffd42f8cb0559..36b6b57a6b05e906 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1507,6 +1507,19 @@ config GPIO_VIPERBOARD
>  
>  endmenu
>  
> +config GPIO_AGGREGATOR
> +	tristate "GPIO Aggregator/Repeater"
> +	help
> +	  Say yes here to enable the GPIO Aggregator and repeater, which

Inconsistent capitalization (Aggregator vs. repeater).

> +	  provides a way to aggregate and/or repeat existing GPIOs into a new
> +	  GPIO device.
> +	  This can serve the following purposes:
> +	    1. Assign a collection of GPIOs to a user, or export them to a
> +	       virtual machine,
> +	    2. Support GPIOs that are connected to a physical inverter,
> +	    3. Provide a generic driver for a GPIO-operated device, to be
> +               controlled from userspace using the GPIO chardev interface.
> +
>  config GPIO_MOCKUP
>  	tristate "GPIO Testing Driver"
>  	select IRQ_SIM
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 34eb8b2b12dd656c..f9971eeb1f32335f 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_GPIO_74XX_MMIO)		+= gpio-74xx-mmio.o
>  obj-$(CONFIG_GPIO_ADNP)			+= gpio-adnp.o
>  obj-$(CONFIG_GPIO_ADP5520)		+= gpio-adp5520.o
>  obj-$(CONFIG_GPIO_ADP5588)		+= gpio-adp5588.o
> +obj-$(CONFIG_GPIO_AGGREGATOR)		+= gpio-aggregator.o
>  obj-$(CONFIG_GPIO_ALTERA_A10SR)		+= gpio-altera-a10sr.o
>  obj-$(CONFIG_GPIO_ALTERA)  		+= gpio-altera.o
>  obj-$(CONFIG_GPIO_AMD8111)		+= gpio-amd8111.o
> diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> new file mode 100644
> index 0000000000000000..873578c6f9683db8
> --- /dev/null
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -0,0 +1,587 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// GPIO Aggregator and Repeater
> +//
> +// Copyright (C) 2019 Glider bvba
> +
> +#define DRV_NAME       "gpio-aggregator"
> +#define pr_fmt(fmt)	DRV_NAME ": " fmt
> +
> +#include <linux/bitmap.h>
> +#include <linux/bitops.h>
> +#include <linux/ctype.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/machine.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/overflow.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.h>
> +
> +#include "gpiolib.h"
> +
> +
> +	/*
> +	 * GPIO Aggregator sysfs interface
> +	 */

Should this comment really be indented?

> +
> +struct gpio_aggregator {
> +	struct gpiod_lookup_table *lookups;
> +	struct platform_device *pdev;
> +	char args[];
> +};
> +
> +static DEFINE_MUTEX(gpio_aggregator_lock);	/* protects idr */
> +static DEFINE_IDR(gpio_aggregator_idr);
> +
> +static char *get_arg(char **args)
> +{
> +	char *start = *args, *end;
> +
> +	while (isspace(*start))
> +		start++;

"start = skip_spaces(start);", perhaps. (Looking at a grep through drivers/, I would say usage of while(isspace(...))... vs. skip_spaces() is about 60/40.)

> +
> +	if (!*start)
> +		return NULL;
> +
> +	if (*start == '"') {
> +		/* Quoted arg */
> +		end = strchr(++start, '"');
> +		if (!end)
> +			return ERR_PTR(-EINVAL);
> +	} else {
> +		/* Unquoted arg */
> +		for (end = start; *end && !isspace(*end); end++) ;
> +	}
> +
> +	if (*end)
> +		*end++ = '\0';
> +
> +	*args = end;
> +	return start;
> +}
> +
> +static bool isrange(const char *s)
> +{
> +	size_t n = strlen(s);
> +
> +	if (IS_ERR_OR_NULL(s))
> +		return false;
> +
> +	while (1) {
> +		n = strspn(s, "0123456789");
> +		if (!n)
> +			return false;
> +
> +		s += n;
> +
> +		switch (*s++) {
> +		case '\0':
> +			return true;
> +
> +		case '-':
> +		case ',':
> +			break;
> +
> +		default:
> +			return false;
> +		}
> +	}
> +}
> +
> +static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *label,
> +			 int hwnum, unsigned int *n)
> +{
> +	struct gpiod_lookup_table *lookups;
> +
> +	lookups = krealloc(aggr->lookups, struct_size(lookups, table, *n + 2),
> +			   GFP_KERNEL);
> +	if (!lookups)
> +		return -ENOMEM;
> +
> +	lookups->table[*n].chip_label = label;
> +	lookups->table[*n].chip_hwnum = hwnum;
> +	lookups->table[*n].idx = *n;
> +
> +	(*n)++;
> +	memset(&lookups->table[*n], 0, sizeof(lookups->table[*n]));
> +
> +	aggr->lookups = lookups;
> +	return 0;
> +}
> +
> +static int aggr_parse(struct gpio_aggregator *aggr)
> +{
> +	char *name, *offsets, *first, *last, *next;
> +	unsigned int a, b, i, n = 0;

Too many single-letter variables. "first_index" and "last_index" instead of "a" and "b" would make it easier to understand.

> +	char *args = aggr->args;
> +	int error;
> +
> +	for (name = get_arg(&args), offsets = get_arg(&args); name;
> +	     offsets = get_arg(&args)) {
> +		if (IS_ERR(name)) {
> +			pr_err("Cannot get GPIO specifier: %ld\n",
> +			       PTR_ERR(name));
> +			return PTR_ERR(name);
> +		}
> +
> +		if (!isrange(offsets)) {
> +			/* Named GPIO line */
> +			error = aggr_add_gpio(aggr, name, -1, &n);

The "-1" magic could use a #define, IMO.

> +			if (error)
> +				return error;
> +
> +			name = offsets;
> +			continue;
> +		}
> +
> +		/* GPIO chip + offset(s) */
> +		for (first = offsets; *first; first = next) {
> +			next = strchrnul(first, ',');
> +			if (*next)
> +				*next++ = '\0';
> +
> +			last = strchr(first, '-');
> +			if (last)
> +				*last++ = '\0';
> +
> +			if (kstrtouint(first, 10, &a)) {
> +				pr_err("Cannot parse GPIO index %s\n", first);
> +				return -EINVAL;
> +			}
> +
> +			if (!last) {
> +				b = a;
> +			} else if (kstrtouint(last, 10, &b)) {
> +				pr_err("Cannot parse GPIO index %s\n", last);
> +				return -EINVAL;
> +			}
> +
> +			for (i = a; i <= b; i++) {
> +				error = aggr_add_gpio(aggr, name, i, &n);
> +				if (error)
> +					return error;
> +			}
> +		}
> +
> +		name = get_arg(&args);
> +	}
> +
> +	if (!n) {
> +		pr_err("No GPIOs specified\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> +				size_t count)
> +{
> +	struct gpio_aggregator *aggr;
> +	struct platform_device *pdev;
> +	int res, id;
> +
> +	/* kernfs guarantees string termination, so count + 1 is safe */
> +	aggr = kzalloc(sizeof(*aggr) + count + 1, GFP_KERNEL);
> +	if (!aggr)
> +		return -ENOMEM;
> +
> +	memcpy(aggr->args, buf, count + 1);
> +
> +	aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
> +				GFP_KERNEL);
> +	if (!aggr->lookups) {
> +		res = -ENOMEM;
> +		goto free_ga;
> +	}
> +
> +	mutex_lock(&gpio_aggregator_lock);
> +	id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
> +	mutex_unlock(&gpio_aggregator_lock);
> +
> +	if (id < 0) {
> +		res = id;
> +		goto free_table;
> +	}
> +
> +	aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
> +	if (!aggr->lookups) {
> +		res = -ENOMEM;
> +		goto remove_idr;
> +	}
> +
> +	res = aggr_parse(aggr);
> +	if (res)
> +		goto free_dev_id;
> +
> +	gpiod_add_lookup_table(aggr->lookups);
> +
> +	pdev = platform_device_register_simple(DRV_NAME, id, NULL, 0);
> +	if (IS_ERR(pdev)) {
> +		res = PTR_ERR(pdev);
> +		goto remove_table;
> +	}
> +
> +	aggr->pdev = pdev;
> +	return count;
> +
> +remove_table:
> +	gpiod_remove_lookup_table(aggr->lookups);
> +free_dev_id:
> +	kfree(aggr->lookups->dev_id);
> +remove_idr:
> +	mutex_lock(&gpio_aggregator_lock);
> +	idr_remove(&gpio_aggregator_idr, id);
> +	mutex_unlock(&gpio_aggregator_lock);
> +free_table:
> +	kfree(aggr->lookups);
> +free_ga:
> +	kfree(aggr);
> +	return res;
> +}
> +
> +static DRIVER_ATTR_WO(new_device);
> +
> +static void gpio_aggregator_free(struct gpio_aggregator *aggr)
> +{
> +	platform_device_unregister(aggr->pdev);
> +	gpiod_remove_lookup_table(aggr->lookups);
> +	kfree(aggr->lookups->dev_id);
> +	kfree(aggr->lookups);
> +	kfree(aggr);
> +}
> +
> +static ssize_t delete_device_store(struct device_driver *driver,
> +				   const char *buf, size_t count)
> +{
> +	struct gpio_aggregator *aggr;
> +	unsigned int id;
> +	int error;
> +
> +	if (!str_has_prefix(buf, DRV_NAME "."))
> +		return -EINVAL;
> +
> +	error = kstrtouint(buf + strlen(DRV_NAME "."), 10, &id);
> +	if (error)
> +		return error;
> +
> +	mutex_lock(&gpio_aggregator_lock);
> +	aggr = idr_remove(&gpio_aggregator_idr, id);
> +	mutex_unlock(&gpio_aggregator_lock);
> +	if (!aggr)
> +		return -ENOENT;
> +
> +	gpio_aggregator_free(aggr);
> +	return count;
> +}
> +static DRIVER_ATTR_WO(delete_device);
> +
> +static struct attribute *gpio_aggregator_attrs[] = {
> +	&driver_attr_new_device.attr,
> +	&driver_attr_delete_device.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(gpio_aggregator);
> +
> +static int __exit gpio_aggregator_idr_remove(int id, void *p, void *data)
> +{
> +	gpio_aggregator_free(p);
> +	return 0;
> +}
> +
> +static void __exit gpio_aggregator_remove_all(void)
> +{
> +	mutex_lock(&gpio_aggregator_lock);
> +	idr_for_each(&gpio_aggregator_idr, gpio_aggregator_idr_remove, NULL);
> +	idr_destroy(&gpio_aggregator_idr);
> +	mutex_unlock(&gpio_aggregator_lock);
> +}
> +
> +
> +	/*
> +	 *  Common GPIO Forwarder
> +	 */
> +
> +struct gpiochip_fwd {
> +	struct gpio_chip chip;
> +	struct gpio_desc **descs;
> +	union {
> +		struct mutex mlock;	/* protects tmp[] if can_sleep */
> +		spinlock_t slock;	/* protects tmp[] if !can_sleep */
> +	};
> +	unsigned long tmp[];		/* values and descs for multiple ops */
> +};
> +
> +static int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +	return gpiod_get_direction(fwd->descs[offset]);
> +}
> +
> +static int gpio_fwd_direction_input(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +	return gpiod_direction_input(fwd->descs[offset]);
> +}
> +
> +static int gpio_fwd_direction_output(struct gpio_chip *chip,
> +				     unsigned int offset, int value)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +	return gpiod_direction_output(fwd->descs[offset], value);
> +}
> +
> +static int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +	return gpiod_get_value(fwd->descs[offset]);
> +}
> +
> +static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> +				 unsigned long *bits)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +	unsigned long *values, flags;
> +	struct gpio_desc **descs;
> +	unsigned int i, j = 0;
> +	int error;
> +
> +	if (chip->can_sleep)
> +		mutex_lock(&fwd->mlock);
> +	else
> +		spin_lock_irqsave(&fwd->slock, flags);
> +
> +	values = &fwd->tmp[0];
> +	bitmap_clear(values, 0, fwd->chip.ngpio);
> +	descs = (void *)&fwd->tmp[BITS_TO_LONGS(fwd->chip.ngpio)];

This use of the tmp array, which I presume serves to avoid extra memory allocations at the time of device creation, is odd enough to deserve a comment.

> +
> +	for_each_set_bit(i, mask, fwd->chip.ngpio)
> +		descs[j++] = fwd->descs[i];
> +
> +	error = gpiod_get_array_value(j, descs, NULL, values);
> +	if (!error) {
> +		j = 0;
> +		for_each_set_bit(i, mask, fwd->chip.ngpio)
> +			__assign_bit(i, bits, test_bit(j++, values));
> +	}
> +
> +	if (chip->can_sleep)
> +		mutex_unlock(&fwd->mlock);
> +	else
> +		spin_unlock_irqrestore(&fwd->slock, flags);
> +
> +	return error;
> +}
> +
> +static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +	gpiod_set_value(fwd->descs[offset], value);
> +}
> +
> +static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> +				  unsigned long *bits)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +	unsigned long *values, flags;
> +	struct gpio_desc **descs;
> +	unsigned int i, j = 0;
> +
> +	if (chip->can_sleep)
> +		mutex_lock(&fwd->mlock);
> +	else
> +		spin_lock_irqsave(&fwd->slock, flags);
> +
> +	values = &fwd->tmp[0];
> +	descs = (void *)&fwd->tmp[BITS_TO_LONGS(fwd->chip.ngpio)];
> +
> +	for_each_set_bit(i, mask, fwd->chip.ngpio) {
> +		__assign_bit(j, values, test_bit(i, bits));
> +		descs[j++] = fwd->descs[i];
> +	}
> +
> +	gpiod_set_array_value(j, descs, NULL, values);
> +
> +	if (chip->can_sleep)
> +		mutex_unlock(&fwd->mlock);
> +	else
> +		spin_unlock_irqrestore(&fwd->slock, flags);
> +}
> +
> +static int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
> +			       unsigned long config)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +	chip = fwd->descs[offset]->gdev->chip;
> +	if (chip->set_config)
> +		return chip->set_config(chip, offset, config);
> +
> +	return -ENOTSUPP;
> +}
> +
> +static int gpio_fwd_init_valid_mask(struct gpio_chip *chip,
> +				    unsigned long *valid_mask,
> +				    unsigned int ngpios)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +	unsigned int i;
> +
> +	for (i = 0; i < ngpios; i++) {
> +		if (!gpiochip_line_is_valid(fwd->descs[i]->gdev->chip,
> +					    gpio_chip_hwgpio(fwd->descs[i])))
> +			clear_bit(i, valid_mask);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * gpiochip_fwd_create() - Create a new GPIO forwarder
> + * @dev: Parent device pointer
> + * @ngpios: Number of GPIOs in the forwarder.
> + * @descs: Array containing the GPIO descriptors to forward to.
> + *         This array must contain @ngpios entries, and must not be deallocated
> + *         before the forwarder has been destroyed again.
> + *
> + * This function creates a new gpiochip, which forwards all GPIO operations to
> + * the passed GPIO descriptors.
> + *
> + * Return: An opaque object pointer, or an ERR_PTR()-encoded negative error
> + *         code on failure.
> + */
> +static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
> +						unsigned int ngpios,
> +						struct gpio_desc *descs[])
> +{
> +	const char *label = dev_name(dev);
> +	struct gpiochip_fwd *fwd;
> +	struct gpio_chip *chip;
> +	unsigned int i;
> +	int error;
> +
> +	fwd = devm_kzalloc(dev, struct_size(fwd, tmp,
> +			   BITS_TO_LONGS(ngpios) + ngpios), GFP_KERNEL);
> +	if (!fwd)
> +		return ERR_PTR(-ENOMEM);
> +
> +	chip = &fwd->chip;
> +
> +	for (i = 0; i < ngpios; i++) {
> +		dev_dbg(dev, "gpio %u => gpio-%d (%s)\n", i,
> +			desc_to_gpio(descs[i]), descs[i]->label ? : "?");
> +
> +		if (gpiod_cansleep(descs[i]))
> +			chip->can_sleep = true;
> +		if (descs[i]->gdev->chip->set_config)
> +			chip->set_config = gpio_fwd_set_config;
> +		if (descs[i]->gdev->chip->init_valid_mask)
> +			chip->init_valid_mask = gpio_fwd_init_valid_mask;
> +	}
> +
> +	chip->label = label;
> +	chip->parent = dev;
> +	chip->owner = THIS_MODULE;
> +	chip->get_direction = gpio_fwd_get_direction;
> +	chip->direction_input = gpio_fwd_direction_input;
> +	chip->direction_output = gpio_fwd_direction_output;
> +	chip->get = gpio_fwd_get;
> +	chip->get_multiple = gpio_fwd_get_multiple;
> +	chip->set = gpio_fwd_set;
> +	chip->set_multiple = gpio_fwd_set_multiple;
> +	chip->base = -1;
> +	chip->ngpio = ngpios;
> +	fwd->descs = descs;
> +
> +	if (chip->can_sleep)
> +		mutex_init(&fwd->mlock);
> +	else
> +		spin_lock_init(&fwd->slock);
> +
> +	error = devm_gpiochip_add_data(dev, chip, fwd);
> +	if (error)
> +		return ERR_PTR(error);
> +
> +	return fwd;
> +}
> +
> +
> +	/*
> +	 *  Common GPIO Aggregator/Repeater platform device
> +	 */
> +
> +static int gpio_aggregator_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct gpio_desc **descs;
> +	struct gpiochip_fwd *fwd;
> +	int i, n;
> +
> +	n = gpiod_count(dev, NULL);
> +	if (n < 0)
> +		return n;
> +
> +	descs = devm_kmalloc_array(dev, n, sizeof(*descs), GFP_KERNEL);
> +	if (!descs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < n; i++) {
> +		descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
> +		if (IS_ERR(descs[i]))
> +			return PTR_ERR(descs[i]);
> +	}
> +
> +	fwd = gpiochip_fwd_create(dev, n, descs);
> +	if (IS_ERR(fwd))
> +		return PTR_ERR(fwd);
> +
> +	platform_set_drvdata(pdev, fwd);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id gpio_aggregator_dt_ids[] = {
> +	{ .compatible = "gpio-repeater" },
> +	/*
> +	 * Add GPIO-operated devices controlled from userspace below,
> +	 * or use "driver_override" in sysfs
> +	 */
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, gpio_aggregator_dt_ids);
> +#endif
> +
> +static struct platform_driver gpio_aggregator_driver = {
> +	.probe = gpio_aggregator_probe,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.groups = gpio_aggregator_groups,
> +		.of_match_table = of_match_ptr(gpio_aggregator_dt_ids),
> +	},
> +};
> +
> +static int __init gpio_aggregator_init(void)
> +{
> +	return platform_driver_register(&gpio_aggregator_driver);
> +}
> +module_init(gpio_aggregator_init);
> +
> +static void __exit gpio_aggregator_exit(void)
> +{
> +	gpio_aggregator_remove_all();
> +	platform_driver_unregister(&gpio_aggregator_driver);
> +}
> +module_exit(gpio_aggregator_exit);
> +
> +MODULE_AUTHOR("Geert Uytterhoeven <geert+renesas@glider.be>");
> +MODULE_DESCRIPTION("GPIO Aggregator/Repeater");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
>

CU
Uli

> +static int gpio_aggregator_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct gpio_desc **descs;
> +	struct gpiochip_fwd *fwd;
> +	int i, n;
> +
> +	n = gpiod_count(dev, NULL);
> +	if (n < 0)
> +		return n;
> +
> +	descs = devm_kmalloc_array(dev, n, sizeof(*descs), GFP_KERNEL);
> +	if (!descs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < n; i++) {
> +		descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);

can you please add this check as well as we need to return EPROBE_DEFER.

if (desc[i] == ERR_PTR(-ENOENT))
<                 return -EPROBE_DEFER;


> +		if (IS_ERR(descs[i]))
> +			return PTR_ERR(descs[i]);
> +	}
> +
> +	fwd = gpiochip_fwd_create(dev, n, descs);
> +	if (IS_ERR(fwd))
> +		return PTR_ERR(fwd);
> +
> +	platform_set_drvdata(pdev, fwd);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id gpio_aggregator_dt_ids[] = {
> +	{ .compatible = "gpio-repeater" },
> +	/*
> +	 * Add GPIO-operated devices controlled from userspace below,
> +	 * or use "driver_override" in sysfs
> +	 */
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, gpio_aggregator_dt_ids);
> +#endif
> +
> +static struct platform_driver gpio_aggregator_driver = {
> +	.probe = gpio_aggregator_probe,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.groups = gpio_aggregator_groups,
> +		.of_match_table = of_match_ptr(gpio_aggregator_dt_ids),
> +	},
> +};
> +
> +static int __init gpio_aggregator_init(void)
> +{
> +	return platform_driver_register(&gpio_aggregator_driver);
> +}
> +module_init(gpio_aggregator_init);
> +
> +static void __exit gpio_aggregator_exit(void)
> +{
> +	gpio_aggregator_remove_all();
> +	platform_driver_unregister(&gpio_aggregator_driver);
> +}
> +module_exit(gpio_aggregator_exit);
> +
> +MODULE_AUTHOR("Geert Uytterhoeven <geert+renesas@glider.be>");
> +MODULE_DESCRIPTION("GPIO Aggregator/Repeater");
> +MODULE_LICENSE("GPL v2");

Hi Harish,

On Tue, Dec 3, 2019 at 6:42 AM Harish Jenny K N
<harish_kandiga@mentor.com> wrote:
> > +static int gpio_aggregator_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct gpio_desc **descs;
> > +     struct gpiochip_fwd *fwd;
> > +     int i, n;
> > +
> > +     n = gpiod_count(dev, NULL);
> > +     if (n < 0)
> > +             return n;
> > +
> > +     descs = devm_kmalloc_array(dev, n, sizeof(*descs), GFP_KERNEL);
> > +     if (!descs)
> > +             return -ENOMEM;
> > +
> > +     for (i = 0; i < n; i++) {
> > +             descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
>
> can you please add this check as well as we need to return EPROBE_DEFER.
>
> if (desc[i] == ERR_PTR(-ENOENT))
> <                 return -EPROBE_DEFER;

So gpiod_get_index() nevers return -EPROBE_DEFER, but returns -ENOENT
instead?
How can a driver distinguish between "GPIO not found" and "gpiochip driver
not yet initialized"?
Worse, so the *_optional() variants will return NULL in both cases, too, so
the caller will always fall back to optional GPIO not present?

Or am I missing something?

Gr{oetje,eeting}s,

                        Geert

On 03/12/19 1:47 PM, Geert Uytterhoeven wrote:
> Hi Harish,
>
> On Tue, Dec 3, 2019 at 6:42 AM Harish Jenny K N
> <harish_kandiga@mentor.com> wrote:
>>> +static int gpio_aggregator_probe(struct platform_device *pdev)
>>> +{
>>> +     struct device *dev = &pdev->dev;
>>> +     struct gpio_desc **descs;
>>> +     struct gpiochip_fwd *fwd;
>>> +     int i, n;
>>> +
>>> +     n = gpiod_count(dev, NULL);
>>> +     if (n < 0)
>>> +             return n;
>>> +
>>> +     descs = devm_kmalloc_array(dev, n, sizeof(*descs), GFP_KERNEL);
>>> +     if (!descs)
>>> +             return -ENOMEM;
>>> +
>>> +     for (i = 0; i < n; i++) {
>>> +             descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
>> can you please add this check as well as we need to return EPROBE_DEFER.
>>
>> if (desc[i] == ERR_PTR(-ENOENT))
>> <                 return -EPROBE_DEFER;
> So gpiod_get_index() nevers return -EPROBE_DEFER, but returns -ENOENT
> instead?
> How can a driver distinguish between "GPIO not found" and "gpiochip driver
> not yet initialized"?
> Worse, so the *_optional() variants will return NULL in both cases, too, so
> the caller will always fall back to optional GPIO not present?
>
> Or am I missing something?
>
> Gr{oetje,eeting}s,
>
>                         Geert


We had earlier tested our changes on 4.14 kernel and the explicit return of -EPROBE_DEFER was needed in the inverter driver.

probably the commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ( gpiolib: Keep returning EPROBE_DEFER when we should)

has fixed the issue and now it returns -EPROBE_DEFER.  you can ignore this comment as of now. I will test and let you know if needed.


Thanks,

Harish

Hi Geert,

On Wed, Nov 27, 2019 at 09:42:51AM +0100, Geert Uytterhoeven wrote:
> +static int gpio_aggregator_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct gpio_desc **descs;
> +	struct gpiochip_fwd *fwd;
> +	int i, n;

FWIW/FTR, doing some blind creation and deletion of gpio aggregator
chips [1] on R-Car H3ULCB overnight, kmemleak reported once [2]. Not
sure this is something 100% reproducible.

[1] while true; do \
   echo e6055400.gpio 12,13 > /sys/bus/platform/drivers/gpio-aggregator/new_device; \
   echo gpio-aggregator.0 > /sys/bus/platform/drivers/gpio-aggregator/delete_device; \
   done 

[2] unreferenced object 0xffff0006d2c2e000 (size 128):
  comm "kworker/3:1", pid 55, jiffies 4294676978 (age 38546.676s)
  hex dump (first 32 bytes):
    00 d9 d2 d3 06 00 ff ff 0c 00 e0 0f ff ff ff ff  ................
    01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<00000000a8e18c13>] slab_post_alloc_hook+0x8c/0x94
    [<000000006f419a4f>] __kmalloc+0x170/0x218
    [<0000000060d185ea>] kobj_map+0x78/0x1c0
    [<00000000c96645f3>] cdev_add+0x68/0x94
    [<00000000a7a5a8ac>] cdev_device_add+0x74/0x90
    [<00000000497871d3>] gpiochip_setup_dev+0x84/0x1f0
    [<00000000b993f95f>] gpiochip_add_data_with_key+0xbcc/0x11f0
    [<00000000fd728c0e>] devm_gpiochip_add_data+0x60/0xa8
    [<00000000442e34c1>] gpio_aggregator_probe+0x210/0x3c8
    [<00000000076e13fb>] platform_drv_probe+0x70/0xe4
    [<00000000de84b58b>] really_probe+0x2d8/0x434
    [<00000000c95c9784>] driver_probe_device+0x15c/0x16c
    [<00000000afb7dd4f>] __device_attach_driver+0xdc/0x120
    [<00000000efa40cae>] bus_for_each_drv+0x12c/0x154
    [<00000000c149acef>] __device_attach+0x148/0x1e0
    [<00000000a74fd158>] device_initial_probe+0x24/0x30

Hi Geert!

Thanks for this interesting patch!

On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
<geert+renesas@glider.be> 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 GPIO driver to aggregate existing GPIOs, and expose them as
> a new gpiochip.
>
> This supports the following use cases:
>   1. Aggregating GPIOs using Sysfs
>      This is useful for implementing access control, and assigning a set
>      of GPIOs to a specific user or virtual machine.
>
>   2. GPIO Repeater in Device Tree
>      This supports modelling e.g. GPIO inverters in DT.
>
>   3. Generic GPIO Driver
>      This provides userspace access to a simple GPIO-operated device
>      described in DT, cfr. e.g. spidev for SPI-operated devices.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Overall I like how this is developing!

> +config GPIO_AGGREGATOR
> +       tristate "GPIO Aggregator/Repeater"
> +       help
> +         Say yes here to enable the GPIO Aggregator and repeater, which
> +         provides a way to aggregate and/or repeat existing GPIOs into a new
> +         GPIO device.

Should it say a "new virtual GPIO chip"?

> +         This can serve the following purposes:
> +           1. Assign a collection of GPIOs to a user, or export them to a
> +              virtual machine,

This is ambiguous. What is a "user"? A process calling from
userspace? A device tree node?

I would write "assign a collection of GPIO lines from any lines on
existing physical GPIO chips to form a new virtual GPIO chip"

That should be to the point, right?

> +           2. Support GPIOs that are connected to a physical inverter,

s/to/through/g

> +           3. Provide a generic driver for a GPIO-operated device, to be
> +               controlled from userspace using the GPIO chardev interface.

I don't understand this, it needs to be elaborated. What is meant
by a "GPIO-operated device" in this context? Example?

I consistently use the term "GPIO line" as opposed to "GPIO"
or "GPIO number" etc that are abigous, so please rephrase using
"GPIO lines" rather than just "GPIOs" above.

> +#include "gpiolib.h"

Whenever this is included in a driver I want it to come with a comment
explicitly stating exactly why and which internal symbols the driver
needs to access. Ideally all drivers should just need <linux/gpio/driver.h>...

> +static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *label,
> +                        int hwnum, unsigned int *n)

u16 hwnum for the hardware number but if it is always -1/U16_MAX
then why pass the parameter at all.

Is "label" the right name of this parameter if that is going to actually
be line_name then use that.

> +{
> +       struct gpiod_lookup_table *lookups;
> +
> +       lookups = krealloc(aggr->lookups, struct_size(lookups, table, *n + 2),
> +                          GFP_KERNEL);
> +       if (!lookups)
> +               return -ENOMEM;
> +
> +       lookups->table[*n].chip_label = label;

This is pending the discussion on whether to just use "key" for this
name.

> +       lookups->table[*n].chip_hwnum = hwnum;

If this is always going to be U16_MAX (-1 in the current code)
then it can just be assigned as that here instead of passed as
parameter.

> +static int aggr_parse(struct gpio_aggregator *aggr)
> +{
> +       char *name, *offsets, *first, *last, *next;
> +       unsigned int a, b, i, n = 0;
> +       char *args = aggr->args;
> +       int error;
> +
> +       for (name = get_arg(&args), offsets = get_arg(&args); name;
> +            offsets = get_arg(&args)) {
> +               if (IS_ERR(name)) {
> +                       pr_err("Cannot get GPIO specifier: %ld\n",
> +                              PTR_ERR(name));
> +                       return PTR_ERR(name);
> +               }
> +
> +               if (!isrange(offsets)) {
> +                       /* Named GPIO line */
> +                       error = aggr_add_gpio(aggr, name, -1, &n);

So the third argument woule be U16_MAX here. Or not pass
a parameter at all.

But honestly, when I look at this I don't understand why you
have to avoid so hard to use offsets for the GPIO lines on
your aggregator?

Just put a u16 ngpios in your
struct gpio_aggregator and count it up every time you
add some new offsets here and you have
offset numbers for all your GPIO lines on the aggregator
and you can just drop the patch for lookup up lines by line
names.

Is there something wrong with my reasoning here?

At the pointe later when the lines are counted from the
allocated lookups using gpiod_count() that will just figure
out this number anyways, so it is not like we don't know
it at the end of the day.

So it seems the patch to gpiolib is just to use machine
descriptor tables as a substitute for a simple counter
variable in this local struct to me.

> +static void __exit gpio_aggregator_remove_all(void)
> +{
> +       mutex_lock(&gpio_aggregator_lock);
> +       idr_for_each(&gpio_aggregator_idr, gpio_aggregator_idr_remove, NULL);
> +       idr_destroy(&gpio_aggregator_idr);
> +       mutex_unlock(&gpio_aggregator_lock);
> +}
> +
> +
> +       /*
> +        *  Common GPIO Forwarder
> +        */
> +

Nitpick: lots and weird spacing here.

> +struct gpiochip_fwd {
> +       struct gpio_chip chip;
> +       struct gpio_desc **descs;
> +       union {
> +               struct mutex mlock;     /* protects tmp[] if can_sleep */
> +               spinlock_t slock;       /* protects tmp[] if !can_sleep */
> +       };

That was a very elegant use of union!

> +static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> +                                unsigned long *bits)
> +static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> +                                 unsigned long *bits)

I guess these can both be optimized to use get/set_multiple on
the target chip if the offsets are consecutive?

However that is going to be tricky so I'm not saying you should
implement that. So for now, let's say just add a TODO: comment
about it.

> +static int gpio_fwd_init_valid_mask(struct gpio_chip *chip,
> +                                   unsigned long *valid_mask,
> +                                   unsigned int ngpios)
> +{
> +       struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +       unsigned int i;
> +
> +       for (i = 0; i < ngpios; i++) {
> +               if (!gpiochip_line_is_valid(fwd->descs[i]->gdev->chip,
> +                                           gpio_chip_hwgpio(fwd->descs[i])))
> +                       clear_bit(i, valid_mask);
> +       }

This is what uses "gpiolib.h" is it not?

devm_gpiod_get_index() will not succeed if the line
is not valid so I think this can be just dropped, since
what you do before this is exactly devm_gpiod_get_index()
on each line, then you call gpiochip_fwd_create()
with the result.

So I think you can just drop this entire function.
This will not happen.

If it does happen, add a comment above this loop
explaining which circumstances would make lines on
the forwarder invalid.

> +       for (i = 0; i < ngpios; i++) {
> +               dev_dbg(dev, "gpio %u => gpio-%d (%s)\n", i,
> +                       desc_to_gpio(descs[i]), descs[i]->label ? : "?");
> +
> +               if (gpiod_cansleep(descs[i]))
> +                       chip->can_sleep = true;
> +               if (descs[i]->gdev->chip->set_config)
> +                       chip->set_config = gpio_fwd_set_config;
> +               if (descs[i]->gdev->chip->init_valid_mask)
> +                       chip->init_valid_mask = gpio_fwd_init_valid_mask;
> +       }

I do not think you should need to inspect the init_valid_mask()
as explained above.

Add a comment above the loop that if any of the GPIO lines
are sleeping then the entire forwarder will be sleeping
and if any of the chips support .set_config() we will support
setting configs.

However the way that the .gpio_fwd_set_config() is coded
it looks like you can just unconditionally assign it and
only check the cansleep condition in this loop.

> +}
> +
> +
> +       /*
> +        *  Common GPIO Aggregator/Repeater platform device
> +        */
> +

Nitpick: weird and excess spacing again.

> +       for (i = 0; i < n; i++) {
> +               descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
> +               if (IS_ERR(descs[i]))
> +                       return PTR_ERR(descs[i]);
> +       }

If this succeeds none of the obtained gpio_desc:s can be
invalid.

Yours,
Linus Walleij

Hi Linus,

On Thu, Dec 12, 2019 at 3:34 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> <geert+renesas@glider.be> 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 GPIO driver to aggregate existing GPIOs, and expose them as
> > a new gpiochip.
> >
> > This supports the following use cases:
> >   1. Aggregating GPIOs using Sysfs
> >      This is useful for implementing access control, and assigning a set
> >      of GPIOs to a specific user or virtual machine.
> >
> >   2. GPIO Repeater in Device Tree
> >      This supports modelling e.g. GPIO inverters in DT.
> >
> >   3. Generic GPIO Driver
> >      This provides userspace access to a simple GPIO-operated device
> >      described in DT, cfr. e.g. spidev for SPI-operated devices.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Overall I like how this is developing!
>
> > +config GPIO_AGGREGATOR
> > +       tristate "GPIO Aggregator/Repeater"
> > +       help
> > +         Say yes here to enable the GPIO Aggregator and repeater, which
> > +         provides a way to aggregate and/or repeat existing GPIOs into a new
> > +         GPIO device.
>
> Should it say a "new virtual GPIO chip"?

OK.

> > +         This can serve the following purposes:
> > +           1. Assign a collection of GPIOs to a user, or export them to a
> > +              virtual machine,
>
> This is ambiguous. What is a "user"? A process calling from
> userspace? A device tree node?

A user is an entity with a UID, typically listed in /etc/passwd.
This is similar to letting some, not all, people on the machine access
the CD-ROM drive.

> I would write "assign a collection of GPIO lines from any lines on
> existing physical GPIO chips to form a new virtual GPIO chip"
>
> That should be to the point, right?

Yes, that's WHAT it does. The WHY is the granular access control.

> > +           2. Support GPIOs that are connected to a physical inverter,
>
> s/to/through/g

OK.

> > +           3. Provide a generic driver for a GPIO-operated device, to be
> > +               controlled from userspace using the GPIO chardev interface.
>
> I don't understand this, it needs to be elaborated. What is meant
> by a "GPIO-operated device" in this context? Example?

E.g. a motor. Or a door opener.

        door-opener {
                compatible = "mydoor,opener";

                gpios = <&gpio2 19 GPIO_ACTIVE_HIGH>;
        };

You don't need a full-featured kernel driver for that, so just bind the
gpio-aggregator to the door-opener, and control it through libgpiod.

> I consistently use the term "GPIO line" as opposed to "GPIO"
> or "GPIO number" etc that are abigous, so please rephrase using
> "GPIO lines" rather than just "GPIOs" above.

OK.

> > +#include "gpiolib.h"
>
> Whenever this is included in a driver I want it to come with a comment
> explicitly stating exactly why and which internal symbols the driver
> needs to access. Ideally all drivers should just need <linux/gpio/driver.h>...

"gpiolib.h" is needed to access gpio_desc.gdev->chip in
gpio_fwd_set_config().  And for gpio_chip_hwgpio() (see below).

But indeed, I should add #include <linux/gpio/consumer.h>, for e.g. the
various gpiod_[gs]et_*() functions.

> > +static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *label,
> > +                        int hwnum, unsigned int *n)
>
> u16 hwnum for the hardware number but if it is always -1/U16_MAX
> then why pass the parameter at all.
>
> Is "label" the right name of this parameter if that is going to actually
> be line_name then use that.

It's not always -1.
This function can be called either with a gpiochip label/name and an
offset, or a line-name and -1.

> > +{
> > +       struct gpiod_lookup_table *lookups;
> > +
> > +       lookups = krealloc(aggr->lookups, struct_size(lookups, table, *n + 2),
> > +                          GFP_KERNEL);
> > +       if (!lookups)
> > +               return -ENOMEM;
> > +
> > +       lookups->table[*n].chip_label = label;
>
> This is pending the discussion on whether to just use "key" for this
> name.

Which would require touching all users (board files and mfd drivers).

> > +       lookups->table[*n].chip_hwnum = hwnum;
>
> If this is always going to be U16_MAX (-1 in the current code)
> then it can just be assigned as that here instead of passed as
> parameter.

So it's not, see above.

> > +static int aggr_parse(struct gpio_aggregator *aggr)
> > +{
> > +       char *name, *offsets, *first, *last, *next;
> > +       unsigned int a, b, i, n = 0;
> > +       char *args = aggr->args;
> > +       int error;
> > +
> > +       for (name = get_arg(&args), offsets = get_arg(&args); name;
> > +            offsets = get_arg(&args)) {
> > +               if (IS_ERR(name)) {
> > +                       pr_err("Cannot get GPIO specifier: %ld\n",
> > +                              PTR_ERR(name));
> > +                       return PTR_ERR(name);
> > +               }
> > +
> > +               if (!isrange(offsets)) {
> > +                       /* Named GPIO line */
> > +                       error = aggr_add_gpio(aggr, name, -1, &n);
>
> So the third argument woule be U16_MAX here. Or not pass
> a parameter at all.
>
> But honestly, when I look at this I don't understand why you
> have to avoid so hard to use offsets for the GPIO lines on
> your aggregator?
>
> Just put a u16 ngpios in your
> struct gpio_aggregator and count it up every time you
> add some new offsets here and you have
> offset numbers for all your GPIO lines on the aggregator
> and you can just drop the patch for lookup up lines by line
> names.
>
> Is there something wrong with my reasoning here?

Yes, I think there is.
The offsets are not offsets on the aggregated gpiochip, but on the
original target gpiochip.

> At the pointe later when the lines are counted from the
> allocated lookups using gpiod_count() that will just figure
> out this number anyways, so it is not like we don't know
> it at the end of the day.
>
> So it seems the patch to gpiolib is just to use machine
> descriptor tables as a substitute for a simple counter
> variable in this local struct to me.

Nope, it's used for looking up the target GPIO lines.

> > +static void __exit gpio_aggregator_remove_all(void)
> > +{
> > +       mutex_lock(&gpio_aggregator_lock);
> > +       idr_for_each(&gpio_aggregator_idr, gpio_aggregator_idr_remove, NULL);
> > +       idr_destroy(&gpio_aggregator_idr);
> > +       mutex_unlock(&gpio_aggregator_lock);
> > +}
> > +
> > +
> > +       /*
> > +        *  Common GPIO Forwarder
> > +        */
> > +
>
> Nitpick: lots and weird spacing here.

OK.

> > +struct gpiochip_fwd {
> > +       struct gpio_chip chip;
> > +       struct gpio_desc **descs;
> > +       union {
> > +               struct mutex mlock;     /* protects tmp[] if can_sleep */
> > +               spinlock_t slock;       /* protects tmp[] if !can_sleep */
> > +       };
>
> That was a very elegant use of union!
>
> > +static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> > +                                unsigned long *bits)
> > +static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> > +                                 unsigned long *bits)
>
> I guess these can both be optimized to use get/set_multiple on
> the target chip if the offsets are consecutive?
>
> However that is going to be tricky so I'm not saying you should
> implement that. So for now, let's say just add a TODO: comment
> about it.

Doesn't gpiod_[gs]et_array_value() already call .[gs]et_multiple()?

> > +static int gpio_fwd_init_valid_mask(struct gpio_chip *chip,
> > +                                   unsigned long *valid_mask,
> > +                                   unsigned int ngpios)
> > +{
> > +       struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < ngpios; i++) {
> > +               if (!gpiochip_line_is_valid(fwd->descs[i]->gdev->chip,
> > +                                           gpio_chip_hwgpio(fwd->descs[i])))
> > +                       clear_bit(i, valid_mask);
> > +       }
>
> This is what uses "gpiolib.h" is it not?
>
> devm_gpiod_get_index() will not succeed if the line
> is not valid so I think this can be just dropped, since
> what you do before this is exactly devm_gpiod_get_index()
> on each line, then you call gpiochip_fwd_create()
> with the result.
>
> So I think you can just drop this entire function.
> This will not happen.

OK, if all lines are valid, the mask handling is indeed not needed.

> If it does happen, add a comment above this loop
> explaining which circumstances would make lines on
> the forwarder invalid.

OK, so cannot happen.

> > +       for (i = 0; i < ngpios; i++) {
> > +               dev_dbg(dev, "gpio %u => gpio-%d (%s)\n", i,
> > +                       desc_to_gpio(descs[i]), descs[i]->label ? : "?");
> > +
> > +               if (gpiod_cansleep(descs[i]))
> > +                       chip->can_sleep = true;
> > +               if (descs[i]->gdev->chip->set_config)
> > +                       chip->set_config = gpio_fwd_set_config;
> > +               if (descs[i]->gdev->chip->init_valid_mask)
> > +                       chip->init_valid_mask = gpio_fwd_init_valid_mask;
> > +       }
>
> I do not think you should need to inspect the init_valid_mask()
> as explained above.

OK.

> Add a comment above the loop that if any of the GPIO lines
> are sleeping then the entire forwarder will be sleeping
> and if any of the chips support .set_config() we will support
> setting configs.

OK.

> However the way that the .gpio_fwd_set_config() is coded
> it looks like you can just unconditionally assign it and
> only check the cansleep condition in this loop.

I wanted to avoid the overhead of calling into gpio_fwd_set_config() if
none of the targets gpiochips support .set_config(), see
gpiod_set_transitory().

> > +}
> > +
> > +
> > +       /*
> > +        *  Common GPIO Aggregator/Repeater platform device
> > +        */
> > +
>
> Nitpick: weird and excess spacing again.

Yeah, this dates back from when the aggregator, repeater, and
forwarder were all separate files and modules.

> > +       for (i = 0; i < n; i++) {
> > +               descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
> > +               if (IS_ERR(descs[i]))
> > +                       return PTR_ERR(descs[i]);
> > +       }
>
> If this succeeds none of the obtained gpio_desc:s can be
> invalid.

OK.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Sorry for slowness... christmas.

On Thu, Dec 12, 2019 at 4:24 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Dec 12, 2019 at 3:34 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> > > +         This can serve the following purposes:
> > > +           1. Assign a collection of GPIOs to a user, or export them to a
> > > +              virtual machine,
> >
> > This is ambiguous. What is a "user"? A process calling from
> > userspace? A device tree node?
>
> A user is an entity with a UID, typically listed in /etc/passwd.
> This is similar to letting some, not all, people on the machine access
> the CD-ROM drive.

Ah I get it. Maybe we can say "assign permissions for a collection
of GPIOs to a user".

> > I would write "assign a collection of GPIO lines from any lines on
> > existing physical GPIO chips to form a new virtual GPIO chip"
> >
> > That should be to the point, right?
>
> Yes, that's WHAT it does. The WHY is the granular access control.

So I guess we can write both?

> > > +           3. Provide a generic driver for a GPIO-operated device, to be
> > > +               controlled from userspace using the GPIO chardev interface.
> >
> > I don't understand this, it needs to be elaborated. What is meant
> > by a "GPIO-operated device" in this context? Example?
>
> E.g. a motor. Or a door opener.
>
>         door-opener {
>                 compatible = "mydoor,opener";
>
>                 gpios = <&gpio2 19 GPIO_ACTIVE_HIGH>;
>         };
>
> You don't need a full-featured kernel driver for that, so just bind the
> gpio-aggregator to the door-opener, and control it through libgpiod.

Yep it's a perfect industrial control example, I get it.

Maybe we should blurb something about industrial control?

The rest I think we cleared out else I will see it when I review again.

Yours,
Linus Walleij

Hi Linus,

On Sat, Jan 4, 2020 at 1:38 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> Sorry for slowness... christmas.

Np. Happy New Year!

> On Thu, Dec 12, 2019 at 4:24 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Dec 12, 2019 at 3:34 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > > +         This can serve the following purposes:
> > > > +           1. Assign a collection of GPIOs to a user, or export them to a
> > > > +              virtual machine,
> > >
> > > This is ambiguous. What is a "user"? A process calling from
> > > userspace? A device tree node?
> >
> > A user is an entity with a UID, typically listed in /etc/passwd.
> > This is similar to letting some, not all, people on the machine access
> > the CD-ROM drive.
>
> Ah I get it. Maybe we can say "assign permissions for a collection
> of GPIOs to a user".

OK

> > > I would write "assign a collection of GPIO lines from any lines on
> > > existing physical GPIO chips to form a new virtual GPIO chip"
> > >
> > > That should be to the point, right?
> >
> > Yes, that's WHAT it does. The WHY is the granular access control.
>
> So I guess we can write both?

OK.

> > > > +           3. Provide a generic driver for a GPIO-operated device, to be
> > > > +               controlled from userspace using the GPIO chardev interface.
> > >
> > > I don't understand this, it needs to be elaborated. What is meant
> > > by a "GPIO-operated device" in this context? Example?
> >
> > E.g. a motor. Or a door opener.
> >
> >         door-opener {
> >                 compatible = "mydoor,opener";
> >
> >                 gpios = <&gpio2 19 GPIO_ACTIVE_HIGH>;
> >         };
> >
> > You don't need a full-featured kernel driver for that, so just bind the
> > gpio-aggregator to the door-opener, and control it through libgpiod.
>
> Yep it's a perfect industrial control example, I get it.
>
> Maybe we should blurb something about industrial control?

OK.

> The rest I think we cleared out else I will see it when I review again.

The remaining discussion point is "GPIO Repeater in Device Tree", i.e.
the GPIO inverter usecase, which might be solved better by adding a
GPIO_INVERTED flag.

Shall I rip that out, incorporate review comments, and report?

Thanks!



--
Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

On Mon, Jan 6, 2020 at 9:23 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > The rest I think we cleared out else I will see it when I review again.
>
> The remaining discussion point is "GPIO Repeater in Device Tree", i.e.
> the GPIO inverter usecase, which might be solved better by adding a
> GPIO_INVERTED flag.
>
> Shall I rip that out, incorporate review comments, and report?

Don't know, if it blocks progress I guess the diplomatic solution is to
do that, divide and conquer. But review is a social process so I don't really
know the right strategy.

Yours,
Linus Walleij

Hi Eugeniu,

On Tue, Dec 3, 2019 at 11:51 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> On Wed, Nov 27, 2019 at 09:42:51AM +0100, Geert Uytterhoeven wrote:
> > +static int gpio_aggregator_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct gpio_desc **descs;
> > +     struct gpiochip_fwd *fwd;
> > +     int i, n;
>
> FWIW/FTR, doing some blind creation and deletion of gpio aggregator
> chips [1] on R-Car H3ULCB overnight, kmemleak reported once [2]. Not
> sure this is something 100% reproducible.
>
> [1] while true; do \
>    echo e6055400.gpio 12,13 > /sys/bus/platform/drivers/gpio-aggregator/new_device; \
>    echo gpio-aggregator.0 > /sys/bus/platform/drivers/gpio-aggregator/delete_device; \
>    done
>
> [2] unreferenced object 0xffff0006d2c2e000 (size 128):
>   comm "kworker/3:1", pid 55, jiffies 4294676978 (age 38546.676s)
>   hex dump (first 32 bytes):
>     00 d9 d2 d3 06 00 ff ff 0c 00 e0 0f ff ff ff ff  ................
>     01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<00000000a8e18c13>] slab_post_alloc_hook+0x8c/0x94
>     [<000000006f419a4f>] __kmalloc+0x170/0x218
>     [<0000000060d185ea>] kobj_map+0x78/0x1c0
>     [<00000000c96645f3>] cdev_add+0x68/0x94
>     [<00000000a7a5a8ac>] cdev_device_add+0x74/0x90
>     [<00000000497871d3>] gpiochip_setup_dev+0x84/0x1f0
>     [<00000000b993f95f>] gpiochip_add_data_with_key+0xbcc/0x11f0
>     [<00000000fd728c0e>] devm_gpiochip_add_data+0x60/0xa8
>     [<00000000442e34c1>] gpio_aggregator_probe+0x210/0x3c8
>     [<00000000076e13fb>] platform_drv_probe+0x70/0xe4
>     [<00000000de84b58b>] really_probe+0x2d8/0x434
>     [<00000000c95c9784>] driver_probe_device+0x15c/0x16c
>     [<00000000afb7dd4f>] __device_attach_driver+0xdc/0x120
>     [<00000000efa40cae>] bus_for_each_drv+0x12c/0x154
>     [<00000000c149acef>] __device_attach+0x148/0x1e0
>     [<00000000a74fd158>] device_initial_probe+0x24/0x30

This is the allocation of the GPIO character device, which is allocated
in response to the creation of the GPIO chip, from .probe().
As that is done using devm_gpiochip_add_data(), the chardev should be
deallocated automatically by devm_gpio_chip_release() when
platform_device_unregister() is called.

Weird...

Gr{oetje,eeting}s,

                        Geert

Hi Geert,

On Thu, Jan 09, 2020 at 02:35:10PM +0100, Geert Uytterhoeven wrote:
> On Tue, Dec 3, 2019 at 11:51 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> >
> > FWIW/FTR, doing some blind creation and deletion of gpio aggregator
> > chips [1] on R-Car H3ULCB overnight, kmemleak reported once [2]. Not
> > sure this is something 100% reproducible.
> >
> > [1] while true; do \
> >    echo e6055400.gpio 12,13 > /sys/bus/platform/drivers/gpio-aggregator/new_device; \
> >    echo gpio-aggregator.0 > /sys/bus/platform/drivers/gpio-aggregator/delete_device; \
> >    done
> >
> > [2] unreferenced object 0xffff0006d2c2e000 (size 128):
> >   comm "kworker/3:1", pid 55, jiffies 4294676978 (age 38546.676s)
> >   hex dump (first 32 bytes):
> >     00 d9 d2 d3 06 00 ff ff 0c 00 e0 0f ff ff ff ff  ................
> >     01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >   backtrace:
> >     [<00000000a8e18c13>] slab_post_alloc_hook+0x8c/0x94
> >     [<000000006f419a4f>] __kmalloc+0x170/0x218
> >     [<0000000060d185ea>] kobj_map+0x78/0x1c0
> >     [<00000000c96645f3>] cdev_add+0x68/0x94
> >     [<00000000a7a5a8ac>] cdev_device_add+0x74/0x90
> >     [<00000000497871d3>] gpiochip_setup_dev+0x84/0x1f0
> >     [<00000000b993f95f>] gpiochip_add_data_with_key+0xbcc/0x11f0
> >     [<00000000fd728c0e>] devm_gpiochip_add_data+0x60/0xa8
> >     [<00000000442e34c1>] gpio_aggregator_probe+0x210/0x3c8
> >     [<00000000076e13fb>] platform_drv_probe+0x70/0xe4
> >     [<00000000de84b58b>] really_probe+0x2d8/0x434
> >     [<00000000c95c9784>] driver_probe_device+0x15c/0x16c
> >     [<00000000afb7dd4f>] __device_attach_driver+0xdc/0x120
> >     [<00000000efa40cae>] bus_for_each_drv+0x12c/0x154
> >     [<00000000c149acef>] __device_attach+0x148/0x1e0
> >     [<00000000a74fd158>] device_initial_probe+0x24/0x30
> 
> This is the allocation of the GPIO character device, which is allocated
> in response to the creation of the GPIO chip, from .probe().
> As that is done using devm_gpiochip_add_data(), the chardev should be
> deallocated automatically by devm_gpio_chip_release() when
> platform_device_unregister() is called.
> 
> Weird...

It might have been a false positive. Kmemleak is not w/o flaws.
I will retest and report later. In any case, it does not look
severe to me.