Re: [PATCH] input: gpio_keys: Don't report events on gpio failure

Re: [PATCH] input: gpio_keys: Don't report events on gpio failure

[Bjorn Andersson]

On Tue 28 Jul 14:00 PDT 2015, Dmitry Torokhov wrote:

> Hi Bjorn,
> 
> On Mon, Jul 27, 2015 at 06:50:04PM -0700, Bjorn Andersson wrote:
> > In the cases where the gpio chip fails to acquire the current state an
> > error is reported back to gpio_keys. This is currently interpreted as if
> > the line went high, which just confuses the developer.
> > 
> > This patch introduces an error print in this case and skipps the
> > reporting of a input event; to aid in debugging this issue.
> > 
> > Reported-by: John Stultz <john.stultz@linaro.org>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > ---
> >  drivers/input/keyboard/gpio_keys.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> > index ddf4045de084..3ce3298ac09e 100644
> > --- a/drivers/input/keyboard/gpio_keys.c
> > +++ b/drivers/input/keyboard/gpio_keys.c
> > @@ -336,8 +336,14 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
> >  	const struct gpio_keys_button *button = bdata->button;
> >  	struct input_dev *input = bdata->input;
> >  	unsigned int type = button->type ?: EV_KEY;
> > -	int state = (gpio_get_value_cansleep(button->gpio) ? 1 : 0) ^ button->active_low;
> > +	int state = gpio_get_value_cansleep(button->gpio);
> >  
> > +	if (state < 0) {
> > +		dev_err(input->dev.parent, "failed to get gpio state\n");
> 
> As far as I can see:
> 
> static inline int gpio_get_value_cansleep(unsigned gpio)
> {
> 	return gpiod_get_raw_value_cansleep(gpio_to_desc(gpio));
> }
> 
> int gpiod_get_raw_value_cansleep(const struct gpio_desc *desc)
> {
> 	might_sleep_if(extra_checks);
> 	if (!desc)
> 		return 0;
> 	return _gpiod_get_raw_value(desc);
> }
> 
> static bool _gpiod_get_raw_value(const struct gpio_desc *desc)
> {
> ...
> }
> 
> So how exactly do we get negative here?

I'm sorry, I obviously didn't pay enough attention when running through
that callstack...

But then the question first goes to Linus & co.

gpio_chip->get() can return a negative value to indicate errors (and did
so in this case), all parts of the API seems indicates that we can get
an error (int vs bool).

Should we change _gpiod_get_raw_value() to propagate this error?  Or
should we just ignore this issue and propagate an error as GPIO high
reading?

Regards,
Bjorn

Re: [PATCH] input: gpio_keys: Don't report events on gpio failure

[Linus Walleij]

On Tue, Aug 11, 2015 at 12:41 AM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:

> But then the question first goes to Linus & co.
>
> gpio_chip->get() can return a negative value to indicate errors (and did
> so in this case), all parts of the API seems indicates that we can get
> an error (int vs bool).

Ooops.

> Should we change _gpiod_get_raw_value() to propagate this error?

Yes for now. Can you patch it? :)

>  Or
> should we just ignore this issue and propagate an error as GPIO high
> reading?

I don't know about the future. In some sense GPIOs are so smallish
resources that errorhandling every call to read/write them seem to
be a royal PITA. That is why I wanted to switch them to bool and get
rid of the problem, but now I also see that maybe that was not such a
smart idea, if errors do occur on the set/get_value path.

Alexandre?

Yours,
Linus Walleij

Re: [PATCH] input: gpio_keys: Don't report events on gpio failure

[Alexandre Courbot]

On Thu, Aug 13, 2015 at 10:06 PM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Tue, Aug 11, 2015 at 12:41 AM, Bjorn Andersson
> <bjorn.andersson@sonymobile.com> wrote:
>
>> But then the question first goes to Linus & co.
>>
>> gpio_chip->get() can return a negative value to indicate errors (and did
>> so in this case), all parts of the API seems indicates that we can get
>> an error (int vs bool).
>
> Ooops.
>
>> Should we change _gpiod_get_raw_value() to propagate this error?
>
> Yes for now. Can you patch it? :)
>
>>  Or
>> should we just ignore this issue and propagate an error as GPIO high
>> reading?
>
> I don't know about the future. In some sense GPIOs are so smallish
> resources that errorhandling every call to read/write them seem to
> be a royal PITA. That is why I wanted to switch them to bool and get
> rid of the problem, but now I also see that maybe that was not such a
> smart idea, if errors do occur on the set/get_value path.

Nowadays GPIOs may reside at the other end of an i2c bus, which means
that even the simplest operation like reading a GPIO value can
potentially fail. And it will probably not get better - wait until we
implement GPIO-over-IP! :)

So I'd say it makes sense to propagate errors returned by the driver's
get() hook. This might contradict some of our earlier statements about
simplifying the GPIO API, but is preferrable to having to make a
decision as to which valid value to return if the driver fails...

It should then be made very clear in the documentation that the only
positive values ever returned by the GPIO API will be 0 and 1 (we
already have a clamping mechanism for that IIRC), and that negative
values are propagated as-is.

Linus, does that seem reasonable to you? Does anyone has the intention
to address that one or should I add it to my short-term TODO list?

Re: [PATCH] input: gpio_keys: Don't report events on gpio failure

[Bjorn Andersson]

On Sun 16 Aug 23:59 PDT 2015, Alexandre Courbot wrote:

> On Thu, Aug 13, 2015 at 10:06 PM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
> > On Tue, Aug 11, 2015 at 12:41 AM, Bjorn Andersson
> > <bjorn.andersson@sonymobile.com> wrote:
> >
> >> But then the question first goes to Linus & co.
> >>
> >> gpio_chip->get() can return a negative value to indicate errors (and did
> >> so in this case), all parts of the API seems indicates that we can get
> >> an error (int vs bool).
> >
> > Ooops.
> >
> >> Should we change _gpiod_get_raw_value() to propagate this error?
> >
> > Yes for now. Can you patch it? :)
> >
> >>  Or
> >> should we just ignore this issue and propagate an error as GPIO high
> >> reading?
> >
> > I don't know about the future. In some sense GPIOs are so smallish
> > resources that errorhandling every call to read/write them seem to
> > be a royal PITA. That is why I wanted to switch them to bool and get
> > rid of the problem, but now I also see that maybe that was not such a
> > smart idea, if errors do occur on the set/get_value path.
> 
> Nowadays GPIOs may reside at the other end of an i2c bus, which means
> that even the simplest operation like reading a GPIO value can
> potentially fail. And it will probably not get better - wait until we
> implement GPIO-over-IP! :)
> 

Now that's progress! I can't wait ;)

> So I'd say it makes sense to propagate errors returned by the driver's
> get() hook. This might contradict some of our earlier statements about
> simplifying the GPIO API, but is preferrable to having to make a
> decision as to which valid value to return if the driver fails...
> 

Sounds good.

As we're patching up _gpiod_get_raw_value(), is the lack of a get()
implementation the same as a LOW or is that -ENOTSUPP?

> It should then be made very clear in the documentation that the only
> positive values ever returned by the GPIO API will be 0 and 1 (we
> already have a clamping mechanism for that IIRC), and that negative
> values are propagated as-is.
> 

That makes sense. I'm however not able to find such clamping
macro/mechanism and it would be very beneficial here...

> Linus, does that seem reasonable to you? Does anyone has the intention
> to address that one or should I add it to my short-term TODO list?

If you have some input on above (is lack of get() an error) I can hack
up the patch.

Regards,
Bjorn

Re: [PATCH] input: gpio_keys: Don't report events on gpio failure

[Alexandre Courbot]

On Tue, Aug 18, 2015 at 4:34 AM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> On Sun 16 Aug 23:59 PDT 2015, Alexandre Courbot wrote:
>
>> On Thu, Aug 13, 2015 at 10:06 PM, Linus Walleij
>> <linus.walleij@linaro.org> wrote:
>> > On Tue, Aug 11, 2015 at 12:41 AM, Bjorn Andersson
>> > <bjorn.andersson@sonymobile.com> wrote:
>> >
>> >> But then the question first goes to Linus & co.
>> >>
>> >> gpio_chip->get() can return a negative value to indicate errors (and did
>> >> so in this case), all parts of the API seems indicates that we can get
>> >> an error (int vs bool).
>> >
>> > Ooops.
>> >
>> >> Should we change _gpiod_get_raw_value() to propagate this error?
>> >
>> > Yes for now. Can you patch it? :)
>> >
>> >>  Or
>> >> should we just ignore this issue and propagate an error as GPIO high
>> >> reading?
>> >
>> > I don't know about the future. In some sense GPIOs are so smallish
>> > resources that errorhandling every call to read/write them seem to
>> > be a royal PITA. That is why I wanted to switch them to bool and get
>> > rid of the problem, but now I also see that maybe that was not such a
>> > smart idea, if errors do occur on the set/get_value path.
>>
>> Nowadays GPIOs may reside at the other end of an i2c bus, which means
>> that even the simplest operation like reading a GPIO value can
>> potentially fail. And it will probably not get better - wait until we
>> implement GPIO-over-IP! :)
>>
>
> Now that's progress! I can't wait ;)
>
>> So I'd say it makes sense to propagate errors returned by the driver's
>> get() hook. This might contradict some of our earlier statements about
>> simplifying the GPIO API, but is preferrable to having to make a
>> decision as to which valid value to return if the driver fails...
>>
>
> Sounds good.
>
> As we're patching up _gpiod_get_raw_value(), is the lack of a get()
> implementation the same as a LOW or is that -ENOTSUPP?

I don't see any reason why it should not be -ENOTSUPP if we start to
manage errors properly.

>
>> It should then be made very clear in the documentation that the only
>> positive values ever returned by the GPIO API will be 0 and 1 (we
>> already have a clamping mechanism for that IIRC), and that negative
>> values are propagated as-is.
>>
>
> That makes sense. I'm however not able to find such clamping
> macro/mechanism and it would be very beneficial here...
>
>> Linus, does that seem reasonable to you? Does anyone has the intention
>> to address that one or should I add it to my short-term TODO list?
>
> If you have some input on above (is lack of get() an error) I can hack
> up the patch.

Excellent - since Linus gave his thumb up, I think you can go ahead.
Looking forward to seeing this finally fixed.

Thanks!
Alex.

Re: [PATCH] input: gpio_keys: Don't report events on gpio failure

[Linus Walleij]

On Mon, Aug 17, 2015 at 8:59 AM, Alexandre Courbot <gnurou@gmail.com> wrote:

> So I'd say it makes sense to propagate errors returned by the driver's
> get() hook. This might contradict some of our earlier statements about
> simplifying the GPIO API, but is preferrable to having to make a
> decision as to which valid value to return if the driver fails...
>
> It should then be made very clear in the documentation that the only
> positive values ever returned by the GPIO API will be 0 and 1 (we
> already have a clamping mechanism for that IIRC), and that negative
> values are propagated as-is.
>
> Linus, does that seem reasonable to you? Does anyone has the intention
> to address that one or should I add it to my short-term TODO list?

I'm aligned with this. Go ahead on this path.

Yours,
Linus Walleij