[PATCH v2] gpio: convince line to become input in irq helper

[PATCH v2] gpio: convince line to become input in irq helper

[Linus Walleij]

The generic IRQ helper library just checks if the IRQ line is
set as input before activating it for interrupts. As we
recently started to check things better with .get_dir() it
turns out that it's good to try to convince the line to become
an input before attempting to lock it as IRQ.

Cc: Björn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Propagate the error from .direction_input() so we can rely on
  it being used.
---
 drivers/gpio/gpiolib.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5a21a6acf8af..b195ec406ff4 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1505,6 +1505,25 @@ static int gpiochip_irq_reqres(struct irq_data *d)
 	if (!try_module_get(chip->gpiodev->owner))
 		return -ENODEV;
 
+	/*
+	 * If it is possible to switch this GPIO to an input
+	 * this is a good time to do it.
+	 */
+	if (chip->direction_input) {
+		struct gpio_desc *desc;
+		int ret;
+
+		desc = gpiochip_get_desc(chip, d->hwirq);
+		if (IS_ERR(desc))
+			return PTR_ERR(desc);
+
+	        ret = chip->direction_input(chip, d->hwirq);
+		if (ret)
+			return ret;
+
+		clear_bit(FLAG_IS_OUT, &desc->flags);
+	}
+
 	if (gpiochip_lock_as_irq(chip, d->hwirq)) {
 		chip_err(chip,
 			"unable to lock HW IRQ %lu for IRQ\n",
-- 
2.4.11

--
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

Re: [PATCH v2] gpio: convince line to become input in irq helper

[Bjorn Andersson]

On Wed 22 Jun 14:25 PDT 2016, Linus Walleij wrote:

> The generic IRQ helper library just checks if the IRQ line is
> set as input before activating it for interrupts. As we
> recently started to check things better with .get_dir() it
> turns out that it's good to try to convince the line to become
> an input before attempting to lock it as IRQ.
> 
> Cc: Björn Andersson <bjorn.andersson@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Propagate the error from .direction_input() so we can rely on
>   it being used.
> ---
>  drivers/gpio/gpiolib.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5a21a6acf8af..b195ec406ff4 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1505,6 +1505,25 @@ static int gpiochip_irq_reqres(struct irq_data *d)
>  	if (!try_module_get(chip->gpiodev->owner))
>  		return -ENODEV;
>  
> +	/*
> +	 * If it is possible to switch this GPIO to an input
> +	 * this is a good time to do it.
> +	 */
> +	if (chip->direction_input) {
> +		struct gpio_desc *desc;
> +		int ret;
> +
> +		desc = gpiochip_get_desc(chip, d->hwirq);
> +		if (IS_ERR(desc))
> +			return PTR_ERR(desc);
> +
> +	        ret = chip->direction_input(chip, d->hwirq);
> +		if (ret)
> +			return ret;
> +
> +		clear_bit(FLAG_IS_OUT, &desc->flags);
> +	}
> +
>  	if (gpiochip_lock_as_irq(chip, d->hwirq)) {
>  		chip_err(chip,
>  			"unable to lock HW IRQ %lu for IRQ\n",
> -- 
> 2.4.11
> 
--
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

Re: [PATCH v2] gpio: convince line to become input in irq helper

[Alexandre Courbot]

On Thu, Jun 23, 2016 at 6:25 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> The generic IRQ helper library just checks if the IRQ line is
> set as input before activating it for interrupts. As we
> recently started to check things better with .get_dir() it
> turns out that it's good to try to convince the line to become
> an input before attempting to lock it as IRQ.
>
> Cc: Björn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Propagate the error from .direction_input() so we can rely on
>   it being used.
> ---
>  drivers/gpio/gpiolib.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5a21a6acf8af..b195ec406ff4 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1505,6 +1505,25 @@ static int gpiochip_irq_reqres(struct irq_data *d)
>         if (!try_module_get(chip->gpiodev->owner))
>                 return -ENODEV;
>
> +       /*
> +        * If it is possible to switch this GPIO to an input
> +        * this is a good time to do it.
> +        */
> +       if (chip->direction_input) {
> +               struct gpio_desc *desc;
> +               int ret;
> +
> +               desc = gpiochip_get_desc(chip, d->hwirq);
> +               if (IS_ERR(desc))
> +                       return PTR_ERR(desc);
> +
> +               ret = chip->direction_input(chip, d->hwirq);
> +               if (ret)
> +                       return ret;
> +
> +               clear_bit(FLAG_IS_OUT, &desc->flags);
> +       }
> +

This looks like a good idea.

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
--
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

Re: [PATCH v2] gpio: convince line to become input in irq helper

[Geert Uytterhoeven]

Hi Linus,

On Wed, Jun 22, 2016 at 11:25 PM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> The generic IRQ helper library just checks if the IRQ line is
> set as input before activating it for interrupts. As we
> recently started to check things better with .get_dir() it
> turns out that it's good to try to convince the line to become
> an input before attempting to lock it as IRQ.
>
> Cc: Björn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Propagate the error from .direction_input() so we can rely on
>   it being used.

This patch (commit 7e7c059cb50c7c72 in gpio/for-next) breaks Ethernet on
r8a7795/salvator-x:

    libphy: ravb_mii: probed
[1] gpio_rcar e6052000.gpio: sense irq = 11, type = 8
    ravb e6800000.ethernet eth0: Base address at 0xe6800000,
2e:09:0a:00:83:1e, IRQ 131.
    ...
[2] gpiochip_irq_reqres: gpiochip e6052000.gpio
[3] gpio_rcar e6052000.gpio: gpio_rcar_direction_input: 11
[4] gpiochip_irq_reqres: desc->flags = 0x0
    ravb e6800000.ethernet eth0: limited PHY to 100Mbit/s
    Micrel KSZ9031 Gigabit PHY e6800000.etherne:00: attached PHY
driver [Micrel KSZ9031 Gigabit PHY]
(mii_bus:phy_addr=e6800000.etherne:00, irq=206)
    Waiting up to 110 more seconds for network.
    Waiting up to 100 more seconds for network.
    ...

Reverting it fixes the issue.

phy0 in arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts

        phy0: ethernet-phy@0 {
                ...
                interrupt-parent = <&gpio2>;
                interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
        };

sets up the GPIO for interrupt mode, cfr. [1] in the kernel output above.

> ---
>  drivers/gpio/gpiolib.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5a21a6acf8af..b195ec406ff4 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1505,6 +1505,25 @@ static int gpiochip_irq_reqres(struct irq_data *d)
>         if (!try_module_get(chip->gpiodev->owner))
>                 return -ENODEV;
>
> +       /*
> +        * If it is possible to switch this GPIO to an input
> +        * this is a good time to do it.
> +        */
> +       if (chip->direction_input) {
> +               struct gpio_desc *desc;
> +               int ret;
> +
> +               desc = gpiochip_get_desc(chip, d->hwirq);
> +               if (IS_ERR(desc))
> +                       return PTR_ERR(desc);
> +
> +               ret = chip->direction_input(chip, d->hwirq);

This configures the GPIO for plain input mode, cfr. [3] above, basically
undoing the configuration from [1]. Hence interrupts no longer come through,
and Ethernet fails.

> +               if (ret)
> +                       return ret;
> +
> +               clear_bit(FLAG_IS_OUT, &desc->flags);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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

Re: [PATCH v2] gpio: convince line to become input in irq helper

[Linus Walleij]

I sent a patch for the direction setter to be more careful, but
it's no silver bullet for strange semantics.

On Tue, Jul 5, 2016 at 12:07 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

> [1] gpio_rcar e6052000.gpio: sense irq = 11, type = 8
>     ravb e6800000.ethernet eth0: Base address at 0xe6800000,
> 2e:09:0a:00:83:1e, IRQ 131.
>     ...
> [2] gpiochip_irq_reqres: gpiochip e6052000.gpio
> [3] gpio_rcar e6052000.gpio: gpio_rcar_direction_input: 11
> [4] gpiochip_irq_reqres: desc->flags = 0x0
(...)
> This configures the GPIO for plain input mode, cfr. [3] above, basically
> undoing the configuration from [1]. Hence interrupts no longer come through,
> and Ethernet fails.

The driver is a bit fragile in that it relies on a certain call semantic,
I guess it is not a widespread problem so we should be able to make
a local fix if necessary.

The .set_direction() call should
set the direction. Why is it turning off interrupts as a side effect?

What happens if you apply this, making the .request() function handle
the pin setup and .set_direction() really just setting the
direction?

diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
index 681c93fb9e70..68fb0147caf4 100644
--- a/drivers/gpio/gpio-rcar.c
+++ b/drivers/gpio/gpio-rcar.c
@@ -221,7 +221,20 @@ static void
gpio_rcar_config_general_input_output_mode(struct gpio_chip *chip,
     struct gpio_rcar_priv *p = gpiochip_get_data(chip);
     unsigned long flags;

-    /* follow steps in the GPIO documentation for
+    spin_lock_irqsave(&p->lock, flags);
+
+    /* Select Input Mode or Output Mode in INOUTSEL */
+    gpio_rcar_modify_bit(p, INOUTSEL, gpio, output);
+
+    spin_unlock_irqrestore(&p->lock, flags);
+}
+
+static int gpio_rcar_request(struct gpio_chip *chip, unsigned offset)
+{
+    struct gpio_rcar_priv *p = gpiochip_get_data(chip);
+
+    /*
+     * follow steps in the GPIO documentation for
      * "Setting General Output Mode" and
      * "Setting General Input Mode"
      */
@@ -234,14 +247,8 @@ static void
gpio_rcar_config_general_input_output_mode(struct gpio_chip *chip,
     /* Select "General Input/Output Mode" in IOINTSEL */
     gpio_rcar_modify_bit(p, IOINTSEL, gpio, false);

-    /* Select Input Mode or Output Mode in INOUTSEL */
-    gpio_rcar_modify_bit(p, INOUTSEL, gpio, output);
-
     spin_unlock_irqrestore(&p->lock, flags);
-}

-static int gpio_rcar_request(struct gpio_chip *chip, unsigned offset)
-{
     return pinctrl_request_gpio(chip->base + offset);
 }

.request() is always called before .set_direction() when issuing
gpiod_get() anyways, so the order required according to the comment
will be satisfied when the GPIO is first requested, but if we're just
using the interrupt, we still will be able to set the direction right.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: convince line to become input in irq helper

[Grygorii Strashko]

Hi Linus,

On 07/05/2016 05:52 PM, Linus Walleij wrote:
> I sent a patch for the direction setter to be more careful, but
> it's no silver bullet for strange semantics.
> 
> On Tue, Jul 5, 2016 at 12:07 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> 
>> [1] gpio_rcar e6052000.gpio: sense irq = 11, type = 8
>>      ravb e6800000.ethernet eth0: Base address at 0xe6800000,
>> 2e:09:0a:00:83:1e, IRQ 131.
>>      ...
>> [2] gpiochip_irq_reqres: gpiochip e6052000.gpio
>> [3] gpio_rcar e6052000.gpio: gpio_rcar_direction_input: 11
>> [4] gpiochip_irq_reqres: desc->flags = 0x0
> (...)
>> This configures the GPIO for plain input mode, cfr. [3] above, basically
>> undoing the configuration from [1]. Hence interrupts no longer come through,
>> and Ethernet fails.
> 
> The driver is a bit fragile in that it relies on a certain call semantic,
> I guess it is not a widespread problem so we should be able to make
> a local fix if necessary.
> 
> The .set_direction() call should
> set the direction. Why is it turning off interrupts as a side effect?
> 
> What happens if you apply this, making the .request() function handle
> the pin setup and .set_direction() really just setting the
> direction?
> 
> diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
> index 681c93fb9e70..68fb0147caf4 100644
> --- a/drivers/gpio/gpio-rcar.c
> +++ b/drivers/gpio/gpio-rcar.c
> @@ -221,7 +221,20 @@ static void
> gpio_rcar_config_general_input_output_mode(struct gpio_chip *chip,
>       struct gpio_rcar_priv *p = gpiochip_get_data(chip);
>       unsigned long flags;
> 
> -    /* follow steps in the GPIO documentation for
> +    spin_lock_irqsave(&p->lock, flags);
> +
> +    /* Select Input Mode or Output Mode in INOUTSEL */
> +    gpio_rcar_modify_bit(p, INOUTSEL, gpio, output);
> +
> +    spin_unlock_irqrestore(&p->lock, flags);
> +}
> +
> +static int gpio_rcar_request(struct gpio_chip *chip, unsigned offset)
> +{
> +    struct gpio_rcar_priv *p = gpiochip_get_data(chip);
> +
> +    /*
> +     * follow steps in the GPIO documentation for
>        * "Setting General Output Mode" and
>        * "Setting General Input Mode"
>        */
> @@ -234,14 +247,8 @@ static void
> gpio_rcar_config_general_input_output_mode(struct gpio_chip *chip,
>       /* Select "General Input/Output Mode" in IOINTSEL */
>       gpio_rcar_modify_bit(p, IOINTSEL, gpio, false);
> 
> -    /* Select Input Mode or Output Mode in INOUTSEL */
> -    gpio_rcar_modify_bit(p, INOUTSEL, gpio, output);
> -
>       spin_unlock_irqrestore(&p->lock, flags);
> -}
> 
> -static int gpio_rcar_request(struct gpio_chip *chip, unsigned offset)
> -{
>       return pinctrl_request_gpio(chip->base + offset);
>   }
> 
> .request() is always called before .set_direction() when issuing
> gpiod_get() anyways, so the order required according to the comment
> will be satisfied when the GPIO is first requested, but if we're just
> using the interrupt, we still will be able to set the direction right.

I'd like to note few issues with this patch (commit
7e7c059cb50c7c72d5a393b2c34fc57de1b01b55 in gpio-next)

And sorry for late comment.

first of all for slow GPIO controllers (like pcf857x) it might be unsafe to call
.direction_input() from .irq_request_resources() callback because it 
will be called in atomic context under raw_spin_lock_irqsave().


__setup_irq
   |- raw_spin_lock_irqsave()
 	|- irq_request_resources()
        |- [ __irq_set_trigger() ]
	|- [ irq_startup() ]
	|- [ __enable_irq() ]
   |- raw_spin_unlock_irqrestore()

[and it can be unsafe for fast GPIO chips also ;( , for example if it's
required to do some kind of PM management actions before accessing HW - most of
drivers expect their GPIOchip callbacks to be called only after gpiochip.request() or 
.irq_startup().]

In  my opinion this change breaks orthogonality of IRQchip vs GPIOchip interfaces
because it adds and (what is more important) hides call of GPIOchip callback
from inside IRQchip interface somewhere in  the depths of gpiolib framework.
As result, GPIO drivers lose possibility to properly handle GPIO vs IRQ interface's
differences like: locking, slow vs fast, hw specific settings, features of frameworks.

I propose to revert it, sry, 

PS. But think that it still can be useful to make gpiochip_irq_reqres()/gpiochip_irq_relres()
public (as helpers) in their current form.


-- 
regards,
-grygorii

Re: [PATCH v2] gpio: convince line to become input in irq helper

[Linus Walleij]

On Wed, Jul 6, 2016 at 11:07 AM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> first of all for slow GPIO controllers (like pcf857x) it might be unsafe to call
> .direction_input() from .irq_request_resources() callback because it
> will be called in atomic context under raw_spin_lock_irqsave().
>
>
> __setup_irq
>    |- raw_spin_lock_irqsave()
>         |- irq_request_resources()
>         |- [ __irq_set_trigger() ]
>         |- [ irq_startup() ]
>         |- [ __enable_irq() ]
>    |- raw_spin_unlock_irqrestore()
>
> [and it can be unsafe for fast GPIO chips also ;( , for example if it's
> required to do some kind of PM management actions before accessing HW - most of
> drivers expect their GPIOchip callbacks to be called only after gpiochip.request() or
> .irq_startup().]
>
> In  my opinion this change breaks orthogonality of IRQchip vs GPIOchip interfaces
> because it adds and (what is more important) hides call of GPIOchip callback
> from inside IRQchip interface somewhere in  the depths of gpiolib framework.
> As result, GPIO drivers lose possibility to properly handle GPIO vs IRQ interface's
> differences like: locking, slow vs fast, hw specific settings, features of frameworks.
>
> I propose to revert it, sry,

Aw typical, you are right.

OK I'll take this patch and it's fixes out of the GPIO tree :(

Yours,
Linus Walleij