[PATCH 0/3] PM / wakeup / ACPI: Suspend-to-idle wakeup fixes and optimization

[PATCH 0/3] PM / wakeup / ACPI: Suspend-to-idle wakeup fixes and optimization

[Rafael J. Wysocki]

Hi,

Two recent commits related to suspend-to-idle introduced a couple of issues
that need to be fixed.

In addition to that, there turns out to be a way to avoid carrying out the
"noirq" phase of device resume in order to check if wakeup triggered by an
ACPI SCI is a real one, which patch [3/3] is about.

Refer to patch changelogs for details.

Thanks,
Rafael

[PATCH 1/3] PM / wakeup: Fix up wakeup_source_report_event()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Commit 8a537ece3d94 (PM / wakeup: Integrate mechanism to abort
transitions in progress) modified wakeup_source_report_event()
and wakeup_source_activate() to make it possible to call
pm_system_wakeup() from the latter if so indicated by the
caller of the former (via a new function argument added by that
commit), but it overlooked the fact that in some situations
wakeup_source_report_event() is called to signal a "hard" event
(ie. such that should abort a system suspend in progress) after
pm_stay_awake() has been called for the same wakeup source object,
in which case the pm_system_wakeup() will not trigger.

To work around this issue, modify wakeup_source_activate() and
wakeup_source_report_event() again so that pm_system_wakeup() is
called by the latter directly (if its last argument is true), in
which case the additional argument does not need to be passed
to wakeup_source_activate() any more, so drop it from there.

Fixes: 8a537ece3d94 (PM / wakeup: Integrate mechanism to abort transitions in progress)
Reported-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/wakeup.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/base/power/wakeup.c
===================================================================
--- linux-pm.orig/drivers/base/power/wakeup.c
+++ linux-pm/drivers/base/power/wakeup.c
@@ -512,13 +512,12 @@ static bool wakeup_source_not_registered
 /**
  * wakup_source_activate - Mark given wakeup source as active.
  * @ws: Wakeup source to handle.
- * @hard: If set, abort suspends in progress and wake up from suspend-to-idle.
  *
  * Update the @ws' statistics and, if @ws has just been activated, notify the PM
  * core of the event by incrementing the counter of of wakeup events being
  * processed.
  */
-static void wakeup_source_activate(struct wakeup_source *ws, bool hard)
+static void wakeup_source_activate(struct wakeup_source *ws)
 {
 	unsigned int cec;
 
@@ -526,9 +525,6 @@ static void wakeup_source_activate(struc
 			"unregistered wakeup source\n"))
 		return;
 
-	if (hard)
-		pm_system_wakeup();
-
 	ws->active = true;
 	ws->active_count++;
 	ws->last_time = ktime_get();
@@ -554,7 +550,10 @@ static void wakeup_source_report_event(s
 		ws->wakeup_count++;
 
 	if (!ws->active)
-		wakeup_source_activate(ws, hard);
+		wakeup_source_activate(ws);
+
+	if (hard)
+		pm_system_wakeup();
 }
 
 /**

[PATCH 2/3] RTC: rtc-cmos: Fix wakeup from suspend-to-idle

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Commit eed4d47efe95 (ACPI / sleep: Ignore spurious SCI wakeups from
suspend-to-idle) modified the core suspend-to-idle code to filter
out spurious SCI interrupts received while suspended, which requires
ACPI event source handlers to report wakeup events in a way that
will trigger a wakeup from suspend to idle (or abort system suspends
in progress, which is equivalent).

That needs to be done in the rtc-cmos driver too, which was overlooked
by the above commit, so do that now.

Fixes: eed4d47efe95 (ACPI / sleep: Ignore spurious SCI wakeups from suspend-to-idle)
Reported-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/rtc/rtc-cmos.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-pm/drivers/rtc/rtc-cmos.c
===================================================================
--- linux-pm.orig/drivers/rtc/rtc-cmos.c
+++ linux-pm/drivers/rtc/rtc-cmos.c
@@ -1085,7 +1085,7 @@ static u32 rtc_handler(void *context)
 	}
 	spin_unlock_irqrestore(&rtc_lock, flags);
 
-	pm_wakeup_event(dev, 0);
+	pm_wakeup_hard_event(dev);
 	acpi_clear_event(ACPI_EVENT_RTC);
 	acpi_disable_event(ACPI_EVENT_RTC, 0);
 	return ACPI_INTERRUPT_HANDLED;

Re: [PATCH 2/3] RTC: rtc-cmos: Fix wakeup from suspend-to-idle

[Alexandre Belloni]

Hi Rafael,

On 14/05/2017 at 01:55:32 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Commit eed4d47efe95 (ACPI / sleep: Ignore spurious SCI wakeups from
> suspend-to-idle) modified the core suspend-to-idle code to filter
> out spurious SCI interrupts received while suspended, which requires
> ACPI event source handlers to report wakeup events in a way that
> will trigger a wakeup from suspend to idle (or abort system suspends
> in progress, which is equivalent).
> 
> That needs to be done in the rtc-cmos driver too, which was overlooked
> by the above commit, so do that now.
> 
> Fixes: eed4d47efe95 (ACPI / sleep: Ignore spurious SCI wakeups from suspend-to-idle)
> Reported-by: David E. Box <david.e.box@linux.intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/rtc/rtc-cmos.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-pm/drivers/rtc/rtc-cmos.c
> ===================================================================
> --- linux-pm.orig/drivers/rtc/rtc-cmos.c
> +++ linux-pm/drivers/rtc/rtc-cmos.c
> @@ -1085,7 +1085,7 @@ static u32 rtc_handler(void *context)
>  	}
>  	spin_unlock_irqrestore(&rtc_lock, flags);
>  
> -	pm_wakeup_event(dev, 0);
> +	pm_wakeup_hard_event(dev);
>  	acpi_clear_event(ACPI_EVENT_RTC);
>  	acpi_disable_event(ACPI_EVENT_RTC, 0);
>  	return ACPI_INTERRUPT_HANDLED;
> 

This seems good to me, do you expect it to go through my tree?

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/3] RTC: rtc-cmos: Fix wakeup from suspend-to-idle

[Alexandre Belloni]

On 31/05/2017 at 11:21:55 +0200, Alexandre Belloni wrote:
> Hi Rafael,
> 
> On 14/05/2017 at 01:55:32 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Commit eed4d47efe95 (ACPI / sleep: Ignore spurious SCI wakeups from
> > suspend-to-idle) modified the core suspend-to-idle code to filter
> > out spurious SCI interrupts received while suspended, which requires
> > ACPI event source handlers to report wakeup events in a way that
> > will trigger a wakeup from suspend to idle (or abort system suspends
> > in progress, which is equivalent).
> > 
> > That needs to be done in the rtc-cmos driver too, which was overlooked
> > by the above commit, so do that now.
> > 
> > Fixes: eed4d47efe95 (ACPI / sleep: Ignore spurious SCI wakeups from suspend-to-idle)
> > Reported-by: David E. Box <david.e.box@linux.intel.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/rtc/rtc-cmos.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Index: linux-pm/drivers/rtc/rtc-cmos.c
> > ===================================================================
> > --- linux-pm.orig/drivers/rtc/rtc-cmos.c
> > +++ linux-pm/drivers/rtc/rtc-cmos.c
> > @@ -1085,7 +1085,7 @@ static u32 rtc_handler(void *context)
> >  	}
> >  	spin_unlock_irqrestore(&rtc_lock, flags);
> >  
> > -	pm_wakeup_event(dev, 0);
> > +	pm_wakeup_hard_event(dev);
> >  	acpi_clear_event(ACPI_EVENT_RTC);
> >  	acpi_disable_event(ACPI_EVENT_RTC, 0);
> >  	return ACPI_INTERRUPT_HANDLED;
> > 
> 
> This seems good to me, do you expect it to go through my tree?
> 

OK, I see it has already been pulled by Linus.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 3/3] ACPI / sleep: Simplify suspend-to-idle event processing loop

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Commit eed4d47efe95 (ACPI / sleep: Ignore spurious SCI wakeups from
suspend-to-idle) modified the core suspend-to-idle code to filter
out spurious SCI interrupts received while suspended, but it
implemented that by resuming the system partially every time an
SCI triggers wakeup, so that the SCI handler can run (and possibly
further event handlers invoked by it can run too), which requires the
"noirq" phase of device resume to be carried out.  One drawback of
that implementation is that PCI devices are put into the full-power
state (D0) during the "noirq" resume phase and they need to be put
into low-power states again in case the wakeup event turns out to be
spurious, which may cause some unpleasant power fluctuations in the
system to happen among other things.

However, that can be avoided by using the observation that it is
generally possible to call the SCI handler directly from the ACPI
suspend-to-idle ->wake callback and the processing initiated by it
can be carried out before the "noirq" phase of device resume starts,
so if the SCI interrupt turns out to be non-wakeup, the system can
go back to sleep immediately.

Implement that idea, drop the suspend-to-idle ->sync callback added
by commit eed4d47efe95 as it is not necessary any more and simplify
the suspend-to-idle event processing loop in the core code.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/internal.h |    2 ++
 drivers/acpi/osl.c      |    9 +++++++++
 drivers/acpi/sleep.c    |   17 ++++++++---------
 include/linux/suspend.h |    1 -
 kernel/power/suspend.c  |   12 +++++-------
 5 files changed, 24 insertions(+), 17 deletions(-)

Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -197,6 +197,8 @@ void acpi_ec_remove_query_handler(struct
 /*--------------------------------------------------------------------------
                                   Suspend/Resume
   -------------------------------------------------------------------------- */
+void acpi_irq_run(void);
+
 #ifdef CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT
 extern int acpi_sleep_init(void);
 #else
Index: linux-pm/drivers/acpi/osl.c
===================================================================
--- linux-pm.orig/drivers/acpi/osl.c
+++ linux-pm/drivers/acpi/osl.c
@@ -589,6 +589,15 @@ acpi_status acpi_os_remove_interrupt_han
 	return AE_OK;
 }
 
+void acpi_irq_run(void)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	acpi_irq(acpi_gbl_FADT.sci_interrupt, NULL);
+	local_irq_restore(flags);
+}
+
 /*
  * Running in interpreter thread context, safe to sleep
  */
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -669,18 +669,18 @@ static int acpi_freeze_prepare(void)
 
 static void acpi_freeze_wake(void)
 {
+	if (!acpi_sci_irq_valid() ||
+	    irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq)))
+		return;
+
 	/*
 	 * If IRQD_WAKEUP_ARMED is not set for the SCI at this point, it means
 	 * that the SCI has triggered while suspended, so cancel the wakeup in
-	 * case it has not been a wakeup event (the GPEs will be checked later).
+	 * case it has not been a wakeup event and call the SCI handler directly
+	 * to allow it to check the event sources.
 	 */
-	if (acpi_sci_irq_valid() &&
-	    !irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq)))
-		pm_system_cancel_wakeup();
-}
-
-static void acpi_freeze_sync(void)
-{
+	pm_system_cancel_wakeup();
+	acpi_irq_run();
 	/*
 	 * Process all pending events in case there are any wakeup ones.
 	 *
@@ -709,7 +709,6 @@ static const struct platform_freeze_ops
 	.begin = acpi_freeze_begin,
 	.prepare = acpi_freeze_prepare,
 	.wake = acpi_freeze_wake,
-	.sync = acpi_freeze_sync,
 	.restore = acpi_freeze_restore,
 	.end = acpi_freeze_end,
 };
Index: linux-pm/include/linux/suspend.h
===================================================================
--- linux-pm.orig/include/linux/suspend.h
+++ linux-pm/include/linux/suspend.h
@@ -190,7 +190,6 @@ struct platform_freeze_ops {
 	int (*begin)(void);
 	int (*prepare)(void);
 	void (*wake)(void);
-	void (*sync)(void);
 	void (*restore)(void);
 	void (*end)(void);
 };
Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -106,21 +106,17 @@ static void freeze_enter(void)
 
 static void s2idle_loop(void)
 {
-	do {
+	for (;;) {
 		freeze_enter();
 
 		if (freeze_ops && freeze_ops->wake)
 			freeze_ops->wake();
 
-		dpm_resume_noirq(PMSG_RESUME);
-		if (freeze_ops && freeze_ops->sync)
-			freeze_ops->sync();
-
 		if (pm_wakeup_pending())
 			break;
 
 		pm_wakeup_clear(false);
-	} while (!dpm_suspend_noirq(PMSG_SUSPEND));
+	}
 }
 
 void freeze_wake(void)
@@ -395,7 +391,7 @@ static int suspend_enter(suspend_state_t
 	 */
 	if (state == PM_SUSPEND_FREEZE) {
 		s2idle_loop();
-		goto Platform_early_resume;
+		goto Devices_resume_noirq;
 	}
 
 	error = disable_nonboot_cpus();
@@ -429,6 +425,8 @@ static int suspend_enter(suspend_state_t
 
  Platform_wake:
 	platform_resume_noirq(state);
+
+ Devices_resume_noirq:
 	dpm_resume_noirq(PMSG_RESUME);
 
  Platform_early_resume:

Re: [PATCH 3/3] ACPI / sleep: Simplify suspend-to-idle event processing loop

[Rafael J. Wysocki]

On Sunday, May 14, 2017 01:57:13 AM Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Commit eed4d47efe95 (ACPI / sleep: Ignore spurious SCI wakeups from
> suspend-to-idle) modified the core suspend-to-idle code to filter
> out spurious SCI interrupts received while suspended, but it
> implemented that by resuming the system partially every time an
> SCI triggers wakeup, so that the SCI handler can run (and possibly
> further event handlers invoked by it can run too), which requires the
> "noirq" phase of device resume to be carried out.  One drawback of
> that implementation is that PCI devices are put into the full-power
> state (D0) during the "noirq" resume phase and they need to be put
> into low-power states again in case the wakeup event turns out to be
> spurious, which may cause some unpleasant power fluctuations in the
> system to happen among other things.
> 
> However, that can be avoided by using the observation that it is
> generally possible to call the SCI handler directly from the ACPI
> suspend-to-idle ->wake callback and the processing initiated by it
> can be carried out before the "noirq" phase of device resume starts,
> so if the SCI interrupt turns out to be non-wakeup, the system can
> go back to sleep immediately.
> 
> Implement that idea, drop the suspend-to-idle ->sync callback added
> by commit eed4d47efe95 as it is not necessary any more and simplify
> the suspend-to-idle event processing loop in the core code.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Scratch this one.

It is not sufficient to run the SCI handler alone, the SCI IRQ has to be
re-armed properly for system wakeup too, so resume_device_irqs()
and suspend_device_irqs() need to run for that.

Thanks,
Rafael