[PATCH] gpio: rcar: Check for irq_set_irq_wake() failures

[PATCH] gpio: rcar: Check for irq_set_irq_wake() failures

[Geert Uytterhoeven]

If an interrupt controller doesn't support wake-up configuration, trying
to deconfigure wake-up will cause a warning:

    WARNING: CPU: 1 PID: 1341 at kernel/irq/manage.c:540 irq_set_irq_wake+0x9c/0xf8()
    Unbalanced IRQ 26 wake disable

To fix this, refrain from any further parent interrupt controller
(de)configuration if irq_set_irq_wake() failed.

Fixes: ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable when wake-up is enabled")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
This is an alternative for:
  - calling "gic_set_irqchip_flags(IRQCHIP_SKIP_SET_WAKE)" from the
    platform code, OR
  - setting "gic_chip.flags = IRQCHIP_SKIP_SET_WAKE" in the GIC driver
    code.
---
 drivers/gpio/gpio-rcar.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
index fd39774659484fa6..1e14a6c74ed13941 100644
--- a/drivers/gpio/gpio-rcar.c
+++ b/drivers/gpio/gpio-rcar.c
@@ -177,8 +177,17 @@ static int gpio_rcar_irq_set_wake(struct irq_data *d, unsigned int on)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct gpio_rcar_priv *p = container_of(gc, struct gpio_rcar_priv,
 						gpio_chip);
-
-	irq_set_irq_wake(p->irq_parent, on);
+	int error;
+
+	if (p->irq_parent) {
+		error = irq_set_irq_wake(p->irq_parent, on);
+		if (error) {
+			dev_dbg(&p->pdev->dev,
+				"irq %u doesn't support irq_set_wake\n",
+				p->irq_parent);
+			p->irq_parent = 0;
+		}
+	}
 
 	if (!p->clk)
 		return 0;
-- 
1.9.1

Re: [PATCH] gpio: rcar: Check for irq_set_irq_wake() failures

[Linus Walleij]

On Mon, May 4, 2015 at 4:09 PM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> If an interrupt controller doesn't support wake-up configuration, trying
> to deconfigure wake-up will cause a warning:
>
>     WARNING: CPU: 1 PID: 1341 at kernel/irq/manage.c:540 irq_set_irq_wake+0x9c/0xf8()
>     Unbalanced IRQ 26 wake disable
>
> To fix this, refrain from any further parent interrupt controller
> (de)configuration if irq_set_irq_wake() failed.
>
> Fixes: ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable when wake-up is enabled")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> This is an alternative for:
>   - calling "gic_set_irqchip_flags(IRQCHIP_SKIP_SET_WAKE)" from the
>     platform code, OR
>   - setting "gic_chip.flags = IRQCHIP_SKIP_SET_WAKE" in the GIC driver
>     code.

This should be moved to the commit message I think.

> -       irq_set_irq_wake(p->irq_parent, on);
> +       int error;
> +
> +       if (p->irq_parent) {
> +               error = irq_set_irq_wake(p->irq_parent, on);
> +               if (error) {
> +                       dev_dbg(&p->pdev->dev,
> +                               "irq %u doesn't support irq_set_wake\n",
> +                               p->irq_parent);
> +                       p->irq_parent = 0;
> +               }
> +       }

Does the SH maintainers really like this... Warning
appear once and is squelched.

Isn't it better to make sure it doesn't happen or something.

It looks hacky. Any other suggestions?

Also, please convert this driver to use GPIOLIB_IRQCHIP
like everyone else.

Yours,
Linus Walleij

Re: [PATCH] gpio: rcar: Check for irq_set_irq_wake() failures

[Geert Uytterhoeven]

On Tue, May 12, 2015 at 9:55 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, May 4, 2015 at 4:09 PM, Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
>> If an interrupt controller doesn't support wake-up configuration, trying
>> to deconfigure wake-up will cause a warning:
>>
>>     WARNING: CPU: 1 PID: 1341 at kernel/irq/manage.c:540 irq_set_irq_wake+0x9c/0xf8()
>>     Unbalanced IRQ 26 wake disable
>>
>> To fix this, refrain from any further parent interrupt controller
>> (de)configuration if irq_set_irq_wake() failed.
>>
>> Fixes: ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable when wake-up is enabled")
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> This is an alternative for:
>>   - calling "gic_set_irqchip_flags(IRQCHIP_SKIP_SET_WAKE)" from the
>>     platform code, OR

Which is hacky, considering the below.

>>   - setting "gic_chip.flags = IRQCHIP_SKIP_SET_WAKE" in the GIC driver
>>     code.

Which was rejected by Marc, as the hardware doesn't support it.

> This should be moved to the commit message I think.

So that's why I put it below the "---".

>> -       irq_set_irq_wake(p->irq_parent, on);
>> +       int error;
>> +
>> +       if (p->irq_parent) {
>> +               error = irq_set_irq_wake(p->irq_parent, on);
>> +               if (error) {
>> +                       dev_dbg(&p->pdev->dev,
>> +                               "irq %u doesn't support irq_set_wake\n",
>> +                               p->irq_parent);
>> +                       p->irq_parent = 0;
>> +               }
>> +       }
>
> Does the SH maintainers really like this... Warning
> appear once and is squelched.
>
> Isn't it better to make sure it doesn't happen or something.
>
> It looks hacky. Any other suggestions?

The first call to irq_set_irq_wake() (on = true) doesn't print a warning.
It returns an error code, to indicate that the operation is not supported.

Calling irq_set_irq_wake() again (on = false, during resume) would print
a warning, as it wouldn't match internal state ("Unbalanced IRQ 26 wake
disable"). That one is gone now.

> Also, please convert this driver to use GPIOLIB_IRQCHIP
> like everyone else.

What old branch are you looking at, that it doesn't have commit
c7f3c5d3ac2d6831 ("gpio: rcar: Switch to use gpiolib irqchip helpers")
yet ;-)?

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

Re: [PATCH] gpio: rcar: Check for irq_set_irq_wake() failures

[Linus Walleij]

On Tue, May 12, 2015 at 10:07 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, May 12, 2015 at 9:55 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> This should be moved to the commit message I think.
>
> So that's why I put it below the "---".

So it was below the --- and that is why I ask you to move this
useful information above the --- and into the commit message.

>>> -       irq_set_irq_wake(p->irq_parent, on);
>>> +       int error;
>>> +
>>> +       if (p->irq_parent) {
>>> +               error = irq_set_irq_wake(p->irq_parent, on);
>>> +               if (error) {
>>> +                       dev_dbg(&p->pdev->dev,
>>> +                               "irq %u doesn't support irq_set_wake\n",
>>> +                               p->irq_parent);
>>> +                       p->irq_parent = 0;
>>> +               }
>>> +       }
>>
>> Does the SH maintainers really like this... Warning
>> appear once and is squelched.
>>
>> Isn't it better to make sure it doesn't happen or something.
>>
>> It looks hacky. Any other suggestions?
>
> The first call to irq_set_irq_wake() (on = true) doesn't print a warning.
> It returns an error code, to indicate that the operation is not supported.
>
> Calling irq_set_irq_wake() again (on = false, during resume) would print
> a warning, as it wouldn't match internal state ("Unbalanced IRQ 26 wake
> disable"). That one is gone now.

Yeah :/

There are a few different patches floating for irq_set_wake() things
and some seem to be causing regressions, I just want some
broader discussion on how we solve this across all platforms.

Maybe the GPIOLIB_IRQCHIP should handle the set_wake()
similarly for all chips?

>> Also, please convert this driver to use GPIOLIB_IRQCHIP
>> like everyone else.
>
> What old branch are you looking at, that it doesn't have commit
> c7f3c5d3ac2d6831 ("gpio: rcar: Switch to use gpiolib irqchip helpers")
> yet ;-)?

Argh sorry. (And that was an awesome patch.)

Yours,
Linus Walleij