[PATCH resend 1/2] gpio: pcf857x: Propagate wake-up setting to parent irq controller

[PATCH resend 1/2] gpio: pcf857x: Propagate wake-up setting to parent irq controller

[Geert Uytterhoeven]

The pcf857x GPIO and interrupt controller uses dummy_irq_chip, which
does not implement irq_chip.irq_set_wake() and does not set
IRQCHIP_SKIP_SET_WAKE.

This causes two s2ram issues if wake-up is enabled for the pcf857x GPIO
pins:
  1. During resume from s2ram, the following warning is printed:

     WARNING: CPU: 0 PID: 1046 at kernel/irq/manage.c:537 irq_set_irq_wake+0x9c/0xf8()
     Unbalanced IRQ 113 wake disable

  2. Wake-up through the pcf857x GPIO pins may fail, as the parent
     interrupt controller may be suspended.

Migrate the pcf857x GPIO and interrupt controller from dummy_irq_chip to
its own irq_chip. This irq chip implements irq_chip.irq_set_wake() to
propagate its wake-up setting to the parent interrupt controller.

This fixes wake-up through gpio-keys on sh73a0/kzm9g, where the pcf857x
interrupt is cascaded to irq-renesas-intc-irqpin, and the latter must
not be suspended when wake-up is enabled.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Resend: Use Santosh' new email address

 drivers/gpio/gpio-pcf857x.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
index 236708ad0a5ba0ab..22dbecb004cf6c5b 100644
--- a/drivers/gpio/gpio-pcf857x.c
+++ b/drivers/gpio/gpio-pcf857x.c
@@ -218,14 +218,45 @@ static irqreturn_t pcf857x_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+/*
+ * NOP functions
+ */
+static void noop(struct irq_data *data) { }
+
+static unsigned int noop_ret(struct irq_data *data)
+{
+	return 0;
+}
+
+static int pcf857x_irq_set_wake(struct irq_data *data, unsigned int on)
+{
+	struct pcf857x *gpio = irq_data_get_irq_chip_data(data);
+
+	irq_set_irq_wake(gpio->client->irq, on);
+	return 0;
+}
+
+static struct irq_chip pcf857x_irq_chip = {
+	.name		= "pcf857x",
+	.irq_startup	= noop_ret,
+	.irq_shutdown	= noop,
+	.irq_enable	= noop,
+	.irq_disable	= noop,
+	.irq_ack	= noop,
+	.irq_mask	= noop,
+	.irq_unmask	= noop,
+	.irq_set_wake	= pcf857x_irq_set_wake,
+};
+
 static int pcf857x_irq_domain_map(struct irq_domain *domain, unsigned int irq,
 				 irq_hw_number_t hw)
 {
 	struct pcf857x *gpio = domain->host_data;
 
 	irq_set_chip_and_handler(irq,
-				 &dummy_irq_chip,
+				 &pcf857x_irq_chip,
 				 handle_level_irq);
+	irq_set_chip_data(irq, gpio);
 #ifdef CONFIG_ARM
 	set_irq_flags(irq, IRQF_VALID);
 #else
-- 
1.9.1

[PATCH resend 2/2] [RFC] genirq: Set IRQCHIP_SKIP_SET_WAKE for no_irq_chip and dummy_irq_chip

[Geert Uytterhoeven]

If no_irq_chip or dummy_irq_chip are used for wake up (e.g. gpio-keys
with a simple GPIO controller), the following warning is printed on
resume from s2ram:

    WANING: CPU: 0 PID: 1046 at kernel/irq/manage.c:537 irq_set_irq_wake+0x9c/0xf8()
    Unbalanced IRQ 113 wake disable

This happens because no_irq_chip and dummy_irq_chip do not implement
irq_chip.irq_set_wake(), causing set_irq_wake_real() to return -ENXIO,
and irq_set_irq_wake() to reset the wake_depth to zero.

Set IRQCHIP_SKIP_SET_WAKE to indicate that irq_chip.irq_set_wake() is
not implemented.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Alternatively, can't we remove IRQCHIP_SKIP_SET_WAKE, and just check for
the presence of irq_chip.irq_set_wake()?
I'll be happy to send a patch to do that instead...

Is there anything that relies on this -ENXIO error code?
All irq_chip implementations that set IRQCHIP_SKIP_SET_WAKE do not
implement irq_chip.irq_set_wake(). There are probably more of them that
forgot to set IRQCHIP_SKIP_SET_WAKE though.
Am I missing something?
Commit 60f96b41f71d2a13 ("genirq: Add IRQCHIP_SKIP_SET_WAKE flag")
doesn't explain why adding the flag was chosen.

Thanks!
---
Resend: Use Santosh' new email address

 kernel/irq/dummychip.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/irq/dummychip.c b/kernel/irq/dummychip.c
index 988dc58e8847f6eb..326a67f2410bf95c 100644
--- a/kernel/irq/dummychip.c
+++ b/kernel/irq/dummychip.c
@@ -42,6 +42,7 @@ struct irq_chip no_irq_chip = {
 	.irq_enable	= noop,
 	.irq_disable	= noop,
 	.irq_ack	= ack_bad,
+	.flags		= IRQCHIP_SKIP_SET_WAKE,
 };
 
 /*
@@ -57,5 +58,6 @@ struct irq_chip dummy_irq_chip = {
 	.irq_ack	= noop,
 	.irq_mask	= noop,
 	.irq_unmask	= noop,
+	.flags		= IRQCHIP_SKIP_SET_WAKE,
 };
 EXPORT_SYMBOL_GPL(dummy_irq_chip);
-- 
1.9.1

Re: [PATCH resend 2/2] [RFC] genirq: Set IRQCHIP_SKIP_SET_WAKE for no_irq_chip and dummy_irq_chip

[santosh shilimkar]

On 1/12/2015 8:04 AM, Geert Uytterhoeven wrote:
> If no_irq_chip or dummy_irq_chip are used for wake up (e.g. gpio-keys
> with a simple GPIO controller), the following warning is printed on
> resume from s2ram:
>
>      WANING: CPU: 0 PID: 1046 at kernel/irq/manage.c:537 irq_set_irq_wake+0x9c/0xf8()
>      Unbalanced IRQ 113 wake disable
>
> This happens because no_irq_chip and dummy_irq_chip do not implement
> irq_chip.irq_set_wake(), causing set_irq_wake_real() to return -ENXIO,
> and irq_set_irq_wake() to reset the wake_depth to zero.
>
> Set IRQCHIP_SKIP_SET_WAKE to indicate that irq_chip.irq_set_wake() is
> not implemented.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Alternatively, can't we remove IRQCHIP_SKIP_SET_WAKE, and just check for
> the presence of irq_chip.irq_set_wake()?
> I'll be happy to send a patch to do that instead...
>
> Is there anything that relies on this -ENXIO error code?
> All irq_chip implementations that set IRQCHIP_SKIP_SET_WAKE do not
> implement irq_chip.irq_set_wake(). There are probably more of them that
> forgot to set IRQCHIP_SKIP_SET_WAKE though.
> Am I missing something?
> Commit 60f96b41f71d2a13 ("genirq: Add IRQCHIP_SKIP_SET_WAKE flag")
> doesn't explain why adding the flag was chosen.
>
The flag was added to avoid dummy irq_set_wake() implementation
as described in the commit.

------------------
commit 60f96b41f71d2a13d1c0a457b8b77958f77142d1
Author: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date:   Fri Sep 9 13:59:35 2011 +0530

     genirq: Add IRQCHIP_SKIP_SET_WAKE flag

     Some irq chips need the irq_set_wake() functionality, but do not
     require a irq_set_wake() callback. Instead of forcing an empty
     callback to be implemented add a flag which notes this fact. Check for
     the flag in set_irq_wake_real() and return success when set.

     Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
     Cc: Thomas Gleixner <tglx@linutronix.de>
------------------

Here is the relevant thread.
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-September/064590.html

As you can read from thread, the idea is to handle the need at
genirq level. Either with a flag or a dummy function.

Hope this helps.

Regards,
Santosh

Re: [PATCH resend 2/2] [RFC] genirq: Set IRQCHIP_SKIP_SET_WAKE for no_irq_chip and dummy_irq_chip

[Geert Uytterhoeven]

Hi Santosh,

On Mon, Jan 12, 2015 at 5:37 PM, santosh shilimkar
<santosh.shilimkar@oracle.com> wrote:
> On 1/12/2015 8:04 AM, Geert Uytterhoeven wrote:
>>
>> If no_irq_chip or dummy_irq_chip are used for wake up (e.g. gpio-keys
>> with a simple GPIO controller), the following warning is printed on
>> resume from s2ram:
>>
>>      WANING: CPU: 0 PID: 1046 at kernel/irq/manage.c:537
>> irq_set_irq_wake+0x9c/0xf8()
>>      Unbalanced IRQ 113 wake disable
>>
>> This happens because no_irq_chip and dummy_irq_chip do not implement
>> irq_chip.irq_set_wake(), causing set_irq_wake_real() to return -ENXIO,
>> and irq_set_irq_wake() to reset the wake_depth to zero.
>>
>> Set IRQCHIP_SKIP_SET_WAKE to indicate that irq_chip.irq_set_wake() is
>> not implemented.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> Alternatively, can't we remove IRQCHIP_SKIP_SET_WAKE, and just check for
>> the presence of irq_chip.irq_set_wake()?
>> I'll be happy to send a patch to do that instead...
>>
>> Is there anything that relies on this -ENXIO error code?
>> All irq_chip implementations that set IRQCHIP_SKIP_SET_WAKE do not
>> implement irq_chip.irq_set_wake(). There are probably more of them that
>> forgot to set IRQCHIP_SKIP_SET_WAKE though.
>> Am I missing something?
>> Commit 60f96b41f71d2a13 ("genirq: Add IRQCHIP_SKIP_SET_WAKE flag")
>> doesn't explain why adding the flag was chosen.
>>
> The flag was added to avoid dummy irq_set_wake() implementation
> as described in the commit.
>
> ------------------
> commit 60f96b41f71d2a13d1c0a457b8b77958f77142d1
> Author: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Date:   Fri Sep 9 13:59:35 2011 +0530
>
>     genirq: Add IRQCHIP_SKIP_SET_WAKE flag
>
>     Some irq chips need the irq_set_wake() functionality, but do not
>     require a irq_set_wake() callback. Instead of forcing an empty
>     callback to be implemented add a flag which notes this fact. Check for
>     the flag in set_irq_wake_real() and return success when set.
>
>     Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>     Cc: Thomas Gleixner <tglx@linutronix.de>
> ------------------

I had read that commit description.

> Here is the relevant thread.
> http://lists.infradead.org/pipermail/linux-arm-kernel/2011-September/064590.html
>
> As you can read from thread, the idea is to handle the need at
> genirq level. Either with a flag or a dummy function.

But it's not handled at genirq level. Every driver that doesn't implement the
.irq_set_wake() method has to set the flag. Several of these don't, causing
the warning.

Instead of having to fix them all, can't we remove IRQCHIP_SKIP_SET_WAKE,
and just check for the absence of irq_chip.irq_set_wake() instead?

Is there ever a valid use case for a driver to not provide a .irq_set_wake(),
and not set the flag?

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