Re: [PATCH] genirq: do not disable IRQ_WAKEUP marked irqs on suspend

Re: [PATCH] genirq: do not disable IRQ_WAKEUP marked irqs on suspend

[Kevin Hilman]

Thomas Gleixner <tglx@linutronix.de> 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 <tglx@linutronix.de>
> Cc: stable@kernel.org

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] genirq: do not disable IRQ_WAKEUP marked irqs on suspend

[Thomas Gleixner]

On Fri, 12 Jun 2009, Kevin Hilman wrote:
> Thomas Gleixner <tglx@linutronix.de> 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 <tglx@linutronix.de>
> > Cc: stable@kernel.org
> 
> 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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] genirq: do not disable IRQ_WAKEUP marked irqs on suspend

[Kevin Hilman]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Fri, 12 Jun 2009, Kevin Hilman wrote:
>> Thomas Gleixner <tglx@linutronix.de> 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 <tglx@linutronix.de>
>> > Cc: stable@kernel.org
>> 
>> 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 majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at  http://www.tux.org/lkml/
>> 

Re: [PATCH] genirq: do not disable IRQ_WAKEUP marked irqs on suspend

[Thomas Gleixner]

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

Re: [PATCH] genirq: do not disable IRQ_WAKEUP marked irqs on suspend

[Andrew Morton]

On Fri, 12 Jun 2009 21:52:46 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> 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.
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^

Re: [PATCH] genirq: do not disable IRQ_WAKEUP marked irqs on suspend

[Rafael J. Wysocki]

On Monday 22 June 2009, Andrew Morton wrote:
> On Fri, 12 Jun 2009 21:52:46 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> 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.
> >    ^^^^^^^^^^^^^^^^^^^^^^^^^^

Re: [PATCH] genirq: do not disable IRQ_WAKEUP marked irqs on suspend

[Rafael J. Wysocki]

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

[PATCH] genirq: do not disable IRQ_WAKEUP marked irqs on suspend

[Thomas Gleixner]

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 <tglx@linutronix.de>
Cc: stable@kernel.org

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);
 	}