| Message ID | 1441731636-17610-2-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Tue, 8 Sep 2015, Geert Uytterhoeven wrote: > The renesas-intc-irqpin interrupt controller is cascaded to the GIC, but > its driver doesn't propagate wake-up settings to the parent interrupt > controller. > > Since commit aec89ef72ba6c944 ("irqchip/gic: Enable SKIP_SET_WAKE and > MASK_ON_SUSPEND"), the GIC driver masks interrupts during suspend, and > wake-up through gpio-keys now fails on r8a7740/armadillo and > sh73a0/kzm9g. > > Fix this by propagating wake-up settings to the parent interrupt > controller. There's no need to handle irq_set_irq_wake() failures, as > the renesas-intc-irqpin interrupt controller is always cascaded to a > GIC, and the GIC driver always sets SKIP_SET_WAKE since the > aforementioned commit. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/irqchip/irq-renesas-intc-irqpin.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/irqchip/irq-renesas-intc-irqpin.c b/drivers/irqchip/irq-renesas-intc-irqpin.c > index 0670ab4e3897bf61..457d22c09e352d56 100644 > --- a/drivers/irqchip/irq-renesas-intc-irqpin.c > +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c > @@ -283,6 +283,9 @@ static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type) > static int intc_irqpin_irq_set_wake(struct irq_data *d, unsigned int on) > { > struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d); > + int hw_irq = irqd_to_hwirq(d); > + > + irq_set_irq_wake(p->irq[hw_irq].requested_irq, on); Are you sure that this does not make lockdep unhappy due to lock nesting? Thanks, tglx -- 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
Hi Thomas, On Tue, Sep 8, 2015 at 9:05 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > On Tue, 8 Sep 2015, Geert Uytterhoeven wrote: >> The renesas-intc-irqpin interrupt controller is cascaded to the GIC, but >> its driver doesn't propagate wake-up settings to the parent interrupt >> controller. >> >> Since commit aec89ef72ba6c944 ("irqchip/gic: Enable SKIP_SET_WAKE and >> MASK_ON_SUSPEND"), the GIC driver masks interrupts during suspend, and >> wake-up through gpio-keys now fails on r8a7740/armadillo and >> sh73a0/kzm9g. >> >> Fix this by propagating wake-up settings to the parent interrupt >> controller. There's no need to handle irq_set_irq_wake() failures, as >> the renesas-intc-irqpin interrupt controller is always cascaded to a >> GIC, and the GIC driver always sets SKIP_SET_WAKE since the >> aforementioned commit. >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> --- >> drivers/irqchip/irq-renesas-intc-irqpin.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/irqchip/irq-renesas-intc-irqpin.c b/drivers/irqchip/irq-renesas-intc-irqpin.c >> index 0670ab4e3897bf61..457d22c09e352d56 100644 >> --- a/drivers/irqchip/irq-renesas-intc-irqpin.c >> +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c >> @@ -283,6 +283,9 @@ static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type) >> static int intc_irqpin_irq_set_wake(struct irq_data *d, unsigned int on) >> { >> struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d); >> + int hw_irq = irqd_to_hwirq(d); >> + >> + irq_set_irq_wake(p->irq[hw_irq].requested_irq, on); > > Are you sure that this does not make lockdep unhappy due to lock > nesting? Actually I did see one lockdep warning, so I'm aware we're probably gonna need a similar solution like commit a0a8bcf4670c2c69 ("gpiolib: irqchip: use different lockdep class for each gpio irqchip")? To be honest, these lockdep warnings aren't helping much here: on embedded we typical have several stacked interrupt controllers, so wake-up settings have to propagate from the bottom to the top of the stack. E.g. on sh73a0/kzm9g gpio-keys wake-up goes through 3 interrupt controllers: pcf875x -> renesas-intc-irqpin -> gic. Thanks! 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, 8 Sep 2015, Geert Uytterhoeven wrote: > >> --- a/drivers/irqchip/irq-renesas-intc-irqpin.c > >> +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c > >> @@ -283,6 +283,9 @@ static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type) > >> static int intc_irqpin_irq_set_wake(struct irq_data *d, unsigned int on) > >> { > >> struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d); > >> + int hw_irq = irqd_to_hwirq(d); > >> + > >> + irq_set_irq_wake(p->irq[hw_irq].requested_irq, on); > > > > Are you sure that this does not make lockdep unhappy due to lock > > nesting? > > Actually I did see one lockdep warning, so I'm aware we're probably gonna > need a similar solution like commit a0a8bcf4670c2c69 ("gpiolib: irqchip: > use different lockdep class for each gpio irqchip")? > > To be honest, these lockdep warnings aren't helping much here: on embedded > we typical have several stacked interrupt controllers, so wake-up settings > have to propagate from the bottom to the top of the stack. But ignoring them does not help much either, right? > E.g. on sh73a0/kzm9g gpio-keys wake-up goes through 3 interrupt controllers: > pcf875x -> renesas-intc-irqpin -> gic. So, yes a seperate locking class for that intc trainwreck is probably required. Thanks, tglx -- 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
Hi, On 09/08/2015 11:03 PM, Thomas Gleixner wrote: > On Tue, 8 Sep 2015, Geert Uytterhoeven wrote: >>>> --- a/drivers/irqchip/irq-renesas-intc-irqpin.c >>>> +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c >>>> @@ -283,6 +283,9 @@ static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type) >>>> static int intc_irqpin_irq_set_wake(struct irq_data *d, unsigned int on) >>>> { >>>> struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d); >>>> + int hw_irq = irqd_to_hwirq(d); >>>> + >>>> + irq_set_irq_wake(p->irq[hw_irq].requested_irq, on); >>> >>> Are you sure that this does not make lockdep unhappy due to lock >>> nesting? >> >> Actually I did see one lockdep warning, so I'm aware we're probably gonna >> need a similar solution like commit a0a8bcf4670c2c69 ("gpiolib: irqchip: >> use different lockdep class for each gpio irqchip")? >> >> To be honest, these lockdep warnings aren't helping much here: on embedded >> we typical have several stacked interrupt controllers, so wake-up settings >> have to propagate from the bottom to the top of the stack. > > But ignoring them does not help much either, right? > >> E.g. on sh73a0/kzm9g gpio-keys wake-up goes through 3 interrupt controllers: >> pcf875x -> renesas-intc-irqpin -> gic. > > So, yes a seperate locking class for that intc trainwreck is probably > required. > Just as an option, May be we can proceed with patch: [PATCH v2 2/6] genirq: fix irqchip_set_wake_parent if IRQCHIP_SKIP_SET_WAKE http://www.spinics.net/lists/linux-omap/msg121262.html As result, irq_chip_set_wake_parent() can be used here and no lockdep issues. Marc, what do you think? renesas-intc-irqpin has own .irq_set_wake() implementation and IRQCHIP_SKIP_SET_WAKE can't be used here as W/A.
On Wed, 9 Sep 2015, Grygorii Strashko wrote: > On 09/08/2015 11:03 PM, Thomas Gleixner wrote: > > So, yes a seperate locking class for that intc trainwreck is probably > > required. > > > > Just as an option, May be we can proceed with patch: > [PATCH v2 2/6] genirq: fix irqchip_set_wake_parent if IRQCHIP_SKIP_SET_WAKE > http://www.spinics.net/lists/linux-omap/msg121262.html > > As result, irq_chip_set_wake_parent() can be used here and no lockdep issues. The driver in question is not using hierarchical irq domains. So what would that patch solve? Thanks, tglx -- 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 09/09/2015 11:53 AM, Thomas Gleixner wrote: > On Wed, 9 Sep 2015, Grygorii Strashko wrote: >> On 09/08/2015 11:03 PM, Thomas Gleixner wrote: >>> So, yes a seperate locking class for that intc trainwreck is probably >>> required. >>> >> >> Just as an option, May be we can proceed with patch: >> [PATCH v2 2/6] genirq: fix irqchip_set_wake_parent if IRQCHIP_SKIP_SET_WAKE >> http://www.spinics.net/lists/linux-omap/msg121262.html >> >> As result, irq_chip_set_wake_parent() can be used here and no lockdep issues. > > The driver in question is not using hierarchical irq domains. So what > would that patch solve? > You are right. I misunderstood it. Sorry for the noise.
diff --git a/drivers/irqchip/irq-renesas-intc-irqpin.c b/drivers/irqchip/irq-renesas-intc-irqpin.c index 0670ab4e3897bf61..457d22c09e352d56 100644 --- a/drivers/irqchip/irq-renesas-intc-irqpin.c +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c @@ -283,6 +283,9 @@ static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type) static int intc_irqpin_irq_set_wake(struct irq_data *d, unsigned int on) { struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d); + int hw_irq = irqd_to_hwirq(d); + + irq_set_irq_wake(p->irq[hw_irq].requested_irq, on); if (!p->clk) return 0;
The renesas-intc-irqpin interrupt controller is cascaded to the GIC, but its driver doesn't propagate wake-up settings to the parent interrupt controller. Since commit aec89ef72ba6c944 ("irqchip/gic: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND"), the GIC driver masks interrupts during suspend, and wake-up through gpio-keys now fails on r8a7740/armadillo and sh73a0/kzm9g. Fix this by propagating wake-up settings to the parent interrupt controller. There's no need to handle irq_set_irq_wake() failures, as the renesas-intc-irqpin interrupt controller is always cascaded to a GIC, and the GIC driver always sets SKIP_SET_WAKE since the aforementioned commit. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/irqchip/irq-renesas-intc-irqpin.c | 3 +++ 1 file changed, 3 insertions(+)