implement sleeping GPIO chip removal

[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 <[email protected]>

drivers/gpio/gpiolib.c +1 -1

index 21da9c1..a41f614 100644
@@ -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.
  *

drivers/gpio/gpiolib.c +9 -0

 	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?
+ *

drivers/gpio/gpiolib.c +4 -0

+			/* mark chip alive again */
+			chip->dead = 0;
+			chip->removing_task = NULL;
+

drivers/gpio/gpiolib.c +19 -0

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

include/asm-generic/gpio.h +4 -0

index ff5c660..8576732 100644
@@ -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));

Quoted 7 lines · 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.

Quoted 4 lines

This patch implements a new function which sleeps until device is free instead of returning -EBUSY like gpiochip_remove().

Signed-off-by Maciej Szmigiero <[email protected]>

This patch makes me uncomfortable, but I'm not entirely sure why. Is

Quoted 11 lines

[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 <[email protected]>

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

Quoted 4 lines

drivers/gpio/gpiolib.c +0 -0

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

drivers/gpio/gpiolib.c static int gpio_ensure_requested(struct gpio_desc *desc, unsigned offset) · line 101

Quoted 4 lines

drivers/gpio/gpiolib.c +4 -0

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

drivers/gpio/gpiolib.c int gpiochip_remove(struct gpio_chip *chip) · line 1159

Quoted 7 lines

drivers/gpio/gpiolib.c +7 -0

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

Quoted 4 lines

drivers/gpio/gpiolib.c +4 -0

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

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

W dniu 10.11.2010 10:49, Thomas Gleixner pisze:

Quoted 4 lines

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.

Quoted 11 lines

W dniu 10.11.2010 10:49, Thomas Gleixner pisze:

Quoted 4 lines

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.

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

Quoted 1 line · Maciej Szmigiero

You misunderstood me.

No, I didnt.

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 24 lines · Thomas Gleixner

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

Quoted 1 line · Maciej Szmigiero

You misunderstood me.

No, I didnt.

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 4 lines · Grant Likely

Quoted 2 lines · Thomas Gleixner

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

Quoted 4 lines

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

Quoted 13 lines · Grant Likely

Quoted 11 lines · Thomas Gleixner

Quoted 5 lines

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.

Quoted 4 lines

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

W dniu 10.11.2010 21:42, Thomas Gleixner pisze:

Quoted 30 lines · Maciej Szmigiero

Quoted 11 lines

W dniu 10.11.2010 10:49, Thomas Gleixner pisze:

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 46 lines

W dniu 10.11.2010 21:42, Thomas Gleixner pisze:

Quoted 30 lines · Maciej Szmigiero

Quoted 11 lines

W dniu 10.11.2010 10:49, Thomas Gleixner pisze:

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 3 lines

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.