Re: [GPIO]implement sleeping GPIO chip removal

[Grant Likely <grant.likely@secretlab.ca>]

On Sun, Nov 07, 2010 at 07:30:33PM +0100, Maciej Szmigiero wrote:
> [GPIO]implement sleeping GPIO chip removal
> 
> Existing GPIO chip removal code is only of "non-blocking" type: if the chip is currently
> requested it just returns -EBUSY.
> This is bad for devices which disappear and reappear, like those on hot pluggable buses,
> because it forces the driver to call gpiochip_remove() in loop until it returns 0.
> 
> This patch implements a new function which sleeps until device is free instead of
> returning -EBUSY like gpiochip_remove().
> 
> Signed-off-by: Maciej Szmigiero <mhej@o2.pl>

This patch makes me uncomfortable, but I'm not entirely sure why.  Is
there a reason that the process is manipulated directly instead of
using a completion?  Perhaps I'm bother by the joint use of
->dead + ->removing_task that is bothering me.  I need to mull on this
one some more.

Also, comments below...

> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 21da9c1..a41f614 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -11,7 +11,7 @@
>  #include <linux/of_gpio.h>
>  #include <linux/idr.h>
>  #include <linux/slab.h>
> -
> +#include <linux/sched.h>
>  
>  /* Optional implementation infrastructure for GPIO interfaces.
>   *
> @@ -95,6 +95,10 @@ static int gpio_ensure_requested(struct gpio_desc *desc, unsigned offset)
>  	const struct gpio_chip *chip = desc->chip;
>  	const int gpio = chip->base + offset;
>  
> +	/* no new requests if chip is being deregistered */
> +	if ((chip->dead) && (test_bit(FLAG_REQUESTED, &desc->flags) == 0))
> +		return -ENODEV;
> +

Not holding spin lock.  Race condition.

>  	if (WARN(test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0,
>  			"autorequest GPIO-%d\n", gpio)) {
>  		if (!try_module_get(chip->owner)) {
> @@ -1041,6 +1045,11 @@ int gpiochip_add(struct gpio_chip *chip)
>  		goto fail;
>  	}
>  
> +	/* make sure is not registered as already dead */
> +	chip->dead = 0;
> +
> +	chip->removing_task = NULL;
> +
>  	spin_lock_irqsave(&gpio_lock, flags);
>  
>  	if (base < 0) {
> @@ -1134,6 +1143,75 @@ int gpiochip_remove(struct gpio_chip *chip)
>  EXPORT_SYMBOL_GPL(gpiochip_remove);
>  
>  /**
> + * gpiochip_remove_sleeping() - unregister a gpio_chip sleeping when needed
> + * @chip: the chip to unregister
> + * @interruptible: should the sleep be interruptible?
> + *
> + * If any of GPIOs are still requested this function will wait for them
> + * to be freed.
> + */
> +int gpiochip_remove_sleeping(struct gpio_chip *chip, int interruptible)
> +{
> +	unsigned	id;
> +	unsigned long	flags;
> +
> +	/* prevent new requests */
> +	chip->dead = 1;

race, grab spinlock first.

> +
> +	spin_lock_irqsave(&gpio_lock, flags);
> +
> +	while (1) {
> +		int busy = 0;
> +
> +		for (id = chip->base; id < chip->base + chip->ngpio; id++) {
> +			if (test_bit(FLAG_REQUESTED, &gpio_desc[id].flags)) {
> +				/* printk("ID %u is still requested\n", id); */

Drop the commented out printk

> +				busy = 1;
> +				break;
> +			}
> +		}

There has to be a better way to determine if a chip is still used
without resorting to a loop each and every time through.  At the very
least, this is a duplicate code block from gpiochip_remove which
should be generalized instead of duplicated.

In fact, gpiochip_remove could be called directly here (with some
spin_lock refactoring) and exit the loop if it doesn't return -EBUSY.

> +
> +		if (!busy)
> +			break;
> +
> +		set_current_state(interruptible ?
> +				TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
> +		chip->removing_task = current;
> +
> +		spin_unlock_irqrestore(&gpio_lock, flags);
> +
> +		schedule();
> +
> +		/* printk("GPIO remove woken up\n"); */

remove

> +
> +		spin_lock_irqsave(&gpio_lock, flags);
> +
> +		if (interruptible && (signal_pending(current))) {
> +			/* printk("GPIO remove signal pending\n"); */

remove

> +			/* mark chip alive again */
> +			chip->dead = 0;
> +			chip->removing_task = NULL;
> +
> +			spin_unlock_irqrestore(&gpio_lock, flags);
> +
> +			return -EINTR;
> +		}
> +	}
> +
> +	of_gpiochip_remove(chip);
> +
> +	for (id = chip->base; id < chip->base + chip->ngpio; id++)
> +		gpio_desc[id].chip = NULL;

Don't open code this.  Generalize the code in gpiochip_remove() instead.

> +
> +	spin_unlock_irqrestore(&gpio_lock, flags);
> +
> +	gpiochip_unexport(chip);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gpiochip_remove_sleeping);
> +
> +/**
>   * gpiochip_find() - iterator for locating a specific gpio_chip
>   * @data: data to pass to match function
>   * @callback: Callback function to check gpio_chip
> @@ -1186,6 +1264,12 @@ int gpio_request(unsigned gpio, const char *label)
>  	if (chip == NULL)
>  		goto done;
>  
> +	/* chip is being deregistered, prohibit new requests */
> +	if (chip->dead) {
> +		status = -ENODEV;
> +		goto done;
> +	}
> +
>  	if (!try_module_get(chip->owner))
>  		goto done;
>  
> @@ -1254,6 +1338,9 @@ void gpio_free(unsigned gpio)
>  		module_put(desc->chip->owner);
>  		clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
>  		clear_bit(FLAG_REQUESTED, &desc->flags);
> +
> +		if (chip->removing_task != NULL)
> +			wake_up_process(chip->removing_task);
>  	} else
>  		WARN_ON(extra_checks);
>  
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index ff5c660..8576732 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -119,6 +119,9 @@ struct gpio_chip {
>  	const char		*const *names;
>  	unsigned		can_sleep:1;
>  	unsigned		exported:1;
> +	unsigned		dead:1;
> +
> +	struct task_struct	*removing_task;
>  
>  #if defined(CONFIG_OF_GPIO)
>  	/*
> @@ -139,6 +142,7 @@ extern int __must_check gpiochip_reserve(int start, int ngpio);
>  /* add/remove chips */
>  extern int gpiochip_add(struct gpio_chip *chip);
>  extern int __must_check gpiochip_remove(struct gpio_chip *chip);
> +extern int gpiochip_remove_sleeping(struct gpio_chip *chip, int interruptible);
>  extern struct gpio_chip *gpiochip_find(void *data,
>  					int (*match)(struct gpio_chip *chip,
>  						     void *data));
> 
>