Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Kevin Hilman]

Arve Hjønnevåg <arve@xxxxxxxxxxx> writes:

> On Thu, May 7, 2009 at 9:19 AM, Kevin Hilman
> <khilman@xxxxxxxxxxxxxxxxxxx> wrote:
>> From a3f359c66bd0ae1bb2603e7cf120d9d4d68591b7 Mon Sep 17 00:00:00 2001
>> From: Kevin Hilman <khilman@xxxxxxxxxxxxxxxxxxx>
>> Date: Wed, 6 May 2009 16:00:07 -0700
>> Subject: [PATCH] genirq: ensure IRQs are lazy disabled before suspend
>>
>> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
>> behavior of disable_irq() was changed to delay the disable until it is
>> next handled.
>>
>> However, this leaves open the possibility that the system can go into
>> suspend with an interrupt enabled.  For example, if a driver calls
>> disable_irq() in its suspend_hook there's now a possibility that the
>> system is suspended before the lazy disable happens.
>>
>> The result is an unwanted wakeup from suspend if the IRQ is capable of
>> waking the system (common on embedded SoCs.)
>
> If the interrupt contoller uses the same enable register for wakeup
> and interrupts, I think it is the responsibility of the platform code,
> not individual drivers, to disable the interrupts that are not marked
> for wakeup before entering suspend.

I agree, for wakeup interrupts, drivers should use
[set|disable]_irq_wake() and the platform code should handle this.

I used wakeup interrupts in this description as an example which
turned out to be a bad example.  The 2nd version of this patch I
posted, I removed the reference to wakeup interrupts in favor of just
talking about the delayed disable piece.

But ignoring wakup interrupts, would you agree that the delayed
disable of an interrupt should not wait until after resume?

>> This patch ensures that the lazy disable is done, and masked by
>> the irq_chip before the system goes into suspend.
>
> This will create a window where wakeup interrupts can be lost if the
> driver has masked the interrupt (by calling disable_irq). If the
> hardware does not allow edge detection on disabled interrupts (the msm
> platform has this limitation) then this change will turn off the edge
> detection. If suspend_ops->enter does not turn the interrupt (and edge
> detection) back on (without this change it may never need to turn on
> any interrupt) it will not wakeup at all.

Not sure I follow you here...

It seems like you're relying on the delayed disable to wait until
after resume so that disabled interrupts can wake the system.  How
did this work before the delayed disable patch?

If the interrupt is being used as a wakeup, why would anyone be
calling disable_irq()?

Kevin


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/