[PATCH 13/54] gpio: pcf857x: Be sure to clamp return value

[PATCH 13/54] gpio: pcf857x: Be sure to clamp return value

[Linus Walleij]

As we want gpio_chip .get() calls to be able to return negative
error codes and propagate to drivers, we need to go over all
drivers and make sure their return values are clamped to [0,1].
We do this by using the ret = !!(val) design pattern.

Also start returning the error code if something fails, as the
end of the series augment the core to support this.

Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: George Cherian <george.cherian@ti.com>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-pcf857x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
index bf511c0efa48..f64380a7d004 100644
--- a/drivers/gpio/gpio-pcf857x.c
+++ b/drivers/gpio/gpio-pcf857x.c
@@ -154,7 +154,7 @@ static int pcf857x_get(struct gpio_chip *chip, unsigned offset)
 	int		value;
 
 	value = gpio->read(gpio->client);
-	return (value < 0) ? 0 : (value & (1 << offset));
+	return (value < 0) ? value : !!(value & (1 << offset));
 }
 
 static int pcf857x_output(struct gpio_chip *chip, unsigned offset, int value)
-- 
2.4.3

Re: [PATCH 13/54] gpio: pcf857x: Be sure to clamp return value

[Laurent Pinchart]

Hi Linus,

Thank you for the patch.

On Tuesday 22 December 2015 15:20:09 Linus Walleij wrote:
> As we want gpio_chip .get() calls to be able to return negative
> error codes and propagate to drivers, we need to go over all
> drivers and make sure their return values are clamped to [0,1].
> We do this by using the ret = !!(val) design pattern.

The patch itself looks good to me, but wouldn't it be easier to patch the 
caller to clamp positive values to [0,1] and propagate negative values 
untouched ?

> Also start returning the error code if something fails, as the
> end of the series augment the core to support this.
> 
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Cc: George Cherian <george.cherian@ti.com>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpio-pcf857x.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
> index bf511c0efa48..f64380a7d004 100644
> --- a/drivers/gpio/gpio-pcf857x.c
> +++ b/drivers/gpio/gpio-pcf857x.c
> @@ -154,7 +154,7 @@ static int pcf857x_get(struct gpio_chip *chip, unsigned
> offset) int		value;
> 
>  	value = gpio->read(gpio->client);
> -	return (value < 0) ? 0 : (value & (1 << offset));
> +	return (value < 0) ? value : !!(value & (1 << offset));
>  }
> 
>  static int pcf857x_output(struct gpio_chip *chip, unsigned offset, int
> value)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 13/54] gpio: pcf857x: Be sure to clamp return value

[Vladimir Zapolskiy]

Hi Laurent,

On 27.12.2015 00:35, Laurent Pinchart wrote:
> Hi Linus,
> 
> Thank you for the patch.
> 
> On Tuesday 22 December 2015 15:20:09 Linus Walleij wrote:
>> As we want gpio_chip .get() calls to be able to return negative
>> error codes and propagate to drivers, we need to go over all
>> drivers and make sure their return values are clamped to [0,1].
>> We do this by using the ret = !!(val) design pattern.
> 
> The patch itself looks good to me, but wouldn't it be easier to patch the 
> caller to clamp positive values to [0,1] and propagate negative values 
> untouched ?

this has been done in v4.3 e20538b82f1f ("gpio: Propagate errors from
chip->get()"), but the change causes problems with GPIO line id 31 and
the change is temporarily reverted by 45ad7db90b ("gpio: revert get() to
non-errorprogating behaviour").

See also a recent discussion related to this problem
http://www.spinics.net/lists/linux-gpio/msg10677.html

>> Also start returning the error code if something fails, as the
>> end of the series augment the core to support this.
>>
>> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
>> Cc: George Cherian <george.cherian@ti.com>
>> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>  drivers/gpio/gpio-pcf857x.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
>> index bf511c0efa48..f64380a7d004 100644
>> --- a/drivers/gpio/gpio-pcf857x.c
>> +++ b/drivers/gpio/gpio-pcf857x.c
>> @@ -154,7 +154,7 @@ static int pcf857x_get(struct gpio_chip *chip, unsigned
>> offset) int		value;
>>
>>  	value = gpio->read(gpio->client);
>> -	return (value < 0) ? 0 : (value & (1 << offset));
>> +	return (value < 0) ? value : !!(value & (1 << offset));
>>  }

Back to your question, assume here in unmodified version the case of
(offset == 31) [1], on upper level the returned value will be
misapprehended as a negative number.

[1] (offset == 31) may be an invalid GPIO line id in this particular
driver, but some other gpiochip drivers should support line id 31.

--
With best wishes,
Vladimir

>>
>>  static int pcf857x_output(struct gpio_chip *chip, unsigned offset, int
>> value)
> 

Re: [PATCH 13/54] gpio: pcf857x: Be sure to clamp return value

[Laurent Pinchart]

Hi Vladimir,

On Sunday 27 December 2015 02:35:36 Vladimir Zapolskiy wrote:
> On 27.12.2015 00:35, Laurent Pinchart wrote:
> > On Tuesday 22 December 2015 15:20:09 Linus Walleij wrote:
> >> As we want gpio_chip .get() calls to be able to return negative
> >> error codes and propagate to drivers, we need to go over all
> >> drivers and make sure their return values are clamped to [0,1].
> >> We do this by using the ret = !!(val) design pattern.
> > 
> > The patch itself looks good to me, but wouldn't it be easier to patch the
> > caller to clamp positive values to [0,1] and propagate negative values
> > untouched ?
> 
> this has been done in v4.3 e20538b82f1f ("gpio: Propagate errors from
> chip->get()"), but the change causes problems with GPIO line id 31 and
> the change is temporarily reverted by 45ad7db90b ("gpio: revert get() to
> non-errorprogating behaviour").
> 
> See also a recent discussion related to this problem
> http://www.spinics.net/lists/linux-gpio/msg10677.html
> 
> >> Also start returning the error code if something fails, as the
> >> end of the series augment the core to support this.
> >> 
> >> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> >> Cc: George Cherian <george.cherian@ti.com>
> >> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> >> ---
> >> 
> >>  drivers/gpio/gpio-pcf857x.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
> >> index bf511c0efa48..f64380a7d004 100644
> >> --- a/drivers/gpio/gpio-pcf857x.c
> >> +++ b/drivers/gpio/gpio-pcf857x.c
> >> @@ -154,7 +154,7 @@ static int pcf857x_get(struct gpio_chip *chip,
> >> unsigned
> >> offset) int		value;
> >> 
> >>  	value = gpio->read(gpio->client);
> >> 
> >> -	return (value < 0) ? 0 : (value & (1 << offset));
> >> +	return (value < 0) ? value : !!(value & (1 << offset));
> >> 
> >>  }
> 
> Back to your question, assume here in unmodified version the case of
> (offset == 31) [1], on upper level the returned value will be
> misapprehended as a negative number.
> 
> [1] (offset == 31) may be an invalid GPIO line id in this particular
> driver, but some other gpiochip drivers should support line id 31.

Would something like the following make sense ?

	value = chip->get ? chip->get(chip, offset) : -EIO;
	value = IS_ERR_VALUE(value) ? value : !!value;

Granted, GPIO drivers would still need to make sure that the value they return 
from register reads don't get considered as an error code, but any val & (1 << 
offset) would be fine, which is the most common case.

If you still think that patching all GPIO drivers is better I won't oppose to 
that.

> >>  static int pcf857x_output(struct gpio_chip *chip, unsigned offset, int
> >> 
> >> value)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 13/54] gpio: pcf857x: Be sure to clamp return value

[Linus Walleij]

On Sun, Dec 27, 2015 at 8:47 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> Would something like the following make sense ?
>
>         value = chip->get ? chip->get(chip, offset) : -EIO;
>         value = IS_ERR_VALUE(value) ? value : !!value;

Yeah that makes sense, I didn't think about using IS_ERR_VALUE()
to mitigate the situation.

Patches welcome :)

> Granted, GPIO drivers would still need to make sure that the value they return
> from register reads don't get considered as an error code, but any val & (1 <<
> offset) would be fine, which is the most common case.
>
> If you still think that patching all GPIO drivers is better I won't oppose to
> that.

I have already patches all the drivers with this problem, merged the
larger part of it and will merge the remainders for v4.6. The above is
still more elegant than the code we have so I'd take it to put some
nice robustness icing on top...

Yours,
Linus Walleij