Re: [PATCH] GPIO button wth wakeup attribute is supposed to wake the system up

[Dmitry Torokhov]

On Wed, Jul 09, 2014 at 02:59:33AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, July 08, 2014 05:15:06 PM Dmitry Torokhov wrote:
> > On Wed, Jul 09, 2014 at 01:06:07AM +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, July 08, 2014 03:11:14 PM Dmitry Torokhov wrote:
> > > > On Tue, Jul 08, 2014 at 11:47:01PM +0200, Rafael J. Wysocki wrote:
> > > > > On Tuesday, July 08, 2014 02:12:59 PM Dmitry Torokhov wrote:
> > > > > > On Tue, Jul 08, 2014 at 11:06:17PM +0200, Rafael J. Wysocki wrote:
> > > > > > > On Tuesday, July 08, 2014 01:45:30 PM Dmitry Torokhov wrote:
> > > > > > > > On Tue, Jul 08, 2014 at 10:52:52PM +0200, Rafael J. Wysocki wrote:
> > > > > > > > > On Thursday, June 19, 2014 08:51:25 AM Li, Aubrey wrote:
> > > > > > > > > > When the wakeup attribute is set, the GPIO button is capable of
> > > > > > > > > > waking up the system from sleep states, including the "freeze"
> > > > > > > > > > sleep state.  For that to work, its driver needs to pass the
> > > > > > > > > > IRQF_NO_SUSPEND flag to devm_request_any_context_irq(), or the
> > > > > > > > > > interrupt will be disabled by suspend_device_irqs() and the
> > > > > > > > > > system won't be woken up by it from the "freeze" sleep state.
> > > > > > > > > > 
> > > > > > > > > > The suspend_device_irqs() routine is a workaround for drivers
> > > > > > > > > > that mishandle interrupts triggered when the devices handled
> > > > > > > > > > by them are suspended, so it is safe to use IRQF_NO_SUSPEND in
> > > > > > > > > > all drivers that don't have that problem.
> > > > > > > > > > 
> > > > > > > > > > The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Aubrey Li <aubrey.li@xxxxxxxxxxxxxxx>
> > > > > > > > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > > > > > > > 
> > > > > > > > > OK
> > > > > > > > > 
> > > > > > > > > Due to the lack of response (ie. no objections) and because the issue
> > > > > > > > > addressed by this patch is real, I'm queuing it up as a PM-related fix
> > > > > > > > > for 3.17.
> > > > > > > > 
> > > > > > > > Please do not. The response is till the same: board code should make sure
> > > > > > > > that enable_irq_wake() does the right thing and keeps interrupts enabled.
> > > > > > > 
> > > > > > > Which board code?  That's nothing like that for the platforms in question.
> > > > > > 
> > > > > > Then it needs to be written.
> > > > > 
> > > > > Well, excuse me, but I don't get it.  Why would I need to write any board code
> > > > > for an ACPI-based system?
> > > > 
> > > > Why would not you? What is the difference between ACPI systems and all
> > > > other systems? If ACPI-based systems need certain behavior they need to
> > > > implement it, the same as DT-based systems or custom boards.
> > > 
> > > I'm not sure what you're talking about.
> > > 
> > > This isn't an ACPI-based system that needs certain behavior.  That's certain
> > > behavior we want from GPIO buttons that can wake up *in* *general*.
> > > 
> > > Regardless of the platform, we want interrupts from those buttons to happen
> > > after calling suspend_device_irqs().  Why?  Because we want them to be able
> > > to happen while freeze_enter() is being executed and *that* is platform
> > > independent.
> > 
> > Tell me this: do we always call suspend_device_irqs()?
> 
> Yes, we do.

OK, I see.

> 
> > > > > > > > It is wrong to patch drivers for this.
> > > > > > > 
> > > > > > > Why is it?  Only drivers know if they can handle incoming interrupts after
> > > > > > > having suspended their devices.
> > > > > > 
> > > > > > The driver correctly used enable_irq_wake() to indicate that interrupt should
> > > > > > be a wakeup source, the now the core/board needs to make sure the interrupt
> > > > > > gets delivered to the driver properly. We should not be patching every driver
> > > > > > that uses enable_irq_wake() with IRQF_NO_SUSPEND.
> > > > > 
> > > > > Interrupts that can wake up from the "freeze" sleep state need not be set up
> > > > > with enable_irq_wake() and the flag doesn't say "this is a wakeup interrupt".
> > > > > It says "do not suspend this interrupt, I can handle it after the device has
> > > > > been suspended" (as I said).
> > > > 
> > > > And the driver does not really care about this and whether the sleep
> > > > state is suspend or freeze or something else, it is your platform that
> > > > cares and has certain limitations that require interrupts to be not
> > > > suspended in certain cases.
> > > 
> > > Not in "certain cases", but actually never and it is not about any platform
> > > limitations.  I'm not sure what other words I need to use to make it more
> > > clear.
> > > 
> > > > From the driver POV it says that the device can be a waekup source
> > > > (again it does not care about details as to which sleep state we woudl
> > > > be waking from) and it expects the PM core to handle the things
> > > > properly. If certain sleep state requires interrupts to be kept on then
> > > > PM core should make them as such, not driver.
> > > 
> > > That would be the case in an ideal world, but the real one is not ideal,
> > > unfortunately.  The problem is that the PM core actually cannot decide
> > > which interrupts to keep on, because it doesn't know which drivers can
> > > cope with interrupts that happen after their devices have been suspended.
> > > Using IRQF_NO_SUSPEND is the way to experess that by a driver.
> > 
> > When device driver marks IRQ as a wakeup source I believe it is prepared
> > to handle it (or it would shut it off explicitly).
> 
> I can agree with that.
> 
> Are you suggesting that __disable_irq() should check irq_data for
> IRQD_WAKEUP_STATE and skip the IRQ (in the 'suspend' case) if that is set?

Yes, something like that.

> 
> That might work too.
> 
> > > And again, that has nothing to do with platform limitations or requirements.
> > > All this is about is whether or not to allow interrupts to be *handled* by
> > > drivers after certain point in the suspend sequence, which is
> > > suspend_device_irqs().  By using IRQF_NO_SUSPEND drivers say, basically "it is
> > > OK to leave this IRQ as is, I promise to take care of it going forward" and
> > > that covers *all* suspend/hibernate transitions.
> > > 
> > > The reason why the "freeze" sleep state is somewhat special is that it doesn't
> > > do any platform-specific magic and needs *normal* device interrupts to work
> > > after suspend_device_irqs().  And I would *love* *to* drop suspend_device_irqs()
> > > at this point, but I can't, because there still are drivers that would have
> > > broken had I done that.
> > > 
> > > And I'm saying "somewhat" above, because that behavior is actually needed to
> > > prevent wakeup events occuring *during* suspend transitions from being lost
> > > for all kinds of those transitions (if someone actually cares).
> > > 
> > > > > 
> > > > > And if it is OK for a driver to set IRQF_SHARED, it is equally OK for it to
> > > > > set IRQF_NO_SUSPEND, because, in fact, those two flags are related.
> > > > 
> > > > Are you proposing for IRQ core to automatically set IRQF_NO_SUSPEND for
> > > > IRQF_SHARED interrupts? That wold be fine with me.
> > > 
> > > No, I'm not.
> > 
> > Then they are not really related that closely, are they?
> 
> Yes, they are.  The original underlying problem was that some drivers with
> IRQF_SHARED set mishandled interrupts triggered by the other device sharing the
> IRQ line (so they did not expect the interrupt to trigger at that point, but it did).

Right, well-behaved driver using IRQF_SHARED should expect IRQ at any
time, even if the device supposed to be quiesced at the time. But I
guess we are still not comfortable setting IRQF_NO_SUSPEND even though
it should be OK. 

> 
> > > The first choice basically is to go through all drivers that don't use IRQF_NO_SUSPEND
> > > and which have _noirq suspend callbacks (that covers all PCI drivers and some
> > > non-PCI ones too IIRC) and audit their interrupt handlers to check whether or not
> > > they can cope with interrupts coming after their devices have been suspended.  Fix
> > > the ones that can't and we can drop suspend_device_irqs().
> > > 
> > > But I guess I may be forgiven for regarding that as rather unattractive.  And we
> > > actually could have done that to start with, guess why we didn't?
> > > 
> > > The second choice is to use IRQF_NO_SUSPEND in the drivers that are OK.  That
> > > isn't too attractive either but has been practice for quite a while.
> > > 
> > > > > > If you look at the earlier patch discussion Tegra folks managed to implement
> > > > > > this behavior just fine.
> > > > > 
> > > > > I'm not sure whose idea it was that IRQF_NO_SUSPEND was not to be set by drivers,
> > > > > but it is not a correct one.  I know why suspend_device_irqs() was introduced
> > > > > and I'm telling you this has nothing to do with setting up the IRQ chip to do
> > > > > system wakeup.
> > > > 
> > > > I do not believe I asked why suspend_device_irqs() was introduced.
> > > 
> > > But you should, because suspend_device_irqs() is the very reason why
> > > IRQF_NO_SUSPEND exists. :-)
> > 
> > Then you should have shared this knowledge instead of asserting that you
> > possess it.
> 
> Well, I just thought you knew that too.
> 
> > > > > 
> > > > > And please grep for IRQF_NO_SUSPEND to see how drivers generally use it.
> > > > 
> > > > I see that just handful of them use IRQF_NO_SUSPEND (not sure how many
> > > > are actully required), I see that a lot more drivers use
> > > > enable_irq_wake() and do not bother setting IRQF_NO_SUSPEND.
> > > 
> > > And they will have problems with the "freeze" sleep state.
> > > 
> > > enable_irq_wake() is *not* a replacement for IRQF_NO_SUSPEND, nor the other
> > > way around.  They are different things.
> > 
> > However what I hear is that one has to use one when using another, and
> > form this fact comes my request:
> 
> That isn't exactly the case.  IRQF_NO_SUSPEND need not imply enable_irq_wake().

Right, but I am hoping that enable_irq_wake() can imply IRQF_NO_SUSPEND.

> 
> > when entering freeze sleep state
> > interrupts that are marked as wakeup sources should be automatically
> > excluded form the list of IRQs that need to be suspended.
> 
> That seems to be the case for all sleep states, doesn't it?

I was under impression that some of sleep paths were skipping freezer.
If not then they all need this.

Thanks.

-- 
Dmitry
--
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/