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: Thu, 28 Aug 2014 00:51:09 +0200 [thread overview]
Message-ID: <7346724.A5YknVMkmd@vostro.rjw.lan> (raw)
In-Reply-To: <alpine.DEB.2.10.1408272130350.3323@nanos>
On Wednesday, August 27, 2014 10:32:23 PM Thomas Gleixner wrote:
> On Wed, 27 Aug 2014, Rafael J. Wysocki wrote:
> > The line of reasoning leading to that is as follows.
> >
> > The way suspend_device_irqs() works and the existing code in
> > check_wakeup_irqs(), called by syscore_suspend(), imply that:
> >
> > (1) Interrupt handlers are not invoked for wakeup interrupts
> > after suspend_device_irqs().
> >
> > (2) All interrups from system wakeup IRQs received after\
> > suspend_device_irqs() cause full system suspends to be aborted.
> >
> > In addition to the above, there is the requirement that
> >
> > (3) System wakeup interrupts should wake up the system from
> > suspend-to-idle.
> >
> > It immediately follows from (1) and (2) that no effort is made to
> > distinguish "genuine" wakeup interrupts from "spurious" ones. They
> > all are treated in the same way. Since (3) means that "genuine"
> > wakeup interrupts are supposed to wake up the system from
> > suspend-to-idle too, consistency with (1) and (2) requires that
> > "spurious" wakeup interrupts should do the same thing. Thus there is
> > no reason to invoke interrupt handlers for wakeup interrups after
> > suspend_device_irqs() in the suspend-to-idle case. Moreover, doing
> > so would go against rule (1).
>
> I agree with that, but I disagree with the implementation.
>
> We now have two separate mechanisms to abort suspend:
>
> 1) The existing suspend_device_irqs() / check_wakeup_irqs()
>
> 2) The new suspend_device_irqs() /
> reenable_stuff_and_fiddle_with_irq_action()
>
> So why do we need those two mechanisms in the first place?
>
> AFAICT there is no reason why we cant use the abort_suspend mechanics
> to replace the suspend_device_irqs() / check_wakeup_irqs() pair.
>
> All it needs is to do the handler substitution in
> suspend_device_irqs() right away and replace the loop in
> check_wakeup_irqs() with a check for abort_suspend == true. The roll
> back of the handler substitution can happen in resume_device_irqs()
> for both scenarios.
We can do that of course.
> Aside of that the whole irqaction based substitution is silly. What's
> wrong with doing it at the real interrupt handler level?
Nothing I suppose. :-)
> static void handle_wakeup_irq(unsigned int irq, struct irq_desc *desc)
> {
> raw_spin_lock(&desc->lock);
>
> desc->istate |= IRQS_SUSPENDED | IRQS_PENDING;
> desc->depth++;
> irq_disable(desc);
> pm_system_wakeup();
>
> raw_spin_unlock(&desc->lock);
> }
>
> 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().
>
> /* 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)?
> raw_spin_unlock_irqrestore(&desc->lock, flags);
>
> /* Reenable the wakeup irq */
> enable_irq(irq);
> }
> }
>
> /* Move that into the pm core code */
> bool check_wakeup_irqs(void)
> {
> return abort_suspend;
> }
>
> void resume_device_irqs(void)
> {
> for_each_irq_desc(irq, desc) {
>
> /* Prevent the wakeup handler from running */
> disable_irq();
>
> raw_spin_lock_irqsave(&desc->lock, flags);
>
> /* Do we need to restore the handler? */
> if (desc->handler == handle_wakeup_irq)
> desc->handler = desc->saved_handler;
>
> /* Is the irq a wakeup source? */
> if (!irqd_is_wakeup_set(&desc->irq_data))
> __enable_irq(irq, desc);
>
> /* Did it get disabled in the wakeup handler? */
> else if (desc->istate & IRQS_SUSPENDED)
> __enable_irq(irq, desc);
>
> raw_spin_unlock_irqrestore(&desc->lock, flags);
>
> enable_irq();
> }
> }
>
> Hmm?
OK
There is quite some ugliness related to resume_irqs(), the want_early thing
and IRQF_EARLY_RESUME / IRQF_FORCE_RESUME. I guess that needs to be preserved?
> One thing we might think about is having flow specific
> handle_wakeup_irq variants as some hardware might require an ack or
> eoi, but that's a simple to solve problem and way simpler than
> fiddling with the irqaction chain and avoids the whole mess of
> sprinkling irq_pm_saved_id() and irq_pm_restore_handler() calls all
> over the place. I wonder why you added them to __free_irq() at all,
> but no, we dont want that.
I was concerned about the (unlikely) possibility of freeing an interrupt
having a temporary handler. Never mind.
Rafael
next prev parent reply other threads:[~2014-08-27 22: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 [this message]
2014-08-28 9:23 ` Thomas Gleixner
2014-08-29 1:51 ` Rafael J. Wysocki
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=7346724.A5YknVMkmd@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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).