| Message ID | 1401449454-30895-2-git-send-email-berthe.ab@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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
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
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 > >
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
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. Then fix the GPIO code properly, don't add a new panic() to the kernel. greg k-h
On 05/31/2014 01:29 AM, Greg KH 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. > > Then fix the GPIO code properly, don't add a new panic() to the kernel. Until somebody comes up with a patch that fixes this for good I think that patch is still an improvement over the current situation. Rather than keeping the kernel running in a inconsistent state, which might cause all kinds of undefined behavior and which will lead to a crash eventually, we might as well just crash the kernel at the cause of the inconsistent state. This will make it obvious why it crashed (compared to a random stacktrace) and will also prevent the kernel from running in a undefined state. - Lars
On Sat, May 31, 2014 at 09:35:39AM +0200, Lars-Peter Clausen wrote: > On 05/31/2014 01:29 AM, Greg KH 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. > > > >Then fix the GPIO code properly, don't add a new panic() to the kernel. > > Until somebody comes up with a patch that fixes this for good I > think that patch is still an improvement over the current situation. > Rather than keeping the kernel running in a inconsistent state, > which might cause all kinds of undefined behavior and which will > lead to a crash eventually, we might as well just crash the kernel > at the cause of the inconsistent state. This will make it obvious > why it crashed (compared to a random stacktrace) and will also > prevent the kernel from running in a undefined state. > Really, adding a panic here is not ok. With a WARN() then you have time to save the dmesg to a file. This CC list is way too huge. We're all wondering how often this stuff crashes anyway? Have you tried to fix the bug instead of just calling panic()? Is there a bugzilla entry or something? Is there a thread on the list? Just add a WARN() and fix the bug then cleanup the error handling. regards, dan carpenter
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?
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. - Lars
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));
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(-)