[v1] gpiolib: Respect error code of ->get_direction()

In case we try to lock GPIO pin as IRQ when something going wrong
we print a misleading message.

Correct this by checking an error code from ->get_direction() in
gpiochip_lock_as_irq() and printing a corresponding message.

Note, this doesn't change overall behavior because error code used to
be recognized as direction output which anyway led to an abortion of
request.

Fixes: 9c10280d85c1 ("gpio: flush direction status in gpiochip_lock_as_irq()")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 6 ++++++
 1 file changed, 6 insertions(+)

On Tue, 2018-07-03 at 03:38 +0300, Andy Shevchenko wrote:
> In case we try to lock GPIO pin as IRQ when something going wrong
> we print a misleading message.
> 
> Correct this by checking an error code from ->get_direction() in
> gpiochip_lock_as_irq() and printing a corresponding message.
> 

> Note, this doesn't change overall behavior because error code used to
> be recognized as direction output which anyway led to an abortion of
> request.

This paragraph is completely wrong, sorry.
The dir != 0 means input.

Thus it does change behaviour to bail out if we can't get direction.
This needs to be thought of.

> 
> Fixes: 9c10280d85c1 ("gpio: flush direction status in 
> gpiochip_lock_as_irq()")

Certainly it doesn't fix anything for now.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/gpio/gpiolib.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index c6f77e806cb8..0ff89b3f354b 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -3264,6 +3264,12 @@ int gpiochip_lock_as_irq(struct gpio_chip
> *chip, unsigned int offset)
>  	if (!chip->can_sleep && chip->get_direction) {
>  		int dir = chip->get_direction(chip, offset);
>  
> +		if (dir < 0) {
> +			chip_err(chip, "%s: cannot get GPIO
> direction\n",
> +				 __func__);
> +			return dir;
> +		}
> +
>  		if (dir)
>  			clear_bit(FLAG_IS_OUT, &desc->flags);
>  		else

On Tue, Jul 3, 2018 at 2:38 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> In case we try to lock GPIO pin as IRQ when something going wrong
> we print a misleading message.
>
> Correct this by checking an error code from ->get_direction() in
> gpiochip_lock_as_irq() and printing a corresponding message.
>
> Note, this doesn't change overall behavior because error code used to
> be recognized as direction output which anyway led to an abortion of
> request.
>
> Fixes: 9c10280d85c1 ("gpio: flush direction status in gpiochip_lock_as_irq()")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

OK per followup I stripped the Fixes tag and the last paragraph and
applied.

I am a bit worried that there might be platforms that have survived
by igoring any error code from get direction (interpreting it as
"input) but they will hopefully notice ... thanks to the new error
print.

Did you look around to see if some driver might be returning
error codes from get_direction()? I suspect mostly I2C-based
expanders and if it's an error it's an error and this is the right
thing to do but you never know what people are doing...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, 2018-07-09 at 15:13 +0200, Linus Walleij wrote:
> On Tue, Jul 3, 2018 at 2:38 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > In case we try to lock GPIO pin as IRQ when something going wrong
> > we print a misleading message.
> > 
> > Correct this by checking an error code from ->get_direction() in
> > gpiochip_lock_as_irq() and printing a corresponding message.
> > 
> > Note, this doesn't change overall behavior because error code used
> > to
> > be recognized as direction output which anyway led to an abortion of
> > request.
> > 
> > Fixes: 9c10280d85c1 ("gpio: flush direction status in
> > gpiochip_lock_as_irq()")
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> OK per followup I stripped the Fixes tag and the last paragraph and
> applied.

Thanks!

> I am a bit worried that there might be platforms that have survived
> by igoring any error code from get direction (interpreting it as
> "input) but they will hopefully notice ... thanks to the new error
> print.

And then suddenly they might get no interrupt b/c pin, for example,
still an output (should be caught by other API calls I think, though
failing earlier is better for my opinion).

> 
> Did you look around to see if some driver might be returning
> error codes from get_direction()? I suspect mostly I2C-based
> expanders and if it's an error it's an error and this is the right
> thing to 
> do but you never know what people are doing...

In the GPIO/pinctrl folders: both tegras, bcm2835, stm32 and most of
Intel.

In the drivers: i2c-mux-ltc4306.

So, not many.

On Mon, 2018-07-09 at 21:41 +0300, Andy Shevchenko wrote:
> On Mon, 2018-07-09 at 15:13 +0200, Linus Walleij wrote:
> > 

> > 
> > Did you look around to see if some driver might be returning
> > error codes from get_direction()? I suspect mostly I2C-based
> > expanders and if it's an error it's an error and this is the right
> > thing to 
> > do but you never know what people are doing...
> 
> In the GPIO/pinctrl folders: both tegras, bcm2835, stm32 and most of
> Intel.
> 
> In the drivers: i2c-mux-ltc4306.
> 
> So, not many.

Okay, those are returning error codes explicitly, I missed the ones
which returns inherited error code.

So, there are plenty of such, most like you noticed are GPIO expanders.