linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Linux PM list <linux-pm@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux PCI <linux-pci@vger.kernel.org>,
	Dmitry Torokhov <dtor@google.com>,
	Aubrey Li <aubrey.li@intel.com>
Subject: Re: [PATCH 2/5 v3] irq / PM: Make wakeup interrupts work with suspend-to-idle
Date: Fri, 29 Aug 2014 03:51:35 +0200	[thread overview]
Message-ID: <2033792.OsOnfZ7sX5@vostro.rjw.lan> (raw)
In-Reply-To: <alpine.DEB.2.10.1408281109110.23342@nanos>

On Thursday, August 28, 2014 11:23:11 AM Thomas Gleixner wrote:
> On Thu, 28 Aug 2014, Rafael J. Wysocki wrote:
> > On Wednesday, August 27, 2014 10:32:23 PM Thomas Gleixner wrote:
> > > void suspend_device_irqs(void)
> > > {
> > > 	for_each_irq_desc(irq, desc) {
> > > 		/* Disable the interrupt unconditionally */	       
> > > 		disable_irq(irq);
> > 
> > We still need to skip the IRQF_NO_SUSPEND stuff (eg. timers), so I guess
> > everything left disabled here needs to be IRQS_SUSPENDED, so we know which
> > ones to re-enable in resume_device_irqs().
> 
> Right. I skipped that one for simplicity. I wanted to look into the
> whole maze today again with brain awake. I think it's simple to
> integrate the no suspend magic here and have a separate handler for
> it.

Well, I've already read your message about this particular thing. :-)

Before that I was about to say that I'd rather leave the no suspend stuff as
is for now until we have working wakeup IRQs for suspend-to-idle at least.

> > > 
> > > 		/* Is the irq a wakeup source? */
> > > 		if (!irqd_is_wakeup_set(&desc->irq_data))
> > > 			continue;
> > > 
> > > 		/* Replace the handler */
> > > 		raw_spin_lock_irqsave(&desc->lock, flags);
> > > 	     	desc->saved_handler = desc->handler;
> > > 		desc->handler = handle_wakeup_irq;
> > 
> > Hmm.  There's no handler field in struct irq_desc (/me is puzzled).
> > 
> > Did you mean handle_irq (I think you did)?
> 
> Yup.

So I got that to work earlier today, but with some more stuff in the handler
which doesn't look particularly generic to me.  Namely, my (working) wakeup
handler looks like this at the moment:

static void handle_wakeup_irq(unsigned int irq, struct irq_desc *desc)
{
	struct irq_chip *chip = irq_desc_get_chip(desc);

	raw_spin_lock(&desc->lock);

	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
	kstat_incr_irqs_this_cpu(irq, desc);

	if (irqd_irq_disabled(&desc->irq_data)) {
		desc->istate |= IRQS_PENDING;
		mask_irq(desc);
	} else {
		desc->istate |= IRQS_SUSPENDED | IRQS_PENDING;
		desc->depth++;
		irq_disable(desc);
		pm_system_wakeup();
	}

	if (chip->irq_eoi && !(chip->flags & IRQCHIP_EOI_IF_HANDLED))
		chip->irq_eoi(&desc->irq_data);

	raw_spin_unlock(&desc->lock);
}

but it only works on a particular machine with a particular wakeup interrupt.

I had to add the

	if (chip->irq_eoi && !(chip->flags & IRQCHIP_EOI_IF_HANDLED))
		chip->irq_eoi(&desc->irq_data);

check, because otherwise it hanged the system solid once the interrupt happened.

I also had to add this:


	if (irqd_irq_disabled(&desc->irq_data)) {
		desc->istate |= IRQS_PENDING;
		mask_irq(desc);
	} ...

because otherwise the IRQ didn't work correctly after resume.

However, both those things appear to be needed just because that's a fastEOI
interrupt and some other manipulations would need to be done for edge interrupts
etc.

I'm not sure, then, if we really can come up with a perfectly generic handle_irq
wakeup interrupt handler that would work with all kinds of IRQs.  It looks like
the part that could be replaced in all of them in principle is the handle_irq_event()
call, but the rest seems to be too specific to me.

So I'm not sure what to do at this point.

It is tempting to do the following:
(1) Add a handle_event callback to struct irq_desc.
(2) Rename the existing handle_irq_event() to handle_irq_event_common().
(3) Point desc->handle_event to handle_irq_event_common() for every desc at init.
(4) Make (new, possibly static inline) handle_irq_event() invoke desc->handle_event().
(5) During suspend_device_irqs() point desc->handle_event to handle_irq_event_wakeup()
    for all wakeup IRQs.
(6) During resume_device_irqs() point desc->handle_event back to handle_irq_event_common()
    for everybody.
Then, for CONFIG_PM_SLEEP unset, handle_irq_event() can be defined as
handle_irq_event_common().

Of course, that adds a function pointer dereference overhead to every interrupt
event, so I'm not sure if it's acceptable.

One alternative may be to add a handle_wakeup_irq pointer to struct irq_desc and
check in handle_irq_event() if that is present and call it instead of the usual
stuff if that's the case.

Another way may be to introduce an irq_desc flag that will be checked by
handle_irq_event() and will indicate "wakeup mode" to it, in which it will do
the simplified wakeup handling instead of the usual stuff.  This, however, like
the previous one, will add a branch to handle_irq_event() that will be checked
every time even though it only is really necessary during system suspend.

And handlers can be replaced at the irqaction level in a couple of ways too.

Rafael


  reply	other threads:[~2014-08-29  1:32 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-08-11 13:56 [PATCH 0/6 v2] irq / PM: Shared IRQs vs IRQF_NO_SUSPEND and suspend-to-idle wakeup interrupts Rafael J. Wysocki
2014-08-11 13:58 ` [PATCH 1/6 v2] PM / sleep: Mechanism for aborting system suspends unconditionally Rafael J. Wysocki
2014-08-11 13:59 ` [PATCH 2/6 v2] irq / PM: Make IRQF_NO_SUSPEND work with shared interrupts Rafael J. Wysocki
2014-08-11 14:00 ` [PATCH 3/6 v2] irq / PM: Make wakeup interrupts work with suspend-to-idle Rafael J. Wysocki
2014-08-11 14:01 ` [PATCH 4/6 v2] x86 / PM: Set IRQCHIP_SKIP_SET_WAKE for IOAPIC IRQ chip objects Rafael J. Wysocki
2014-08-11 14:02 ` [PATCH 5/6 v2] PCI / PM: Make PCIe PME interrupts wake up from suspend-to-idle Rafael J. Wysocki
2014-08-11 14:03 ` [PATCH 6/6 v2] irq / PM: Document rules related to system suspend and interrupts Rafael J. Wysocki
2014-08-26 23:46 ` [PATCH 0/5 v3] irq / PM: Suspend-to-idle wakeup interrupts Rafael J. Wysocki
2014-08-26 23:47   ` [PATCH 1/5 v3] PM / sleep: Mechanism for aborting system suspends unconditionally Rafael J. Wysocki
2014-08-26 23:49   ` [PATCH 2/5 v3] irq / PM: Make wakeup interrupts work with suspend-to-idle Rafael J. Wysocki
2014-08-27 20:32     ` Thomas Gleixner
2014-08-27 22:51       ` Rafael J. Wysocki
2014-08-28  9:23         ` Thomas Gleixner
2014-08-29  1:51           ` Rafael J. Wysocki [this message]
2014-08-26 23:50   ` [PATCH 3/5 v3] x86 / PM: Set IRQCHIP_SKIP_SET_WAKE for IOAPIC IRQ chip objects Rafael J. Wysocki
2014-08-26 23:51   ` [PATCH 4/5 v3] PCI / PM: Make PCIe PME interrupts wake up from suspend-to-idle Rafael J. Wysocki
2014-08-26 23:52   ` [PATCH 5/5 v3] irq / PM: Document rules related to system suspend and interrupts Rafael J. Wysocki
2014-08-28 22:44   ` [PATCH 0/5 v3] irq / PM: Suspend-to-idle wakeup interrupts Thomas Gleixner
2014-08-29  0:54     ` Rafael J. Wysocki
2014-08-29  1:09       ` Thomas Gleixner
2014-09-01 14:18   ` [PATCH 00/13] genirq / PM: Wakeup interrupts handling rework (related to suspend-to-idle) Rafael J. Wysocki
2014-09-01 14:19     ` [PATCH 01/13] PM / sleep: Mechanism for aborting system suspends unconditionally Rafael J. Wysocki
2014-09-01 14:20     ` [PATCH 02/13] genirq: Move suspend/resume logic into irq/pm code Rafael J. Wysocki
2014-09-01 14:21     ` [PATCH 03/13] genirq: Add sanity checks for PM options on shared interrupt lines Rafael J. Wysocki
2014-09-01 14:22     ` [PATCH 04/13] genirq: Make use of pm misfeature accounting Rafael J. Wysocki
2014-09-01 14:22     ` [PATCH 05/13] genirq: Move MASK_ON_SUSPEND handling into suspend_device_irqs() Rafael J. Wysocki
2014-09-01 14:23     ` [PATCH 06/13] genirq: Avoid double loop on suspend Rafael J. Wysocki
2014-09-01 14:24     ` [PATCH 07/13] genirq: Distangle edge handler entry Rafael J. Wysocki
2014-09-01 14:24     ` [PATCH 08/13] genirq: Create helper for flow handler entry check Rafael J. Wysocki
2014-09-01 14:26     ` [PATCH 09/13] genirq: Mark wakeup sources as armed on suspend Rafael J. Wysocki
2014-09-01 14:27     ` [PATCH 10/13] genirq: Simplify wakeup mechanism Rafael J. Wysocki
2014-09-01 14:28     ` [PATCH 11/13] x86 / PM: Set IRQCHIP_SKIP_SET_WAKE for IOAPIC IRQ chip objects Rafael J. Wysocki
2014-09-01 14:28     ` [PATCH 12/13] PCI / PM: Make PCIe PME interrupts wake up from suspend-to-idle Rafael J. Wysocki
2014-09-01 14:29     ` [PATCH 13/13] PM / genirq: Document rules related to system suspend and interrupts Rafael J. Wysocki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2033792.OsOnfZ7sX5@vostro.rjw.lan \
    --to=rjw@rjwysocki.net \
    --cc=aubrey.li@intel.com \
    --cc=dtor@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --subject='Re: [PATCH 2/5 v3] irq / PM: Make wakeup interrupts work with suspend-to-idle' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).