| Message ID | 1409930524-1529-1-git-send-email-octavian.purdila@intel.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(). -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)