Arve Hjønnevåg <arve@xxxxxxxxxxx> writes: > 2009/5/8 Kevin Hilman <khilman@xxxxxxxxxxxxxxxxxxx>: >> 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? >> > > No. The platform code needs to turn off interrupts that are not wakeup > interrupts anyway, so there is not much point in disabling some > interrupts early. Also, if the interrupt in question is not a wakeup > interrupt you leave it in a state where it does not detect an edge. A > driver that enables its hardware in resume, then unmasks the interrupt > would loose an interrupt that triggered between enabling the hardware > and unmasking the interrupt. Got it. Thanks for your (repeated) explanations. I understand now your reasons for allowing the interrupt to remain across suspend. I will modify my platform to rely on [enable|disable]_irq_wake() in addition to ensuring platform code is disabling non wakup IRQs. >>>> 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? >> > > It did not. > >> If the interrupt is being used as a wakeup, why would anyone be >> calling disable_irq()? >> > > Drivers call disable_irq to make sure their interrupt handler does not > get called. A driver may not be able take interrupts after its suspend > hook has been called. If it calls disable_irq on a wakeup interrupt > then this interrupt should abort suspend or wake up from suspend. The > driver will then see the interrupt when it call enable_irq in its > resume handler. OK. Kevin _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
