| Message ID | 1430748592-6394-1-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)