Re: [PATCH 1/3] irq / PM: New driver interface for wakeup interruptsn

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Friday, August 01, 2014 11:40:55 AM Thomas Gleixner wrote:
> On Fri, 1 Aug 2014, Rafael J. Wysocki wrote:
> > On Friday, August 01, 2014 12:16:23 AM Thomas Gleixner wrote:

[cut]

> > > > And now there's one more piece of it which is suspend-to-idle (aka "freeze").
> > > > That doesn't go all the way to syscore_suspend(), but basically stops after
> > > > finishing the "noirq" phase of suspending devices.  Then, it idles the CPUs
> > > > and waits for interrupts that will take them out of idle.  Only some of those
> > > > interrupts are wakeup events, so it only resumes when __pm_wakeup_event() or
> > > > __pm_relax() is called in the process of handling the interrupts.
> > > >
> > > > Of course, it could be implemented differently, but that was the simplest
> > > > way to do that.  It still can be changed, but I'd really like it not to have
> > > > to go through all of the disabling nonboot CPUs and sysfore_suspend(), because
> > > > that simply isn't necessary (and it avoids a metric ton of problems with CPU
> > > > offline/online).  And of course, it has to work on x86 too.
> > > 
> > > Right. So one thing which would make the situation more clear is that
> > > the interrupt handler needs to tell the core code about this by
> > > returning something like IRQ_HANDLED_PMWAKE and the core kicks the
> > > suspend-to-idle logic back to life. I'm not sure whether the extra
> > > return value is actually necessary, we might even map it to
> > > IRQ_HANDLED in the core as we know that we are in the suspend
> > > state.
> > 
> > I'm not sure I'm following you here.  Do you mean that upon receiving
> > IRQ_HANDLED_PMWAKE from an interrupt handler the core will know that
> > this was a wakeup event and will trigger a resume from suspend-to-idle?
> 
> Correct. Whether we need the extra return code is debatable. But yes,
> we want to talk to the PM/suspend/resume thing at core level instead
> of letting drivers use random interfaces which happen to be exported
> for one reason or the other, but definitely not for the purpose of
> random driver.

OK, I guess "IRQ_HANDLED from a wakeup interrupt" may be interpreted as
IRQ_HANDLED_PMWAKE.  On the other hand, if that's going to be handled in
handle_irq_event_percpu(), then using a special return code would save us
a brach for IRQ_HANDLED interrupts.  We could convert it to IRQ_HANDLED
immediately then.

[cut]

> > I'm not sure about the ordering, though.  It would be good to have a working
> > replacement for the IRQF_NO_SUSPEND things that we'll be removing in 1, for
> > example.  So since we need to do 3) IRQF_SHARED for both IRQF_NO_SUSPEND and
> > wakeup, as you said, would it be practical to start with that one?
> 
> The numbering was not meant as ordering, it was just to seperate the
> various issues which we need to look at.

OK, I'll take a stab at the IRQF_SHARED thing if you don't mind.

Here's my current understanding of what can be done for IRQF_NO_SUSPEND.

In suspend_device_irqs():

(1) If all actions in the list have the same setting (eg. IRQF_NO_SUSPEND unset),
    keep the current behavior.
(2) If the actions have different settings:
    - Actions with IRQF_NO_SUSPEND set are not modified.
    - Actions with IRQF_NO_SUSPEND unset are switched over to a stub handler.
    - IRQS_SUSPEND_MODE (new flag) is set for the IRQ.

In note_interrupt():

  If action_ret is IRQ_NONE and IRQS_SUSPEND_MODE is set for the IRQ, disable the
  IRQ, set IRQS_SUSPENDED for it and call system_wakeup(BAD_INTERRUPT) (that will
  abort suspend if still in progress or break the suspend-to-idle loop).

In resume_device_irqs():

(1) If IRQS_SUSPEND_MODE is set, switch over actions with IRQF_NO_SUSPEND unset
    to their original handlers and clear the flag.  Fall through.
(2) If IRQS_SUSPENDED is set, clear the flag and enable the interrupt.

The stub handler only needs to return IRQ_NONE unconditionally in that case.

Does that make sense?

Rafael