diff mbox

[2/2] gpio: gpiolib: set gpiochip_remove retval to void

Message ID 1401449454-30895-2-git-send-email-berthe.ab@gmail.com
State Awaiting Upstream
Headers show

Commit Message

abdoulaye berthe May 30, 2014, 11:30 a.m. UTC
This avoids handling gpiochip remove error in device
remove handler.

Signed-off-by: abdoulaye berthe <berthe.ab@gmail.com>
---
 drivers/gpio/gpiolib.c      | 24 +++++++-----------------
 include/linux/gpio/driver.h |  2 +-
 2 files changed, 8 insertions(+), 18 deletions(-)

Comments

Geert Uytterhoeven May 30, 2014, 11:39 a.m. UTC | #1
On Fri, May 30, 2014 at 1:30 PM, abdoulaye berthe <berthe.ab@gmail.com> wrote:
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1263,10 +1263,9 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip);
>   *
>   * A gpio_chip with any GPIOs still requested may not be removed.
>   */
> -int gpiochip_remove(struct gpio_chip *chip)
> +void gpiochip_remove(struct gpio_chip *chip)
>  {
>         unsigned long   flags;
> -       int             status = 0;
>         unsigned        id;
>
>         acpi_gpiochip_remove(chip);
> @@ -1278,24 +1277,15 @@ int gpiochip_remove(struct gpio_chip *chip)
>         of_gpiochip_remove(chip);
>
>         for (id = 0; id < chip->ngpio; id++) {
> -               if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
> -                       status = -EBUSY;
> -                       break;
> -               }
> -       }
> -       if (status == 0) {
> -               for (id = 0; id < chip->ngpio; id++)
> -                       chip->desc[id].chip = NULL;
> -
> -               list_del(&chip->list);
> +               if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags))
> +                       panic("gpio: removing gpiochip with gpios still requested\n");

panic?

Is this likely to happen?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ralf Baechle May 30, 2014, 3:48 p.m. UTC | #2
On Fri, May 30, 2014 at 01:39:15PM +0200, Geert Uytterhoeven wrote:

> > +               if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags))
> > +                       panic("gpio: removing gpiochip with gpios still requested\n");
> 
> panic?
> 
> Is this likely to happen?

And while we're at it - panic() is going to add a \n itself so don't pass a
string ending in \n to panic().

  Ralf
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Daney May 30, 2014, 5:33 p.m. UTC | #3
On 05/30/2014 04:39 AM, Geert Uytterhoeven wrote:
> On Fri, May 30, 2014 at 1:30 PM, abdoulaye berthe <berthe.ab@gmail.com> wrote:
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -1263,10 +1263,9 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip);
>>    *
>>    * A gpio_chip with any GPIOs still requested may not be removed.
>>    */
>> -int gpiochip_remove(struct gpio_chip *chip)
>> +void gpiochip_remove(struct gpio_chip *chip)
>>   {
>>          unsigned long   flags;
>> -       int             status = 0;
>>          unsigned        id;
>>
>>          acpi_gpiochip_remove(chip);
>> @@ -1278,24 +1277,15 @@ int gpiochip_remove(struct gpio_chip *chip)
>>          of_gpiochip_remove(chip);
>>
>>          for (id = 0; id < chip->ngpio; id++) {
>> -               if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
>> -                       status = -EBUSY;
>> -                       break;
>> -               }
>> -       }
>> -       if (status == 0) {
>> -               for (id = 0; id < chip->ngpio; id++)
>> -                       chip->desc[id].chip = NULL;
>> -
>> -               list_del(&chip->list);
>> +               if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags))
>> +                       panic("gpio: removing gpiochip with gpios still requested\n");
>
> panic?

NACK to the patch for this reason.  The strongest thing you should do 
here is WARN.

That said, I am not sure why we need this whole patch set in the first 
place.

David Daney



>
> Is this likely to happen?
>
> Gr{oetje,eeting}s,
>
>                          Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen May 30, 2014, 6:16 p.m. UTC | #4
On 05/30/2014 07:33 PM, David Daney wrote:
> On 05/30/2014 04:39 AM, Geert Uytterhoeven wrote:
>> On Fri, May 30, 2014 at 1:30 PM, abdoulaye berthe <berthe.ab@gmail.com>
>> wrote:
>>> --- a/drivers/gpio/gpiolib.c
>>> +++ b/drivers/gpio/gpiolib.c
>>> @@ -1263,10 +1263,9 @@ static void gpiochip_irqchip_remove(struct
>>> gpio_chip *gpiochip);
>>>    *
>>>    * A gpio_chip with any GPIOs still requested may not be removed.
>>>    */
>>> -int gpiochip_remove(struct gpio_chip *chip)
>>> +void gpiochip_remove(struct gpio_chip *chip)
>>>   {
>>>          unsigned long   flags;
>>> -       int             status = 0;
>>>          unsigned        id;
>>>
>>>          acpi_gpiochip_remove(chip);
>>> @@ -1278,24 +1277,15 @@ int gpiochip_remove(struct gpio_chip *chip)
>>>          of_gpiochip_remove(chip);
>>>
>>>          for (id = 0; id < chip->ngpio; id++) {
>>> -               if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
>>> -                       status = -EBUSY;
>>> -                       break;
>>> -               }
>>> -       }
>>> -       if (status == 0) {
>>> -               for (id = 0; id < chip->ngpio; id++)
>>> -                       chip->desc[id].chip = NULL;
>>> -
>>> -               list_del(&chip->list);
>>> +               if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags))
>>> +                       panic("gpio: removing gpiochip with gpios still
>>> requested\n");
>>
>> panic?
>
> NACK to the patch for this reason.  The strongest thing you should do here
> is WARN.
>
> That said, I am not sure why we need this whole patch set in the first place.

Well, what currently happens when you remove a device that is a provider of 
a gpio_chip which is still in use, is that the kernel crashes. Probably with 
a rather cryptic error message. So this patch doesn't really change the 
behavior, but makes it more explicit what is actually wrong. And even if you 
replace the panic() by a WARN() it will again just crash slightly later.

This is a design flaw in the GPIO subsystem that needs to be fixed.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrzej Hajda June 9, 2014, 1:15 p.m. UTC | #5
On 06/09/2014 01:29 PM, Lars-Peter Clausen wrote:
> On 06/09/2014 01:18 AM, Ben Dooks wrote:
>> On Fri, May 30, 2014 at 08:16:59PM +0200, Lars-Peter Clausen wrote:
>>> On 05/30/2014 07:33 PM, David Daney wrote:
>>>> On 05/30/2014 04:39 AM, Geert Uytterhoeven wrote:
>>>>> On Fri, May 30, 2014 at 1:30 PM, abdoulaye berthe <berthe.ab@gmail.com>
>>>>> wrote:
>>>>>> --- a/drivers/gpio/gpiolib.c
>>>>>> +++ b/drivers/gpio/gpiolib.c
>>>>>> @@ -1263,10 +1263,9 @@ static void gpiochip_irqchip_remove(struct
>>>>>> gpio_chip *gpiochip);
>>>>>>    *
>>>>>>    * A gpio_chip with any GPIOs still requested may not be removed.
>>>>>>    */
>>>>>> -int gpiochip_remove(struct gpio_chip *chip)
>>>>>> +void gpiochip_remove(struct gpio_chip *chip)
>>>>>>   {
>>>>>>          unsigned long   flags;
>>>>>> -       int             status = 0;
>>>>>>          unsigned        id;
>>>>>>
>>>>>>          acpi_gpiochip_remove(chip);
>>>>>> @@ -1278,24 +1277,15 @@ int gpiochip_remove(struct gpio_chip *chip)
>>>>>>          of_gpiochip_remove(chip);
>>>>>>
>>>>>>          for (id = 0; id < chip->ngpio; id++) {
>>>>>> -               if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
>>>>>> -                       status = -EBUSY;
>>>>>> -                       break;
>>>>>> -               }
>>>>>> -       }
>>>>>> -       if (status == 0) {
>>>>>> -               for (id = 0; id < chip->ngpio; id++)
>>>>>> -                       chip->desc[id].chip = NULL;
>>>>>> -
>>>>>> -               list_del(&chip->list);
>>>>>> +               if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags))
>>>>>> +                       panic("gpio: removing gpiochip with gpios still
>>>>>> requested\n");
>>>>>
>>>>> panic?
>>>>
>>>> NACK to the patch for this reason.  The strongest thing you should do here
>>>> is WARN.
>>>>
>>>> That said, I am not sure why we need this whole patch set in the first place.
>>>
>>> Well, what currently happens when you remove a device that is a
>>> provider of a gpio_chip which is still in use, is that the kernel
>>> crashes. Probably with a rather cryptic error message. So this patch
>>> doesn't really change the behavior, but makes it more explicit what
>>> is actually wrong. And even if you replace the panic() by a WARN()
>>> it will again just crash slightly later.
>>>
>>> This is a design flaw in the GPIO subsystem that needs to be fixed.
>>
>> Surely then the best way is to error out to the module unload and
>> stop the driver being unloaded?
>>
> 
> You can't error out on module unload, although that's not really relevant 
> here. gpiochip_remove() is typically called when the device that registered 
> the GPIO chip is unbound. And despite some remove() callbacks having a 
> return type of int you can not abort the removal of a device.

It is a design flaw in many subsystems having providers and consumers,
not only GPIO. The same situation is with clock providers, regulators,
phys, drm_panels, ..., at least it was such last time I have tested it.

The problem is that many frameworks assumes that lifetime of provider is
always bigger than lifetime of its consumers, and this is wrong
assumption - usually it is not possible to prevent unbinding driver from
device, so if the device is a provider there is no way to inform
consumers about his removal.

Some solution for such problems is to use some kind of availability
callbacks for requesting resources (gpios, clocks, regulators,...)
instead of simple 'getters' (clk_get, gpiod_get). Callbacks should
guarantee that the resource is always valid between callback reporting
its availability and callback reporting its removal. Such approach seems
to be complicated at the first sight but it should allow to make the
code safe and as a bonus it will allow to avoid deferred probing.
Btw I have send already RFC for such framework [1].

[1]: https://lkml.org/lkml/2014/4/30/345

Regards
Andrzej


> 
> - Lars
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrzej Hajda June 10, 2014, 6:57 a.m. UTC | #6
On 06/09/2014 03:43 PM, David Laight wrote:
> From: Of Andrzej Hajda
> ...
>>> You can't error out on module unload, although that's not really relevant
>>> here. gpiochip_remove() is typically called when the device that registered
>>> the GPIO chip is unbound. And despite some remove() callbacks having a
>>> return type of int you can not abort the removal of a device.
>>
>> It is a design flaw in many subsystems having providers and consumers,
>> not only GPIO. The same situation is with clock providers, regulators,
>> phys, drm_panels, ..., at least it was such last time I have tested it.
>>
>> The problem is that many frameworks assumes that lifetime of provider is
>> always bigger than lifetime of its consumers, and this is wrong
>> assumption - usually it is not possible to prevent unbinding driver from
>> device, so if the device is a provider there is no way to inform
>> consumers about his removal.
>>
>> Some solution for such problems is to use some kind of availability
>> callbacks for requesting resources (gpios, clocks, regulators,...)
>> instead of simple 'getters' (clk_get, gpiod_get). Callbacks should
>> guarantee that the resource is always valid between callback reporting
>> its availability and callback reporting its removal. Such approach seems
>> to be complicated at the first sight but it should allow to make the
>> code safe and as a bonus it will allow to avoid deferred probing.
>> Btw I have send already RFC for such framework [1].
> 
> Callbacks for delete are generally a locking nightmare.
> A two-way handshake is also usually needed to avoid problems
> with concurrent disconnect requests.

The framework I have proposed is lock-less[1] and concurrent requests
are serialized so both objections are invalid.

[1]: in fact the framework uses spinlock, but only to protect internal
list simple operations, and even this could be converted to fully
lock-less implementation.

Andrzej

> 
> 	David
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f48817d..022543f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1263,10 +1263,9 @@  static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip);
  *
  * A gpio_chip with any GPIOs still requested may not be removed.
  */
-int gpiochip_remove(struct gpio_chip *chip)
+void gpiochip_remove(struct gpio_chip *chip)
 {
 	unsigned long	flags;
-	int		status = 0;
 	unsigned	id;
 
 	acpi_gpiochip_remove(chip);
@@ -1278,24 +1277,15 @@  int gpiochip_remove(struct gpio_chip *chip)
 	of_gpiochip_remove(chip);
 
 	for (id = 0; id < chip->ngpio; id++) {
-		if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
-			status = -EBUSY;
-			break;
-		}
-	}
-	if (status == 0) {
-		for (id = 0; id < chip->ngpio; id++)
-			chip->desc[id].chip = NULL;
-
-		list_del(&chip->list);
+		if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags))
+			panic("gpio: removing gpiochip with gpios still requested\n");
 	}
+	for (id = 0; id < chip->ngpio; id++)
+		chip->desc[id].chip = NULL;
 
+	list_del(&chip->list);
 	spin_unlock_irqrestore(&gpio_lock, flags);
-
-	if (status == 0)
-		gpiochip_unexport(chip);
-
-	return status;
+	gpiochip_unexport(chip);
 }
 EXPORT_SYMBOL_GPL(gpiochip_remove);
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 1827b43..72ed256 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -138,7 +138,7 @@  extern const char *gpiochip_is_requested(struct gpio_chip *chip,
 
 /* add/remove chips */
 extern int gpiochip_add(struct gpio_chip *chip);
-extern int __must_check gpiochip_remove(struct gpio_chip *chip);
+void gpiochip_remove(struct gpio_chip *chip);
 extern struct gpio_chip *gpiochip_find(void *data,
 			      int (*match)(struct gpio_chip *chip, void *data));