[PATCH] gpio: clamp returned values to the boolean range

[PATCH] gpio: clamp returned values to the boolean range

[Alexandre Courbot]

Nothing prevents GPIO drivers from returning values outside the
boolean range, and as it turns out a few drivers are actually doing so.
These values were passed as-is to unsuspecting consumers and created
confusion.

This patch makes the internal _gpiod_get_raw_value() function return a
bool, effectively clamping the GPIO value to the boolean range no
matter what the driver does.

While we are at it, we also change the value parameter of
_gpiod_set_raw_value() to bool type before drivers start doing funny
things with non-boolean values.

Another way to fix this would be to change the prototypes of the driver
interface to use bool directly, but this would require a huge
cross-systems patch so this simpler solution is preferred.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
This issue was discussed some time ago already
(https://www.mail-archive.com/linux-gpio@vger.kernel.org/msg02019.html),
here is finally the fix for it. Not exactly what we planned but who would
take a hundreds-lines change when a two-liner does the same?

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 50c4922fe53a..45cefccbf4bf 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1928,7 +1928,7 @@ EXPORT_SYMBOL_GPL(gpiod_is_active_low);
  * that the GPIO was actually requested.
  */
 
-static int _gpiod_get_raw_value(const struct gpio_desc *desc)
+static bool _gpiod_get_raw_value(const struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
 	int value;
@@ -2041,7 +2041,7 @@ static void _gpio_set_open_source_value(struct gpio_desc *desc, int value)
 			  __func__, err);
 }
 
-static void _gpiod_set_raw_value(struct gpio_desc *desc, int value)
+static void _gpiod_set_raw_value(struct gpio_desc *desc, bool value)
 {
 	struct gpio_chip	*chip;
 
-- 
1.9.0

Re: [PATCH] gpio: clamp returned values to the boolean range

[Linus Walleij]

On Wed, Feb 26, 2014 at 9:40 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:

> Nothing prevents GPIO drivers from returning values outside the
> boolean range, and as it turns out a few drivers are actually doing so.
> These values were passed as-is to unsuspecting consumers and created
> confusion.
>
> This patch makes the internal _gpiod_get_raw_value() function return a
> bool, effectively clamping the GPIO value to the boolean range no
> matter what the driver does.

No, that will not be the semantic effect of this patch, bool is just
another name for an int, maybe some static checkers will be able
to use it however.

If you really want the desired semantic effect, do this:

static int _gpiod_get_raw_value(const struct gpio_desc *desc)
{
        struct gpio_chip        *chip;
-        int value;
+       bool value;
        int offset;

        chip = desc->chip;
        offset = gpio_chip_hwgpio(desc);
-        value = chip->get ? chip->get(chip, offset) : 0;
+        value = chip->get ? !!(chip->get(chip, offset)) : false;
        trace_gpio_value(desc_to_gpio(desc), 1, value);
        return value;
}

Can you please update the patch accordingly, maybe also the
commit message.

Yours,
Linus Walleij

Re: [PATCH] gpio: clamp returned values to the boolean range

[Joe Perches]

On Wed, 2014-03-05 at 09:49 +0800, Linus Walleij wrote:
> On Wed, Feb 26, 2014 at 9:40 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> > Nothing prevents GPIO drivers from returning values outside the
> > boolean range, and as it turns out a few drivers are actually doing so.
> > These values were passed as-is to unsuspecting consumers and created
> > confusion.
> >
> > This patch makes the internal _gpiod_get_raw_value() function return a
> > bool, effectively clamping the GPIO value to the boolean range no
> > matter what the driver does.
> 
> No, that will not be the semantic effect of this patch, bool is just
> another name for an int, maybe some static checkers will be able
> to use it however.

No, a bool is not an int.

It's really different.
include/linux/types.h:typedef _Bool                     bool;

Re: [PATCH] gpio: clamp returned values to the boolean range

[Alexandre Courbot]

On Wed, Mar 5, 2014 at 11:04 AM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2014-03-05 at 09:49 +0800, Linus Walleij wrote:
>> On Wed, Feb 26, 2014 at 9:40 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>>
>> > Nothing prevents GPIO drivers from returning values outside the
>> > boolean range, and as it turns out a few drivers are actually doing so.
>> > These values were passed as-is to unsuspecting consumers and created
>> > confusion.
>> >
>> > This patch makes the internal _gpiod_get_raw_value() function return a
>> > bool, effectively clamping the GPIO value to the boolean range no
>> > matter what the driver does.
>>
>> No, that will not be the semantic effect of this patch, bool is just
>> another name for an int, maybe some static checkers will be able
>> to use it however.
>
> No, a bool is not an int.
>
> It's really different.
> include/linux/types.h:typedef _Bool                     bool;

It indeed seems that _Bool is an actual boolean type in C99. However I
could not find in the C99 standard how ints are supposed to be
converted to it. So in the end it is probably safer to perform this
change the way Linus suggested.

Alex.

Re: [PATCH] gpio: clamp returned values to the boolean range

[Joe Perches]

On Wed, 2014-03-05 at 11:14 +0900, Alexandre Courbot wrote:
> On Wed, Mar 5, 2014 at 11:04 AM, Joe Perches <joe@perches.com> wrote:
> > On Wed, 2014-03-05 at 09:49 +0800, Linus Walleij wrote:
> >> On Wed, Feb 26, 2014 at 9:40 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> >>
> >> > Nothing prevents GPIO drivers from returning values outside the
> >> > boolean range, and as it turns out a few drivers are actually doing so.
> >> > These values were passed as-is to unsuspecting consumers and created
> >> > confusion.
> >> >
> >> > This patch makes the internal _gpiod_get_raw_value() function return a
> >> > bool, effectively clamping the GPIO value to the boolean range no
> >> > matter what the driver does.
> >>
> >> No, that will not be the semantic effect of this patch, bool is just
> >> another name for an int, maybe some static checkers will be able
> >> to use it however.
> >
> > No, a bool is not an int.
> >
> > It's really different.
> > include/linux/types.h:typedef _Bool                     bool;
> 
> It indeed seems that _Bool is an actual boolean type in C99. However I
> could not find in the C99 standard how ints are supposed to be
> converted to it.

6.3.1.2 Boolean type

When any scalar value is converted to _Bool, the result is 0 if the
value compares equal to 0; otherwise, the result is 1.

> So in the end it is probably safer to perform this
> change the way Linus suggested.

Not really.

Re: [PATCH] gpio: clamp returned values to the boolean range

[Alexandre Courbot]

On Wed, Mar 5, 2014 at 11:19 AM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2014-03-05 at 11:14 +0900, Alexandre Courbot wrote:
>> On Wed, Mar 5, 2014 at 11:04 AM, Joe Perches <joe@perches.com> wrote:
>> > On Wed, 2014-03-05 at 09:49 +0800, Linus Walleij wrote:
>> >> On Wed, Feb 26, 2014 at 9:40 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> >>
>> >> > Nothing prevents GPIO drivers from returning values outside the
>> >> > boolean range, and as it turns out a few drivers are actually doing so.
>> >> > These values were passed as-is to unsuspecting consumers and created
>> >> > confusion.
>> >> >
>> >> > This patch makes the internal _gpiod_get_raw_value() function return a
>> >> > bool, effectively clamping the GPIO value to the boolean range no
>> >> > matter what the driver does.
>> >>
>> >> No, that will not be the semantic effect of this patch, bool is just
>> >> another name for an int, maybe some static checkers will be able
>> >> to use it however.
>> >
>> > No, a bool is not an int.
>> >
>> > It's really different.
>> > include/linux/types.h:typedef _Bool                     bool;
>>
>> It indeed seems that _Bool is an actual boolean type in C99. However I
>> could not find in the C99 standard how ints are supposed to be
>> converted to it.
>
> 6.3.1.2 Boolean type
>
> When any scalar value is converted to _Bool, the result is 0 if the
> value compares equal to 0; otherwise, the result is 1.
>
>> So in the end it is probably safer to perform this
>> change the way Linus suggested.
>
> Not really.

Ok, you are obviously correct here. Linus, what do you think?

Re: [PATCH] gpio: clamp returned values to the boolean range

[Linus Walleij]

On Wed, Mar 5, 2014 at 10:22 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Wed, Mar 5, 2014 at 11:19 AM, Joe Perches <joe@perches.com> wrote:
>> On Wed, 2014-03-05 at 11:14 +0900, Alexandre Courbot wrote:
>>> On Wed, Mar 5, 2014 at 11:04 AM, Joe Perches <joe@perches.com> wrote:
>>> > On Wed, 2014-03-05 at 09:49 +0800, Linus Walleij wrote:
>>> >> On Wed, Feb 26, 2014 at 9:40 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>>> >>
>>> >> > Nothing prevents GPIO drivers from returning values outside the
>>> >> > boolean range, and as it turns out a few drivers are actually doing so.
>>> >> > These values were passed as-is to unsuspecting consumers and created
>>> >> > confusion.
>>> >> >
>>> >> > This patch makes the internal _gpiod_get_raw_value() function return a
>>> >> > bool, effectively clamping the GPIO value to the boolean range no
>>> >> > matter what the driver does.
>>> >>
>>> >> No, that will not be the semantic effect of this patch, bool is just
>>> >> another name for an int, maybe some static checkers will be able
>>> >> to use it however.
>>> >
>>> > No, a bool is not an int.
>>> >
>>> > It's really different.
>>> > include/linux/types.h:typedef _Bool                     bool;
>>>
>>> It indeed seems that _Bool is an actual boolean type in C99. However I
>>> could not find in the C99 standard how ints are supposed to be
>>> converted to it.
>>
>> 6.3.1.2 Boolean type
>>
>> When any scalar value is converted to _Bool, the result is 0 if the
>> value compares equal to 0; otherwise, the result is 1.
>>
>>> So in the end it is probably safer to perform this
>>> change the way Linus suggested.
>>
>> Not really.
>
> Ok, you are obviously correct here. Linus, what do you think?

Yeah I was wrong ... too old and not keeping up with standards
development :-)

Anyway, the local "value" variable in the function should still be
converted to a bool as well right? And the assignment should still
be "false" not 0. So I would still add my hunk of code...

Yours,
Linus Walleij

Re: [PATCH] gpio: clamp returned values to the boolean range

[Joe Perches]

On Wed, 2014-03-05 at 10:33 +0800, Linus Walleij wrote:
> the local "value" variable in the function should still be
> converted to a bool as well right? And the assignment should still
> be "false" not 0. So I would still add my hunk of code...

No idea.  I don't know the code, just wanted to point
out that a bool is not an int.

Generically, I think converting an int to a bool in
any "raw" function probably isn't the right thing to do.

Especially if any of these GPIOs are ever used as ports
(aggregates) in any access.