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 <[email protected]>
Signed-off-by: Bjorn Andersson <[email protected]>
---
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");
+ return;
+ }
+
+ state = (state ? 1 : 0) ^ button->active_low;
if (type == EV_ABS) {
if (state)
input_event(input, type, button->code, button->value);
--
1.8.2.2
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 <[email protected]>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> 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?
Thanks.
--
Dmitry
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 <[email protected]>
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> > 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
On Tue, Aug 11, 2015 at 12:41 AM, Bjorn Andersson
<[email protected]> 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
On Thu, Aug 13, 2015 at 10:06 PM, Linus Walleij
<[email protected]> wrote:
> On Tue, Aug 11, 2015 at 12:41 AM, Bjorn Andersson
> <[email protected]> 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?
On Sun 16 Aug 23:59 PDT 2015, Alexandre Courbot wrote:
> On Thu, Aug 13, 2015 at 10:06 PM, Linus Walleij
> <[email protected]> wrote:
> > On Tue, Aug 11, 2015 at 12:41 AM, Bjorn Andersson
> > <[email protected]> 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
On Mon, Aug 17, 2015 at 8:59 AM, Alexandre Courbot <[email protected]> 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
It's possible to have gpio chips hanging off unreliable remote buses
where the get() operation will fail to acquire a readout of the current
gpio state. Propagate these errors to the consumer so that they can
act on, retry or ignore these failing reads, instead of treating them as
the line being held high.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/gpio/gpiolib.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 3b5e516298e0..dc17dbf8c234 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1173,15 +1173,16 @@ EXPORT_SYMBOL_GPL(gpiod_is_active_low);
* that the GPIO was actually requested.
*/
-static bool _gpiod_get_raw_value(const struct gpio_desc *desc)
+static int _gpiod_get_raw_value(const struct gpio_desc *desc)
{
struct gpio_chip *chip;
- bool value;
int offset;
+ int value;
chip = desc->chip;
offset = gpio_chip_hwgpio(desc);
- value = chip->get ? chip->get(chip, offset) : false;
+ value = chip->get ? chip->get(chip, offset) : -EIO;
+ value = value < 0 ? value : !!value;
trace_gpio_value(desc_to_gpio(desc), 1, value);
return value;
}
@@ -1191,7 +1192,7 @@ static bool _gpiod_get_raw_value(const struct gpio_desc *desc)
* @desc: gpio whose value will be returned
*
* Return the GPIO's raw value, i.e. the value of the physical line disregarding
- * its ACTIVE_LOW status.
+ * its ACTIVE_LOW status, or negative errno on failure.
*
* This function should be called from contexts where we cannot sleep, and will
* complain if the GPIO chip functions potentially sleep.
@@ -1211,7 +1212,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_raw_value);
* @desc: gpio whose value will be returned
*
* Return the GPIO's logical value, i.e. taking the ACTIVE_LOW status into
- * account.
+ * account, or negative errno on failure.
*
* This function should be called from contexts where we cannot sleep, and will
* complain if the GPIO chip functions potentially sleep.
@@ -1225,6 +1226,9 @@ int gpiod_get_value(const struct gpio_desc *desc)
WARN_ON(desc->chip->can_sleep);
value = _gpiod_get_raw_value(desc);
+ if (value < 0)
+ return value;
+
if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
value = !value;
@@ -1547,7 +1551,7 @@ EXPORT_SYMBOL_GPL(gpiochip_unlock_as_irq);
* @desc: gpio whose value will be returned
*
* Return the GPIO's raw value, i.e. the value of the physical line disregarding
- * its ACTIVE_LOW status.
+ * its ACTIVE_LOW status, or negative errno on failure.
*
* This function is to be called from contexts that can sleep.
*/
@@ -1565,7 +1569,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_raw_value_cansleep);
* @desc: gpio whose value will be returned
*
* Return the GPIO's logical value, i.e. taking the ACTIVE_LOW status into
- * account.
+ * account, or negative errno on failure.
*
* This function is to be called from contexts that can sleep.
*/
@@ -1578,6 +1582,9 @@ int gpiod_get_value_cansleep(const struct gpio_desc *desc)
return 0;
value = _gpiod_get_raw_value(desc);
+ if (value < 0)
+ return value;
+
if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
value = !value;
--
1.8.2.2
On Tue, Aug 18, 2015 at 4:34 AM, Bjorn Andersson
<[email protected]> wrote:
> On Sun 16 Aug 23:59 PDT 2015, Alexandre Courbot wrote:
>
>> On Thu, Aug 13, 2015 at 10:06 PM, Linus Walleij
>> <[email protected]> wrote:
>> > On Tue, Aug 11, 2015 at 12:41 AM, Bjorn Andersson
>> > <[email protected]> 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.