[PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kevin Hilman]

Interrupts that are flagged as wakeup sources via set_irq_wake()
should not be disabled for suspend.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 kernel/irq/pm.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 638d8be..99113bd 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -28,6 +28,9 @@ void suspend_device_irqs(void)
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
 
+		if (desc->status & IRQ_WAKEUP)
+			continue;
+
 		spin_lock_irqsave(&desc->lock, flags);
 		__disable_irq(desc, irq, true);
 		spin_unlock_irqrestore(&desc->lock, flags);
-- 
1.6.2.2

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Andrew Morton]

On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:

> Interrupts that are flagged as wakeup sources via set_irq_wake()
> should not be disabled for suspend.
> 

Why not?

> 
> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> index 638d8be..99113bd 100644
> --- a/kernel/irq/pm.c
> +++ b/kernel/irq/pm.c
> @@ -28,6 +28,9 @@ void suspend_device_irqs(void)
>  	for_each_irq_desc(irq, desc) {
>  		unsigned long flags;
>  
> +		if (desc->status & IRQ_WAKEUP)
> +			continue;
> +
>  		spin_lock_irqsave(&desc->lock, flags);
>  		__disable_irq(desc, irq, true);
>  		spin_unlock_irqrestore(&desc->lock, flags);

If this fixes some bug then please provide a description of that bug?

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Vitaly Wool]

On Tue, May 5, 2009 at 10:54 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>
>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>> should not be disabled for suspend.
>>
>
> Why not?

Sounds like an interesting problem. If a wakeup depends on the
interrupt line, then no they definitely should not be disabled, as no
wakeup would happen otherwise. OTOH, this is not necessarily the case;
some SoCs configure wakeup sources as GPIOs (i.MXs do AFAIR) so it's
not clear if we should change generic source code in this case.

Vitaly

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kevin Hilman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>
>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>> should not be disabled for suspend.
>> 
>
> Why not?
>

If an interrupt is a wakeup source, and it is disabled at the chip
level, it will no longer generate interrupts, and thus no longer wake
up the system.

I'd be interested in hearing why wakeup interrupts should be disabled
during suspend.

>> 
>> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
>> index 638d8be..99113bd 100644
>> --- a/kernel/irq/pm.c
>> +++ b/kernel/irq/pm.c
>> @@ -28,6 +28,9 @@ void suspend_device_irqs(void)
>>  	for_each_irq_desc(irq, desc) {
>>  		unsigned long flags;
>>  
>> +		if (desc->status & IRQ_WAKEUP)
>> +			continue;
>> +
>>  		spin_lock_irqsave(&desc->lock, flags);
>>  		__disable_irq(desc, irq, true);
>>  		spin_unlock_irqrestore(&desc->lock, flags);
>
> If this fixes some bug then please provide a description of that bug?

The bug is that on TI OMAP, interrupts that are used for wakeup events
are disabled by this code causing the system to no longer wake up.

I'll update the patch description.

Kevin

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kevin Hilman]

Vitaly Wool <vitalywool@gmail.com> writes:

> On Tue, May 5, 2009 at 10:54 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>>
>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>>> should not be disabled for suspend.
>>>
>>
>> Why not?
>
> Sounds like an interesting problem. If a wakeup depends on the
> interrupt line, then no they definitely should not be disabled, as no
> wakeup would happen otherwise. 

Agreed.

> OTOH, this is not necessarily the case; some SoCs configure wakeup
> sources as GPIOs (i.MXs do AFAIR) so it's not clear if we should
> change generic source code in this case.

In the case of GPIO IRQs, then the call to __disable_irq() in
suspend_device_irqs() will trickle down to the irq_chip disable hook
in the GPIO code.  If that code stops the GPIO from triggering
interrupts, which it should, then my guess is your GPIO will no longer
wake the system either.

Kevin

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Arve Hjønnevåg]

On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
>
>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>>
>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>>> should not be disabled for suspend.
>>>
>>
>> Why not?
>>
>
> If an interrupt is a wakeup source, and it is disabled at the chip
> level, it will no longer generate interrupts, and thus no longer wake
> up the system.
>
> I'd be interested in hearing why wakeup interrupts should be disabled
> during suspend.
>
>>>
>>> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
>>> index 638d8be..99113bd 100644
>>> --- a/kernel/irq/pm.c
>>> +++ b/kernel/irq/pm.c
>>> @@ -28,6 +28,9 @@ void suspend_device_irqs(void)
>>>      for_each_irq_desc(irq, desc) {
>>>              unsigned long flags;
>>>
>>> +            if (desc->status & IRQ_WAKEUP)
>>> +                    continue;
>>> +
>>>              spin_lock_irqsave(&desc->lock, flags);
>>>              __disable_irq(desc, irq, true);
>>>              spin_unlock_irqrestore(&desc->lock, flags);
>>
>> If this fixes some bug then please provide a description of that bug?
>
> The bug is that on TI OMAP, interrupts that are used for wakeup events
> are disabled by this code causing the system to no longer wake up.

What do you do if the interrupt triggers right after your driver has
returned from its late suspend hook? By leaving the interrupt enabled
you prevent check_wakeup_irqs from aborting suspend.

-- 
Arve Hjønnevåg

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kevin Hilman]

Arve Hjønnevåg <arve@android.com> writes:

> On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
>> Andrew Morton <akpm@linux-foundation.org> writes:
>>
>>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>>>
>>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>>>> should not be disabled for suspend.
>>>>
>>>
>>> Why not?
>>>
>>
>> If an interrupt is a wakeup source, and it is disabled at the chip
>> level, it will no longer generate interrupts, and thus no longer wake
>> up the system.
>>
>> I'd be interested in hearing why wakeup interrupts should be disabled
>> during suspend.
>>

[...]

>>>
>>> If this fixes some bug then please provide a description of that bug?
>>
>> The bug is that on TI OMAP, interrupts that are used for wakeup events
>> are disabled by this code causing the system to no longer wake up.
>
> What do you do if the interrupt triggers right after your driver has
> returned from its late suspend hook?  

If it's a wakeup IRQ, I assume you want it to prevent suspend.

But I don't see how that can happen in the current code. IIUC, by the
time your late suspend hook is run, your device IRQ is already
disabled, so it won't trigger an interrupt that will be caught by
check_wakeup_irqs() anyways.

> By leaving the interrupt enabled you prevent check_wakeup_irqs from
> aborting suspend.

Yes, but it doesn't prevent platform-specific code from aborting
suspend.  On OMAP, the platform-specific suspend enter hook does a
last check for pending enabled interrupts very late in the sequence.
It seems to me that the platform specific code is the best place to do
this.

On a related note, what happens if your device triggers an interrupt
between check_wakeup_irqs() and the actual suspend?  Since it is a
wakeup IRQ, wouldn't you want it to abort the suspend too?

But all this discussion still leaves the bigger question unanswered:

Without my patch, how do we expect wakeup-enabled device IRQs to:

1) abort a suspend between suspend_device_irqs() and actual HW suspend, and
2) wake up the system *after it is suspended*

Kevin

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Rafael J. Wysocki]

On Wednesday 06 May 2009, Kevin Hilman wrote:
> Arve Hjønnevåg <arve@android.com> writes:
> 
> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
> > <khilman@deeprootsystems.com> wrote:
> >> Andrew Morton <akpm@linux-foundation.org> writes:
> >>
> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
> >>>
> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
> >>>> should not be disabled for suspend.
> >>>>
> >>>
> >>> Why not?
> >>>
> >>
> >> If an interrupt is a wakeup source, and it is disabled at the chip
> >> level, it will no longer generate interrupts, and thus no longer wake
> >> up the system.
> >>
> >> I'd be interested in hearing why wakeup interrupts should be disabled
> >> during suspend.

That depends on whether or not they are used for anything else than wake-up.

> 
> [...]
> 
> >>>
> >>> If this fixes some bug then please provide a description of that bug?
> >>
> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
> >> are disabled by this code causing the system to no longer wake up.
> >
> > What do you do if the interrupt triggers right after your driver has
> > returned from its late suspend hook?  
> 
> If it's a wakeup IRQ, I assume you want it to prevent suspend.
> 
> But I don't see how that can happen in the current code. IIUC, by the
> time your late suspend hook is run, your device IRQ is already
> disabled, so it won't trigger an interrupt that will be caught by
> check_wakeup_irqs() anyways.

My understanding of __disable_irq() was that it didn't actually disable the
IRQ at the hardware level, allowing the CPU to actually receive the interrupt
and acknowledge it, but preventing the device driver for receiving it.  Does
it work differently on the affected systems?

Rafael

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Arve Hjønnevåg]

2009/5/5 Kevin Hilman <khilman@deeprootsystems.com>:
> Arve Hjønnevåg <arve@android.com> writes:
>
>> On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
> [...]
>
>> By leaving the interrupt enabled you prevent check_wakeup_irqs from
>> aborting suspend.
>
> Yes, but it doesn't prevent platform-specific code from aborting
> suspend.  On OMAP, the platform-specific suspend enter hook does a
> last check for pending enabled interrupts very late in the sequence.
> It seems to me that the platform specific code is the best place to do
> this.

It will not be pending anymore if the interrupt triggered before all
interrupts were masked at the cpu.

> On a related note, what happens if your device triggers an interrupt
> between check_wakeup_irqs() and the actual suspend?  Since it is a
> wakeup IRQ, wouldn't you want it to abort the suspend too?
>

check_wakeup_irqs is called after masking interrupts, so your existing
platform specific check should catch it.

-- 
Arve Hjønnevåg

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Arve Hjønnevåg]

On Tue, May 5, 2009 at 4:27 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday 06 May 2009, Kevin Hilman wrote:
>> Arve Hjønnevåg <arve@android.com> writes:
>>
>> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
>> > <khilman@deeprootsystems.com> wrote:
>> >> Andrew Morton <akpm@linux-foundation.org> writes:
>> >>
>> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>> >>>
>> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>> >>>> should not be disabled for suspend.
>> >>>>
>> >>>
>> >>> Why not?
>> >>>
>> >>
>> >> If an interrupt is a wakeup source, and it is disabled at the chip
>> >> level, it will no longer generate interrupts, and thus no longer wake
>> >> up the system.
>> >>
>> >> I'd be interested in hearing why wakeup interrupts should be disabled
>> >> during suspend.
>
> That depends on whether or not they are used for anything else than wake-up.
>
>>
>> [...]
>>
>> >>>
>> >>> If this fixes some bug then please provide a description of that bug?
>> >>
>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
>> >> are disabled by this code causing the system to no longer wake up.
>> >
>> > What do you do if the interrupt triggers right after your driver has
>> > returned from its late suspend hook?
>>
>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
>>
>> But I don't see how that can happen in the current code. IIUC, by the
>> time your late suspend hook is run, your device IRQ is already
>> disabled, so it won't trigger an interrupt that will be caught by
>> check_wakeup_irqs() anyways.
>
> My understanding of __disable_irq() was that it didn't actually disable the
> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
> and acknowledge it, but preventing the device driver for receiving it.  Does
> it work differently on the affected systems?

If the irq_chip implements mask and unmask, then this is how it works,
but it can override this by also implementing disable. If the irq_chip
disable hook turns off the edge detection for an edge triggered wakeup
interrupt, it will not allow drivers or the framework to use
disable_irq to mask an interrupt.

-- 
Arve Hjønnevåg

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Wednesday 06 May 2009, Kevin Hilman wrote:
>> Arve Hjønnevåg <arve@android.com> writes:
>> 
>> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
>> > <khilman@deeprootsystems.com> wrote:
>> >> Andrew Morton <akpm@linux-foundation.org> writes:
>> >>
>> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>> >>>
>> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>> >>>> should not be disabled for suspend.
>> >>>>
>> >>>
>> >>> Why not?
>> >>>
>> >>
>> >> If an interrupt is a wakeup source, and it is disabled at the chip
>> >> level, it will no longer generate interrupts, and thus no longer wake
>> >> up the system.
>> >>
>> >> I'd be interested in hearing why wakeup interrupts should be disabled
>> >> during suspend.
>
> That depends on whether or not they are used for anything else than wake-up.
>
>> 
>> [...]
>> 
>> >>>
>> >>> If this fixes some bug then please provide a description of that bug?
>> >>
>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
>> >> are disabled by this code causing the system to no longer wake up.
>> >
>> > What do you do if the interrupt triggers right after your driver has
>> > returned from its late suspend hook?  
>> 
>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
>> 
>> But I don't see how that can happen in the current code. IIUC, by the
>> time your late suspend hook is run, your device IRQ is already
>> disabled, so it won't trigger an interrupt that will be caught by
>> check_wakeup_irqs() anyways.
>
> My understanding of __disable_irq() was that it didn't actually disable the
> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
> and acknowledge it, but preventing the device driver for receiving it.  

Hmm, that's not normally what I think of as disabled.  ;)

> Does it work differently on the affected systems?

Yes.

__disable_irq() calls the irq_chip's disable method which is platform
specific.  On OMAP, this masks the IRQ at the hardware level
preventing the CPU from seeing the interrupt.

Kevin

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kim Kyuwon]

On Wed, May 6, 2009 at 8:27 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday 06 May 2009, Kevin Hilman wrote:
>> Arve Hjønnevåg <arve@android.com> writes:
>>
>> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
>> > <khilman@deeprootsystems.com> wrote:
>> >> Andrew Morton <akpm@linux-foundation.org> writes:
>> >>
>> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>> >>>
>> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>> >>>> should not be disabled for suspend.
>> >>>>
>> >>>
>> >>> Why not?
>> >>>
>> >>
>> >> If an interrupt is a wakeup source, and it is disabled at the chip
>> >> level, it will no longer generate interrupts, and thus no longer wake
>> >> up the system.
>> >>
>> >> I'd be interested in hearing why wakeup interrupts should be disabled
>> >> during suspend.
>
> That depends on whether or not they are used for anything else than wake-up.
>
>>
>> [...]
>>
>> >>>
>> >>> If this fixes some bug then please provide a description of that bug?
>> >>
>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
>> >> are disabled by this code causing the system to no longer wake up.
>> >
>> > What do you do if the interrupt triggers right after your driver has
>> > returned from its late suspend hook?
>>
>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
>>
>> But I don't see how that can happen in the current code. IIUC, by the
>> time your late suspend hook is run, your device IRQ is already
>> disabled, so it won't trigger an interrupt that will be caught by
>> check_wakeup_irqs() anyways.
>
> My understanding of __disable_irq() was that it didn't actually disable the
> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
> and acknowledge it, but preventing the device driver for receiving it.  Does
> it work differently on the affected systems?

Hi Rafael,

You can refer to:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5461af5af5c6a7fee78978aafe720541bf3a2f55

Thanks,
Kyuwon

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kevin Hilman]

Kevin Hilman <khilman@deeprootsystems.com> writes:

> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>
>> On Wednesday 06 May 2009, Kevin Hilman wrote:
>>> Arve Hjønnevåg <arve@android.com> writes:
>>> 
>>> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
>>> > <khilman@deeprootsystems.com> wrote:
>>> >> Andrew Morton <akpm@linux-foundation.org> writes:
>>> >>
>>> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>>> >>>
>>> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>>> >>>> should not be disabled for suspend.
>>> >>>>
>>> >>>
>>> >>> Why not?
>>> >>>
>>> >>
>>> >> If an interrupt is a wakeup source, and it is disabled at the chip
>>> >> level, it will no longer generate interrupts, and thus no longer wake
>>> >> up the system.
>>> >>
>>> >> I'd be interested in hearing why wakeup interrupts should be disabled
>>> >> during suspend.
>>
>> That depends on whether or not they are used for anything else than wake-up.
>>
>>> 
>>> [...]
>>> 
>>> >>>
>>> >>> If this fixes some bug then please provide a description of that bug?
>>> >>
>>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
>>> >> are disabled by this code causing the system to no longer wake up.
>>> >
>>> > What do you do if the interrupt triggers right after your driver has
>>> > returned from its late suspend hook?  
>>> 
>>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
>>> 
>>> But I don't see how that can happen in the current code. IIUC, by the
>>> time your late suspend hook is run, your device IRQ is already
>>> disabled, so it won't trigger an interrupt that will be caught by
>>> check_wakeup_irqs() anyways.
>>
>> My understanding of __disable_irq() was that it didn't actually disable the
>> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
>> and acknowledge it, but preventing the device driver for receiving it.  
>
> Hmm, that's not normally what I think of as disabled.  ;)
>
>> Does it work differently on the affected systems?
>
> Yes.
>
> __disable_irq() calls the irq_chip's disable method which is platform
> specific.  On OMAP, this masks the IRQ at the hardware level
> preventing the CPU from seeing the interrupt.

So just as a test, I just removed the 'disable' hook from my platforms
irq_chip and this allows me to wakeup without using my proposed patch,
although I'm not sure it is the right behavior either.

The 'struct irq_chip' comments are a bit misleading here as it says

 * @disable:		disable the interrupt (defaults to chip->mask if NULL)

And since my irq_chip->disable was doing basically the same thing as
my irq_chip->mask, I didn't expect it to change behavior.  But in
kernel/irq/chip.c, disable gets set to an empty default_disable if the
irq_chip's version is NULL.

The result is that if irq_chip->disable == NULL, suspend_device_irqs() is a 
big NOP, albiet one that does lots of locking. :)

So, should the irq_chip code be fixed to match the comment?  Something
like the patch below?  If I fix the IRQ chip code, then I'm back to
needing my patch since my irq_chip mask function still masks the IRQ
at the hardware.

Kevin


commit f9b534f23ac7835eead99fb0a9cec7c505fe1e85
Author: Kevin Hilman <khilman@deeprootsystems.com>
Date:   Tue May 5 17:32:59 2009 -0700

    IRQ: chip->disable should default to chip->mask if NULL
    
    The struct irq_chip comments in <linux/irq.h> state:
    
     * @disable:	disable the interrupt (defaults to chip->mask if NULL)
    
    However, the code in kernel/irq/chip.c does otherwise by setting
    a NULL disable hook to an empty default_disable function.
    
    This patch makes the default_disable function call the ->mask hook
    to match the comments.
    
    Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index c687ba4..0fb690a 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -238,6 +238,10 @@ static void default_enable(unsigned int irq)
  */
 static void default_disable(unsigned int irq)
 {
+	struct irq_desc *desc = irq_to_desc(irq);
+
+	desc->chip->mask(irq);
+	desc->status |= IRQ_MASKED;
 }
 
 /*

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kevin Hilman]

Kevin Hilman <khilman@deeprootsystems.com> writes:

> Kevin Hilman <khilman@deeprootsystems.com> writes:
>
>> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>>
>>> On Wednesday 06 May 2009, Kevin Hilman wrote:
>>>> Arve Hjønnevåg <arve@android.com> writes:
>>>> 
>>>> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
>>>> > <khilman@deeprootsystems.com> wrote:
>>>> >> Andrew Morton <akpm@linux-foundation.org> writes:
>>>> >>
>>>> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>>>> >>>
>>>> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>>>> >>>> should not be disabled for suspend.
>>>> >>>>
>>>> >>>
>>>> >>> Why not?
>>>> >>>
>>>> >>
>>>> >> If an interrupt is a wakeup source, and it is disabled at the chip
>>>> >> level, it will no longer generate interrupts, and thus no longer wake
>>>> >> up the system.
>>>> >>
>>>> >> I'd be interested in hearing why wakeup interrupts should be disabled
>>>> >> during suspend.
>>>
>>> That depends on whether or not they are used for anything else than wake-up.
>>>
>>>> 
>>>> [...]
>>>> 
>>>> >>>
>>>> >>> If this fixes some bug then please provide a description of that bug?
>>>> >>
>>>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
>>>> >> are disabled by this code causing the system to no longer wake up.
>>>> >
>>>> > What do you do if the interrupt triggers right after your driver has
>>>> > returned from its late suspend hook?  
>>>> 
>>>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
>>>> 
>>>> But I don't see how that can happen in the current code. IIUC, by the
>>>> time your late suspend hook is run, your device IRQ is already
>>>> disabled, so it won't trigger an interrupt that will be caught by
>>>> check_wakeup_irqs() anyways.
>>>
>>> My understanding of __disable_irq() was that it didn't actually disable the
>>> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
>>> and acknowledge it, but preventing the device driver for receiving it.  
>>
>> Hmm, that's not normally what I think of as disabled.  ;)
>>
>>> Does it work differently on the affected systems?
>>
>> Yes.
>>
>> __disable_irq() calls the irq_chip's disable method which is platform
>> specific.  On OMAP, this masks the IRQ at the hardware level
>> preventing the CPU from seeing the interrupt.
>
> So just as a test, I just removed the 'disable' hook from my platforms
> irq_chip and this allows me to wakeup without using my proposed patch,
> although I'm not sure it is the right behavior either.
>
> The 'struct irq_chip' comments are a bit misleading here as it says
>
>  * @disable:		disable the interrupt (defaults to chip->mask if NULL)
>
> And since my irq_chip->disable was doing basically the same thing as
> my irq_chip->mask, I didn't expect it to change behavior.  But in
> kernel/irq/chip.c, disable gets set to an empty default_disable if the
> irq_chip's version is NULL.
>
> The result is that if irq_chip->disable == NULL, suspend_device_irqs() is a 
> big NOP, albiet one that does lots of locking. :)
>
> So, should the irq_chip code be fixed to match the comment?  Something
> like the patch below?  If I fix the IRQ chip code, then I'm back to
> needing my patch since my irq_chip mask function still masks the IRQ
> at the hardware.

Please ignore my suggested patch.  I just saw Ingo's commit that
modified the default_disable().

Kevin

>
> Kevin
>
>
> commit f9b534f23ac7835eead99fb0a9cec7c505fe1e85
> Author: Kevin Hilman <khilman@deeprootsystems.com>
> Date:   Tue May 5 17:32:59 2009 -0700
>
>     IRQ: chip->disable should default to chip->mask if NULL
>     
>     The struct irq_chip comments in <linux/irq.h> state:
>     
>      * @disable:	disable the interrupt (defaults to chip->mask if NULL)
>     
>     However, the code in kernel/irq/chip.c does otherwise by setting
>     a NULL disable hook to an empty default_disable function.
>     
>     This patch makes the default_disable function call the ->mask hook
>     to match the comments.
>     
>     Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index c687ba4..0fb690a 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -238,6 +238,10 @@ static void default_enable(unsigned int irq)
>   */
>  static void default_disable(unsigned int irq)
>  {
> +	struct irq_desc *desc = irq_to_desc(irq);
> +
> +	desc->chip->mask(irq);
> +	desc->status |= IRQ_MASKED;
>  }
>  
>  /*

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kevin Hilman]

Kevin Hilman <khilman@deeprootsystems.com> writes:

> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>
>> On Wednesday 06 May 2009, Kevin Hilman wrote:

[...]

>>> 
>>> [...]
>>> 
>>> >>>
>>> >>> If this fixes some bug then please provide a description of that bug?
>>> >>
>>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
>>> >> are disabled by this code causing the system to no longer wake up.
>>> >
>>> > What do you do if the interrupt triggers right after your driver has
>>> > returned from its late suspend hook?  
>>> 
>>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
>>> 
>>> But I don't see how that can happen in the current code. IIUC, by the
>>> time your late suspend hook is run, your device IRQ is already
>>> disabled, so it won't trigger an interrupt that will be caught by
>>> check_wakeup_irqs() anyways.
>>
>> My understanding of __disable_irq() was that it didn't actually disable the
>> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
>> and acknowledge it, but preventing the device driver for receiving it.  
>
>> Does it work differently on the affected systems?
>
> Yes.
>
> __disable_irq() calls the irq_chip's disable method which is platform
> specific.  On OMAP, this masks the IRQ at the hardware level
> preventing the CPU from seeing the interrupt.

Looking at x86, the i8259 disable hook also seems to mask the IRQ at
the PIC level.

The various IO-APIC irq_chips do not have a disable hook so the
__disable_irq() here is a NOP.

Kevin

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Rafael J. Wysocki]

On Wednesday 06 May 2009, Kevin Hilman wrote:
> Kevin Hilman <khilman@deeprootsystems.com> writes:
> 
> > "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> >
> >> On Wednesday 06 May 2009, Kevin Hilman wrote:
> 
> [...]
> 
> >>> 
> >>> [...]
> >>> 
> >>> >>>
> >>> >>> If this fixes some bug then please provide a description of that bug?
> >>> >>
> >>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
> >>> >> are disabled by this code causing the system to no longer wake up.
> >>> >
> >>> > What do you do if the interrupt triggers right after your driver has
> >>> > returned from its late suspend hook?  
> >>> 
> >>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
> >>> 
> >>> But I don't see how that can happen in the current code. IIUC, by the
> >>> time your late suspend hook is run, your device IRQ is already
> >>> disabled, so it won't trigger an interrupt that will be caught by
> >>> check_wakeup_irqs() anyways.
> >>
> >> My understanding of __disable_irq() was that it didn't actually disable the
> >> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
> >> and acknowledge it, but preventing the device driver for receiving it.  
> >
> >> Does it work differently on the affected systems?
> >
> > Yes.
> >
> > __disable_irq() calls the irq_chip's disable method which is platform
> > specific.  On OMAP, this masks the IRQ at the hardware level
> > preventing the CPU from seeing the interrupt.
> 
> Looking at x86, the i8259 disable hook also seems to mask the IRQ at
> the PIC level.
> 
> The various IO-APIC irq_chips do not have a disable hook so the
> __disable_irq() here is a NOP.

Except that it sets IRQ_DISABLED.

All right there.

We can either avoid disabling wake-up interrupts, in which case we should
drop check_wakeup_irqs() IMO, or rework things so that check_wakeup_irqs() will
catch them.  Doing both doesn't seem to make sense to me.

Which one would be the right approach, then?

Rafael

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Wednesday 06 May 2009, Kevin Hilman wrote:
>> Kevin Hilman <khilman@deeprootsystems.com> writes:
>> 
>> > "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> >
>> >> On Wednesday 06 May 2009, Kevin Hilman wrote:
>> 
>> [...]
>> 
>> >>> 
>> >>> [...]
>> >>> 
>> >>> >>>
>> >>> >>> If this fixes some bug then please provide a description of that bug?
>> >>> >>
>> >>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
>> >>> >> are disabled by this code causing the system to no longer wake up.
>> >>> >
>> >>> > What do you do if the interrupt triggers right after your driver has
>> >>> > returned from its late suspend hook?  
>> >>> 
>> >>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
>> >>> 
>> >>> But I don't see how that can happen in the current code. IIUC, by the
>> >>> time your late suspend hook is run, your device IRQ is already
>> >>> disabled, so it won't trigger an interrupt that will be caught by
>> >>> check_wakeup_irqs() anyways.
>> >>
>> >> My understanding of __disable_irq() was that it didn't actually disable the
>> >> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
>> >> and acknowledge it, but preventing the device driver for receiving it.  
>> >
>> >> Does it work differently on the affected systems?
>> >
>> > Yes.
>> >
>> > __disable_irq() calls the irq_chip's disable method which is platform
>> > specific.  On OMAP, this masks the IRQ at the hardware level
>> > preventing the CPU from seeing the interrupt.
>> 
>> Looking at x86, the i8259 disable hook also seems to mask the IRQ at
>> the PIC level.
>> 
>> The various IO-APIC irq_chips do not have a disable hook so the
>> __disable_irq() here is a NOP.
>
> Except that it sets IRQ_DISABLED.
>
> All right there.

OK, right. I missed the setting of IRQ_DISABLED there.

> We can either avoid disabling wake-up interrupts, in which case we
> should drop check_wakeup_irqs() IMO, or rework things so that
> check_wakeup_irqs() will catch them.  Doing both doesn't seem to
> make sense to me.
>
> Which one would be the right approach, then?

Not sure if there is right one as they are solving two different
problems.

check_wakeup_irqs() seems meant to address handling interrupts that
happen during the suspend path.  My fix for not disabling wakups is
meant to allow those interrupts after suspend.

An alternative to my original approach is the one taken on x86
IO-APICs where the irq_chip's disable hook is empty, thus not masking
in HW but still preventing the ISR from running.

I need to explore whether just dropping the irq_chip's disable hook
would work for us on OMAP.

There is at least one problem with that which is why Kyuwon Kim added
the ->disable hook to OMAP's irq_chip.  The problem is with drivers
that call disable_irq() in their suspend hook, usually done to prevent
the device from waking the system since on OMAP, any IRQ can be
configured to wake the system.

If a driver's suspend hook calls disable_irq() and the system is
suspended before the lazy disable happens in the next handler, then
the system will be suspended with that device's IRQ still enabled.
Without an irq_chip->disable hook, that will result in that device IRQ
waking up the system if it fires.

I now have a patch for this which ensures that the lazy-disable
happens in the suspend path using the ->mask hook just like
it does in the handler.  Will send to LKML and here shortly.

Kevin

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Arve Hjønnevåg]

On Wed, May 6, 2009 at 5:16 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>
>> On Wednesday 06 May 2009, Kevin Hilman wrote:
>>> Kevin Hilman <khilman@deeprootsystems.com> writes:

> There is at least one problem with that which is why Kyuwon Kim added
> the ->disable hook to OMAP's irq_chip.  The problem is with drivers
> that call disable_irq() in their suspend hook, usually done to prevent
> the device from waking the system since on OMAP, any IRQ can be
> configured to wake the system.
>

This does not sound correct. disable_irq_wake should be used for this.
A driver may need to mask its interrupt before suspending but this
should not also disable it as a wakeup source.

> If a driver's suspend hook calls disable_irq() and the system is
> suspended before the lazy disable happens in the next handler, then
> the system will be suspended with that device's IRQ still enabled.
> Without an irq_chip->disable hook, that will result in that device IRQ
> waking up the system if it fires.

The platform suspend code needs to write the wakeup mask into
interrupt controller the before entering suspend.


-- 
Arve Hjønnevåg

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kim Kyuwon]

2009/5/7 Arve Hjønnevåg <arve@android.com>:
> On Wed, May 6, 2009 at 5:16 PM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
>> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>>
>>> On Wednesday 06 May 2009, Kevin Hilman wrote:
>>>> Kevin Hilman <khilman@deeprootsystems.com> writes:
>
>> There is at least one problem with that which is why Kyuwon Kim added
>> the ->disable hook to OMAP's irq_chip.  The problem is with drivers
>> that call disable_irq() in their suspend hook, usually done to prevent
>> the device from waking the system since on OMAP, any IRQ can be
>> configured to wake the system.
>>
>
> This does not sound correct. disable_irq_wake should be used for this.
> A driver may need to mask its interrupt before suspending but this
> should not also disable it as a wakeup source.

I wish I could use disable_irq_wake(), but it doesn't work in OMAP.

>> If a driver's suspend hook calls disable_irq() and the system is
>> suspended before the lazy disable happens in the next handler, then
>> the system will be suspended with that device's IRQ still enabled.
>> Without an irq_chip->disable hook, that will result in that device IRQ
>> waking up the system if it fires.
>
> The platform suspend code needs to write the wakeup mask into
> interrupt controller the before entering suspend.
>

ditto

> --
> Arve Hjønnevåg
> --
> 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/
>



-- 
Kyuwon (규원)
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Arve Hjønnevåg]

2009/5/6 Kim Kyuwon <chammoru@gmail.com>:
> 2009/5/7 Arve Hjønnevåg <arve@android.com>:
>> On Wed, May 6, 2009 at 5:16 PM, Kevin Hilman
>> <khilman@deeprootsystems.com> wrote:
>>> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>>>
>>>> On Wednesday 06 May 2009, Kevin Hilman wrote:
>>>>> Kevin Hilman <khilman@deeprootsystems.com> writes:
>>
>>> There is at least one problem with that which is why Kyuwon Kim added
>>> the ->disable hook to OMAP's irq_chip.  The problem is with drivers
>>> that call disable_irq() in their suspend hook, usually done to prevent
>>> the device from waking the system since on OMAP, any IRQ can be
>>> configured to wake the system.
>>>
>>
>> This does not sound correct. disable_irq_wake should be used for this.
>> A driver may need to mask its interrupt before suspending but this
>> should not also disable it as a wakeup source.
>
> I wish I could use disable_irq_wake(), but it doesn't work in OMAP.

This does not sound like a hardware problem.

-- 
Arve Hjønnevåg

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kim Kyuwon]

2009/5/7 Arve Hjønnevåg <arve@android.com>:
> 2009/5/6 Kim Kyuwon <chammoru@gmail.com>:
>> 2009/5/7 Arve Hjønnevåg <arve@android.com>:
>>> On Wed, May 6, 2009 at 5:16 PM, Kevin Hilman
>>> <khilman@deeprootsystems.com> wrote:
>>>> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>>>>
>>>>> On Wednesday 06 May 2009, Kevin Hilman wrote:
>>>>>> Kevin Hilman <khilman@deeprootsystems.com> writes:
>>>
>>>> There is at least one problem with that which is why Kyuwon Kim added
>>>> the ->disable hook to OMAP's irq_chip.  The problem is with drivers
>>>> that call disable_irq() in their suspend hook, usually done to prevent
>>>> the device from waking the system since on OMAP, any IRQ can be
>>>> configured to wake the system.
>>>>
>>>
>>> This does not sound correct. disable_irq_wake should be used for this.
>>> A driver may need to mask its interrupt before suspending but this
>>> should not also disable it as a wakeup source.
>>
>> I wish I could use disable_irq_wake(), but it doesn't work in OMAP.
>
> This does not sound like a hardware problem.

We may need advices of TI engineers.
However, as far as I know, It is impossible to disable 'interrupt
wake-up' with interrupt enabled. Because an interrupt itself generate
a system wake-up event in OMAP3430 (Hardware level).
-- 
Kyuwon

> --
> Arve Hjønnevåg

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kevin Hilman]

Kim Kyuwon <chammoru@gmail.com> writes:

> 2009/5/7 Arve Hjønnevåg <arve@android.com>:
>> 2009/5/6 Kim Kyuwon <chammoru@gmail.com>:
>>> 2009/5/7 Arve Hjønnevåg <arve@android.com>:
>>>> On Wed, May 6, 2009 at 5:16 PM, Kevin Hilman
>>>> <khilman@deeprootsystems.com> wrote:
>>>>> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>>>>>
>>>>>> On Wednesday 06 May 2009, Kevin Hilman wrote:
>>>>>>> Kevin Hilman <khilman@deeprootsystems.com> writes:
>>>>
>>>>> There is at least one problem with that which is why Kyuwon Kim added
>>>>> the ->disable hook to OMAP's irq_chip.  The problem is with drivers
>>>>> that call disable_irq() in their suspend hook, usually done to prevent
>>>>> the device from waking the system since on OMAP, any IRQ can be
>>>>> configured to wake the system.
>>>>>
>>>>
>>>> This does not sound correct. disable_irq_wake should be used for this.
>>>> A driver may need to mask its interrupt before suspending but this
>>>> should not also disable it as a wakeup source.
>>>
>>> I wish I could use disable_irq_wake(), but it doesn't work in OMAP.
>>
>> This does not sound like a hardware problem.
>
> We may need advices of TI engineers.
> However, as far as I know, It is impossible to disable 'interrupt
> wake-up' with interrupt enabled. Because an interrupt itself generate
> a system wake-up event in OMAP3430 (Hardware level).

Interrupt wakeups can be disabled at the PRCM level.  Or more simply
we can keep a mask of wakeup-enable interrupts and use that.

I will experiment with getting disable_irq_wake() working.

Kevin

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Rafael J. Wysocki]

On Thursday 07 May 2009, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > On Wednesday 06 May 2009, Kevin Hilman wrote:
> >> Kevin Hilman <khilman@deeprootsystems.com> writes:
> >> 
> >> > "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> >> >
> >> >> On Wednesday 06 May 2009, Kevin Hilman wrote:
> >> 
> >> [...]
> >> 
> >> >>> 
> >> >>> [...]
> >> >>> 
> >> >>> >>>
> >> >>> >>> If this fixes some bug then please provide a description of that bug?
> >> >>> >>
> >> >>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
> >> >>> >> are disabled by this code causing the system to no longer wake up.
> >> >>> >
> >> >>> > What do you do if the interrupt triggers right after your driver has
> >> >>> > returned from its late suspend hook?  
> >> >>> 
> >> >>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
> >> >>> 
> >> >>> But I don't see how that can happen in the current code. IIUC, by the
> >> >>> time your late suspend hook is run, your device IRQ is already
> >> >>> disabled, so it won't trigger an interrupt that will be caught by
> >> >>> check_wakeup_irqs() anyways.
> >> >>
> >> >> My understanding of __disable_irq() was that it didn't actually disable the
> >> >> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
> >> >> and acknowledge it, but preventing the device driver for receiving it.  
> >> >
> >> >> Does it work differently on the affected systems?
> >> >
> >> > Yes.
> >> >
> >> > __disable_irq() calls the irq_chip's disable method which is platform
> >> > specific.  On OMAP, this masks the IRQ at the hardware level
> >> > preventing the CPU from seeing the interrupt.
> >> 
> >> Looking at x86, the i8259 disable hook also seems to mask the IRQ at
> >> the PIC level.
> >> 
> >> The various IO-APIC irq_chips do not have a disable hook so the
> >> __disable_irq() here is a NOP.
> >
> > Except that it sets IRQ_DISABLED.
> >
> > All right there.
> 
> OK, right. I missed the setting of IRQ_DISABLED there.
> 
> > We can either avoid disabling wake-up interrupts, in which case we
> > should drop check_wakeup_irqs() IMO, or rework things so that
> > check_wakeup_irqs() will catch them.  Doing both doesn't seem to
> > make sense to me.
> >
> > Which one would be the right approach, then?
> 
> Not sure if there is right one as they are solving two different
> problems.
> 
> check_wakeup_irqs() seems meant to address handling interrupts that
> happen during the suspend path.  My fix for not disabling wakups is
> meant to allow those interrupts after suspend.

Still, with your fix in place, check_wakeup_irqs() is useless, because
the wake-up interrupts are not going to have IRQ_PENDING set.

> An alternative to my original approach is the one taken on x86
> IO-APICs where the irq_chip's disable hook is empty, thus not masking
> in HW but still preventing the ISR from running.

That sounds quite right.

> I need to explore whether just dropping the irq_chip's disable hook
> would work for us on OMAP.
> 
> There is at least one problem with that which is why Kyuwon Kim added
> the ->disable hook to OMAP's irq_chip.  The problem is with drivers
> that call disable_irq() in their suspend hook, usually done to prevent
> the device from waking the system since on OMAP, any IRQ can be
> configured to wake the system.
> 
> If a driver's suspend hook calls disable_irq() and the system is
> suspended before the lazy disable happens in the next handler, then
> the system will be suspended with that device's IRQ still enabled.
> Without an irq_chip->disable hook, that will result in that device IRQ
> waking up the system if it fires.
> 
> I now have a patch for this which ensures that the lazy-disable
> happens in the suspend path using the ->mask hook just like
> it does in the handler.  Will send to LKML and here shortly.

Great, thanks!

Best,
Rafael

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kim Kyuwon]

On Wed, May 6, 2009 at 8:27 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday 06 May 2009, Kevin Hilman wrote:
>> Arve Hjønnevåg <arve@android.com> writes:
>>
>> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
>> > <khilman@deeprootsystems.com> wrote:
>> >> Andrew Morton <akpm@linux-foundation.org> writes:
>> >>
>> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>> >>>
>> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>> >>>> should not be disabled for suspend.
>> >>>>
>> >>>
>> >>> Why not?
>> >>>
>> >>
>> >> If an interrupt is a wakeup source, and it is disabled at the chip
>> >> level, it will no longer generate interrupts, and thus no longer wake
>> >> up the system.
>> >>
>> >> I'd be interested in hearing why wakeup interrupts should be disabled
>> >> during suspend.
>
> That depends on whether or not they are used for anything else than wake-up.
>
>>
>> [...]
>>
>> >>>
>> >>> If this fixes some bug then please provide a description of that bug?
>> >>
>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
>> >> are disabled by this code causing the system to no longer wake up.
>> >
>> > What do you do if the interrupt triggers right after your driver has
>> > returned from its late suspend hook?
>>
>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
>>
>> But I don't see how that can happen in the current code. IIUC, by the
>> time your late suspend hook is run, your device IRQ is already
>> disabled, so it won't trigger an interrupt that will be caught by
>> check_wakeup_irqs() anyways.
>
> My understanding of __disable_irq() was that it didn't actually disable the
> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
> and acknowledge it, but preventing the device driver for receiving it.  Does
> it work differently on the affected systems?

Hi, Rafael.
Sorry for bring the old issue but please let me ask you about
suspend_device_irqs() function.

__disable_irq() disables the IRQ at the hardware level in the
following irq_chips

i8259A_chip
i8259_pic
i8259A_chip
bfin_internal_irqchip
crisv10_irq_type
crisv32_irq_type
h8300irq_chip
m_irq_chip
mn10300_cpu_pic_level
xtensa_irq_chip
iop13xx_msi_chip
msi_irq

Because these irq_chips mask interrupts in 'disable' hook.

Thus, your suspend_device_irqs() function disables all IRQs at the
hardware level on all architectures which use irq_chips listed above
in suspend state.
Is this really what you wanted?

If interrupt can wake up the system from suspend in some architectures
and if disable_irq_wake is not supported in these architectures, I
wonder if suspend_device_irqs() don't allow waking up by interrupt.

Regards,
Kyuwon

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kim Kyuwon]

On Fri, May 22, 2009 at 11:53 AM, Kim Kyuwon <chammoru@gmail.com> wrote:
> On Wed, May 6, 2009 at 8:27 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Wednesday 06 May 2009, Kevin Hilman wrote:
>>> Arve Hjønnevåg <arve@android.com> writes:
>>>
>>> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
>>> > <khilman@deeprootsystems.com> wrote:
>>> >> Andrew Morton <akpm@linux-foundation.org> writes:
>>> >>
>>> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>>> >>>
>>> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>>> >>>> should not be disabled for suspend.
>>> >>>>
>>> >>>
>>> >>> Why not?
>>> >>>
>>> >>
>>> >> If an interrupt is a wakeup source, and it is disabled at the chip
>>> >> level, it will no longer generate interrupts, and thus no longer wake
>>> >> up the system.
>>> >>
>>> >> I'd be interested in hearing why wakeup interrupts should be disabled
>>> >> during suspend.
>>
>> That depends on whether or not they are used for anything else than wake-up.
>>
>>>
>>> [...]
>>>
>>> >>>
>>> >>> If this fixes some bug then please provide a description of that bug?
>>> >>
>>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
>>> >> are disabled by this code causing the system to no longer wake up.
>>> >
>>> > What do you do if the interrupt triggers right after your driver has
>>> > returned from its late suspend hook?
>>>
>>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
>>>
>>> But I don't see how that can happen in the current code. IIUC, by the
>>> time your late suspend hook is run, your device IRQ is already
>>> disabled, so it won't trigger an interrupt that will be caught by
>>> check_wakeup_irqs() anyways.
>>
>> My understanding of __disable_irq() was that it didn't actually disable the
>> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
>> and acknowledge it, but preventing the device driver for receiving it.  Does
>> it work differently on the affected systems?
>
> Hi, Rafael.
> Sorry for bring the old issue but please let me ask you about
> suspend_device_irqs() function.
>
> __disable_irq() disables the IRQ at the hardware level in the
> following irq_chips
>
> i8259A_chip
> i8259_pic
> i8259A_chip
> bfin_internal_irqchip
> crisv10_irq_type
> crisv32_irq_type
> h8300irq_chip
> m_irq_chip
> mn10300_cpu_pic_level
> xtensa_irq_chip
> iop13xx_msi_chip
> msi_irq
>
> Because these irq_chips mask interrupts in 'disable' hook.
>
> Thus, your suspend_device_irqs() function disables all IRQs at the
> hardware level on all architectures which use irq_chips listed above
> in suspend state.
> Is this really what you wanted?
>
> If interrupt can wake up the system from suspend in some architectures
> and if disable_irq_wake is not supported in these architectures, I
> wonder if suspend_device_irqs() don't allow waking up by interrupt.
>
> Regards,
> Kyuwon
>

I saw resume_device_irqs() is invoked after arch_suspend_enable_irqs()
in your resume code.
So in this gap between resume_device_irqs() and
arch_suspend_enable_irqs(), a few interrupts would be discarded.
i.e, a few data would be lost.

If keypad wake up the system, first key pressed information would be lost.
If I2C, USB, SPI, UART wake up the system, first a few data would be lost.

Did you also consider this issue?

-- 
Kyuwon (규원)
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Rafael J. Wysocki]

On Friday 22 May 2009, Kim Kyuwon wrote:
> On Wed, May 6, 2009 at 8:27 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday 06 May 2009, Kevin Hilman wrote:
> >> Arve Hjønnevåg <arve@android.com> writes:
> >>
> >> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
> >> > <khilman@deeprootsystems.com> wrote:
> >> >> Andrew Morton <akpm@linux-foundation.org> writes:
> >> >>
> >> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
> >> >>>
> >> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
> >> >>>> should not be disabled for suspend.
> >> >>>>
> >> >>>
> >> >>> Why not?
> >> >>>
> >> >>
> >> >> If an interrupt is a wakeup source, and it is disabled at the chip
> >> >> level, it will no longer generate interrupts, and thus no longer wake
> >> >> up the system.
> >> >>
> >> >> I'd be interested in hearing why wakeup interrupts should be disabled
> >> >> during suspend.
> >
> > That depends on whether or not they are used for anything else than wake-up.
> >
> >>
> >> [...]
> >>
> >> >>>
> >> >>> If this fixes some bug then please provide a description of that bug?
> >> >>
> >> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
> >> >> are disabled by this code causing the system to no longer wake up.
> >> >
> >> > What do you do if the interrupt triggers right after your driver has
> >> > returned from its late suspend hook?
> >>
> >> If it's a wakeup IRQ, I assume you want it to prevent suspend.
> >>
> >> But I don't see how that can happen in the current code. IIUC, by the
> >> time your late suspend hook is run, your device IRQ is already
> >> disabled, so it won't trigger an interrupt that will be caught by
> >> check_wakeup_irqs() anyways.
> >
> > My understanding of __disable_irq() was that it didn't actually disable the
> > IRQ at the hardware level, allowing the CPU to actually receive the interrupt
> > and acknowledge it, but preventing the device driver for receiving it.  Does
> > it work differently on the affected systems?
> 
> Hi, Rafael.
> Sorry for bring the old issue but please let me ask you about
> suspend_device_irqs() function.
> 
> __disable_irq() disables the IRQ at the hardware level in the
> following irq_chips
> 
> i8259A_chip
> i8259_pic
> i8259A_chip
> bfin_internal_irqchip
> crisv10_irq_type
> crisv32_irq_type
> h8300irq_chip
> m_irq_chip
> mn10300_cpu_pic_level
> xtensa_irq_chip
> iop13xx_msi_chip
> msi_irq
> 
> Because these irq_chips mask interrupts in 'disable' hook.
> 
> Thus, your suspend_device_irqs() function disables all IRQs at the
> hardware level on all architectures which use irq_chips listed above
> in suspend state.
> Is this really what you wanted?

What we wanted was to resolve specific issue related to the handling of
interrupts during suspend and resume which caused observable breakage and
from the point of view of fixing this issue it doesn't really matter whether or
not interrupts are masked in the disable hook.

> If interrupt can wake up the system from suspend in some architectures
> and if disable_irq_wake is not supported in these architectures, I
> wonder if suspend_device_irqs() don't allow waking up by interrupt.

I think that the platforms that may be affected by this issue will have to take
care of it.

Thanks,
Rafael
 

> 
> Regards,
> Kyuwon
> 
> 


-- 
Everyone knows that debugging is twice as hard as writing a program
in the first place.  So if you're as clever as you can be when you write it,
how will you ever debug it? --- Brian Kernighan

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Rafael J. Wysocki]

On Friday 22 May 2009, Kim Kyuwon wrote:
> On Fri, May 22, 2009 at 11:53 AM, Kim Kyuwon <chammoru@gmail.com> wrote:
> > On Wed, May 6, 2009 at 8:27 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> On Wednesday 06 May 2009, Kevin Hilman wrote:
> >>> Arve Hjønnevåg <arve@android.com> writes:
> >>>
> >>> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
> >>> > <khilman@deeprootsystems.com> wrote:
> >>> >> Andrew Morton <akpm@linux-foundation.org> writes:
> >>> >>
> >>> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
> >>> >>>
> >>> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
> >>> >>>> should not be disabled for suspend.
> >>> >>>>
> >>> >>>
> >>> >>> Why not?
> >>> >>>
> >>> >>
> >>> >> If an interrupt is a wakeup source, and it is disabled at the chip
> >>> >> level, it will no longer generate interrupts, and thus no longer wake
> >>> >> up the system.
> >>> >>
> >>> >> I'd be interested in hearing why wakeup interrupts should be disabled
> >>> >> during suspend.
> >>
> >> That depends on whether or not they are used for anything else than wake-up.
> >>
> >>>
> >>> [...]
> >>>
> >>> >>>
> >>> >>> If this fixes some bug then please provide a description of that bug?
> >>> >>
> >>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
> >>> >> are disabled by this code causing the system to no longer wake up.
> >>> >
> >>> > What do you do if the interrupt triggers right after your driver has
> >>> > returned from its late suspend hook?
> >>>
> >>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
> >>>
> >>> But I don't see how that can happen in the current code. IIUC, by the
> >>> time your late suspend hook is run, your device IRQ is already
> >>> disabled, so it won't trigger an interrupt that will be caught by
> >>> check_wakeup_irqs() anyways.
> >>
> >> My understanding of __disable_irq() was that it didn't actually disable the
> >> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
> >> and acknowledge it, but preventing the device driver for receiving it.  Does
> >> it work differently on the affected systems?
> >
> > Hi, Rafael.
> > Sorry for bring the old issue but please let me ask you about
> > suspend_device_irqs() function.
> >
> > __disable_irq() disables the IRQ at the hardware level in the
> > following irq_chips
> >
> > i8259A_chip
> > i8259_pic
> > i8259A_chip
> > bfin_internal_irqchip
> > crisv10_irq_type
> > crisv32_irq_type
> > h8300irq_chip
> > m_irq_chip
> > mn10300_cpu_pic_level
> > xtensa_irq_chip
> > iop13xx_msi_chip
> > msi_irq
> >
> > Because these irq_chips mask interrupts in 'disable' hook.
> >
> > Thus, your suspend_device_irqs() function disables all IRQs at the
> > hardware level on all architectures which use irq_chips listed above
> > in suspend state.
> > Is this really what you wanted?
> >
> > If interrupt can wake up the system from suspend in some architectures
> > and if disable_irq_wake is not supported in these architectures, I
> > wonder if suspend_device_irqs() don't allow waking up by interrupt.
> >
> > Regards,
> > Kyuwon
> >
> 
> I saw resume_device_irqs() is invoked after arch_suspend_enable_irqs()
> in your resume code.
> So in this gap between resume_device_irqs() and
> arch_suspend_enable_irqs(), a few interrupts would be discarded.
> i.e, a few data would be lost.
> 
> If keypad wake up the system, first key pressed information would be lost.
> If I2C, USB, SPI, UART wake up the system, first a few data would be lost.
> 
> Did you also consider this issue?

I think it would happen anyway with the old code, wouldn't it?

Best,
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kim Kyuwon]

On Sat, May 23, 2009 at 6:25 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday 22 May 2009, Kim Kyuwon wrote:
>> On Fri, May 22, 2009 at 11:53 AM, Kim Kyuwon <chammoru@gmail.com> wrote:
>> > On Wed, May 6, 2009 at 8:27 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> On Wednesday 06 May 2009, Kevin Hilman wrote:
>> >>> Arve Hjønnevåg <arve@android.com> writes:
>> >>>
>> >>> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
>> >>> > <khilman@deeprootsystems.com> wrote:
>> >>> >> Andrew Morton <akpm@linux-foundation.org> writes:
>> >>> >>
>> >>> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>> >>> >>>
>> >>> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>> >>> >>>> should not be disabled for suspend.
>> >>> >>>>
>> >>> >>>
>> >>> >>> Why not?
>> >>> >>>
>> >>> >>
>> >>> >> If an interrupt is a wakeup source, and it is disabled at the chip
>> >>> >> level, it will no longer generate interrupts, and thus no longer wake
>> >>> >> up the system.
>> >>> >>
>> >>> >> I'd be interested in hearing why wakeup interrupts should be disabled
>> >>> >> during suspend.
>> >>
>> >> That depends on whether or not they are used for anything else than wake-up.
>> >>
>> >>>
>> >>> [...]
>> >>>
>> >>> >>>
>> >>> >>> If this fixes some bug then please provide a description of that bug?
>> >>> >>
>> >>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
>> >>> >> are disabled by this code causing the system to no longer wake up.
>> >>> >
>> >>> > What do you do if the interrupt triggers right after your driver has
>> >>> > returned from its late suspend hook?
>> >>>
>> >>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
>> >>>
>> >>> But I don't see how that can happen in the current code. IIUC, by the
>> >>> time your late suspend hook is run, your device IRQ is already
>> >>> disabled, so it won't trigger an interrupt that will be caught by
>> >>> check_wakeup_irqs() anyways.
>> >>
>> >> My understanding of __disable_irq() was that it didn't actually disable the
>> >> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
>> >> and acknowledge it, but preventing the device driver for receiving it.  Does
>> >> it work differently on the affected systems?
>> >
>> > Hi, Rafael.
>> > Sorry for bring the old issue but please let me ask you about
>> > suspend_device_irqs() function.
>> >
>> > __disable_irq() disables the IRQ at the hardware level in the
>> > following irq_chips
>> >
>> > i8259A_chip
>> > i8259_pic
>> > i8259A_chip
>> > bfin_internal_irqchip
>> > crisv10_irq_type
>> > crisv32_irq_type
>> > h8300irq_chip
>> > m_irq_chip
>> > mn10300_cpu_pic_level
>> > xtensa_irq_chip
>> > iop13xx_msi_chip
>> > msi_irq
>> >
>> > Because these irq_chips mask interrupts in 'disable' hook.
>> >
>> > Thus, your suspend_device_irqs() function disables all IRQs at the
>> > hardware level on all architectures which use irq_chips listed above
>> > in suspend state.
>> > Is this really what you wanted?
>> >
>> > If interrupt can wake up the system from suspend in some architectures
>> > and if disable_irq_wake is not supported in these architectures, I
>> > wonder if suspend_device_irqs() don't allow waking up by interrupt.
>> >
>> > Regards,
>> > Kyuwon
>> >
>>
>> I saw resume_device_irqs() is invoked after arch_suspend_enable_irqs()
>> in your resume code.
>> So in this gap between resume_device_irqs() and
>> arch_suspend_enable_irqs(), a few interrupts would be discarded.
>> i.e, a few data would be lost.
>>
>> If keypad wake up the system, first key pressed information would be lost.
>> If I2C, USB, SPI, UART wake up the system, first a few data would be lost.
>>
>> Did you also consider this issue?
>
> I think it would happen anyway with the old code, wouldn't it?

That's not quite right.

For example, let's assume a keypad device is alive in suspend/resume
state to wake up the system. Before arch_suspend_enable_irqs(), none
of keypad irqs is dropped. It is just pending.
But in your code, a few irqs are discarded due to your resume_device_irqs().

Regards,
Kyuwon

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Rafael J. Wysocki]

On Saturday 23 May 2009, Kim Kyuwon wrote:
> On Sat, May 23, 2009 at 6:25 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Friday 22 May 2009, Kim Kyuwon wrote:
> >> On Fri, May 22, 2009 at 11:53 AM, Kim Kyuwon <chammoru@gmail.com> wrote:
> >> > On Wed, May 6, 2009 at 8:27 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> >> On Wednesday 06 May 2009, Kevin Hilman wrote:
> >> >>> Arve Hjønnevåg <arve@android.com> writes:
> >> >>>
> >> >>> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
> >> >>> > <khilman@deeprootsystems.com> wrote:
> >> >>> >> Andrew Morton <akpm@linux-foundation.org> writes:
> >> >>> >>
> >> >>> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
> >> >>> >>>
> >> >>> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
> >> >>> >>>> should not be disabled for suspend.
> >> >>> >>>>
> >> >>> >>>
> >> >>> >>> Why not?
> >> >>> >>>
> >> >>> >>
> >> >>> >> If an interrupt is a wakeup source, and it is disabled at the chip
> >> >>> >> level, it will no longer generate interrupts, and thus no longer wake
> >> >>> >> up the system.
> >> >>> >>
> >> >>> >> I'd be interested in hearing why wakeup interrupts should be disabled
> >> >>> >> during suspend.
> >> >>
> >> >> That depends on whether or not they are used for anything else than wake-up.
> >> >>
> >> >>>
> >> >>> [...]
> >> >>>
> >> >>> >>>
> >> >>> >>> If this fixes some bug then please provide a description of that bug?
> >> >>> >>
> >> >>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
> >> >>> >> are disabled by this code causing the system to no longer wake up.
> >> >>> >
> >> >>> > What do you do if the interrupt triggers right after your driver has
> >> >>> > returned from its late suspend hook?
> >> >>>
> >> >>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
> >> >>>
> >> >>> But I don't see how that can happen in the current code. IIUC, by the
> >> >>> time your late suspend hook is run, your device IRQ is already
> >> >>> disabled, so it won't trigger an interrupt that will be caught by
> >> >>> check_wakeup_irqs() anyways.
> >> >>
> >> >> My understanding of __disable_irq() was that it didn't actually disable the
> >> >> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
> >> >> and acknowledge it, but preventing the device driver for receiving it.  Does
> >> >> it work differently on the affected systems?
> >> >
> >> > Hi, Rafael.
> >> > Sorry for bring the old issue but please let me ask you about
> >> > suspend_device_irqs() function.
> >> >
> >> > __disable_irq() disables the IRQ at the hardware level in the
> >> > following irq_chips
> >> >
> >> > i8259A_chip
> >> > i8259_pic
> >> > i8259A_chip
> >> > bfin_internal_irqchip
> >> > crisv10_irq_type
> >> > crisv32_irq_type
> >> > h8300irq_chip
> >> > m_irq_chip
> >> > mn10300_cpu_pic_level
> >> > xtensa_irq_chip
> >> > iop13xx_msi_chip
> >> > msi_irq
> >> >
> >> > Because these irq_chips mask interrupts in 'disable' hook.
> >> >
> >> > Thus, your suspend_device_irqs() function disables all IRQs at the
> >> > hardware level on all architectures which use irq_chips listed above
> >> > in suspend state.
> >> > Is this really what you wanted?
> >> >
> >> > If interrupt can wake up the system from suspend in some architectures
> >> > and if disable_irq_wake is not supported in these architectures, I
> >> > wonder if suspend_device_irqs() don't allow waking up by interrupt.
> >> >
> >> > Regards,
> >> > Kyuwon
> >> >
> >>
> >> I saw resume_device_irqs() is invoked after arch_suspend_enable_irqs()
> >> in your resume code.
> >> So in this gap between resume_device_irqs() and
> >> arch_suspend_enable_irqs(), a few interrupts would be discarded.
> >> i.e, a few data would be lost.
> >>
> >> If keypad wake up the system, first key pressed information would be lost.
> >> If I2C, USB, SPI, UART wake up the system, first a few data would be lost.
> >>
> >> Did you also consider this issue?
> >
> > I think it would happen anyway with the old code, wouldn't it?
> 
> That's not quite right.
> 
> For example, let's assume a keypad device is alive in suspend/resume
> state to wake up the system. Before arch_suspend_enable_irqs(), none
> of keypad irqs is dropped. It is just pending.
>
> But in your code, a few irqs are discarded due to your resume_device_irqs().

You can't say for sure they are discarded, but there's a small window in which
they can be discarded.  The question is whether it does cause problems in
practice.

Anyway, IMO the device that caused a wake-up event should be deactivated before
arch_suspend_enable_irqs() and remain inactive until its driver is actually
ready to handle interrupts generated by it.

Thanks,
Rafael

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kim Kyuwon]

On Sat, May 23, 2009 at 8:47 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> >>> > What do you do if the interrupt triggers right after your driver has
>> >> >>> > returned from its late suspend hook?
>> >> >>>
>> >> >>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
>> >> >>>
>> >> >>> But I don't see how that can happen in the current code. IIUC, by the
>> >> >>> time your late suspend hook is run, your device IRQ is already
>> >> >>> disabled, so it won't trigger an interrupt that will be caught by
>> >> >>> check_wakeup_irqs() anyways.
>> >> >>
>> >> >> My understanding of __disable_irq() was that it didn't actually disable the
>> >> >> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
>> >> >> and acknowledge it, but preventing the device driver for receiving it.  Does
>> >> >> it work differently on the affected systems?
>> >> >
>> >> > Hi, Rafael.
>> >> > Sorry for bring the old issue but please let me ask you about
>> >> > suspend_device_irqs() function.
>> >> >
>> >> > __disable_irq() disables the IRQ at the hardware level in the
>> >> > following irq_chips
>> >> >
>> >> > i8259A_chip
>> >> > i8259_pic
>> >> > i8259A_chip
>> >> > bfin_internal_irqchip
>> >> > crisv10_irq_type
>> >> > crisv32_irq_type
>> >> > h8300irq_chip
>> >> > m_irq_chip
>> >> > mn10300_cpu_pic_level
>> >> > xtensa_irq_chip
>> >> > iop13xx_msi_chip
>> >> > msi_irq
>> >> >
>> >> > Because these irq_chips mask interrupts in 'disable' hook.
>> >> >
>> >> > Thus, your suspend_device_irqs() function disables all IRQs at the
>> >> > hardware level on all architectures which use irq_chips listed above
>> >> > in suspend state.
>> >> > Is this really what you wanted?
>> >> >
>> >> > If interrupt can wake up the system from suspend in some architectures
>> >> > and if disable_irq_wake is not supported in these architectures, I
>> >> > wonder if suspend_device_irqs() don't allow waking up by interrupt.
>> >> >
>> >> > Regards,
>> >> > Kyuwon
>> >> >
>> >>
>> >> I saw resume_device_irqs() is invoked after arch_suspend_enable_irqs()
>> >> in your resume code.
>> >> So in this gap between resume_device_irqs() and
>> >> arch_suspend_enable_irqs(), a few interrupts would be discarded.
>> >> i.e, a few data would be lost.
>> >>
>> >> If keypad wake up the system, first key pressed information would be lost.
>> >> If I2C, USB, SPI, UART wake up the system, first a few data would be lost.
>> >>
>> >> Did you also consider this issue?
>> >
>> > I think it would happen anyway with the old code, wouldn't it?
>>
>> That's not quite right.
>>
>> For example, let's assume a keypad device is alive in suspend/resume
>> state to wake up the system. Before arch_suspend_enable_irqs(), none
>> of keypad irqs is dropped. It is just pending.
>>
>> But in your code, a few irqs are discarded due to your resume_device_irqs().
>
> You can't say for sure they are discarded, but there's a small window in which
> they can be discarded.  The question is whether it does cause problems in
> practice.

I tested it on my S3C6410(NCP) Board with MAX7359 keypad which is in
the Dmitry's Input repo.

> Anyway, IMO the device that caused a wake-up event should be deactivated before
> arch_suspend_enable_irqs() and remain inactive until its driver is actually
> ready to handle interrupts generated by it.

Some devices such as Bluetooth chip, keypad(including gpio-key),
Modem(Telephony, Wlan) chip and etc, should be activated while the
system suspend/resume state. Because these devices may communicate
other systems and should receive external wake-up events.

Thanks
Kyuwon

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kim Kyuwon]

On Sat, May 23, 2009 at 6:23 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday 22 May 2009, Kim Kyuwon wrote:
>> On Wed, May 6, 2009 at 8:27 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Wednesday 06 May 2009, Kevin Hilman wrote:
>> >> Arve Hjønnevåg <arve@android.com> writes:
>> >>
>> >> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
>> >> > <khilman@deeprootsystems.com> wrote:
>> >> >> Andrew Morton <akpm@linux-foundation.org> writes:
>> >> >>
>> >> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>> >> >>>
>> >> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>> >> >>>> should not be disabled for suspend.
>> >> >>>>
>> >> >>>
>> >> >>> Why not?
>> >> >>>
>> >> >>
>> >> >> If an interrupt is a wakeup source, and it is disabled at the chip
>> >> >> level, it will no longer generate interrupts, and thus no longer wake
>> >> >> up the system.
>> >> >>
>> >> >> I'd be interested in hearing why wakeup interrupts should be disabled
>> >> >> during suspend.
>> >
>> > That depends on whether or not they are used for anything else than wake-up.
>> >
>> >>
>> >> [...]
>> >>
>> >> >>>
>> >> >>> If this fixes some bug then please provide a description of that bug?
>> >> >>
>> >> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
>> >> >> are disabled by this code causing the system to no longer wake up.
>> >> >
>> >> > What do you do if the interrupt triggers right after your driver has
>> >> > returned from its late suspend hook?
>> >>
>> >> If it's a wakeup IRQ, I assume you want it to prevent suspend.
>> >>
>> >> But I don't see how that can happen in the current code. IIUC, by the
>> >> time your late suspend hook is run, your device IRQ is already
>> >> disabled, so it won't trigger an interrupt that will be caught by
>> >> check_wakeup_irqs() anyways.
>> >
>> > My understanding of __disable_irq() was that it didn't actually disable the
>> > IRQ at the hardware level, allowing the CPU to actually receive the interrupt
>> > and acknowledge it, but preventing the device driver for receiving it.  Does
>> > it work differently on the affected systems?
>>
>> Hi, Rafael.
>> Sorry for bring the old issue but please let me ask you about
>> suspend_device_irqs() function.
>>
>> __disable_irq() disables the IRQ at the hardware level in the
>> following irq_chips
>>
>> i8259A_chip
>> i8259_pic
>> i8259A_chip
>> bfin_internal_irqchip
>> crisv10_irq_type
>> crisv32_irq_type
>> h8300irq_chip
>> m_irq_chip
>> mn10300_cpu_pic_level
>> xtensa_irq_chip
>> iop13xx_msi_chip
>> msi_irq
>>
>> Because these irq_chips mask interrupts in 'disable' hook.
>>
>> Thus, your suspend_device_irqs() function disables all IRQs at the
>> hardware level on all architectures which use irq_chips listed above
>> in suspend state.
>> Is this really what you wanted?
>
> What we wanted was to resolve specific issue related to the handling of
> interrupts during suspend and resume which caused observable breakage and
> from the point of view of fixing this issue it doesn't really matter whether or
> not interrupts are masked in the disable hook.
>
>> If interrupt can wake up the system from suspend in some architectures
>> and if disable_irq_wake is not supported in these architectures, I
>> wonder if suspend_device_irqs() don't allow waking up by interrupt.
>
> I think that the platforms that may be affected by this issue will have to take
> care of it.

You changed the really important part of Linux, which may affect most
processor architectures. I think you should be careful. If some of
architectures can't take care of it (they can implement
disable_irq_wake correctly in H/W level, will you revert your changes?

Sincerely,
Kyuwon.

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Rafael J. Wysocki]

On Saturday 23 May 2009, Kim Kyuwon wrote:
> On Sat, May 23, 2009 at 6:23 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Friday 22 May 2009, Kim Kyuwon wrote:
> >> On Wed, May 6, 2009 at 8:27 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > On Wednesday 06 May 2009, Kevin Hilman wrote:
> >> >> Arve Hjønnevåg <arve@android.com> writes:
> >> >>
> >> >> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
> >> >> > <khilman@deeprootsystems.com> wrote:
> >> >> >> Andrew Morton <akpm@linux-foundation.org> writes:
> >> >> >>
> >> >> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
> >> >> >>>
> >> >> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
> >> >> >>>> should not be disabled for suspend.
> >> >> >>>>
> >> >> >>>
> >> >> >>> Why not?
> >> >> >>>
> >> >> >>
> >> >> >> If an interrupt is a wakeup source, and it is disabled at the chip
> >> >> >> level, it will no longer generate interrupts, and thus no longer wake
> >> >> >> up the system.
> >> >> >>
> >> >> >> I'd be interested in hearing why wakeup interrupts should be disabled
> >> >> >> during suspend.
> >> >
> >> > That depends on whether or not they are used for anything else than wake-up.
> >> >
> >> >>
> >> >> [...]
> >> >>
> >> >> >>>
> >> >> >>> If this fixes some bug then please provide a description of that bug?
> >> >> >>
> >> >> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
> >> >> >> are disabled by this code causing the system to no longer wake up.
> >> >> >
> >> >> > What do you do if the interrupt triggers right after your driver has
> >> >> > returned from its late suspend hook?
> >> >>
> >> >> If it's a wakeup IRQ, I assume you want it to prevent suspend.
> >> >>
> >> >> But I don't see how that can happen in the current code. IIUC, by the
> >> >> time your late suspend hook is run, your device IRQ is already
> >> >> disabled, so it won't trigger an interrupt that will be caught by
> >> >> check_wakeup_irqs() anyways.
> >> >
> >> > My understanding of __disable_irq() was that it didn't actually disable the
> >> > IRQ at the hardware level, allowing the CPU to actually receive the interrupt
> >> > and acknowledge it, but preventing the device driver for receiving it.  Does
> >> > it work differently on the affected systems?
> >>
> >> Hi, Rafael.
> >> Sorry for bring the old issue but please let me ask you about
> >> suspend_device_irqs() function.
> >>
> >> __disable_irq() disables the IRQ at the hardware level in the
> >> following irq_chips
> >>
> >> i8259A_chip
> >> i8259_pic
> >> i8259A_chip
> >> bfin_internal_irqchip
> >> crisv10_irq_type
> >> crisv32_irq_type
> >> h8300irq_chip
> >> m_irq_chip
> >> mn10300_cpu_pic_level
> >> xtensa_irq_chip
> >> iop13xx_msi_chip
> >> msi_irq
> >>
> >> Because these irq_chips mask interrupts in 'disable' hook.
> >>
> >> Thus, your suspend_device_irqs() function disables all IRQs at the
> >> hardware level on all architectures which use irq_chips listed above
> >> in suspend state.
> >> Is this really what you wanted?
> >
> > What we wanted was to resolve specific issue related to the handling of
> > interrupts during suspend and resume which caused observable breakage and
> > from the point of view of fixing this issue it doesn't really matter whether or
> > not interrupts are masked in the disable hook.
> >
> >> If interrupt can wake up the system from suspend in some architectures
> >> and if disable_irq_wake is not supported in these architectures, I
> >> wonder if suspend_device_irqs() don't allow waking up by interrupt.
> >
> > I think that the platforms that may be affected by this issue will have to take
> > care of it.
> 
> You changed the really important part of Linux, which may affect most
> processor architectures. I think you should be careful. If some of
> architectures can't take care of it (they can implement
> disable_irq_wake correctly in H/W level, will you revert your changes?

No, the changes are not going to be reverted.  In fact things should have been
done like this already much earlier.

Now, do you have any particular example of a problem related to these changes
or is it only a theoretical issue?

Rafael

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kim Kyuwon]

On Sat, May 23, 2009 at 7:29 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Saturday 23 May 2009, Kim Kyuwon wrote:
>> On Sat, May 23, 2009 at 6:23 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Friday 22 May 2009, Kim Kyuwon wrote:
>> >> On Wed, May 6, 2009 at 8:27 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> > On Wednesday 06 May 2009, Kevin Hilman wrote:
>> >> >> Arve Hjønnevåg <arve@android.com> writes:
>> >> >>
>> >> >> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman
>> >> >> > <khilman@deeprootsystems.com> wrote:
>> >> >> >> Andrew Morton <akpm@linux-foundation.org> writes:
>> >> >> >>
>> >> >> >>> On Mon,  4 May 2009 17:27:04 -0700 Kevin Hilman <khilman@deeprootsystems.com> wrote:
>> >> >> >>>
>> >> >> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake()
>> >> >> >>>> should not be disabled for suspend.
>> >> >> >>>>
>> >> >> >>>
>> >> >> >>> Why not?
>> >> >> >>>
>> >> >> >>
>> >> >> >> If an interrupt is a wakeup source, and it is disabled at the chip
>> >> >> >> level, it will no longer generate interrupts, and thus no longer wake
>> >> >> >> up the system.
>> >> >> >>
>> >> >> >> I'd be interested in hearing why wakeup interrupts should be disabled
>> >> >> >> during suspend.
>> >> >
>> >> > That depends on whether or not they are used for anything else than wake-up.
>> >> >
>> >> >>
>> >> >> [...]
>> >> >>
>> >> >> >>>
>> >> >> >>> If this fixes some bug then please provide a description of that bug?
>> >> >> >>
>> >> >> >> The bug is that on TI OMAP, interrupts that are used for wakeup events
>> >> >> >> are disabled by this code causing the system to no longer wake up.
>> >> >> >
>> >> >> > What do you do if the interrupt triggers right after your driver has
>> >> >> > returned from its late suspend hook?
>> >> >>
>> >> >> If it's a wakeup IRQ, I assume you want it to prevent suspend.
>> >> >>
>> >> >> But I don't see how that can happen in the current code. IIUC, by the
>> >> >> time your late suspend hook is run, your device IRQ is already
>> >> >> disabled, so it won't trigger an interrupt that will be caught by
>> >> >> check_wakeup_irqs() anyways.
>> >> >
>> >> > My understanding of __disable_irq() was that it didn't actually disable the
>> >> > IRQ at the hardware level, allowing the CPU to actually receive the interrupt
>> >> > and acknowledge it, but preventing the device driver for receiving it.  Does
>> >> > it work differently on the affected systems?
>> >>
>> >> Hi, Rafael.
>> >> Sorry for bring the old issue but please let me ask you about
>> >> suspend_device_irqs() function.
>> >>
>> >> __disable_irq() disables the IRQ at the hardware level in the
>> >> following irq_chips
>> >>
>> >> i8259A_chip
>> >> i8259_pic
>> >> i8259A_chip
>> >> bfin_internal_irqchip
>> >> crisv10_irq_type
>> >> crisv32_irq_type
>> >> h8300irq_chip
>> >> m_irq_chip
>> >> mn10300_cpu_pic_level
>> >> xtensa_irq_chip
>> >> iop13xx_msi_chip
>> >> msi_irq
>> >>
>> >> Because these irq_chips mask interrupts in 'disable' hook.
>> >>
>> >> Thus, your suspend_device_irqs() function disables all IRQs at the
>> >> hardware level on all architectures which use irq_chips listed above
>> >> in suspend state.
>> >> Is this really what you wanted?
>> >
>> > What we wanted was to resolve specific issue related to the handling of
>> > interrupts during suspend and resume which caused observable breakage and
>> > from the point of view of fixing this issue it doesn't really matter whether or
>> > not interrupts are masked in the disable hook.
>> >
>> >> If interrupt can wake up the system from suspend in some architectures
>> >> and if disable_irq_wake is not supported in these architectures, I
>> >> wonder if suspend_device_irqs() don't allow waking up by interrupt.
>> >
>> > I think that the platforms that may be affected by this issue will have to take
>> > care of it.
>>
>> You changed the really important part of Linux, which may affect most
>> processor architectures. I think you should be careful. If some of
>> architectures can't take care of it (they can implement
>> disable_irq_wake correctly in H/W level, will you revert your changes?
>
> No, the changes are not going to be reverted.  In fact things should have been
> done like this already much earlier.
>
> Now, do you have any particular example of a problem related to these changes
> or is it only a theoretical issue?

I'd CCing you when I'm sending a mail for this particular example of a example.
http://markmail.org/thread/fvt7d62arofon5xx

Regards,
Kyuwon

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Rafael J. Wysocki]

On Saturday 23 May 2009, Kim Kyuwon wrote:
> On Sat, May 23, 2009 at 7:29 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Saturday 23 May 2009, Kim Kyuwon wrote:
[--snip--]
> >> You changed the really important part of Linux, which may affect most
> >> processor architectures. I think you should be careful. If some of
> >> architectures can't take care of it (they can implement
> >> disable_irq_wake correctly in H/W level, will you revert your changes?
> >
> > No, the changes are not going to be reverted.  In fact things should have been
> > done like this already much earlier.
> >
> > Now, do you have any particular example of a problem related to these changes
> > or is it only a theoretical issue?
> 
> I'd CCing you when I'm sending a mail for this particular example of a example.
> http://markmail.org/thread/fvt7d62arofon5xx

Well, as I said above, reverting the changes that introduced
[suspend|resume]_device_irqs() is not an option, becuase it was the only sane
way to achieve the goal they were added for.  So, we need to fix the wake-up
problem on your platform with the assumption that
[suspend|resume]_device_irqs() are going to stay.

For starters, would it be possible to teach the 'disable' hook of your
platform's interrupt controller not to mask the IRQs that have both
IRQ_WAKEUP and IRQ_SUSPENDED set?  That apparently would work around the
wake-up interrupts problem.

Best,
Rafael

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kim Kyuwon]

Rafael J. Wysocki wrote:
> On Saturday 23 May 2009, Kim Kyuwon wrote:
>> On Sat, May 23, 2009 at 7:29 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> On Saturday 23 May 2009, Kim Kyuwon wrote:
> [--snip--]
>>>> You changed the really important part of Linux, which may affect most
>>>> processor architectures. I think you should be careful. If some of
>>>> architectures can't take care of it (they can implement
>>>> disable_irq_wake correctly in H/W level, will you revert your changes?
>>> No, the changes are not going to be reverted.  In fact things should have been
>>> done like this already much earlier.
>>>
>>> Now, do you have any particular example of a problem related to these changes
>>> or is it only a theoretical issue?
>> I'd CCing you when I'm sending a mail for this particular example of a example.
>> http://markmail.org/thread/fvt7d62arofon5xx
> 
> Well, as I said above, reverting the changes that introduced
> [suspend|resume]_device_irqs() is not an option, becuase it was the only sane
> way to achieve the goal they were added for.  So, we need to fix the wake-up
> problem on your platform with the assumption that
> [suspend|resume]_device_irqs() are going to stay.
> 
> For starters, would it be possible to teach the 'disable' hook of your
> platform's interrupt controller not to mask the IRQs that have both
> IRQ_WAKEUP and IRQ_SUSPENDED set?  That apparently would work around the
> wake-up interrupts problem.

Thank you for considering this issue and spending your time. In order to 
make your idea work, we need to add a dummy 'set_wake' hook which 
returns always zero. Anyway, IMO, I think your idea is good to work 
around this problem. But Kevin Hilman(OMAP PM Maintainer) would make 
final decision.

Buy the way, how can you handle the problem that a few interrupt are 
discarded in a small window? I can be sure they are discarded, because I 
have debugged defects which generate in sleep/resume state hundreds of 
times on ARM Processors(PXA310, S3C6410, OMAP3430). Wake-up interrupts 
are generated as soon as arch_suspend_enable_irqs() invoked.

Regards,
Kyuwon

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Rafael J. Wysocki]

On Monday 25 May 2009, Kim Kyuwon wrote:
> Rafael J. Wysocki wrote:
> > On Saturday 23 May 2009, Kim Kyuwon wrote:
> >> On Sat, May 23, 2009 at 7:29 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >>> On Saturday 23 May 2009, Kim Kyuwon wrote:
> > [--snip--]
> >>>> You changed the really important part of Linux, which may affect most
> >>>> processor architectures. I think you should be careful. If some of
> >>>> architectures can't take care of it (they can implement
> >>>> disable_irq_wake correctly in H/W level, will you revert your changes?
> >>> No, the changes are not going to be reverted.  In fact things should have been
> >>> done like this already much earlier.
> >>>
> >>> Now, do you have any particular example of a problem related to these changes
> >>> or is it only a theoretical issue?
> >> I'd CCing you when I'm sending a mail for this particular example of a example.
> >> http://markmail.org/thread/fvt7d62arofon5xx
> > 
> > Well, as I said above, reverting the changes that introduced
> > [suspend|resume]_device_irqs() is not an option, becuase it was the only sane
> > way to achieve the goal they were added for.  So, we need to fix the wake-up
> > problem on your platform with the assumption that
> > [suspend|resume]_device_irqs() are going to stay.
> > 
> > For starters, would it be possible to teach the 'disable' hook of your
> > platform's interrupt controller not to mask the IRQs that have both
> > IRQ_WAKEUP and IRQ_SUSPENDED set?  That apparently would work around the
> > wake-up interrupts problem.
> 
> Thank you for considering this issue and spending your time. In order to 
> make your idea work, we need to add a dummy 'set_wake' hook which 
> returns always zero. Anyway, IMO, I think your idea is good to work 
> around this problem. But Kevin Hilman(OMAP PM Maintainer) would make 
> final decision.
> 
> Buy the way, how can you handle the problem that a few interrupt are 
> discarded in a small window? I can be sure they are discarded, because I 
> have debugged defects which generate in sleep/resume state hundreds of 
> times on ARM Processors(PXA310, S3C6410, OMAP3430). Wake-up interrupts 
> are generated as soon as arch_suspend_enable_irqs() invoked.

Sorry for the delayed response.

If the wake-up interrupts are not masked, they will be delivered to the drivers
as soon as arch_suspend_enable_irqs() has run.  So, if the drivers are able to
handle them at this point (ie. before resume_device_irqs() is called), they
won't be lost.  The only problem I see is that the drivers may expect their
->resume_noirq() callbacks to be executed first.

Thanks,
Rafael

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kim Kyuwon]

On Sat, May 30, 2009 at 8:35 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday 25 May 2009, Kim Kyuwon wrote:
>> Rafael J. Wysocki wrote:
>> > On Saturday 23 May 2009, Kim Kyuwon wrote:
>> >> On Sat, May 23, 2009 at 7:29 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >>> On Saturday 23 May 2009, Kim Kyuwon wrote:
>> > [--snip--]
>> >>>> You changed the really important part of Linux, which may affect most
>> >>>> processor architectures. I think you should be careful. If some of
>> >>>> architectures can't take care of it (they can implement
>> >>>> disable_irq_wake correctly in H/W level, will you revert your changes?
>> >>> No, the changes are not going to be reverted.  In fact things should have been
>> >>> done like this already much earlier.
>> >>>
>> >>> Now, do you have any particular example of a problem related to these changes
>> >>> or is it only a theoretical issue?
>> >> I'd CCing you when I'm sending a mail for this particular example of a example.
>> >> http://markmail.org/thread/fvt7d62arofon5xx
>> >
>> > Well, as I said above, reverting the changes that introduced
>> > [suspend|resume]_device_irqs() is not an option, becuase it was the only sane
>> > way to achieve the goal they were added for.  So, we need to fix the wake-up
>> > problem on your platform with the assumption that
>> > [suspend|resume]_device_irqs() are going to stay.
>> >
>> > For starters, would it be possible to teach the 'disable' hook of your
>> > platform's interrupt controller not to mask the IRQs that have both
>> > IRQ_WAKEUP and IRQ_SUSPENDED set?  That apparently would work around the
>> > wake-up interrupts problem.
>>
>> Thank you for considering this issue and spending your time. In order to
>> make your idea work, we need to add a dummy 'set_wake' hook which
>> returns always zero. Anyway, IMO, I think your idea is good to work
>> around this problem. But Kevin Hilman(OMAP PM Maintainer) would make
>> final decision.
>>
>> Buy the way, how can you handle the problem that a few interrupt are
>> discarded in a small window? I can be sure they are discarded, because I
>> have debugged defects which generate in sleep/resume state hundreds of
>> times on ARM Processors(PXA310, S3C6410, OMAP3430). Wake-up interrupts
>> are generated as soon as arch_suspend_enable_irqs() invoked.
>
> Sorry for the delayed response.
>
> If the wake-up interrupts are not masked, they will be delivered to the drivers
> as soon as arch_suspend_enable_irqs() has run.  So, if the drivers are able to
> handle them at this point (ie. before resume_device_irqs() is called), they
> won't be lost.

Thank you for your response!

Your suspend_device_irqs() disables all IRQs(except timer IRQ) while
entering suspend. i.e. Before invoking resume_device_irqs() or
resume_noirq callback, all IRQs(except timer IRQ) is in IRQ_DISABLED
status. Right?
But if an IRQ is in  IRQ_DISABLED status, its interrupt handler can be
invoked. (As you know, all IRQs with IRQ_DISABLE are not handled in
handle_level_irq function). Thus, even if the wake-up interrupts are
not masked, the drivers are not able to handle interrupts, because the
interrupt handler can't be invoked due to IRQ_DISABLED set by
suspend_device_irqs().

> The only problem I see is that the drivers may expect their
> ->resume_noirq() callbacks to be executed first.

resume_noirq() callbacks are also invoked after arch_suspend_enable_irqs().

Regards,
Kyuwon

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kim Kyuwon]

On Sat, May 30, 2009 at 4:34 PM, Kim Kyuwon <chammoru@gmail.com> wrote:
> On Sat, May 30, 2009 at 8:35 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Monday 25 May 2009, Kim Kyuwon wrote:
>>> Rafael J. Wysocki wrote:
>>> > On Saturday 23 May 2009, Kim Kyuwon wrote:
>>> >> On Sat, May 23, 2009 at 7:29 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> >>> On Saturday 23 May 2009, Kim Kyuwon wrote:
>>> > [--snip--]
>>> >>>> You changed the really important part of Linux, which may affect most
>>> >>>> processor architectures. I think you should be careful. If some of
>>> >>>> architectures can't take care of it (they can implement
>>> >>>> disable_irq_wake correctly in H/W level, will you revert your changes?
>>> >>> No, the changes are not going to be reverted.  In fact things should have been
>>> >>> done like this already much earlier.
>>> >>>
>>> >>> Now, do you have any particular example of a problem related to these changes
>>> >>> or is it only a theoretical issue?
>>> >> I'd CCing you when I'm sending a mail for this particular example of a example.
>>> >> http://markmail.org/thread/fvt7d62arofon5xx
>>> >
>>> > Well, as I said above, reverting the changes that introduced
>>> > [suspend|resume]_device_irqs() is not an option, becuase it was the only sane
>>> > way to achieve the goal they were added for.  So, we need to fix the wake-up
>>> > problem on your platform with the assumption that
>>> > [suspend|resume]_device_irqs() are going to stay.
>>> >
>>> > For starters, would it be possible to teach the 'disable' hook of your
>>> > platform's interrupt controller not to mask the IRQs that have both
>>> > IRQ_WAKEUP and IRQ_SUSPENDED set?  That apparently would work around the
>>> > wake-up interrupts problem.
>>>
>>> Thank you for considering this issue and spending your time. In order to
>>> make your idea work, we need to add a dummy 'set_wake' hook which
>>> returns always zero. Anyway, IMO, I think your idea is good to work
>>> around this problem. But Kevin Hilman(OMAP PM Maintainer) would make
>>> final decision.
>>>
>>> Buy the way, how can you handle the problem that a few interrupt are
>>> discarded in a small window? I can be sure they are discarded, because I
>>> have debugged defects which generate in sleep/resume state hundreds of
>>> times on ARM Processors(PXA310, S3C6410, OMAP3430). Wake-up interrupts
>>> are generated as soon as arch_suspend_enable_irqs() invoked.
>>
>> Sorry for the delayed response.
>>
>> If the wake-up interrupts are not masked, they will be delivered to the drivers
>> as soon as arch_suspend_enable_irqs() has run.  So, if the drivers are able to
>> handle them at this point (ie. before resume_device_irqs() is called), they
>> won't be lost.
>
> Thank you for your response!
>
> Your suspend_device_irqs() disables all IRQs(except timer IRQ) while
> entering suspend. i.e. Before invoking resume_device_irqs() or
> resume_noirq callback, all IRQs(except timer IRQ) is in IRQ_DISABLED
> status. Right?
> But if an IRQ is in  IRQ_DISABLED status, its interrupt handler can be

Sorry: *Can't be*

> invoked. (As you know, all IRQs with IRQ_DISABLE are not handled in
> handle_level_irq function). Thus, even if the wake-up interrupts are
> not masked, the drivers are not able to handle interrupts, because the
> interrupt handler can't be invoked due to IRQ_DISABLED set by
> suspend_device_irqs().
>
>> The only problem I see is that the drivers may expect their
>> ->resume_noirq() callbacks to be executed first.
>
> resume_noirq() callbacks are also invoked after arch_suspend_enable_irqs().
>
> Regards,
> Kyuwon
>



-- 
Kyuwon (규원)
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Rafael J. Wysocki]

On Saturday 30 May 2009, Kim Kyuwon wrote:
> On Sat, May 30, 2009 at 8:35 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Monday 25 May 2009, Kim Kyuwon wrote:
> >> Rafael J. Wysocki wrote:
> >> > On Saturday 23 May 2009, Kim Kyuwon wrote:
> >> >> On Sat, May 23, 2009 at 7:29 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> >>> On Saturday 23 May 2009, Kim Kyuwon wrote:
> >> > [--snip--]
> >> >>>> You changed the really important part of Linux, which may affect most
> >> >>>> processor architectures. I think you should be careful. If some of
> >> >>>> architectures can't take care of it (they can implement
> >> >>>> disable_irq_wake correctly in H/W level, will you revert your changes?
> >> >>> No, the changes are not going to be reverted.  In fact things should have been
> >> >>> done like this already much earlier.
> >> >>>
> >> >>> Now, do you have any particular example of a problem related to these changes
> >> >>> or is it only a theoretical issue?
> >> >> I'd CCing you when I'm sending a mail for this particular example of a example.
> >> >> http://markmail.org/thread/fvt7d62arofon5xx
> >> >
> >> > Well, as I said above, reverting the changes that introduced
> >> > [suspend|resume]_device_irqs() is not an option, becuase it was the only sane
> >> > way to achieve the goal they were added for.  So, we need to fix the wake-up
> >> > problem on your platform with the assumption that
> >> > [suspend|resume]_device_irqs() are going to stay.
> >> >
> >> > For starters, would it be possible to teach the 'disable' hook of your
> >> > platform's interrupt controller not to mask the IRQs that have both
> >> > IRQ_WAKEUP and IRQ_SUSPENDED set?  That apparently would work around the
> >> > wake-up interrupts problem.
> >>
> >> Thank you for considering this issue and spending your time. In order to
> >> make your idea work, we need to add a dummy 'set_wake' hook which
> >> returns always zero. Anyway, IMO, I think your idea is good to work
> >> around this problem. But Kevin Hilman(OMAP PM Maintainer) would make
> >> final decision.
> >>
> >> Buy the way, how can you handle the problem that a few interrupt are
> >> discarded in a small window? I can be sure they are discarded, because I
> >> have debugged defects which generate in sleep/resume state hundreds of
> >> times on ARM Processors(PXA310, S3C6410, OMAP3430). Wake-up interrupts
> >> are generated as soon as arch_suspend_enable_irqs() invoked.
> >
> > Sorry for the delayed response.
> >
> > If the wake-up interrupts are not masked, they will be delivered to the drivers
> > as soon as arch_suspend_enable_irqs() has run.  So, if the drivers are able to
> > handle them at this point (ie. before resume_device_irqs() is called), they
> > won't be lost.
> 
> Thank you for your response!
> 
> Your suspend_device_irqs() disables all IRQs(except timer IRQ) while
> entering suspend. i.e. Before invoking resume_device_irqs() or
> resume_noirq callback, all IRQs(except timer IRQ) is in IRQ_DISABLED
> status. Right?
> But if an IRQ is in  IRQ_DISABLED status, its interrupt handler can be

s/can be/can't be/ (as you noticed in the follow-up message).

> invoked. (As you know, all IRQs with IRQ_DISABLE are not handled in
> handle_level_irq function). Thus, even if the wake-up interrupts are
> not masked, the drivers are not able to handle interrupts, because the
> interrupt handler can't be invoked due to IRQ_DISABLED set by
> suspend_device_irqs().

The solution to that may be to add some code that will clear the
IRQ_DISABLE flag for the wake-up interrupt that caused the wake-up to happen
in the early resume code of the platform.

> > The only problem I see is that the drivers may expect their
> > ->resume_noirq() callbacks to be executed first.
> 
> resume_noirq() callbacks are also invoked after arch_suspend_enable_irqs().

Yes, they are, and that may be a problem, because a driver may expect
resume_noirq() to be called before it can handle interrupts from the device.

Best,
Rafael