[GPIO]implement sleeping GPIO chip removal

[GPIO]implement sleeping GPIO chip removal

[Maciej Szmigiero]

[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>

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;
+
 	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;
+
+	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); */
+				busy = 1;
+				break;
+			}
+		}
+
+		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"); */
+
+		spin_lock_irqsave(&gpio_lock, flags);
+
+		if (interruptible && (signal_pending(current))) {
+			/* printk("GPIO remove signal pending\n"); */
+			/* 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;
+
+	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));

Re: [GPIO]implement sleeping GPIO chip removal

[Grant Likely]

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));
> 
> 

Re: [GPIO]implement sleeping GPIO chip removal

[Thomas Gleixner]

On Tue, 9 Nov 2010, Grant Likely wrote:

> 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

Maybe because it open codes a sloppy refcounting with a loop and magic
sleeps instead of converting the code to kobjects and proper
refcounting ?

Thanks,

	tglx

Re: [GPIO]implement sleeping GPIO chip removal

[Maciej Szmigiero]

W dniu 10.11.2010 06:09, Grant Likely pisze:
> 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...
> 

>> @@ -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.

Every call to gpio_ensure_requested() in gpiolib.c is done while holding gpio_lock and this function is not exported.

>> +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.

Race with ...?
The only place where chip->dead is changed to zero is later in gpiochip_remove_sleeping() (which cannot be called at the same time for the same
chip more than once anyway) or gpiochip_add() which is new chip registration function.

>> +				busy = 1;
>> +				break;
>> +			}
>> +		}
> 
(..)
> In fact, gpiochip_remove could be called directly here (with some
> spin_lock refactoring) and exit the loop if it doesn't return -EBUSY.
> 
(..)
> Don't open code this.  Generalize the code in gpiochip_remove() instead.

I agree with you on this.

W dniu 10.11.2010 10:49, Thomas Gleixner pisze:
> Maybe because it open codes a sloppy refcounting with a loop and magic
> sleeps instead of converting the code to kobjects and proper
> refcounting ?
> 

The only way to do GPIO chip removal in the current code is to busy-loop.
"Sloppy" (as you called it) waiting is still more CPU-friendly than looping 
in hope that somebody will finally release the chip.
If you would like to implement it as kobject then go ahead and post the code
so it can be used in drivers.

Best regards and thanks for comments,
Maciej Szmigiero

Re: [GPIO]implement sleeping GPIO chip removal

[Thomas Gleixner]

On Wed, 10 Nov 2010, Maciej Szmigiero wrote:
> W dniu 10.11.2010 10:49, Thomas Gleixner pisze:
> > Maybe because it open codes a sloppy refcounting with a loop and magic
> > sleeps instead of converting the code to kobjects and proper
> > refcounting ?
> > 
> 
> The only way to do GPIO chip removal in the current code is to busy-loop.
> "Sloppy" (as you called it) waiting is still more CPU-friendly than looping 
> in hope that somebody will finally release the chip.
> If you would like to implement it as kobject then go ahead and post the code
> so it can be used in drivers.

Wait a moment. You are getting something backwards here.

Fact is that the current code is not designed for easy hotunplugging
and therefor requires looping.

So _you_ propose a work-around to replace the busy-loop by a sleeping
loop with "hope that ....". Hope is the least thing what counts in
programming.

Now a reviewer tells you that your idea of replacing the busy-loop by
a sleeping in hope loop is flawed, because it does not solve the
underlying design problem of the GPIO code. And you get a suggestion
how to solve it correctly.

Now you go and request from that reviewer to implement that? That's
not how it works.

You sent a flawed patch in the first place and people try to tell you
how to do it right. Then it's on you to either go and do it right or
at least ask politely for help and pointers.

Thanks,

	tglx

Re: [GPIO]implement sleeping GPIO chip removal

[Maciej Szmigiero]

W dniu 10.11.2010 21:42, Thomas Gleixner pisze:
> On Wed, 10 Nov 2010, Maciej Szmigiero wrote:
>> W dniu 10.11.2010 10:49, Thomas Gleixner pisze:
>>> Maybe because it open codes a sloppy refcounting with a loop and magic
>>> sleeps instead of converting the code to kobjects and proper
>>> refcounting ?
>>>
>>
>> The only way to do GPIO chip removal in the current code is to busy-loop.
>> "Sloppy" (as you called it) waiting is still more CPU-friendly than looping 
>> in hope that somebody will finally release the chip.
>> If you would like to implement it as kobject then go ahead and post the code
>> so it can be used in drivers.
> 
> Wait a moment. You are getting something backwards here.
> 
> Fact is that the current code is not designed for easy hotunplugging
> and therefor requires looping.
> 
> So _you_ propose a work-around to replace the busy-loop by a sleeping
> loop with "hope that ....". Hope is the least thing what counts in
> programming.
> 
> Now a reviewer tells you that your idea of replacing the busy-loop by
> a sleeping in hope loop is flawed, because it does not solve the
> underlying design problem of the GPIO code. And you get a suggestion
> how to solve it correctly.
> 
> Now you go and request from that reviewer to implement that? That's
> not how it works.
> 
> You sent a flawed patch in the first place and people try to tell you
> how to do it right. Then it's on you to either go and do it right or
> at least ask politely for help and pointers.
> 
> Thanks,
> 
> 	tglx
> 

You misunderstood me.
By "looping in hope that somebody will finally release the chip" I meant the only
real way to handle a GPIO chip unplugging in the current kernel.
Which is way worse that preventing new requests, then waiting for existing one to be released.
And this is exactly what my patch does.

I understand that it could be simplified by removing redundant code (as Grant Likely had suggested before), and
moving it to completion interface instead of manipulating a task structure directly, but this doesn't mean
that the whole GPIO code has to be rewritten just to add one functionality.

Best regards,
Maciej Szmigiero

Re: [GPIO]implement sleeping GPIO chip removal

[Thomas Gleixner]

Can you please use a mail client which does proper line breaks at 78 ?

On Wed, 10 Nov 2010, Maciej Szmigiero wrote:
> You misunderstood me.

No, I didnt.

> By "looping in hope that somebody will finally release the chip" I
> meant the only real way to handle a GPIO chip unplugging in the
> current kernel.  Which is way worse that preventing new requests,
> then waiting for existing one to be released.  And this is exactly
> what my patch does.

That still does not make it a good solution.

> I understand that it could be simplified by removing redundant code
> (as Grant Likely had suggested before), and moving it to completion
> interface instead of manipulating a task structure directly, but
> this doesn't mean that the whole GPIO code has to be rewritten just
> to add one functionality.

It's not about rewriting, it's about fixing the problem in the right
way and not just hacking around it.

If we see a shortcoming like this, we fix it and do not magically work
around it.

Thanks,

	tglx

Re: [GPIO]implement sleeping GPIO chip removal

[Grant Likely]

On Wed, Nov 10, 2010 at 10:07:05PM +0100, Thomas Gleixner wrote:
> Can you please use a mail client which does proper line breaks at 78 ?
> 
> On Wed, 10 Nov 2010, Maciej Szmigiero wrote:
> > You misunderstood me.
> 
> No, I didnt.
> 
> > By "looping in hope that somebody will finally release the chip" I
> > meant the only real way to handle a GPIO chip unplugging in the
> > current kernel.  Which is way worse that preventing new requests,
> > then waiting for existing one to be released.  And this is exactly
> > what my patch does.
> 
> That still does not make it a good solution.
> 
> > I understand that it could be simplified by removing redundant code
> > (as Grant Likely had suggested before), and moving it to completion
> > interface instead of manipulating a task structure directly, but
> > this doesn't mean that the whole GPIO code has to be rewritten just
> > to add one functionality.
> 
> It's not about rewriting, it's about fixing the problem in the right
> way and not just hacking around it.
> 
> If we see a shortcoming like this, we fix it and do not magically work
> around it.

+1

Thomas is right.  kobject reference counting is the correct solution.
Nack on this approach.

g.

Re: [GPIO]implement sleeping GPIO chip removal

[Grant Likely]

On Wed, Nov 10, 2010 at 10:01:40PM +0100, Maciej Szmigiero wrote:
> W dniu 10.11.2010 21:42, Thomas Gleixner pisze:
> > On Wed, 10 Nov 2010, Maciej Szmigiero wrote:
> >> W dniu 10.11.2010 10:49, Thomas Gleixner pisze:
> >>> Maybe because it open codes a sloppy refcounting with a loop and magic
> >>> sleeps instead of converting the code to kobjects and proper
> >>> refcounting ?
> >>>
> >>
> >> The only way to do GPIO chip removal in the current code is to busy-loop.
> >> "Sloppy" (as you called it) waiting is still more CPU-friendly than looping 
> >> in hope that somebody will finally release the chip.
> >> If you would like to implement it as kobject then go ahead and post the code
> >> so it can be used in drivers.
> > 
> > Wait a moment. You are getting something backwards here.
> > 
> > Fact is that the current code is not designed for easy hotunplugging
> > and therefor requires looping.
> > 
> > So _you_ propose a work-around to replace the busy-loop by a sleeping
> > loop with "hope that ....". Hope is the least thing what counts in
> > programming.
> > 
> > Now a reviewer tells you that your idea of replacing the busy-loop by
> > a sleeping in hope loop is flawed, because it does not solve the
> > underlying design problem of the GPIO code. And you get a suggestion
> > how to solve it correctly.
> > 
> > Now you go and request from that reviewer to implement that? That's
> > not how it works.
> > 
> > You sent a flawed patch in the first place and people try to tell you
> > how to do it right. Then it's on you to either go and do it right or
> > at least ask politely for help and pointers.
> > 
> > Thanks,
> > 
> > 	tglx
> > 
> 
> You misunderstood me.
> By "looping in hope that somebody will finally release the chip" I meant the only
> real way to handle a GPIO chip unplugging in the current kernel.
> Which is way worse that preventing new requests, then waiting for existing one to be released.
> And this is exactly what my patch does.
> 
> I understand that it could be simplified by removing redundant code (as Grant Likely had suggested before), and
> moving it to completion interface instead of manipulating a task structure directly, but this doesn't mean
> that the whole GPIO code has to be rewritten just to add one functionality.

BTW, switching to using a kobject != rewriting the whole GPIO code.
kobjects are not all that scary and the biggest impact is adding
kobject get/put operation on the gpio get/release apis.  I don't
expect it to end up being overly complex.

The Documentation/kobject.txt file should offer some insight.

g.

Re: [GPIO]implement sleeping GPIO chip removal

[Greg KH]

On Wed, Nov 10, 2010 at 02:15:40PM -0700, Grant Likely wrote:
> On Wed, Nov 10, 2010 at 10:07:05PM +0100, Thomas Gleixner wrote:
> > Can you please use a mail client which does proper line breaks at 78 ?
> > 
> > On Wed, 10 Nov 2010, Maciej Szmigiero wrote:
> > > You misunderstood me.
> > 
> > No, I didnt.
> > 
> > > By "looping in hope that somebody will finally release the chip" I
> > > meant the only real way to handle a GPIO chip unplugging in the
> > > current kernel.  Which is way worse that preventing new requests,
> > > then waiting for existing one to be released.  And this is exactly
> > > what my patch does.
> > 
> > That still does not make it a good solution.
> > 
> > > I understand that it could be simplified by removing redundant code
> > > (as Grant Likely had suggested before), and moving it to completion
> > > interface instead of manipulating a task structure directly, but
> > > this doesn't mean that the whole GPIO code has to be rewritten just
> > > to add one functionality.
> > 
> > It's not about rewriting, it's about fixing the problem in the right
> > way and not just hacking around it.
> > 
> > If we see a shortcoming like this, we fix it and do not magically work
> > around it.
> 
> +1
> 
> Thomas is right.  kobject reference counting is the correct solution.
> Nack on this approach.

Only use a kobject if you want to be in the sysfs hierarchy (which I
don't think you want to do here.)  If you want proper reference
counting, use a 'struct kref' instead.

thanks,

greg k-h

Re: [GPIO]implement sleeping GPIO chip removal

[Thomas Gleixner]

On Wed, 10 Nov 2010, Greg KH wrote:
> On Wed, Nov 10, 2010 at 02:15:40PM -0700, Grant Likely wrote:
> > On Wed, Nov 10, 2010 at 10:07:05PM +0100, Thomas Gleixner wrote:
> > > If we see a shortcoming like this, we fix it and do not magically work
> > > around it.
> > 
> > +1
> > 
> > Thomas is right.  kobject reference counting is the correct solution.
> > Nack on this approach.
> 
> Only use a kobject if you want to be in the sysfs hierarchy (which I
> don't think you want to do here.)  If you want proper reference
> counting, use a 'struct kref' instead.

Oops, yes. I always confuse those.

Thanks for pointing it out - probably not the first time :)

       tglx

Re: [GPIO]implement sleeping GPIO chip removal

[Paul Mundt]

On Wed, Nov 10, 2010 at 02:45:16PM -0800, Greg KH wrote:
> On Wed, Nov 10, 2010 at 02:15:40PM -0700, Grant Likely wrote:
> > On Wed, Nov 10, 2010 at 10:07:05PM +0100, Thomas Gleixner wrote:
> > > > I understand that it could be simplified by removing redundant code
> > > > (as Grant Likely had suggested before), and moving it to completion
> > > > interface instead of manipulating a task structure directly, but
> > > > this doesn't mean that the whole GPIO code has to be rewritten just
> > > > to add one functionality.
> > > 
> > > It's not about rewriting, it's about fixing the problem in the right
> > > way and not just hacking around it.
> > > 
> > > If we see a shortcoming like this, we fix it and do not magically work
> > > around it.
> > 
> > +1
> > 
> > Thomas is right.  kobject reference counting is the correct solution.
> > Nack on this approach.
> 
> Only use a kobject if you want to be in the sysfs hierarchy (which I
> don't think you want to do here.)  If you want proper reference
> counting, use a 'struct kref' instead.
> 
This is actually an interesting problem. The gpiolib code presently has
its own hand-rolled sysfs support, which is entirely optional. If someone
is going to go through and do some refactoring anyways it would be
worthwile to see how much tidying up using a kobject would permit. 
kobject-based refcounting could in effect be used like kref-refcounting
with the sysfs interface disabled, too.

Re: [GPIO]implement sleeping GPIO chip removal

[Maciej Szmigiero]

W dniu 10.11.2010 23:14, Grant Likely pisze:
> On Wed, Nov 10, 2010 at 10:01:40PM +0100, Maciej Szmigiero wrote:
>> W dniu 10.11.2010 21:42, Thomas Gleixner pisze:
>>> On Wed, 10 Nov 2010, Maciej Szmigiero wrote:
>>>> W dniu 10.11.2010 10:49, Thomas Gleixner pisze:
>>>>> Maybe because it open codes a sloppy refcounting with a loop and magic
>>>>> sleeps instead of converting the code to kobjects and proper
>>>>> refcounting ?
>>>>>
>>>>
>>>> The only way to do GPIO chip removal in the current code is to busy-loop.
>>>> "Sloppy" (as you called it) waiting is still more CPU-friendly than looping 
>>>> in hope that somebody will finally release the chip.
>>>> If you would like to implement it as kobject then go ahead and post the code
>>>> so it can be used in drivers.
>>>
>>> Wait a moment. You are getting something backwards here.
>>>
>>> Fact is that the current code is not designed for easy hotunplugging
>>> and therefor requires looping.
>>>
>>> So _you_ propose a work-around to replace the busy-loop by a sleeping
>>> loop with "hope that ....". Hope is the least thing what counts in
>>> programming.
>>>
>>> Now a reviewer tells you that your idea of replacing the busy-loop by
>>> a sleeping in hope loop is flawed, because it does not solve the
>>> underlying design problem of the GPIO code. And you get a suggestion
>>> how to solve it correctly.
>>>
>>> Now you go and request from that reviewer to implement that? That's
>>> not how it works.
>>>
>>> You sent a flawed patch in the first place and people try to tell you
>>> how to do it right. Then it's on you to either go and do it right or
>>> at least ask politely for help and pointers.
>>>
>>> Thanks,
>>>
>>> 	tglx
>>>
>>
>> You misunderstood me.
>> By "looping in hope that somebody will finally release the chip" I meant the only
>> real way to handle a GPIO chip unplugging in the current kernel.
>> Which is way worse that preventing new requests, then waiting for existing one to be released.
>> And this is exactly what my patch does.
>>
>> I understand that it could be simplified by removing redundant code (as Grant Likely had suggested before), and
>> moving it to completion interface instead of manipulating a task structure directly, but this doesn't mean
>> that the whole GPIO code has to be rewritten just to add one functionality.
> 
> BTW, switching to using a kobject != rewriting the whole GPIO code.
> kobjects are not all that scary and the biggest impact is adding
> kobject get/put operation on the gpio get/release apis.  I don't
> expect it to end up being overly complex.
> 
> The Documentation/kobject.txt file should offer some insight.
> 
> g.
> 

I see two approaches to porting GPIO code in current form to krefs:
either per-GPIO kref or per-chip one.

Per GPIO:
- Because every GPIO can be requested only once the refcount will always
be 1 or 2 so refcounting looks a bit like an overkill,

- Every GPIO still needs FLAG_REQUESTED to prevent being requested more
than once at the same time and to support non-blocking chip removal
without reading refcount from inside of the kref structure,

- Still needs some kind of "dead" indication per chip so nobody can
request another GPIO of this chip while it's being removed.
FLAG_REQUESTED cannot be used for this purpose by itself because then it
will be impossible to differentiate between a GPIO which has this flag
to prevent new requests and one which is currently busy.

- After every wakeup the removal code will have to scan all GPIOs under
the chip being removed and go back to sleep if any of them is still
requested.

Per chip:
- This time refcount might go up to chip->ngpio + 1 (number of GPIOs in
this chip).

- Still needs per GPIO FLAG_REQUESTED,

- This time however the removal code will be woken up just once, so it's
more efficient there.

Any other ideas or thoughts?

Maciej Szmigiero