[PATCH 1/2] gpio: Fix crash in gpiod_set_debounce()

[PATCH 1/2] gpio: Fix crash in gpiod_set_debounce()

[Thierry Reding]

Return an error if neither the ->set() nor the ->set_debounce() function
is implemented by the chip. Furthermore move locking further down so the
lock doesn't have to be unlocked on error. This is safe to do because at
this point the lock doesn't protect anything.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Linus,

Feel free to squash this into the commit that introduced these:

	fc9bbfb: gpio: improve error path in gpiolib

Thierry

 drivers/gpio/gpiolib.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index acd19c9..9f8a134 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1777,14 +1777,15 @@ static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 		return -EINVAL;
 	}
 
-	spin_lock_irqsave(&gpio_lock, flags);
-
 	chip = desc->chip;
 	if (!chip->set || !chip->set_debounce) {
 	  pr_warn("%s: missing set() or set_debounce() operations\n",
 		  __func__);
+		return -EIO;
 	}
 
+	spin_lock_irqsave(&gpio_lock, flags);
+
 	status = gpio_ensure_requested(desc);
 	if (status < 0)
 		goto fail;
-- 
1.8.4

[PATCH 2/2] gpio: Use proper indentation

[Thierry Reding]

Indentation should be done using tabs, not a combination of tabs and
spaces.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Linus,

Feel free to squash this into the commit that introduced these:

	fc9bbfb: gpio: improve error path in gpiolib

Thierry

 drivers/gpio/gpiolib.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9f8a134..b762718 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1636,8 +1636,8 @@ static int gpiod_direction_input(struct gpio_desc *desc)
 
 	chip = desc->chip;
 	if (!chip->get || !chip->direction_input) {
-	  pr_warn("%s: missing get() or direction_input() operations\n",
-		  __func__);
+		pr_warn("%s: missing get() or direction_input() operations\n",
+			__func__);
 		return -EIO;
 	}
 
@@ -1709,8 +1709,8 @@ static int gpiod_direction_output(struct gpio_desc *desc, int value)
 
 	chip = desc->chip;
 	if (!chip->set || !chip->direction_output) {
-	  pr_warn("%s: missing set() or direction_output() operations\n",
-		  __func__);
+		pr_warn("%s: missing set() or direction_output() operations\n",
+			__func__);
 		return -EIO;
 	}
 
@@ -1779,8 +1779,8 @@ static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 
 	chip = desc->chip;
 	if (!chip->set || !chip->set_debounce) {
-	  pr_warn("%s: missing set() or set_debounce() operations\n",
-		  __func__);
+		pr_warn("%s: missing set() or set_debounce() operations\n",
+			__func__);
 		return -EIO;
 	}
 
-- 
1.8.4

Re: [PATCH 1/2] gpio: Fix crash in gpiod_set_debounce()

[Linus Walleij]

On Tue, Sep 3, 2013 at 12:39 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:

> Return an error if neither the ->set() nor the ->set_debounce() function
> is implemented by the chip. Furthermore move locking further down so the
> lock doesn't have to be unlocked on error. This is safe to do because at
> this point the lock doesn't protect anything.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Linus,
>
> Feel free to squash this into the commit that introduced these:
>
>         fc9bbfb: gpio: improve error path in gpiolib

Hm I fixed part of this bug yesterday, but screwed up and left the dangling
spinlock in there, what is wrong with me :-(

Anyway, fixed it _finally_ now, thanks to you.

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio: Use proper indentation

[Linus Walleij]

On Tue, Sep 3, 2013 at 12:39 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:

> Indentation should be done using tabs, not a combination of tabs and
> spaces.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Linus,
>
> Feel free to squash this into the commit that introduced these:
>
>         fc9bbfb: gpio: improve error path in gpiolib

This part I actually fixed up properly yesterday, but thanks anyway.

Thanks,
Linus Walleij

Re: [PATCH 2/2] gpio: Use proper indentation

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

On Tue, Sep 03, 2013 at 02:11:39PM +0200, Linus Walleij wrote:
> On Tue, Sep 3, 2013 at 12:39 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> 
> > Indentation should be done using tabs, not a combination of tabs and
> > spaces.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Linus,
> >
> > Feel free to squash this into the commit that introduced these:
> >
> >         fc9bbfb: gpio: improve error path in gpiolib
> 
> This part I actually fixed up properly yesterday, but thanks anyway.

Yes you did. I was impatient and couldn't wait for today's linux-next to
show up. Sorry for the noise.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] gpio: Fix crash in gpiod_set_debounce()

[Kevin Hilman]

[+Olof who had mentioned lock recursion BUG in -next]

On Tue, Sep 3, 2013 at 5:10 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Sep 3, 2013 at 12:39 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>
>> Return an error if neither the ->set() nor the ->set_debounce() function
>> is implemented by the chip. Furthermore move locking further down so the
>> lock doesn't have to be unlocked on error. This is safe to do because at
>> this point the lock doesn't protect anything.
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>> ---
>> Linus,
>>
>> Feel free to squash this into the commit that introduced these:
>>
>>         fc9bbfb: gpio: improve error path in gpiolib
>
> Hm I fixed part of this bug yesterday, but screwed up and left the dangling
> spinlock in there, what is wrong with me :-(
>
> Anyway, fixed it _finally_ now, thanks to you.

Exiting without unlocking was causing a lock recurision lockup in
next-20130903 on exynos5/arndale.  I just verified that moving the
spinlock down as propsed here fixes the problem in -next.

Thanks,

Kevin

Re: [PATCH 1/2] gpio: Fix crash in gpiod_set_debounce()

[Stephen Warren]

On 09/03/2013 04:39 AM, Thierry Reding wrote:
> Return an error if neither the ->set() nor the ->set_debounce() function
> is implemented by the chip. Furthermore move locking further down so the
> lock doesn't have to be unlocked on error. This is safe to do because at
> this point the lock doesn't protect anything.

> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c

>  	chip = desc->chip;
>  	if (!chip->set || !chip->set_debounce) {
>  	  pr_warn("%s: missing set() or set_debounce() operations\n",
>  		  __func__);
> +		return -EIO;
>  	}

BTW, I'm not sure that error-path should pr_warn(). For example, if this
error-patch is taken due to a call from
gpio_keys.c:gpio_keys_setup_key(), then a timer will be used for
debounce instead, which is all perfectly valid, and probably not
something that should be spewed to the kernel log.

Re: [PATCH 1/2] gpio: Fix crash in gpiod_set_debounce()

[Linus Walleij]

On Tue, Sep 3, 2013 at 10:25 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

>>       chip = desc->chip;
>>       if (!chip->set || !chip->set_debounce) {
>>         pr_warn("%s: missing set() or set_debounce() operations\n",
>>                 __func__);
>> +             return -EIO;
>>       }
>
> BTW, I'm not sure that error-path should pr_warn(). For example, if this
> error-patch is taken due to a call from
> gpio_keys.c:gpio_keys_setup_key(), then a timer will be used for
> debounce instead, which is all perfectly valid, and probably not
> something that should be spewed to the kernel log.

You're right, I wasn't aware of the fallback use-case,
so I've proposed a separate patch fixing this.

Yours,
Linus Walleij