[PATCH 0/3] Ensure twl4030 interrupts are lost during suspend.

[PATCH 0/3] Ensure twl4030 interrupts are lost during suspend.

[NeilBrown]

Sorry for the scatter-gun list of email address, but while each of
these patches only touches one subsystem, they really needed each
other to provide proper context.

The goal is to ensure that an interrupt that arrives on a TWL4030
connected to an OMAP3, during the 'late-suspend' stage will
cause the suspend to abort (if the interrupt is properly configured).
(e.g. power button, RTC alarm, USB plug/unplug)

The interrupt as seen by the OMAP3 is a level-triggered interrupt so
if it arrives at this time, it is masked.  With the current code it
remains masked and so cannot wake the device from suspend.

To fix this:
  The twl4030 drivers must mark the interrupt for wakeup
  The OMAP interrupt handler must allow the interrupt to be
     marked for wakeup
  The core interrupt/PM code must allow masked level-triggered
     interrupts to abort a suspend.

I'm least sure about that last one.  My patch unmasks any masked
interrupt that is suppose to support wakeup.  I suspect that is
correct but I don't know enough about general IRQ handling to be sure.
Maybe there could be cases where a driver has explicitly masked the
interrupt without cleaning the 'wakeup' setting.  Is that allowed?


---

NeilBrown (3):
      twl4030: enable wakeup on twl4030 IRQ.
      IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.
      ARM: omap2+: set IRQCHIP_SKIP_SET_WAKE for INTC interrupts.


 arch/arm/mach-omap2/irq.c |    1 +
 drivers/mfd/twl4030-irq.c |    1 +
 kernel/irq/pm.c           |    6 ++++++
 3 files changed, 8 insertions(+)

-- 
Signature

[PATCH 3/3] twl4030: enable wakeup on twl4030 IRQ.

[NeilBrown]

Most of the interrupts that come through this line should trigger
wakeups:
  power button
  RTC alarm
  power available
  usb plug/unplug

so mark the interrupt as a wakeup interrupt.
This is particularly important for when the interrupt arrives during
the late suspend phase.  Without this setting it will be ignored.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 drivers/mfd/twl4030-irq.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index 5d656e8..ad733d7 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -757,6 +757,7 @@ int twl4030_init_irq(struct device *dev, int irq_num)
 		dev_err(dev, "could not claim irq%d: %d\n", irq_num, status);
 		goto fail_rqirq;
 	}
+	enable_irq_wake(irq_num);
 
 	return irq_base;
 fail_rqirq:

Re: [PATCH 3/3] twl4030: enable wakeup on twl4030 IRQ.

[Kevin Hilman]

NeilBrown <neilb@suse.de> writes:

> Most of the interrupts that come through this line should trigger
> wakeups:
>   power button
>   RTC alarm
>   power available
>   usb plug/unplug
>
> so mark the interrupt as a wakeup interrupt.
> This is particularly important for when the interrupt arrives during
> the late suspend phase.  Without this setting it will be ignored.
>
> Signed-off-by: NeilBrown <neilb@suse.de>

Acked-by: Kevin Hilman <khilman@ti.com>


> ---
>
>  drivers/mfd/twl4030-irq.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
> index 5d656e8..ad733d7 100644
> --- a/drivers/mfd/twl4030-irq.c
> +++ b/drivers/mfd/twl4030-irq.c
> @@ -757,6 +757,7 @@ int twl4030_init_irq(struct device *dev, int irq_num)
>  		dev_err(dev, "could not claim irq%d: %d\n", irq_num, status);
>  		goto fail_rqirq;
>  	}
> +	enable_irq_wake(irq_num);
>  
>  	return irq_base;
>  fail_rqirq:
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] twl4030: enable wakeup on twl4030 IRQ.

[Samuel Ortiz]

Hi Neil,

On Wed, Apr 25, 2012 at 01:05:24PM +1000, NeilBrown wrote:
> Most of the interrupts that come through this line should trigger
> wakeups:
>   power button
>   RTC alarm
>   power available
>   usb plug/unplug
> 
> so mark the interrupt as a wakeup interrupt.
> This is particularly important for when the interrupt arrives during
> the late suspend phase.  Without this setting it will be ignored.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> 
>  drivers/mfd/twl4030-irq.c |    1 +
>  1 file changed, 1 insertion(+)
Patch applied, thanks.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 1/3] ARM: omap2+: set IRQCHIP_SKIP_SET_WAKE for INTC interrupts.

[NeilBrown]

All interrupts can wake-from-sleep (I think) so it should be
permissible to call enable_irq_wake().  Setting this flag allows that.

It is needed because without this, an interrupt which is delivered
during late suspend will get ignored but will not cause suspend to
abort.
If enable_irq_wake() is called and succeeds, check_wakuep_irqs()
will abort the suspend if the interrupt has fired.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 arch/arm/mach-omap2/irq.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-omap2/irq.c b/arch/arm/mach-omap2/irq.c
index 65f0d257..b0790a9 100644
--- a/arch/arm/mach-omap2/irq.c
+++ b/arch/arm/mach-omap2/irq.c
@@ -148,6 +148,7 @@ omap_alloc_gc(void __iomem *base, unsigned int irq_start, unsigned int num)
 	ct->chip.irq_ack = omap_mask_ack_irq;
 	ct->chip.irq_mask = irq_gc_mask_disable_reg;
 	ct->chip.irq_unmask = irq_gc_unmask_enable_reg;
+	ct->chip.flags |= IRQCHIP_SKIP_SET_WAKE;
 
 	ct->regs.ack = INTC_CONTROL;
 	ct->regs.enable = INTC_MIR_CLEAR0;

Re: [PATCH 1/3] ARM: omap2+: set IRQCHIP_SKIP_SET_WAKE for INTC interrupts.

[Kevin Hilman]

NeilBrown <neilb@suse.de> writes:

> All interrupts can wake-from-sleep (I think) so it should be
> permissible to call enable_irq_wake().  Setting this flag allows that.
>
> It is needed because without this, an interrupt which is delivered
> during late suspend will get ignored but will not cause suspend to
> abort.
> If enable_irq_wake() is called and succeeds, check_wakuep_irqs()
> will abort the suspend if the interrupt has fired.
>
> Signed-off-by: NeilBrown <neilb@suse.de>

Acked-by: Kevin Hilman <khilman@ti.com>

Tony, should I queue this one in my OMAP PM features for v3.5 branch?
or do you want to take it?

Kevin

Re: [PATCH 1/3] ARM: omap2+: set IRQCHIP_SKIP_SET_WAKE for INTC interrupts.

[Tony Lindgren]

* Kevin Hilman <khilman@ti.com> [120426 13:11]:
> NeilBrown <neilb@suse.de> writes:
> 
> > All interrupts can wake-from-sleep (I think) so it should be
> > permissible to call enable_irq_wake().  Setting this flag allows that.
> >
> > It is needed because without this, an interrupt which is delivered
> > during late suspend will get ignored but will not cause suspend to
> > abort.
> > If enable_irq_wake() is called and succeeds, check_wakuep_irqs()
> > will abort the suspend if the interrupt has fired.
> >
> > Signed-off-by: NeilBrown <neilb@suse.de>
> 
> Acked-by: Kevin Hilman <khilman@ti.com>
> 
> Tony, should I queue this one in my OMAP PM features for v3.5 branch?
> or do you want to take it?

You can take it since is's PM related.

Tony

Re: [PATCH 1/3] ARM: omap2+: set IRQCHIP_SKIP_SET_WAKE for INTC interrupts.

[Kevin Hilman]

NeilBrown <neilb@suse.de> writes:

> All interrupts can wake-from-sleep (I think) so it should be
> permissible to call enable_irq_wake().  Setting this flag allows that.
>
> It is needed because without this, an interrupt which is delivered
> during late suspend will get ignored but will not cause suspend to
> abort.
> If enable_irq_wake() is called and succeeds, check_wakuep_irqs()
> will abort the suspend if the interrupt has fired.
>
> Signed-off-by: NeilBrown <neilb@suse.de>

The name of this flag and the effect of setting it are somewhat
confusing (e.g. why does skipping set_wake suddenly make wakeups work.)
So I tried to make it clearer with a reworking of the changelog (below.)

If I understood this correctly, and if you're OK with the updated
changelog, I'll queue this up for v3.5.

Thanks,

Kevin

>From 644742ddae59731bc10aacde94645d7c49ca5ecd Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Wed, 25 Apr 2012 13:05:24 +1000
Subject: [PATCH] ARM: omap2+: set IRQCHIP_SKIP_SET_WAKE for INTC interrupts.

Without an ->irq_set_wake() method in an irq_chip, calls to
enable_irq_wake() will fail.  This also causes these interrupts to not
be able to abort suspend (via check_wakeup_irqs() in late suspend.)

Currently, we don't implement ->irq_set_wake() for INTC interrupts
because they default to be wakeup enabled by setting the GRPSEL bits
in PM init.  Even though there is no ->irq_set_wake(), we want
enable_irq_wake() to succeed so these interrupts can abort suspend
when necessary.

To fix, set IRQCHIP_SKIP_SET_WAKE flag for all the INTC
interrupts which avoids trying to check irq_chip->irq_set_wake()
and failing when it doesn't exist.

Longer term, we need to implement ->irq_set_wake() for the INTC
which can manage the appropriate GRPSEL bits.

Signed-off-by: NeilBrown <neilb@suse.de>
[khilman@ti.com: rework changelog]
Signed-off-by: Kevin Hilman <khilman@ti.com>
---
 arch/arm/mach-omap2/irq.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-omap2/irq.c b/arch/arm/mach-omap2/irq.c
index 65f0d257..b0790a9 100644
--- a/arch/arm/mach-omap2/irq.c
+++ b/arch/arm/mach-omap2/irq.c
@@ -148,6 +148,7 @@ omap_alloc_gc(void __iomem *base, unsigned int irq_start, unsigned int num)
 	ct->chip.irq_ack = omap_mask_ack_irq;
 	ct->chip.irq_mask = irq_gc_mask_disable_reg;
 	ct->chip.irq_unmask = irq_gc_unmask_enable_reg;
+	ct->chip.flags |= IRQCHIP_SKIP_SET_WAKE;
 
 	ct->regs.ack = INTC_CONTROL;
 	ct->regs.enable = INTC_MIR_CLEAR0;
-- 
1.7.9.2

Re: [PATCH 1/3] ARM: omap2+: set IRQCHIP_SKIP_SET_WAKE for INTC interrupts.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2917 bytes --]

On Thu, 26 Apr 2012 13:39:07 -0700 Kevin Hilman <khilman@ti.com> wrote:

> NeilBrown <neilb@suse.de> writes:
> 
> > All interrupts can wake-from-sleep (I think) so it should be
> > permissible to call enable_irq_wake().  Setting this flag allows that.
> >
> > It is needed because without this, an interrupt which is delivered
> > during late suspend will get ignored but will not cause suspend to
> > abort.
> > If enable_irq_wake() is called and succeeds, check_wakuep_irqs()
> > will abort the suspend if the interrupt has fired.
> >
> > Signed-off-by: NeilBrown <neilb@suse.de>
> 
> The name of this flag and the effect of setting it are somewhat
> confusing (e.g. why does skipping set_wake suddenly make wakeups work.)
> So I tried to make it clearer with a reworking of the changelog (below.)
> 
> If I understood this correctly, and if you're OK with the updated
> changelog, I'll queue this up for v3.5.
> 

I'm very OK with your updated changelog - thanks.  Filling in various bits
that I didn't know and clarifies the rest :-)

Thanks,
NeilBrown



> Thanks,
> 
> Kevin
> 
> >From 644742ddae59731bc10aacde94645d7c49ca5ecd Mon Sep 17 00:00:00 2001
> From: NeilBrown <neilb@suse.de>
> Date: Wed, 25 Apr 2012 13:05:24 +1000
> Subject: [PATCH] ARM: omap2+: set IRQCHIP_SKIP_SET_WAKE for INTC interrupts.
> 
> Without an ->irq_set_wake() method in an irq_chip, calls to
> enable_irq_wake() will fail.  This also causes these interrupts to not
> be able to abort suspend (via check_wakeup_irqs() in late suspend.)
> 
> Currently, we don't implement ->irq_set_wake() for INTC interrupts
> because they default to be wakeup enabled by setting the GRPSEL bits
> in PM init.  Even though there is no ->irq_set_wake(), we want
> enable_irq_wake() to succeed so these interrupts can abort suspend
> when necessary.
> 
> To fix, set IRQCHIP_SKIP_SET_WAKE flag for all the INTC
> interrupts which avoids trying to check irq_chip->irq_set_wake()
> and failing when it doesn't exist.
> 
> Longer term, we need to implement ->irq_set_wake() for the INTC
> which can manage the appropriate GRPSEL bits.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> [khilman@ti.com: rework changelog]
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> ---
>  arch/arm/mach-omap2/irq.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/mach-omap2/irq.c b/arch/arm/mach-omap2/irq.c
> index 65f0d257..b0790a9 100644
> --- a/arch/arm/mach-omap2/irq.c
> +++ b/arch/arm/mach-omap2/irq.c
> @@ -148,6 +148,7 @@ omap_alloc_gc(void __iomem *base, unsigned int irq_start, unsigned int num)
>  	ct->chip.irq_ack = omap_mask_ack_irq;
>  	ct->chip.irq_mask = irq_gc_mask_disable_reg;
>  	ct->chip.irq_unmask = irq_gc_unmask_enable_reg;
> +	ct->chip.flags |= IRQCHIP_SKIP_SET_WAKE;
>  
>  	ct->regs.ack = INTC_CONTROL;
>  	ct->regs.enable = INTC_MIR_CLEAR0;


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/3] ARM: omap2+: set IRQCHIP_SKIP_SET_WAKE for INTC interrupts.

[Kevin Hilman]

NeilBrown <neilb@suse.de> writes:

> On Thu, 26 Apr 2012 13:39:07 -0700 Kevin Hilman <khilman@ti.com> wrote:
>
>> NeilBrown <neilb@suse.de> writes:
>> 
>> > All interrupts can wake-from-sleep (I think) so it should be
>> > permissible to call enable_irq_wake().  Setting this flag allows that.
>> >
>> > It is needed because without this, an interrupt which is delivered
>> > during late suspend will get ignored but will not cause suspend to
>> > abort.
>> > If enable_irq_wake() is called and succeeds, check_wakuep_irqs()
>> > will abort the suspend if the interrupt has fired.
>> >
>> > Signed-off-by: NeilBrown <neilb@suse.de>
>> 
>> The name of this flag and the effect of setting it are somewhat
>> confusing (e.g. why does skipping set_wake suddenly make wakeups work.)
>> So I tried to make it clearer with a reworking of the changelog (below.)
>> 
>> If I understood this correctly, and if you're OK with the updated
>> changelog, I'll queue this up for v3.5.
>> 
>
> I'm very OK with your updated changelog - thanks.  Filling in various bits
> that I didn't know and clarifies the rest :-)

Great, thanks.

Adding ack from Santosh (thanks!) and queuing for v3.5[1]

Kevin

[1] git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.5/pm-misc

Re: [PATCH 1/3] ARM: omap2+: set IRQCHIP_SKIP_SET_WAKE for INTC interrupts.

[Shilimkar, Santosh]

On Fri, Apr 27, 2012 at 2:09 AM, Kevin Hilman <khilman@ti.com> wrote:
> NeilBrown <neilb@suse.de> writes:
>

[...]

>
> From 644742ddae59731bc10aacde94645d7c49ca5ecd Mon Sep 17 00:00:00 2001
> From: NeilBrown <neilb@suse.de>
> Date: Wed, 25 Apr 2012 13:05:24 +1000
> Subject: [PATCH] ARM: omap2+: set IRQCHIP_SKIP_SET_WAKE for INTC interrupts.
>
> Without an ->irq_set_wake() method in an irq_chip, calls to
> enable_irq_wake() will fail.  This also causes these interrupts to not
> be able to abort suspend (via check_wakeup_irqs() in late suspend.)
>
> Currently, we don't implement ->irq_set_wake() for INTC interrupts
> because they default to be wakeup enabled by setting the GRPSEL bits
> in PM init.  Even though there is no ->irq_set_wake(), we want
> enable_irq_wake() to succeed so these interrupts can abort suspend
> when necessary.
>
> To fix, set IRQCHIP_SKIP_SET_WAKE flag for all the INTC
> interrupts which avoids trying to check irq_chip->irq_set_wake()
> and failing when it doesn't exist.
>
> Longer term, we need to implement ->irq_set_wake() for the INTC
> which can manage the appropriate GRPSEL bits.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
> [khilman@ti.com: rework changelog]
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> ---
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

[PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

[NeilBrown]

Level triggered interrupts do not cause IRQS_PENDING to be set, so
check_wakeup_irqs ignores them.
They don't need to set IRQS_PENDING as the level stays high which
shows that they must be pending.  However if such an interrupt fired
during late suspend, it will have been masked so the fact that it
is still asserted will not cause the suspend to abort.

So if any wakeup interrupt is masked, unmask it when checking wakeup
irqs.  If the interrupt is asserted, suspend will abort.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 kernel/irq/pm.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 15e53b1..0d26206 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -106,6 +106,12 @@ int check_wakeup_irqs(void)
 		if (irqd_is_wakeup_set(&desc->irq_data)) {
 			if (desc->istate & IRQS_PENDING)
 				return -EBUSY;
+			if (irqd_irq_masked(&desc->irq_data))
+				/* Probably a level interrupt
+				 * which fired recently and was
+				 * masked
+				 */
+				unmask_irq(desc);
 			continue;
 		}
 		/*

Re: [PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

[Thomas Gleixner]

On Wed, 25 Apr 2012, NeilBrown wrote:

> Level triggered interrupts do not cause IRQS_PENDING to be set, so
> check_wakeup_irqs ignores them.
> They don't need to set IRQS_PENDING as the level stays high which
> shows that they must be pending.  However if such an interrupt fired
> during late suspend, it will have been masked so the fact that it
> is still asserted will not cause the suspend to abort.
> 
> So if any wakeup interrupt is masked, unmask it when checking wakeup
> irqs.  If the interrupt is asserted, suspend will abort.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> 
>  kernel/irq/pm.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> index 15e53b1..0d26206 100644
> --- a/kernel/irq/pm.c
> +++ b/kernel/irq/pm.c
> @@ -106,6 +106,12 @@ int check_wakeup_irqs(void)
>  		if (irqd_is_wakeup_set(&desc->irq_data)) {
>  			if (desc->istate & IRQS_PENDING)
>  				return -EBUSY;
> +			if (irqd_irq_masked(&desc->irq_data))
> +				/* Probably a level interrupt
> +				 * which fired recently and was
> +				 * masked
> +				 */
> +				unmask_irq(desc);

Oh no. We don't unmask unconditionally. What about an interrupt which
is disabled, has no handler ..... ? That needs more thought.

Thanks,

	tglx

Re: [PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1774 bytes --]

On Wed, 25 Apr 2012 10:50:15 +0200 (CEST) Thomas Gleixner
<tglx@linutronix.de> wrote:

> On Wed, 25 Apr 2012, NeilBrown wrote:
> 
> > Level triggered interrupts do not cause IRQS_PENDING to be set, so
> > check_wakeup_irqs ignores them.
> > They don't need to set IRQS_PENDING as the level stays high which
> > shows that they must be pending.  However if such an interrupt fired
> > during late suspend, it will have been masked so the fact that it
> > is still asserted will not cause the suspend to abort.
> > 
> > So if any wakeup interrupt is masked, unmask it when checking wakeup
> > irqs.  If the interrupt is asserted, suspend will abort.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> > 
> >  kernel/irq/pm.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > index 15e53b1..0d26206 100644
> > --- a/kernel/irq/pm.c
> > +++ b/kernel/irq/pm.c
> > @@ -106,6 +106,12 @@ int check_wakeup_irqs(void)
> >  		if (irqd_is_wakeup_set(&desc->irq_data)) {
> >  			if (desc->istate & IRQS_PENDING)
> >  				return -EBUSY;
> > +			if (irqd_irq_masked(&desc->irq_data))
> > +				/* Probably a level interrupt
> > +				 * which fired recently and was
> > +				 * masked
> > +				 */
> > +				unmask_irq(desc);
> 
> Oh no. We don't unmask unconditionally. What about an interrupt which
> is disabled, has no handler ..... ? That needs more thought.

If there is no handler, then irqd_is_wakeup_set() should fail should it not?

For disabled: would it be OK to check desc->depth?
Something like:
     if (desc->depth == 1 && (desc->state & IRQS_SUSPENDED) &&
         irqd_irq_masked(&desc->irq_data))
              unmask_irq(desc);

??

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

[Thomas Gleixner]

On Wed, 25 Apr 2012, NeilBrown wrote:
> On Wed, 25 Apr 2012 10:50:15 +0200 (CEST) Thomas Gleixner
> <tglx@linutronix.de> wrote:
> 
> > On Wed, 25 Apr 2012, NeilBrown wrote:
> > 
> > > Level triggered interrupts do not cause IRQS_PENDING to be set, so
> > > check_wakeup_irqs ignores them.
> > > They don't need to set IRQS_PENDING as the level stays high which
> > > shows that they must be pending.  However if such an interrupt fired
> > > during late suspend, it will have been masked so the fact that it
> > > is still asserted will not cause the suspend to abort.
> > > 
> > > So if any wakeup interrupt is masked, unmask it when checking wakeup
> > > irqs.  If the interrupt is asserted, suspend will abort.
> > > 
> > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > ---
> > > 
> > >  kernel/irq/pm.c |    6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > > index 15e53b1..0d26206 100644
> > > --- a/kernel/irq/pm.c
> > > +++ b/kernel/irq/pm.c
> > > @@ -106,6 +106,12 @@ int check_wakeup_irqs(void)
> > >  		if (irqd_is_wakeup_set(&desc->irq_data)) {
> > >  			if (desc->istate & IRQS_PENDING)
> > >  				return -EBUSY;
> > > +			if (irqd_irq_masked(&desc->irq_data))
> > > +				/* Probably a level interrupt
> > > +				 * which fired recently and was
> > > +				 * masked
> > > +				 */
> > > +				unmask_irq(desc);
> > 
> > Oh no. We don't unmask unconditionally. What about an interrupt which
> > is disabled, has no handler ..... ? That needs more thought.
> 
> If there is no handler, then irqd_is_wakeup_set() should fail should it not?

Well, it does not. The wakeup state is a permanent flag on the irq
line. Though we don't have IRQS_SUSPENDED on that descriptor.
 
> For disabled: would it be OK to check desc->depth?
> Something like:
>      if (desc->depth == 1 && (desc->state & IRQS_SUSPENDED) &&
>          irqd_irq_masked(&desc->irq_data))
>               unmask_irq(desc);
> 

Why not simply managing the pending bit for level irqs ?

Index: tip/kernel/irq/chip.c
===================================================================
--- tip.orig/kernel/irq/chip.c
+++ tip/kernel/irq/chip.c
@@ -379,8 +379,10 @@ handle_level_irq(unsigned int irq, struc
 	 * If its disabled or no action available
 	 * keep it masked and get out of here
 	 */
-	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data)))
+	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
+		desc->istate |= IRQS_PENDING;
 		goto out_unlock;
+	}
 
 	handle_irq_event(desc);
 
Index: tip/kernel/irq/resend.c
===================================================================
--- tip.orig/kernel/irq/resend.c
+++ tip/kernel/irq/resend.c
@@ -58,10 +58,13 @@ void check_irq_resend(struct irq_desc *d
 	/*
 	 * We do not resend level type interrupts. Level type
 	 * interrupts are resent by hardware when they are still
-	 * active.
+	 * active. Clear the pending bit so suspend/resume does not
+	 * get confused.
 	 */
-	if (irq_settings_is_level(desc))
+	if (irq_settings_is_level(desc)) {
+		desc->istate &= ~IRQS_PENDING;
 		return;
+	}
 	if (desc->istate & IRQS_REPLAY)
 		return;
 	if (desc->istate & IRQS_PENDING) {

And to handle interrupts which have been disabled before suspend add:

Index: tip/kernel/irq/pm.c
===================================================================
--- tip.orig/kernel/irq/pm.c
+++ tip/kernel/irq/pm.c
@@ -103,7 +103,8 @@ int check_wakeup_irqs(void)
 	int irq;
 
 	for_each_irq_desc(irq, desc) {
-		if (irqd_is_wakeup_set(&desc->irq_data)) {
+		if (desc->depth == 1 &&
+		    irqd_is_wakeup_set(&desc->irq_data)) {
 			if (desc->istate & IRQS_PENDING)
 				return -EBUSY;
 			continue;

Re: [PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4533 bytes --]

On Wed, 25 Apr 2012 14:54:54 +0200 (CEST) Thomas Gleixner
<tglx@linutronix.de> wrote:

> On Wed, 25 Apr 2012, NeilBrown wrote:
> > On Wed, 25 Apr 2012 10:50:15 +0200 (CEST) Thomas Gleixner
> > <tglx@linutronix.de> wrote:
> > 
> > > On Wed, 25 Apr 2012, NeilBrown wrote:
> > > 
> > > > Level triggered interrupts do not cause IRQS_PENDING to be set, so
> > > > check_wakeup_irqs ignores them.
> > > > They don't need to set IRQS_PENDING as the level stays high which
> > > > shows that they must be pending.  However if such an interrupt fired
> > > > during late suspend, it will have been masked so the fact that it
> > > > is still asserted will not cause the suspend to abort.
> > > > 
> > > > So if any wakeup interrupt is masked, unmask it when checking wakeup
> > > > irqs.  If the interrupt is asserted, suspend will abort.
> > > > 
> > > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > > ---
> > > > 
> > > >  kernel/irq/pm.c |    6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > > > index 15e53b1..0d26206 100644
> > > > --- a/kernel/irq/pm.c
> > > > +++ b/kernel/irq/pm.c
> > > > @@ -106,6 +106,12 @@ int check_wakeup_irqs(void)
> > > >  		if (irqd_is_wakeup_set(&desc->irq_data)) {
> > > >  			if (desc->istate & IRQS_PENDING)
> > > >  				return -EBUSY;
> > > > +			if (irqd_irq_masked(&desc->irq_data))
> > > > +				/* Probably a level interrupt
> > > > +				 * which fired recently and was
> > > > +				 * masked
> > > > +				 */
> > > > +				unmask_irq(desc);
> > > 
> > > Oh no. We don't unmask unconditionally. What about an interrupt which
> > > is disabled, has no handler ..... ? That needs more thought.
> > 
> > If there is no handler, then irqd_is_wakeup_set() should fail should it not?
> 
> Well, it does not. The wakeup state is a permanent flag on the irq
> line. Though we don't have IRQS_SUSPENDED on that descriptor.
>  
> > For disabled: would it be OK to check desc->depth?
> > Something like:
> >      if (desc->depth == 1 && (desc->state & IRQS_SUSPENDED) &&
> >          irqd_irq_masked(&desc->irq_data))
> >               unmask_irq(desc);
> > 
> 
> Why not simply managing the pending bit for level irqs ?

Primarily because I was aiming for the simplest patch that would have the
desired effect.  Also because 'pending' is implicit in the level so it seems
superfluous to store the bit separately.  And understanding all the
consequences of that change is not something I felt up to.

However: thanks for the patch. I'll try to explore it tomorrow and see if
seems to be behaving correctly.

Thanks,
NeilBrown

> 
> Index: tip/kernel/irq/chip.c
> ===================================================================
> --- tip.orig/kernel/irq/chip.c
> +++ tip/kernel/irq/chip.c
> @@ -379,8 +379,10 @@ handle_level_irq(unsigned int irq, struc
>  	 * If its disabled or no action available
>  	 * keep it masked and get out of here
>  	 */
> -	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data)))
> +	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
> +		desc->istate |= IRQS_PENDING;
>  		goto out_unlock;
> +	}
>  
>  	handle_irq_event(desc);
>  
> Index: tip/kernel/irq/resend.c
> ===================================================================
> --- tip.orig/kernel/irq/resend.c
> +++ tip/kernel/irq/resend.c
> @@ -58,10 +58,13 @@ void check_irq_resend(struct irq_desc *d
>  	/*
>  	 * We do not resend level type interrupts. Level type
>  	 * interrupts are resent by hardware when they are still
> -	 * active.
> +	 * active. Clear the pending bit so suspend/resume does not
> +	 * get confused.
>  	 */
> -	if (irq_settings_is_level(desc))
> +	if (irq_settings_is_level(desc)) {
> +		desc->istate &= ~IRQS_PENDING;
>  		return;
> +	}
>  	if (desc->istate & IRQS_REPLAY)
>  		return;
>  	if (desc->istate & IRQS_PENDING) {
> 
> And to handle interrupts which have been disabled before suspend add:
> 
> Index: tip/kernel/irq/pm.c
> ===================================================================
> --- tip.orig/kernel/irq/pm.c
> +++ tip/kernel/irq/pm.c
> @@ -103,7 +103,8 @@ int check_wakeup_irqs(void)
>  	int irq;
>  
>  	for_each_irq_desc(irq, desc) {
> -		if (irqd_is_wakeup_set(&desc->irq_data)) {
> +		if (desc->depth == 1 &&
> +		    irqd_is_wakeup_set(&desc->irq_data)) {
>  			if (desc->istate & IRQS_PENDING)
>  				return -EBUSY;
>  			continue;


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2988 bytes --]

On Wed, 25 Apr 2012 14:54:54 +0200 (CEST) Thomas Gleixner
<tglx@linutronix.de> wrote:

> Why not simply managing the pending bit for level irqs ?
> 

Hi Thomas,
 thanks again for the patch.  I finally made time to test it and it works as
expected.  I've included it below with a change-log entry and tested-by:
in case that helps.

Thanks,
NeilBrown


From: Thomas Gleixner <tglx@linutronix.de>
Date: Wed, 25 Apr 2012 12:54:54 +0200
Subject: [PATCH] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

Level triggered interrupts do not cause IRQS_PENDING to be set when
they fire-while-disabled as the 'pending' state is always present in
the level - they automatically refire where re-enabled.

However the IRQS_PENDING flag is also used to abort a suspend cycle -
if any 'is_wakeup_set' interrupt is PENDING, check_wakeup_irqs() will
cause suspend to abort. Without IRQS_PENDING, suspend won't abort.

Consequently, level-triggered interrupts that fire during the 'noirq'
phase of suspend do not currently abort suspend.

So set IRQS_PENDING even for level triggered interrupts, and make sure
to clear the flag in check_irq_resend.

Also: in check_wakeup_irqs, ignore 'is_wakeup' interrupts that were
already disabled before suspend_device_irqs() disabled them further.

Tested-by: NeilBrown <neilb@suse.de>

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 6080f6b..741f836 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -379,8 +379,10 @@ handle_level_irq(unsigned int irq, struct irq_desc *desc)
 	 * If its disabled or no action available
 	 * keep it masked and get out of here
 	 */
-	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data)))
+	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
+		desc->istate |= IRQS_PENDING;
 		goto out_unlock;
+	}
 
 	handle_irq_event(desc);
 
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 15e53b1..b858ce3 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -103,7 +103,8 @@ int check_wakeup_irqs(void)
 	int irq;
 
 	for_each_irq_desc(irq, desc) {
-		if (irqd_is_wakeup_set(&desc->irq_data)) {
+		if (desc->depth == 1 &&
+		    irqd_is_wakeup_set(&desc->irq_data)) {
 			if (desc->istate & IRQS_PENDING)
 				return -EBUSY;
 			continue;
diff --git a/kernel/irq/resend.c b/kernel/irq/resend.c
index 14dd576..6454db7 100644
--- a/kernel/irq/resend.c
+++ b/kernel/irq/resend.c
@@ -58,10 +58,13 @@ void check_irq_resend(struct irq_desc *desc, unsigned int irq)
 	/*
 	 * We do not resend level type interrupts. Level type
 	 * interrupts are resent by hardware when they are still
-	 * active.
+	 * active. Clear the pending bit so suspend/resume does not
+	 * get confused.
 	 */
-	if (irq_settings_is_level(desc))
+	if (irq_settings_is_level(desc)) {
+		desc->istate &= ~IRQS_PENDING;
 		return;
+	}
 	if (desc->istate & IRQS_REPLAY)
 		return;
 	if (desc->istate & IRQS_PENDING) {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

[Thomas Gleixner]

Neil,

On Fri, 4 May 2012, NeilBrown wrote:

> On Wed, 25 Apr 2012 14:54:54 +0200 (CEST) Thomas Gleixner
> <tglx@linutronix.de> wrote:
> 
> > Why not simply managing the pending bit for level irqs ?
> > 
> 
> Hi Thomas,
>  thanks again for the patch.  I finally made time to test it and it works as
> expected.  I've included it below with a change-log entry and tested-by:
> in case that helps.

thanks for testing. The changelog is great. You know how to make the
live of lazy buggers easier :)
  
>  	for_each_irq_desc(irq, desc) {
> -		if (irqd_is_wakeup_set(&desc->irq_data)) {
> +		if (desc->depth == 1 &&
> +		    irqd_is_wakeup_set(&desc->irq_data)) {
>  			if (desc->istate & IRQS_PENDING)
>  				return -EBUSY;
>  			continue;

I split that part into a separate patch, as it's really a different
issue.

Thanks,

	tglx

Re: [PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]

On Fri, 4 May 2012 18:01:22 +0200 (CEST) Thomas Gleixner <tglx@linutronix.de>
wrote:

> Neil,
> 
> On Fri, 4 May 2012, NeilBrown wrote:
> 
> > On Wed, 25 Apr 2012 14:54:54 +0200 (CEST) Thomas Gleixner
> > <tglx@linutronix.de> wrote:
> > 
> > > Why not simply managing the pending bit for level irqs ?
> > > 
> > 
> > Hi Thomas,
> >  thanks again for the patch.  I finally made time to test it and it works as
> > expected.  I've included it below with a change-log entry and tested-by:
> > in case that helps.
> 
> thanks for testing. The changelog is great. You know how to make the
> live of lazy buggers easier :)

Just buttering you up so any future patches slip past easily :-)

I think I'll need to ask for IRQS_PENDING to be set for nested interrupts too
but I'll be a little while before  I an look at that issue properly and
propose a patch.

Thanks for your help,
NeilBrown



>   
> >  	for_each_irq_desc(irq, desc) {
> > -		if (irqd_is_wakeup_set(&desc->irq_data)) {
> > +		if (desc->depth == 1 &&
> > +		    irqd_is_wakeup_set(&desc->irq_data)) {
> >  			if (desc->istate & IRQS_PENDING)
> >  				return -EBUSY;
> >  			continue;
> 
> I split that part into a separate patch, as it's really a different
> issue.
> 
> Thanks,
> 
> 	tglx


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]