[PATCH] gpiolib: fix a few issues with gpiochip_remove

[PATCH] gpiolib: fix a few issues with gpiochip_remove

[Octavian Purdila]

The current implementation of gpiochip_remove() does not check to see
if the GPIO pins are busy before removing the associated irqchip and
this is causing the following warning:

WARNING: CPU: 3 PID: 553 at fs/proc/generic.c:521 remove_proc_entry+0x19f/0x1b0()
remove_proc_entry: removing non-empty directory 'irq/24', leaking at least 'bmc150_accel_event'
Call Trace:
 [<ffffffff81a78504>] dump_stack+0x4e/0x7a
 [<ffffffff810c79bd>] warn_slowpath_common+0x7d/0xa0
 [<ffffffff810c7a2c>] warn_slowpath_fmt+0x4c/0x50
 [<ffffffff8125259f>] remove_proc_entry+0x19f/0x1b0
 [<ffffffff811138ae>] unregister_irq_proc+0xce/0xf0
 [<ffffffff8110dbc1>] free_desc+0x31/0x70
 [<ffffffff8110dc3c>] irq_free_descs+0x3c/0x80
 [<ffffffff81113096>] irq_dispose_mapping+0x36/0x50
 [<ffffffff8148549a>] gpiochip_remove+0x5a/0x160
 [<ffffffff814895d8>] dln2_do_remove+0x18/0x80
 [<ffffffff8148966a>] dln2_gpio_remove+0x2a/0x30
 [<ffffffff816143bd>] platform_drv_remove+0x1d/0x40
...

and bug:

BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
in_atomic(): 1, irqs_disabled(): 1, pid: 553, name: khubd
Preemption disabled at:[<ffffffff81485462>] gpiochip_remove+0x22/0x160
Call Trace:
 [<ffffffff81a78504>] dump_stack+0x4e/0x7a
 [<ffffffff810e8dff>] __might_sleep+0x10f/0x180
 [<ffffffff81a7f3f0>] mutex_lock+0x20/0x3d
 [<ffffffff8110dbcd>] free_desc+0x3d/0x70
 [<ffffffff8110dc3c>] irq_free_descs+0x3c/0x80
 [<ffffffff81113096>] irq_dispose_mapping+0x36/0x50
 [<ffffffff8148549a>] gpiochip_remove+0x5a/0x160
 [<ffffffff814895d8>] dln2_do_remove+0x18/0x80
 [<ffffffff8148966a>] dln2_gpio_remove+0x2a/0x30
 [<ffffffff816143bd>] platform_drv_remove+0x1d/0x40
...

The current implementaion also does a partial cleanup if one of the
pins is busy, which makes it impossible to retry the remove operation
later.

A retry operation is needed in the case of MFD devices that bundles a
GPIO device and another device that is an indirect consumer of the
GPIO device (typical an I2C bus).

In this case, when the MFD device is removed, if an I2C device
associated with the I2C bus of the MFD device is using a GPIO pin (as
an interrupt source for example), and the remove routine for the GPIO
device is called first, then the removal of the gpio chip will fail.

However, we can later retry the gpio chip removal, as the I2C bus will
eventually be removed which will cause the I2C device to release the
GPIO pin.

This patch modifies gpiochip_remove to be atomic (i.e. if it fails no
partial cleanup is done) and it also moves gpiochip_irqchip_remove()
out of the spinlock to avoid the bug above.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/gpio/gpiolib.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 15cc0bb..0f53bef 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -314,14 +314,8 @@ int gpiochip_remove(struct gpio_chip *chip)
 	int		status = 0;
 	unsigned	id;
 
-	acpi_gpiochip_remove(chip);
-
 	spin_lock_irqsave(&gpio_lock, flags);
 
-	gpiochip_irqchip_remove(chip);
-	gpiochip_remove_pin_ranges(chip);
-	of_gpiochip_remove(chip);
-
 	for (id = 0; id < chip->ngpio; id++) {
 		if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
 			status = -EBUSY;
@@ -337,8 +331,13 @@ int gpiochip_remove(struct gpio_chip *chip)
 
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
-	if (status == 0)
+	if (status == 0) {
+		gpiochip_irqchip_remove(chip);
+		gpiochip_remove_pin_ranges(chip);
+		of_gpiochip_remove(chip);
+		acpi_gpiochip_remove(chip);
 		gpiochip_unexport(chip);
+	}
 
 	return status;
 }
-- 
1.9.1

Re: [PATCH] gpiolib: fix a few issues with gpiochip_remove

[Alexandre Courbot]

On Sat, Sep 6, 2014 at 12:22 AM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> The current implementation of gpiochip_remove() does not check to see
> if the GPIO pins are busy before removing the associated irqchip and
> this is causing the following warning:
>
> WARNING: CPU: 3 PID: 553 at fs/proc/generic.c:521 remove_proc_entry+0x19f/0x1b0()
> remove_proc_entry: removing non-empty directory 'irq/24', leaking at least 'bmc150_accel_event'
> Call Trace:
>  [<ffffffff81a78504>] dump_stack+0x4e/0x7a
>  [<ffffffff810c79bd>] warn_slowpath_common+0x7d/0xa0
>  [<ffffffff810c7a2c>] warn_slowpath_fmt+0x4c/0x50
>  [<ffffffff8125259f>] remove_proc_entry+0x19f/0x1b0
>  [<ffffffff811138ae>] unregister_irq_proc+0xce/0xf0
>  [<ffffffff8110dbc1>] free_desc+0x31/0x70
>  [<ffffffff8110dc3c>] irq_free_descs+0x3c/0x80
>  [<ffffffff81113096>] irq_dispose_mapping+0x36/0x50
>  [<ffffffff8148549a>] gpiochip_remove+0x5a/0x160
>  [<ffffffff814895d8>] dln2_do_remove+0x18/0x80
>  [<ffffffff8148966a>] dln2_gpio_remove+0x2a/0x30
>  [<ffffffff816143bd>] platform_drv_remove+0x1d/0x40
> ...
>
> and bug:
>
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
> in_atomic(): 1, irqs_disabled(): 1, pid: 553, name: khubd
> Preemption disabled at:[<ffffffff81485462>] gpiochip_remove+0x22/0x160
> Call Trace:
>  [<ffffffff81a78504>] dump_stack+0x4e/0x7a
>  [<ffffffff810e8dff>] __might_sleep+0x10f/0x180
>  [<ffffffff81a7f3f0>] mutex_lock+0x20/0x3d
>  [<ffffffff8110dbcd>] free_desc+0x3d/0x70
>  [<ffffffff8110dc3c>] irq_free_descs+0x3c/0x80
>  [<ffffffff81113096>] irq_dispose_mapping+0x36/0x50
>  [<ffffffff8148549a>] gpiochip_remove+0x5a/0x160
>  [<ffffffff814895d8>] dln2_do_remove+0x18/0x80
>  [<ffffffff8148966a>] dln2_gpio_remove+0x2a/0x30
>  [<ffffffff816143bd>] platform_drv_remove+0x1d/0x40
> ...
>
> The current implementaion also does a partial cleanup if one of the
> pins is busy, which makes it impossible to retry the remove operation
> later.
>
> A retry operation is needed in the case of MFD devices that bundles a
> GPIO device and another device that is an indirect consumer of the
> GPIO device (typical an I2C bus).
>
> In this case, when the MFD device is removed, if an I2C device
> associated with the I2C bus of the MFD device is using a GPIO pin (as
> an interrupt source for example), and the remove routine for the GPIO
> device is called first, then the removal of the gpio chip will fail.
>
> However, we can later retry the gpio chip removal, as the I2C bus will
> eventually be removed which will cause the I2C device to release the
> GPIO pin.
>
> This patch modifies gpiochip_remove to be atomic (i.e. if it fails no
> partial cleanup is done) and it also moves gpiochip_irqchip_remove()
> out of the spinlock to avoid the bug above.
>
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  drivers/gpio/gpiolib.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 15cc0bb..0f53bef 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -314,14 +314,8 @@ int gpiochip_remove(struct gpio_chip *chip)
>         int             status = 0;
>         unsigned        id;
>
> -       acpi_gpiochip_remove(chip);
> -
>         spin_lock_irqsave(&gpio_lock, flags);
>
> -       gpiochip_irqchip_remove(chip);
> -       gpiochip_remove_pin_ranges(chip);
> -       of_gpiochip_remove(chip);
> -
>         for (id = 0; id < chip->ngpio; id++) {
>                 if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
>                         status = -EBUSY;
> @@ -337,8 +331,13 @@ int gpiochip_remove(struct gpio_chip *chip)
>
>         spin_unlock_irqrestore(&gpio_lock, flags);
>
> -       if (status == 0)
> +       if (status == 0) {
> +               gpiochip_irqchip_remove(chip);
> +               gpiochip_remove_pin_ranges(chip);
> +               of_gpiochip_remove(chip);
> +               acpi_gpiochip_remove(chip);
>                 gpiochip_unexport(chip);
> +       }
>
>         return status;
>  }

This seems to be much better this way indeed. But isn't it still
possible for a pin to become requested between the time the spinlock
is released and the time the function exits?

Re: [PATCH] gpiolib: fix a few issues with gpiochip_remove

[Octavian Purdila]

On Fri, Sep 19, 2014 at 11:31 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Sat, Sep 6, 2014 at 12:22 AM, Octavian Purdila
> <octavian.purdila@intel.com> wrote:
>> The current implementation of gpiochip_remove() does not check to see
>> if the GPIO pins are busy before removing the associated irqchip and
>> this is causing the following warning:
>>
>> WARNING: CPU: 3 PID: 553 at fs/proc/generic.c:521 remove_proc_entry+0x19f/0x1b0()
>> remove_proc_entry: removing non-empty directory 'irq/24', leaking at least 'bmc150_accel_event'
>> Call Trace:
>>  [<ffffffff81a78504>] dump_stack+0x4e/0x7a
>>  [<ffffffff810c79bd>] warn_slowpath_common+0x7d/0xa0
>>  [<ffffffff810c7a2c>] warn_slowpath_fmt+0x4c/0x50
>>  [<ffffffff8125259f>] remove_proc_entry+0x19f/0x1b0
>>  [<ffffffff811138ae>] unregister_irq_proc+0xce/0xf0
>>  [<ffffffff8110dbc1>] free_desc+0x31/0x70
>>  [<ffffffff8110dc3c>] irq_free_descs+0x3c/0x80
>>  [<ffffffff81113096>] irq_dispose_mapping+0x36/0x50
>>  [<ffffffff8148549a>] gpiochip_remove+0x5a/0x160
>>  [<ffffffff814895d8>] dln2_do_remove+0x18/0x80
>>  [<ffffffff8148966a>] dln2_gpio_remove+0x2a/0x30
>>  [<ffffffff816143bd>] platform_drv_remove+0x1d/0x40
>> ...
>>
>> and bug:
>>
>> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
>> in_atomic(): 1, irqs_disabled(): 1, pid: 553, name: khubd
>> Preemption disabled at:[<ffffffff81485462>] gpiochip_remove+0x22/0x160
>> Call Trace:
>>  [<ffffffff81a78504>] dump_stack+0x4e/0x7a
>>  [<ffffffff810e8dff>] __might_sleep+0x10f/0x180
>>  [<ffffffff81a7f3f0>] mutex_lock+0x20/0x3d
>>  [<ffffffff8110dbcd>] free_desc+0x3d/0x70
>>  [<ffffffff8110dc3c>] irq_free_descs+0x3c/0x80
>>  [<ffffffff81113096>] irq_dispose_mapping+0x36/0x50
>>  [<ffffffff8148549a>] gpiochip_remove+0x5a/0x160
>>  [<ffffffff814895d8>] dln2_do_remove+0x18/0x80
>>  [<ffffffff8148966a>] dln2_gpio_remove+0x2a/0x30
>>  [<ffffffff816143bd>] platform_drv_remove+0x1d/0x40
>> ...
>>
>> The current implementaion also does a partial cleanup if one of the
>> pins is busy, which makes it impossible to retry the remove operation
>> later.
>>
>> A retry operation is needed in the case of MFD devices that bundles a
>> GPIO device and another device that is an indirect consumer of the
>> GPIO device (typical an I2C bus).
>>
>> In this case, when the MFD device is removed, if an I2C device
>> associated with the I2C bus of the MFD device is using a GPIO pin (as
>> an interrupt source for example), and the remove routine for the GPIO
>> device is called first, then the removal of the gpio chip will fail.
>>
>> However, we can later retry the gpio chip removal, as the I2C bus will
>> eventually be removed which will cause the I2C device to release the
>> GPIO pin.
>>
>> This patch modifies gpiochip_remove to be atomic (i.e. if it fails no
>> partial cleanup is done) and it also moves gpiochip_irqchip_remove()
>> out of the spinlock to avoid the bug above.
>>
>> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
>> ---
>>  drivers/gpio/gpiolib.c | 13 ++++++-------
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 15cc0bb..0f53bef 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -314,14 +314,8 @@ int gpiochip_remove(struct gpio_chip *chip)
>>         int             status = 0;
>>         unsigned        id;
>>
>> -       acpi_gpiochip_remove(chip);
>> -
>>         spin_lock_irqsave(&gpio_lock, flags);
>>
>> -       gpiochip_irqchip_remove(chip);
>> -       gpiochip_remove_pin_ranges(chip);
>> -       of_gpiochip_remove(chip);
>> -
>>         for (id = 0; id < chip->ngpio; id++) {
>>                 if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
>>                         status = -EBUSY;
>> @@ -337,8 +331,13 @@ int gpiochip_remove(struct gpio_chip *chip)
>>
>>         spin_unlock_irqrestore(&gpio_lock, flags);
>>
>> -       if (status == 0)
>> +       if (status == 0) {
>> +               gpiochip_irqchip_remove(chip);
>> +               gpiochip_remove_pin_ranges(chip);
>> +               of_gpiochip_remove(chip);
>> +               acpi_gpiochip_remove(chip);
>>                 gpiochip_unexport(chip);
>> +       }
>>
>>         return status;
>>  }
>
> This seems to be much better this way indeed. But isn't it still
> possible for a pin to become requested between the time the spinlock
> is released and the time the function exits?

Good point. We probably need to add a flag to gpio_chip to mark that
we are in the cleanup process and that we should not allow requesting
pins after gpiochip_remove has been called.

Johan raised another issues in a separate thread: if the pin is used
by sysfs retrying won't help.

For GPIO devices that can be disconnected asynchronously (ike USB), I
think we probably need a separate API that forcefully removes the
gpio_chip.

Re: [PATCH] gpiolib: fix a few issues with gpiochip_remove

[Linus Walleij]

On Fri, Sep 5, 2014 at 5:22 PM, Octavian Purdila
<octavian.purdila@intel.com> wrote:

> The current implementation of gpiochip_remove() does not check to see
> if the GPIO pins are busy before removing the associated irqchip and
> this is causing the following warning:
(...)
> A retry operation is needed in the case of MFD devices that bundles a
> GPIO device and another device that is an indirect consumer of the
> GPIO device (typical an I2C bus).

We have just finalized a set of patches making gpiochip_remove()
not return an error code, because it just must work.

It seems like this patch want us to sort of reverse that whole train
of work and go back to the old style of having gpiochip_remove()
be able to fail :-/

I would suggest that if this usecase is to be supported for thing
like MFD devices, we need to introduce gpiochip_try_remove()
in parallel with the current implementation, so that those drivers
that actually want to retry the remove the chip can use that
interface explicitly.

Can you look into this option?

Please also look over gpiochip_remove() as it looks right now
in linux-next...

Yours,
Linus Walleij

Re: [PATCH] gpiolib: fix a few issues with gpiochip_remove

[Octavian Purdila]

On Wed, Sep 24, 2014 at 11:51 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Fri, Sep 5, 2014 at 5:22 PM, Octavian Purdila
> <octavian.purdila@intel.com> wrote:
>
>> The current implementation of gpiochip_remove() does not check to see
>> if the GPIO pins are busy before removing the associated irqchip and
>> this is causing the following warning:
> (...)
>> A retry operation is needed in the case of MFD devices that bundles a
>> GPIO device and another device that is an indirect consumer of the
>> GPIO device (typical an I2C bus).
>
> We have just finalized a set of patches making gpiochip_remove()
> not return an error code, because it just must work.
>
> It seems like this patch want us to sort of reverse that whole train
> of work and go back to the old style of having gpiochip_remove()
> be able to fail :-/
>
> I would suggest that if this usecase is to be supported for thing
> like MFD devices, we need to introduce gpiochip_try_remove()
> in parallel with the current implementation, so that those drivers
> that actually want to retry the remove the chip can use that
> interface explicitly.
>
> Can you look into this option?
>
> Please also look over gpiochip_remove() as it looks right now
> in linux-next...
>

I was not aware of the changes to gpiochip_remove() when I sent this
patch and I do agree that it is better to have gpiochip_remove() not
be able to fail.

The main issue is actually with USB devices not necessarily MFD. If
the USB device is unplugged while IRQ GPIOs are requested then I see
the issues mentioned in the patch.

I will try to rework the patch to use the assumption that
gpiochip_remove() must not fail. That will likely need adding some
reference counting when calling gpiod_request() and gpiod_free().