Re: gpiolib: add export/unexport by gpio name

[David Brownell <david-b@pacbell.net>]

On Tuesday 04 August 2009, ben@fluff.org wrote:
> Add the facility to export/unexport a gpio by the name assigned to it
> as well as the number.

I guess I'm still a bit puzzled by the notion of adding
second/parallel naming scheme for GPIOs.  As a rule, such
duplication is undesirable ...

But assuming such things are OK, this patch has a couple
technical problems.


> Signed-off-by: Ben Dooks <ben@simtec.co.uk>
> Cc: David Brownell <dbrownell@users.sourceforge.net>
> 
> ---
>  drivers/gpio/gpiolib.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 66 insertions(+), 4 deletions(-)
> 
> Index: b/drivers/gpio/gpiolib.c
> ===================================================================
> --- a/drivers/gpio/gpiolib.c	2009-08-04 11:53:53.000000000 +0100
> +++ b/drivers/gpio/gpiolib.c	2009-08-04 11:54:12.000000000 +0100
> @@ -364,6 +364,64 @@ static const struct attribute_group gpio
>  	.attrs = (struct attribute **) gpiochip_attrs,
>  };
>  
> +static int search_names(char **names, const char *match, int ngpio)
> +{
> +	int len = strlen(match);
> +	char *m;
> +	int ptr;
> +
> +	/* if cr terminated, remove */
> +	if (match[len-1] == '\n')
> +		len--;
> +
> +	if (names) {
> +		for (ptr = 0; ptr < ngpio; ptr++) {
> +			m = names[ptr];
> +			if (!m)
> +				continue;
> +
> +			if (strlen(m) != len)
> +				continue;
> +
> +			if (strncmp(m, match, len) == 0)
> +				return ptr;
> +		}
> +	}
> +
> +	return -ENOENT;
> +}
> +
> +static long gpio_from_string(const char *buf)
> +{
> +	struct gpio_chip *chip;
> +	struct gpio_desc *desc;
> +	long status;
> +	long gpio;
> +	int ptr, off;
> +
> +	status = strict_strtol(buf, 0, &gpio);
> +	if (status < 0) {
> +		for (ptr = 0; ptr < ARCH_NR_GPIOS && status < 0;) {

Nothing is guarding against
 (a) duplicate chip->names[] entries within or between chips,
 (b) concurrent addition/removal of gpio_chip instances

For (a) maybe the answer is to detect them at chip registration
time ... then either fail, or else warn then null out the dup.

For (b) it seems that gpio_lock is required.

> +			desc = gpio_desc + ptr;
> +			chip = desc->chip;
> +
> +			if (!chip) {
> +				ptr++;
> +				continue;
> +			}
> +
> +			off = search_names(chip->names, buf, chip->ngpio);
> +			if (off >= 0)
> +				status = ptr + off;
> +
> +			ptr += chip->ngpio;
> +		}
> +	} else
> +		status = gpio;
> +
> +	return status;
> +}
> +
>  /*
>   * /sys/class/gpio/export ... write-only
>   *	integer N ... number of GPIO to export (full access)
> @@ -375,9 +433,11 @@ static ssize_t export_store(struct class
>  	long	gpio;
>  	int	status;
>  
> -	status = strict_strtol(buf, 0, &gpio);
> -	if (status < 0)
> +	gpio = gpio_from_string(buf);
> +	if (gpio < 0) {
> +		status = gpio;
>  		goto done;
> +	}
>  
>  	/* No extra locking here; FLAG_SYSFS just signifies that the
>  	 * request and export were done by on behalf of userspace, so
> @@ -405,9 +465,11 @@ static ssize_t unexport_store(struct cla
>  	long	gpio;
>  	int	status;
>  
> -	status = strict_strtol(buf, 0, &gpio);
> -	if (status < 0)
> +	gpio = gpio_from_string(buf);
> +	if (gpio < 0) {
> +		status = gpio;
>  		goto done;
> +	}
>  
>  	status = -EINVAL;
>  
> 
> -- 
> 
>