From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lars-Peter Clausen Subject: Re: [PATCH 2/2] gpio: gpiolib: set gpiochip_remove retval to void Date: Mon, 09 Jun 2014 13:29:23 +0200 Message-ID: <53959A93.7080308@metafoo.de> References: <20140530094025.3b78301e@canb.auug.org.au> <1401449454-30895-1-git-send-email-berthe.ab@gmail.com> <1401449454-30895-2-git-send-email-berthe.ab@gmail.com> <5388C0F1.90503@gmail.com> <5388CB1B.3090802@metafoo.de> <20140608231823.GB10112@trinity.fluff.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140608231823.GB10112@trinity.fluff.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: driverdev-devel-bounces@linuxdriverproject.org To: Ben Dooks Cc: Linux MIPS Mailing List , m@bues.ch, Linux-sh list , Linus Walleij , platform-driver-x86@vger.kernel.org, "linux-leds@vger.kernel.org" , driverdevel , Alexandre Courbot , patches@opensource.wolfsonmicro.com, linux-samsungsoc@vger.kernel.org, Geert Uytterhoeven , "linux-input@vger.kernel.org" , Linux Media Mailing List , spear-devel@list.st.com, "linux-gpio@vger.kernel.org" , David Daney , "linux-arm-kernel@lists.infradead.org" , "netdev@vger.kernel.org" , linux-wireless , linux-kernel@vger.kernel. List-Id: linux-leds@vger.kernel.org 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 >>>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Received: with ECARTIS (v1.0.0; list linux-mips); Mon, 09 Jun 2014 13:29:28 +0200 (CEST) Received: from smtp-out-136.synserver.de ([212.40.185.136]:1141 "EHLO smtp-out-136.synserver.de" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S6817165AbaFIL31A0daC (ORCPT ); Mon, 9 Jun 2014 13:29:27 +0200 Received: (qmail 10595 invoked by uid 0); 9 Jun 2014 11:29:26 -0000 X-SynServer-TrustedSrc: 1 X-SynServer-AuthUser: lars@metafoo.de X-SynServer-PPID: 10469 Received: from ppp-188-174-15-59.dynamic.mnet-online.de (HELO ?192.168.178.23?) [188.174.15.59] by 217.119.54.77 with AES256-SHA encrypted SMTP; 9 Jun 2014 11:29:25 -0000 Message-ID: <53959A93.7080308@metafoo.de> Date: Mon, 09 Jun 2014 13:29:23 +0200 From: Lars-Peter Clausen User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20131103 Icedove/17.0.10 MIME-Version: 1.0 To: Ben Dooks CC: David Daney , abdoulaye berthe , Geert Uytterhoeven , Linus Walleij , Alexandre Courbot , m@bues.ch, "linux-gpio@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Linux MIPS Mailing List , "linuxppc-dev@lists.ozlabs.org" , Linux-sh list , linux-wireless , patches@opensource.wolfsonmicro.com, "linux-input@vger.kernel.org" , "linux-leds@vger.kernel.org" , Linux Media Mailing List , linux-samsungsoc@vger.kernel.org, spear-devel@list.st.com, platform-driver-x86@vger.kernel.org, "netdev@vger.kernel.org" , driverdevel Subject: Re: [PATCH 2/2] gpio: gpiolib: set gpiochip_remove retval to void References: <20140530094025.3b78301e@canb.auug.org.au> <1401449454-30895-1-git-send-email-berthe.ab@gmail.com> <1401449454-30895-2-git-send-email-berthe.ab@gmail.com> <5388C0F1.90503@gmail.com> <5388CB1B.3090802@metafoo.de> <20140608231823.GB10112@trinity.fluff.org> In-Reply-To: <20140608231823.GB10112@trinity.fluff.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s linux-mips"> (uid 0) X-Orcpt: rfc822;linux-mips@linux-mips.org Original-Recipient: rfc822;linux-mips@linux-mips.org X-archive-position: 40455 X-ecartis-version: Ecartis v1.0.0 Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org X-original-sender: lars@metafoo.de Precedence: bulk List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: linux-mips X-List-ID: linux-mips List-subscribe: List-owner: List-post: List-archive: X-list: linux-mips 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 >>>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp-out-136.synserver.de ([212.40.185.136]:1141 "EHLO smtp-out-136.synserver.de" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S6817165AbaFIL31A0daC (ORCPT ); Mon, 9 Jun 2014 13:29:27 +0200 Message-ID: <53959A93.7080308@metafoo.de> Date: Mon, 09 Jun 2014 13:29:23 +0200 From: Lars-Peter Clausen MIME-Version: 1.0 Subject: Re: [PATCH 2/2] gpio: gpiolib: set gpiochip_remove retval to void References: <20140530094025.3b78301e@canb.auug.org.au> <1401449454-30895-1-git-send-email-berthe.ab@gmail.com> <1401449454-30895-2-git-send-email-berthe.ab@gmail.com> <5388C0F1.90503@gmail.com> <5388CB1B.3090802@metafoo.de> <20140608231823.GB10112@trinity.fluff.org> In-Reply-To: <20140608231823.GB10112@trinity.fluff.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-Path: Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-subscribe: List-owner: List-post: List-archive: To: Ben Dooks Cc: David Daney , abdoulaye berthe , Geert Uytterhoeven , Linus Walleij , Alexandre Courbot , m@bues.ch, "linux-gpio@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Linux MIPS Mailing List , "linuxppc-dev@lists.ozlabs.org" , Linux-sh list , linux-wireless , patches@opensource.wolfsonmicro.com, "linux-input@vger.kernel.org" , "linux-leds@vger.kernel.org" , Linux Media Mailing List , linux-samsungsoc@vger.kernel.org, spear-devel@list.st.com, platform-driver-x86@vger.kernel.org, "netdev@vger.kernel.org" , driverdevel Message-ID: <20140609112923.MbguQFFlbWyHbiNPoVRkOJQAEUAqPYNFASfzfVb9ecs@z> 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 >>>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out-136.synserver.de (smtp-out-136.synserver.de [212.40.185.136]) by lists.ozlabs.org (Postfix) with ESMTP id 2F3FC1A0039 for ; Mon, 9 Jun 2014 21:29:30 +1000 (EST) Message-ID: <53959A93.7080308@metafoo.de> Date: Mon, 09 Jun 2014 13:29:23 +0200 From: Lars-Peter Clausen MIME-Version: 1.0 To: Ben Dooks Subject: Re: [PATCH 2/2] gpio: gpiolib: set gpiochip_remove retval to void References: <20140530094025.3b78301e@canb.auug.org.au> <1401449454-30895-1-git-send-email-berthe.ab@gmail.com> <1401449454-30895-2-git-send-email-berthe.ab@gmail.com> <5388C0F1.90503@gmail.com> <5388CB1B.3090802@metafoo.de> <20140608231823.GB10112@trinity.fluff.org> In-Reply-To: <20140608231823.GB10112@trinity.fluff.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Cc: Linux MIPS Mailing List , m@bues.ch, Linux-sh list , Linus Walleij , platform-driver-x86@vger.kernel.org, "linux-leds@vger.kernel.org" , driverdevel , Alexandre Courbot , patches@opensource.wolfsonmicro.com, linux-samsungsoc@vger.kernel.org, Geert Uytterhoeven , "linux-input@vger.kernel.org" , Linux Media Mailing List , spear-devel@list.st.com, "linux-gpio@vger.kernel.org" , David Daney , "linux-arm-kernel@lists.infradead.org" , "netdev@vger.kernel.org" , linux-wireless , "linux-kernel@vger.kernel.org" , abdoulaye berthe , "linuxppc-dev@lists.ozlabs.org" List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 >>>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: lars@metafoo.de (Lars-Peter Clausen) Date: Mon, 09 Jun 2014 13:29:23 +0200 Subject: [PATCH 2/2] gpio: gpiolib: set gpiochip_remove retval to void In-Reply-To: <20140608231823.GB10112@trinity.fluff.org> References: <20140530094025.3b78301e@canb.auug.org.au> <1401449454-30895-1-git-send-email-berthe.ab@gmail.com> <1401449454-30895-2-git-send-email-berthe.ab@gmail.com> <5388C0F1.90503@gmail.com> <5388CB1B.3090802@metafoo.de> <20140608231823.GB10112@trinity.fluff.org> Message-ID: <53959A93.7080308@metafoo.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 >>>> 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