Re: [PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips

[Michal Simek <michal.simek@petalogix.com>]

Anton Vorontsov wrote:
> OF GPIO infrastructure is using dynamic GPIO bases, so it is possible
> that of_get_gpio()'s returned GPIO number will be no longer valid, or
> worse, it may point to an unexpected GPIO controller.

I am not able to apply this last patch.

$ git-am < \[PATCH\ 3_3\]\ of_gpio\:\ Introduce\ of_put_gpio\(\)\,\ add\ 
ref\ counting\ for\ OF\ GPIO\ chips.eml
Applying of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO 
chips
error: patch failed: drivers/of/gpio.c:254
error: drivers/of/gpio.c: patch does not apply
Patch failed at 0001.
When you have resolved this problem run "git-am --resolved".
If you would prefer to skip this patch, instead run "git-am --skip".

Below is my git-log which patches I applied.
Please fix it.

Thanks,
Michal

99d5baaa53562252a896080bf426b4ae71a5e55f of: Introduce safe accessors 
for node->data
df6d71af9c0691716ad8a368609a7e26a8dfdaa0 of platforms: Move common 
static initialization to of_node_init()
566969302a9dd70ce8a07c978bd08804ee73d0d6 powerpc/mcu_mpc8349emitx: 
Remove OF GPIO handling stuff
816932ea74919867c1f899758ea4cdd138145453 of/gpio: Implement GPIOLIB 
notifier hooks
3ea035a9e0f446b2b9cd9bacf73395a0c80ae3e9 of/gpio: Implement GPIOLIB 
notifier hooks
b7b96a9ee215c800476d002b06a338c4499f3813 of/gpio: Add support for 
two-stage registration for the of_gpio_chips
15678cc192c03a2b1bbb36da18ff2a3fe2d78897 gpiolib: Introduce chip 
addition/removal notifier
deb0c98c7f6035d47a247e548384517a955314a5 Merge branch 'for-2.6.33' of 
git://linux-nfs.org/~bfields/linux



> 
> This scenario is possible:
> 
> driver A:               driver B:              driver C:
> ---------               ---------              ---------
>                         gpiochip_add()
> gpio = of_get_gpio()
>                         gpiochip_remove()
>                                                gpiochip_add()
> gpio_request(gpio);
> gpio_set_value(gpio);
> 
> That is, driver A assumes that it is working with GPIO from driver B,
> but in practice it may disappear and driver C will take its GPIO base
> number, so it will provide the same GPIO numbers.
> 
> With this patch that situation is no longer possible. Though drivers
> will need to learn to put GPIOs back, so that GPIO controllers could
> be removed.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  drivers/of/gpio.c       |   82 ++++++++++++++++++++++++++++++++++++++++-------
>  include/linux/of_gpio.h |    5 +++
>  2 files changed, 75 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
> index 9d8df77..e94c5c8 100644
> --- a/drivers/of/gpio.c
> +++ b/drivers/of/gpio.c
> @@ -28,6 +28,8 @@
>   * Returns GPIO number to use with Linux generic GPIO API, or one of the errno
>   * value on the error condition. If @flags is not NULL the function also fills
>   * in flags for the GPIO.
> + *
> + * Remeber to put the GPIO back using of_put_gpio() call.
>   */
>  int of_get_gpio_flags(struct device_node *np, int index,
>  		      enum of_gpio_flags *flags)
> @@ -46,6 +48,8 @@ int of_get_gpio_flags(struct device_node *np, int index,
>  		goto err0;
>  	}
>  
> +	spin_lock(&gc->data_lock);
> +
>  	of_gc = gc->data;
>  	if (!of_gc) {
>  		pr_debug("%s: gpio controller %s isn't registered\n",
> @@ -72,15 +76,62 @@ int of_get_gpio_flags(struct device_node *np, int index,
>  		goto err1;
>  
>  	ret += of_gc->chip->base;
> +
> +	if (!try_module_get(of_gc->chip->owner)) {
> +		ret = -EINVAL;
> +		goto err1;
> +	}
> +
> +	of_gc->refcnt++;
>  err1:
> +	spin_unlock(&gc->data_lock);
> +
>  	of_node_put(gc);
>  err0:
>  	pr_debug("%s exited with status %d\n", __func__, ret);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(of_get_gpio_flags);
>  
>  /**
> + * of_put_gpio - Put a GPIO back to the OF subsystem
> + * @np:		device node of the GPIO owner
> + * @index:	index of the GPIO
> + */
> +static inline void of_put_gpio(struct device_node *np, int index)
> +{
> +	int ret;
> +	struct device_node *gc;
> +	struct of_gpio_chip *of_gc;
> +
> +	ret = of_parse_phandles_with_args(np, "gpios", "#gpio-cells", index,
> +					  &gc, NULL);
> +	if (ret) {
> +		pr_debug("%s: can't parse gpios property\n", __func__);
> +		return;
> +	}
> +
> +	spin_lock(&gc->data_lock);
> +
> +	of_gc = gc->data;
> +	if (!of_gc) {
> +		pr_debug("%s: gpio controller %s isn't registered\n",
> +			 np->full_name, gc->full_name);
> +		goto err;
> +	}
> +
> +	if (of_gc->refcnt)
> +		of_gc->refcnt--;
> +	else
> +		WARN_ON(1);
> +
> +	module_put(of_gc->chip->owner);
> +err:
> +	spin_unlock(&gc->data_lock);
> +}
> +
> +/**
>   * of_gpio_count - Count GPIOs for a device
>   * @np:		device node to count GPIOs for
>   *
> @@ -254,11 +305,7 @@ static int of_gpiochip_register_simple(struct gpio_chip *chip,
>  				       struct device_node *np)
>  {
>  	struct of_gpio_chip *of_gc;
> -
> -	if (np->data) {
> -		WARN_ON(1);
> -		return -EBUSY;
> -	}
> +	int ret;
>  
>  	of_gc = kzalloc(sizeof(*of_gc), GFP_KERNEL);
>  	if (!of_gc)
> @@ -267,10 +314,12 @@ static int of_gpiochip_register_simple(struct gpio_chip *chip,
>  	of_gc->gpio_cells = 2;
>  	of_gc->xlate = of_gpio_simple_xlate;
>  	of_gc->chip = chip;
> -	np->data = of_gc;
> -	of_node_get(np);
>  
> -	return 0;
> +	ret = of_node_set_data(np, of_gc);
> +	if (ret)
> +		kfree(of_gc);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(of_gpiochip_register_simple);
>  
> @@ -286,17 +335,26 @@ static int of_gpiochip_unregister(struct gpio_chip *chip,
>  				  struct device_node *np)
>  {
>  	struct of_gpio_chip *of_gc = np->data;
> +	int ret = 0;
>  
>  	if (!of_gc || of_gc->chip != chip) {
>  		WARN_ON(1);
>  		return -EINVAL;
>  	}
>  
> -	np->data = NULL;
> -	kfree(of_gc);
> -	of_node_put(np);
> +	spin_lock(&np->data_lock);
>  
> -	return 0;
> +	if (of_gc->refcnt)
> +		ret = -EBUSY;
> +	else
> +		of_node_release_data_unlocked(np);
> +
> +	spin_unlock(&np->data_lock);
> +
> +	if (!ret)
> +		kfree(of_gc);
> +
> +	return ret;
>  }
>  
>  static int of_gpio_notify(struct notifier_block *nb, unsigned long msg,
> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
> index c74cb37..aca7ab1 100644
> --- a/include/linux/of_gpio.h
> +++ b/include/linux/of_gpio.h
> @@ -38,6 +38,7 @@ enum of_gpio_flags {
>  struct of_gpio_chip {
>  	struct gpio_chip gc; /* legacy, don't use for a new code */
>  	struct gpio_chip *chip;
> +	unsigned int refcnt;
>  	int gpio_cells;
>  	int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
>  		     const void *gpio_spec, enum of_gpio_flags *flags);
> @@ -83,6 +84,8 @@ static inline int of_get_gpio_flags(struct device_node *np, int index,
>  	return -ENOSYS;
>  }
>  
> +static inline void of_put_gpio(struct device_node *np, int index) {}
> +
>  static inline unsigned int of_gpio_count(struct device_node *np)
>  {
>  	return 0;
> @@ -97,6 +100,8 @@ static inline unsigned int of_gpio_count(struct device_node *np)
>   *
>   * Returns GPIO number to use with Linux generic GPIO API, or one of the errno
>   * value on the error condition.
> + *
> + * Remeber to put the GPIO back using of_put_gpio() call.
>   */
>  static inline int of_get_gpio(struct device_node *np, int index)
>  {


-- 
Michal Simek, Ing. (M.Eng)
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com p: +61-7-30090663,+42-0-721842854 f: +61-7-30090663