ACPI: PM: s2idle: Two fixes for wakeup-related issues
Subsystems (5)
cover ACPI: PM: s2idle: Two fixes for wakeup-related issues
Hi All,
Two problems with the handling of wakeup from suspend-to-idle on systems using ACPI and the EC driver (x86 PCs mostly) have been recently discovered, mainly by code inspection.
This series of two patches is intended to fix those problems. Please refer to the patch changelogs for details.
Thanks!
PATCH 1/2 ACPI: PM: s2idle: Cancel wakeup before dispatching EC GPE
From: Rafael J. Wysocki <[email protected]>
Commit 4a9af6cac050 ("ACPI: EC: Rework flushing of EC work while suspended to idle") made acpi_ec_dispatch_gpe() check pm_wakeup_pending(), but that is before canceling the SCI wakeup, so pm_wakeup_pending() is always true. This causes the loop in acpi_ec_dispatch_gpe() to always terminate after one iteration which may not be correct.
Address this issue by canceling the SCI wakeup earlier, from acpi_ec_dispatch_gpe() itself.
--- drivers/acpi/ec.c | 10 ++++++++++ drivers/acpi/sleep.c | 15 +++++---------- 2 files changed, 15 insertions(+), 10 deletions(-)
drivers/acpi/ec.c +10 -0
@@ -2066,6 +2066,16 @@ bool acpi_ec_dispatch_gpe(void)return true;/* +* Cancel the SCI wakeup and process all pending events in case there +* are any wakeup ones in there. +* +* Note that if any non-EC GPEs are active at this point, the SCI will +* retrigger after the rearming in acpi_s2idle_wake(), so no events +* should be missed by canceling the wakeup here. +*/ +pm_system_cancel_wakeup (); + +/* * Dispatch the EC GPE in-band, but do not report wakeup in any case * to allow the caller to process events properly after that. */
drivers/acpi/sleep.c +4 -10
@@ -736,21 +736,15 @@ bool acpi_s2idle_wake(void)return true; } -/* Check non-EC GPE wakeups and dispatch the EC GPE. */ +/* +* Check non-EC GPE wakeups and if there are none, cancel the +* SCI-related wakeup and dispatch the EC GPE. +*/ if (acpi_ec_dispatch_gpe ()) {pm_pr_dbg ("ACPI non-EC GPE wakeup\n" );return true; } -/* -* Cancel the SCI wakeup and process all pending events in case -* there are any wakeup ones in there. -* -* Note that if any non-EC GPEs are active at this point, the -* SCI will retrigger after the rearming below, so no events -* should be missed by canceling the wakeup here. -*/ -pm_system_cancel_wakeup ();acpi_os_wait_events_complete ();/*
PATCH 2/2 PM: s2idle: ACPI: Fix wakeup interrupts handling
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
After commit e3728b50cd9b ("ACPI: PM: s2idle: Avoid possible race related to the EC GPE") wakeup interrupts occurring immediately after the one discarded by acpi_s2idle_wake() may be missed. Moreover, if the SCI triggers again immediately after the rearming in acpi_s2idle_wake(), that wakeup may be missed too.
The problem is that pm_system_irq_wakeup() only calls pm_system_wakeup() when pm_wakeup_irq is 0, but that's not the case any more after the interrupt causing acpi_s2idle_wake() to run until pm_wakeup_irq is cleared by the pm_wakeup_clear() call in s2idle_loop(). However, there may be wakeup interrupts occurring in that time frame and if that happens, they will be missed.
To address that issue first move the clearing of pm_wakeup_irq to the point at which it is known that the interrupt causing acpi_s2idle_wake() to tun will be discarded, before rearming the SCI for wakeup. Moreover, because that only reduces the size of the time window in which the issue may manifest itself, allow pm_system_irq_wakeup() to register two second wakeup interrupts in a row and, when discarding the first one, replace it with the second one. [Of course, this assumes that only one wakeup interrupt can be discarded in one go, but currently that is the case and I am not aware of any plans to change that.]
--- drivers/acpi/sleep.c | 1 + drivers/base/power/wakeup.c | 41 ++++++++++++++++++++++++++++++++++------- include/linux/suspend.h | 4 ++-- kernel/power/main.c | 5 ++++- kernel/power/process.c | 2 +- kernel/power/suspend.c | 2 -- 6 files changed, 42 insertions(+), 13 deletions(-)
include/linux/suspend.h +2 -2
@@ -497,14 +497,14 @@ extern void ksys_sync_helper(void);/* drivers/base/power/wakeup.c */ extern bool events_check_enabled; -extern unsigned int pm_wakeup_irq;extern suspend_state_t pm_suspend_target_state;extern bool pm_wakeup_pending (void );extern void pm_system_wakeup (void );extern void pm_system_cancel_wakeup (void ); -extern void pm_wakeup_clear (bool reset); +extern void pm_wakeup_clear (unsigned int irq_number);extern void pm_system_irq_wakeup (unsigned int irq_number); +extern unsigned int pm_wakeup_irq (void );extern bool pm_get_wakeup_count (unsigned int *count,bool block);extern bool pm_save_wakeup_count (unsigned int count);extern void pm_wakep_autosleep_enabled (bool set);
kernel/power/process.c +1 -1
@@ -134,7 +134,7 @@ int freeze_processes(void)if (!pm_freezing)atomic_inc (&system_freezing_cnt); -pm_wakeup_clear (true); +pm_wakeup_clear (0 );pr_info ("Freezing user space processes ... " ); pm_freezing = true; error =try_to_freeze_tasks (true);
kernel/power/suspend.c +0 -2
@@ -136,8 +136,6 @@ static void s2idle_loop(void)break ; } -pm_wakeup_clear (false); -s2idle_enter (); }
drivers/base/power/wakeup.c +34 -7
60 lines not on this page · raw message
kernel/power/main.c +4 -1
@@ -504,7 +504,10 @@ static ssize_t pm_wakeup_irq_show(struct struct kobj_attribute *attr, char *buf) { - return pm_wakeup_irq ? sprintf(buf, "%u\n", pm_wakeup_irq) : -ENODATA; +if (!pm_wakeup_irq ()) +return -ENODATA ; + +return sprintf (buf,"%u\n" ,pm_wakeup_irq ()); }power_attr_ro (pm_wakeup_irq);
drivers/acpi/sleep.c +1 -0
@@ -760,6 +760,7 @@ bool acpi_s2idle_wake(void)pm_pr_dbg ("Rearming ACPI SCI for wakeup\n" ); +pm_wakeup_clear (acpi_sci_irq);rearm_wake_irq (acpi_sci_irq); }