ACPI: PM: s2idle: Avoid possible race related to the EC GPE

It is theoretically possible for the ACPI EC GPE to be set after the
s2idle_ops->wake() called from s2idle_loop() has returned and before
the subsequent pm_wakeup_pending() check is carried out.  If that
happens, the resulting wakeup event will cause the system to resume
even though it may be a spurious one.

To avoid that race, first make the ->wake() callback in struct
platform_s2idle_ops return a bool value indicating whether or not
to let the system resume and rearrange s2idle_loop() to use that
value instad of the direct pm_wakeup_pending() call if ->wake() is
present.

Next, rework acpi_s2idle_wake() to process EC events and check
pm_wakeup_pending() before re-arming the SCI for system wakeup
to prevent it from triggering prematurely and add comments to
that function to explain the rationale for the new code flow.

Fixes: 56b991849009 ("PM: sleep: Simplify suspend-to-idle control flow")
Cc: 5.4+ <stable@vger.kernel.org> # 5.4+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 4398806..2c695b1 100644 (file)
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -990,21 +990,28 @@ static void acpi_s2idle_sync(void)
        acpi_os_wait_events_complete(); /* synchronize Notify handling */
 }
 
-static void acpi_s2idle_wake(void)
+static bool acpi_s2idle_wake(void)
 {
-       /*
-        * If IRQD_WAKEUP_ARMED is set for the SCI at this point, the SCI has
-        * not triggered while suspended, so bail out.
-        */
-       if (!acpi_sci_irq_valid() ||
-           irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq)))
-               return;
+       if (!acpi_sci_irq_valid())
+               return pm_wakeup_pending();
+
+       while (pm_wakeup_pending()) {
+               /*
+                * If IRQD_WAKEUP_ARMED is set for the SCI at this point, the
+                * SCI has not triggered while suspended, so bail out (the
+                * wakeup is pending anyway and the SCI is not the source of
+                * it).
+                */
+               if (irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq)))
+                       return true;
+
+               /*
+                * If there are no EC events to process, the wakeup is regarded
+                * as a genuine one.
+                */
+               if (!acpi_ec_dispatch_gpe())
+                       return true;
 
-       /*
-        * If there are EC events to process, the wakeup may be a spurious one
-        * coming from the EC.
-        */
-       if (acpi_ec_dispatch_gpe()) {
                /*
                 * Cancel the wakeup and process all pending events in case
                 * there are any wakeup ones in there.
@@ -1017,8 +1024,19 @@ static void acpi_s2idle_wake(void)
 
                acpi_s2idle_sync();
 
+               /*
+                * The SCI is in the "suspended" state now and it cannot produce
+                * new wakeup events till the rearming below, so if any of them
+                * are pending here, they must be resulting from the processing
+                * of EC events above or coming from somewhere else.
+                */
+               if (pm_wakeup_pending())
+                       return true;
+
                rearm_wake_irq(acpi_sci_irq);
        }
+
+       return false;
 }
 
 static void acpi_s2idle_restore_early(void)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 4a230c2..2b2055b 100644 (file)
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -191,7 +191,7 @@ struct platform_s2idle_ops {
        int (*begin)(void);
        int (*prepare)(void);
        int (*prepare_late)(void);
-       void (*wake)(void);
+       bool (*wake)(void);
        void (*restore_early)(void);
        void (*restore)(void);
        void (*end)(void);

diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 2c47280..8b1bb5e 100644 (file)
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -131,11 +131,12 @@ static void s2idle_loop(void)
         * to avoid them upfront.
         */
        for (;;) {
-               if (s2idle_ops && s2idle_ops->wake)
-                       s2idle_ops->wake();
-
-               if (pm_wakeup_pending())
+               if (s2idle_ops && s2idle_ops->wake) {
+                       if (s2idle_ops->wake())
+                               break;
+               } else if (pm_wakeup_pending()) {
                        break;
+               }
 
                pm_wakeup_clear(false);