commit 0a0c5168df (PM: Introduce functions for suspending and resuming
device interrupts) iterates through all interrupts and disables them
on the hardware level. Some architectures have functionality
implemented to mark an interrupt source as wakeup source for suspend,
but the new power management code disables them unconditionally which
breaks the resume on interrupt functionality.
The wakeup interrupts are marked in the status with the IRQ_WAKEUP
bit. Skip the disablement for those interrupts which have the
IRQ_WAKEUP bit set.
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: [email protected]
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 638d8be..bce6afd 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -29,7 +29,8 @@ void suspend_device_irqs(void)
unsigned long flags;
spin_lock_irqsave(&desc->lock, flags);
- __disable_irq(desc, irq, true);
+ if (!(desc->status & IRQ_WAKEUP))
+ __disable_irq(desc, irq, true);
spin_unlock_irqrestore(&desc->lock, flags);
}
Thomas Gleixner <[email protected]> writes:
> commit 0a0c5168df (PM: Introduce functions for suspending and resuming
> device interrupts) iterates through all interrupts and disables them
> on the hardware level. Some architectures have functionality
> implemented to mark an interrupt source as wakeup source for suspend,
> but the new power management code disables them unconditionally which
> breaks the resume on interrupt functionality.
>
> The wakeup interrupts are marked in the status with the IRQ_WAKEUP
> bit. Skip the disablement for those interrupts which have the
> IRQ_WAKEUP bit set.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Cc: [email protected]
Hi Thomas,
I posted the same patch last month and lost the argument, original
thread here:
http://lkml.org/lkml/2009/5/6/549
Kevin
> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> index 638d8be..bce6afd 100644
> --- a/kernel/irq/pm.c
> +++ b/kernel/irq/pm.c
> @@ -29,7 +29,8 @@ void suspend_device_irqs(void)
> unsigned long flags;
>
> spin_lock_irqsave(&desc->lock, flags);
> - __disable_irq(desc, irq, true);
> + if (!(desc->status & IRQ_WAKEUP))
> + __disable_irq(desc, irq, true);
> spin_unlock_irqrestore(&desc->lock, flags);
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Fri, 12 Jun 2009, Kevin Hilman wrote:
> Thomas Gleixner <[email protected]> writes:
>
> > commit 0a0c5168df (PM: Introduce functions for suspending and resuming
> > device interrupts) iterates through all interrupts and disables them
> > on the hardware level. Some architectures have functionality
> > implemented to mark an interrupt source as wakeup source for suspend,
> > but the new power management code disables them unconditionally which
> > breaks the resume on interrupt functionality.
> >
> > The wakeup interrupts are marked in the status with the IRQ_WAKEUP
> > bit. Skip the disablement for those interrupts which have the
> > IRQ_WAKEUP bit set.
> >
> > Signed-off-by: Thomas Gleixner <[email protected]>
> > Cc: [email protected]
>
> Hi Thomas,
>
> I posted the same patch last month and lost the argument, original
> thread here:
>
> http://lkml.org/lkml/2009/5/6/549
Err no. Care to look at the difference ?
I missed the above discussion, but I'm revisiting the delayed disable
issue.
>
> > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > index 638d8be..bce6afd 100644
> > --- a/kernel/irq/pm.c
> > +++ b/kernel/irq/pm.c
> > @@ -29,7 +29,8 @@ void suspend_device_irqs(void)
> > unsigned long flags;
> >
> > spin_lock_irqsave(&desc->lock, flags);
> > - __disable_irq(desc, irq, true);
> > + if (!(desc->status & IRQ_WAKEUP))
> > + __disable_irq(desc, irq, true);
> > spin_unlock_irqrestore(&desc->lock, flags);
> > }
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>
Thomas Gleixner <[email protected]> writes:
> On Fri, 12 Jun 2009, Kevin Hilman wrote:
>> Thomas Gleixner <[email protected]> writes:
>>
>> > commit 0a0c5168df (PM: Introduce functions for suspending and resuming
>> > device interrupts) iterates through all interrupts and disables them
>> > on the hardware level. Some architectures have functionality
>> > implemented to mark an interrupt source as wakeup source for suspend,
>> > but the new power management code disables them unconditionally which
>> > breaks the resume on interrupt functionality.
>> >
>> > The wakeup interrupts are marked in the status with the IRQ_WAKEUP
>> > bit. Skip the disablement for those interrupts which have the
>> > IRQ_WAKEUP bit set.
>> >
>> > Signed-off-by: Thomas Gleixner <[email protected]>
>> > Cc: [email protected]
>>
>> Hi Thomas,
>>
>> I posted the same patch last month and lost the argument, original
>> thread here:
>>
>> http://lkml.org/lkml/2009/5/6/549
>
> Err no. Care to look at the difference ?
Oops, sent link to wrong patch. Here's the one solving the same problem:
http://marc.info/?l=linux-kernel&m=124148347804447&w=2
or
http://lkml.org/lkml/2009/5/4/448
Only difference is I did the checking outside of the lock, which is
probably wrong. In any case, you'll be interested in the thread that
follows.
Kevin
> I missed the above discussion, but I'm revisiting the delayed disable
> issue.
>
>>
>> > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
>> > index 638d8be..bce6afd 100644
>> > --- a/kernel/irq/pm.c
>> > +++ b/kernel/irq/pm.c
>> > @@ -29,7 +29,8 @@ void suspend_device_irqs(void)
>> > unsigned long flags;
>> >
>> > spin_lock_irqsave(&desc->lock, flags);
>> > - __disable_irq(desc, irq, true);
>> > + if (!(desc->status & IRQ_WAKEUP))
>> > + __disable_irq(desc, irq, true);
>> > spin_unlock_irqrestore(&desc->lock, flags);
>> > }
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to [email protected]
>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at http://www.tux.org/lkml/
>>
On Fri, 12 Jun 2009, Kevin Hilman wrote:
> http://lkml.org/lkml/2009/5/4/448
>
> Only difference is I did the checking outside of the lock, which is
> probably wrong. In any case, you'll be interested in the thread that
> follows.
Hmm, darn. That means that on hardware which has trouble with the
delayed disable and therefor uses it's own chip->disable_irq() method
the suspend logic is wreckaged.
But there is always a way to get broken hardware tamed. :)
suspend does:
__disable_irq();
status |= IRQ_SUSPENDED;
chip->disable_irq();
resume does:
__enable_irq();
status &= ~IRQ_SUSPENDED;
chip->enable_irq();
So
- set_irq_handler(handle_level_irq);
+ set_irq_handler(my_own_handler);
+my_own_handler()
+{
+ if (!(status & IRQ_SUSPENDED)) {
+ handle_level_irq();
+ } else {
+ mask_at_hardware_level();
+ status |= IRQ_PENDING;
+ save_important_information();
+ }
+}
my_disable_irq()
{
+ if (!(status & IRQ_SUSPENDED))
mask_at_hardware_level();
}
my_enable_irq()
{
+ if (important_information_has_been_saved)
+ replay_what_happened();
+
unmask_at_hardware_level();
}
Ugly, but that might work somehow. Not sure about the replay part, but
that can be deferred via some more hackery as well :)
Raphael, these delayed disable and the chip->irq_disable() override
implications vs. suspend really need to be documented. The current
comment of suspend_device_irqs() is bogus:
* During system-wide suspend or hibernation device interrupts need to be
* disabled at the chip level and this function is provided for this purpose.
^^^^^^^^^^^^^^^^^^^^^^^^^^
Thanks,
tglx
On Fri, 12 Jun 2009 21:52:46 +0200 (CEST)
Thomas Gleixner <[email protected]> wrote:
> On Fri, 12 Jun 2009, Kevin Hilman wrote:
> > http://lkml.org/lkml/2009/5/4/448
> >
> > Only difference is I did the checking outside of the lock, which is
> > probably wrong. In any case, you'll be interested in the thread that
> > follows.
>
> Hmm, darn. That means that on hardware which has trouble with the
> delayed disable and therefor uses it's own chip->disable_irq() method
> the suspend logic is wreckaged.
Does this maen that your original patch is no longer applicable to
mainline/-stable?
> But there is always a way to get broken hardware tamed. :)
>
> suspend does:
> __disable_irq();
> status |= IRQ_SUSPENDED;
> chip->disable_irq();
>
> resume does:
> __enable_irq();
> status &= ~IRQ_SUSPENDED;
> chip->enable_irq();
>
> So
>
> - set_irq_handler(handle_level_irq);
> + set_irq_handler(my_own_handler);
>
> +my_own_handler()
> +{
> + if (!(status & IRQ_SUSPENDED)) {
> + handle_level_irq();
> + } else {
> + mask_at_hardware_level();
> + status |= IRQ_PENDING;
> + save_important_information();
> + }
> +}
>
> my_disable_irq()
> {
> + if (!(status & IRQ_SUSPENDED))
> mask_at_hardware_level();
> }
>
> my_enable_irq()
> {
> + if (important_information_has_been_saved)
> + replay_what_happened();
> +
> unmask_at_hardware_level();
> }
>
> Ugly, but that might work somehow. Not sure about the replay part, but
> that can be deferred via some more hackery as well :)
>
> Raphael, these delayed disable and the chip->irq_disable() override
> implications vs. suspend really need to be documented. The current
> comment of suspend_device_irqs() is bogus:
>
> * During system-wide suspend or hibernation device interrupts need to be
> * disabled at the chip level and this function is provided for this purpose.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
On Friday 12 June 2009, Thomas Gleixner wrote:
> On Fri, 12 Jun 2009, Kevin Hilman wrote:
> > http://lkml.org/lkml/2009/5/4/448
> >
> > Only difference is I did the checking outside of the lock, which is
> > probably wrong. In any case, you'll be interested in the thread that
> > follows.
>
> Hmm, darn. That means that on hardware which has trouble with the
> delayed disable and therefor uses it's own chip->disable_irq() method
> the suspend logic is wreckaged.
>
> But there is always a way to get broken hardware tamed. :)
>
> suspend does:
> __disable_irq();
> status |= IRQ_SUSPENDED;
> chip->disable_irq();
>
> resume does:
> __enable_irq();
> status &= ~IRQ_SUSPENDED;
> chip->enable_irq();
>
> So
>
> - set_irq_handler(handle_level_irq);
> + set_irq_handler(my_own_handler);
>
> +my_own_handler()
> +{
> + if (!(status & IRQ_SUSPENDED)) {
> + handle_level_irq();
> + } else {
> + mask_at_hardware_level();
> + status |= IRQ_PENDING;
> + save_important_information();
> + }
> +}
>
> my_disable_irq()
> {
> + if (!(status & IRQ_SUSPENDED))
> mask_at_hardware_level();
> }
>
> my_enable_irq()
> {
> + if (important_information_has_been_saved)
> + replay_what_happened();
> +
> unmask_at_hardware_level();
> }
>
> Ugly, but that might work somehow. Not sure about the replay part, but
> that can be deferred via some more hackery as well :)
>
> Raphael, these delayed disable and the chip->irq_disable() override
> implications vs. suspend really need to be documented.
Agreed, but can you please suggest what way would be the best?
> The current comment of suspend_device_irqs() is bogus:
>
> * During system-wide suspend or hibernation device interrupts need to be
> * disabled at the chip level and this function is provided for this purpose.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
Yes, it is, sorry for that. I'll prepare a fix.
Best,
Rafael
On Monday 22 June 2009, Andrew Morton wrote:
> On Fri, 12 Jun 2009 21:52:46 +0200 (CEST)
> Thomas Gleixner <[email protected]> wrote:
>
> > On Fri, 12 Jun 2009, Kevin Hilman wrote:
> > > http://lkml.org/lkml/2009/5/4/448
> > >
> > > Only difference is I did the checking outside of the lock, which is
> > > probably wrong. In any case, you'll be interested in the thread that
> > > follows.
> >
> > Hmm, darn. That means that on hardware which has trouble with the
> > delayed disable and therefor uses it's own chip->disable_irq() method
> > the suspend logic is wreckaged.
>
> Does this maen that your original patch is no longer applicable to
> mainline/-stable?
I'd say so. There are good arguments for not doing this.
> > But there is always a way to get broken hardware tamed. :)
> >
> > suspend does:
> > __disable_irq();
> > status |= IRQ_SUSPENDED;
> > chip->disable_irq();
> >
> > resume does:
> > __enable_irq();
> > status &= ~IRQ_SUSPENDED;
> > chip->enable_irq();
> >
> > So
> >
> > - set_irq_handler(handle_level_irq);
> > + set_irq_handler(my_own_handler);
> >
> > +my_own_handler()
> > +{
> > + if (!(status & IRQ_SUSPENDED)) {
> > + handle_level_irq();
> > + } else {
> > + mask_at_hardware_level();
> > + status |= IRQ_PENDING;
> > + save_important_information();
> > + }
> > +}
> >
> > my_disable_irq()
> > {
> > + if (!(status & IRQ_SUSPENDED))
> > mask_at_hardware_level();
> > }
> >
> > my_enable_irq()
> > {
> > + if (important_information_has_been_saved)
> > + replay_what_happened();
> > +
> > unmask_at_hardware_level();
> > }
> >
> > Ugly, but that might work somehow. Not sure about the replay part, but
> > that can be deferred via some more hackery as well :)
> >
> > Raphael, these delayed disable and the chip->irq_disable() override
> > implications vs. suspend really need to be documented. The current
> > comment of suspend_device_irqs() is bogus:
> >
> > * During system-wide suspend or hibernation device interrupts need to be
> > * disabled at the chip level and this function is provided for this purpose.
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^