[PATCH] gpio: gpio-ich: fix ichx_gpio_check_available() return what callers expect

[PATCH] gpio: gpio-ich: fix ichx_gpio_check_available() return what callers expect

[Mika Westerberg]

ichx_gpio_check_available() returns either 0 or -ENXIO depending on whether
the given GPIO is available or not. However, callers of this function treat
the return value as boolean:

	...
	if (!ichx_gpio_check_available(gpio, nr))
		return -ENXIO;

which erroneusly fails when the GPIO is available and not vice versa.

Fix this by making the function return boolean as expected by the callers.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpio-ich.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-ich.c b/drivers/gpio/gpio-ich.c
index 6f2306d..f9dbd50 100644
--- a/drivers/gpio/gpio-ich.c
+++ b/drivers/gpio/gpio-ich.c
@@ -128,9 +128,9 @@ static int ichx_read_bit(int reg, unsigned nr)
 	return data & (1 << bit) ? 1 : 0;
 }
 
-static int ichx_gpio_check_available(struct gpio_chip *gpio, unsigned nr)
+static bool ichx_gpio_check_available(struct gpio_chip *gpio, unsigned nr)
 {
-	return (ichx_priv.use_gpio & (1 << (nr / 32))) ? 0 : -ENXIO;
+	return ichx_priv.use_gpio & (1 << (nr / 32));
 }
 
 static int ichx_gpio_direction_input(struct gpio_chip *gpio, unsigned nr)
-- 
1.7.10.4

Re: [PATCH] gpio: gpio-ich: fix ichx_gpio_check_available() return what callers expect

[Grant Likely]

On Wed, 27 Feb 2013 17:25:15 +0200, Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> ichx_gpio_check_available() returns either 0 or -ENXIO depending on whether
> the given GPIO is available or not. However, callers of this function treat
> the return value as boolean:
> 
> 	...
> 	if (!ichx_gpio_check_available(gpio, nr))
> 		return -ENXIO;
> 
> which erroneusly fails when the GPIO is available and not vice versa.
> 
> Fix this by making the function return boolean as expected by the callers.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Applied, thanks.

g.

Re: [PATCH] gpio: gpio-ich: fix ichx_gpio_check_available() return what callers expect

[Jean Delvare]

Hi all,

On Sat, 02 Mar 2013 09:18:36 +0000, Grant Likely wrote:
> On Wed, 27 Feb 2013 17:25:15 +0200, Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> > ichx_gpio_check_available() returns either 0 or -ENXIO depending on whether
> > the given GPIO is available or not. However, callers of this function treat
> > the return value as boolean:
> > 
> > 	...
> > 	if (!ichx_gpio_check_available(gpio, nr))
> > 		return -ENXIO;
> > 
> > which erroneusly fails when the GPIO is available and not vice versa.
> > 
> > Fix this by making the function return boolean as expected by the callers.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Applied, thanks.

I am very sorry for introducing this bug. Thanks Andreas and Mika for
catching it and fixing it. Too bad stable kernel 3.7 already reached
end of life :(

My original code has been working for me for months. The reason is
that, of the 4 affected functions, only ichx_gpio_direction_output() is
ever called, and its return value is never checked by the driver
(i2c-mux-gpio.) I'll update the driver to properly check the return
value so that any issue is caught earlier in the future.

Looking at the gpio-ich code, there are two things I do not understand.

The first one is in my own code. I see that there is no usability check
on ichx_gpio_request(), while there is one on ichx_gpio_get(). This
makes little sense to me and I am really curious why I made things that
way. ichx_gpio_get() could be called repeatedly so from a performance
perspective checking for usability again and again is no good. Checking
at GPIO request time would seem preferable, allowing to catch problems
earlier. I'll change that unless anyone can figure out why I made it
the way it is.

The second one is in Peter's code:

static int ichx_gpio_request(struct gpio_chip *chip, unsigned nr)
{
	/*
	 * Note we assume the BIOS properly set a bridge's USE value.  Some
	 * chips (eg Intel 3100) have bogus USE values though, so first see if
	 * the chipset's USE value can be trusted for this specific bit.
	 * If it can't be trusted, assume that the pin can be used as a GPIO.
	 */
	if (ichx_priv.desc->use_sel_ignore[nr / 32] & (1 << (nr & 0x1f)))
		return 1;

	return ichx_read_bit(GPIO_USE_SEL, nr) ? 0 : -ENODEV;
}

Documentation/gpio.txt says: "the return value is zero for success,
else a negative errno." So I have no idea what the "1" returned here is
supposed to mean. Depending on whether the caller checks for "ret" or
"ret < 0" it will see this as success or as error. This doesn't seem
right. If I understand the comment properly, we should return 0
(success) instead. Peter?

-- 
Jean Delvare

Re: [PATCH] gpio: gpio-ich: fix ichx_gpio_check_available() return what callers expect

[Linus Walleij]

Hi Mika,

On Wed, Feb 27, 2013 at 4:25 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> -static int ichx_gpio_check_available(struct gpio_chip *gpio, unsigned nr)
> +static bool ichx_gpio_check_available(struct gpio_chip *gpio, unsigned nr)
>  {
> -       return (ichx_priv.use_gpio & (1 << (nr / 32))) ? 0 : -ENXIO;
> +       return ichx_priv.use_gpio & (1 << (nr / 32));
>  }

Strictly speaking what you're returning there is not a bool.
Shouldn't it be:

return !!(ichx_priv.use_gpio & (1 << (nr / 32)));

?

Yours,
Linus Walleij

Re: [PATCH] gpio: gpio-ich: fix ichx_gpio_check_available() return what callers expect

[Mika Westerberg]

On Thu, Mar 07, 2013 at 04:23:56AM +0100, Linus Walleij wrote:
> Hi Mika,
> 
> On Wed, Feb 27, 2013 at 4:25 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > -static int ichx_gpio_check_available(struct gpio_chip *gpio, unsigned nr)
> > +static bool ichx_gpio_check_available(struct gpio_chip *gpio, unsigned nr)
> >  {
> > -       return (ichx_priv.use_gpio & (1 << (nr / 32))) ? 0 : -ENXIO;
> > +       return ichx_priv.use_gpio & (1 << (nr / 32));
> >  }
> 
> Strictly speaking what you're returning there is not a bool.
> Shouldn't it be:
> 
> return !!(ichx_priv.use_gpio & (1 << (nr / 32)));
> 
> ?

A C reference manual says something like: When converting any scalar value
to type _Bool, all nonzero values are converted to 1, while zero values are
converted to 0 (1 being canonical value for true).

However, I'm happy to send a patch changing this if you like so (Grant
applied the patch already).