Fix IPI missing issue when woken from suspend to idle

Suspend to idle was found to not work on Goldmont CPU recently. And the issue was triggered due to:

1. On Goldmont the CPU in idle can only be woken up via IPIs, not POLLING mode:
Commit 08e237fa56a1 ("x86/cpu: Add workaround for MONITOR instruction erratum on Goldmont based CPUs")
2. When the CPU is entering suspend to idle process, the _TIF_POLLING_NRFLAG is kept on, due to cpuidle_enter_s2idle() doesn't properly match call_cpuidle().
3. Commit b2a02fc43a1f ("smp: Optimize send_call_function_single_ipi()") makes use of _TIF_POLLING_NRFLAG to avoid sending IPIs to idle CPUs.
4. As a result, some IPIs related functions might not work well during suspend to idle on Goldmont. For example, one suspected victim:
tick_unfreeze() -> timekeeping_resume() -> hrtimers_resume() -> clock_was_set() -> on_each_cpu() might wait forever, because the IPIs will not be sent to the CPUs which are sleeping with _TIF_POLLING_NRFLAG set, and Goldmont CPU could not be woken up by only setting _TIF_NEED_RESCHED on the monitor address.

Clear the _TIF_POLLING_NRFLAG flag before entering suspend to idle, and let the driver's enter_s2idle() to decide whether to set _TIF_POLLING_NRFLAG or not. So that to avoid the scenario described above and keep the context consistent with before.

Fixes b2a02fc43a1f ("smp: Optimize send_call_function_single_ipi()")

Reported-by kbuild test robot <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Peter Zijlstra (Intel) <[email protected]>

Cc Len Brown <[email protected]>

Signed-off-by Chen Yu <[email protected]>

---
v2: According to Peter's review, v1 is racy, if someone already
set TIF_NEED_RESCHED this patch just clear POLLING and go to sleep. Check TIF_NEED_RESCHED before entering suspend to idle and adjust the naming to be consistent with call_cpuidle().

v3: According to Rafael, it would be better to do the simplest fix
first and then do the cleanup on top of it.

---
 drivers/cpuidle/cpuidle.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/cpuidle/cpuidle.c +2 -2

index c149d9e20dfd..e092789187c6 100644
@@ -13,6 +13,7 @@
 #include <linux/mutex.h>
 #include <linux/sched.h>
 #include <linux/sched/clock.h>
+#include <linux/sched/idle.h>
 #include <linux/notifier.h>
 #include <linux/pm_qos.h>
 #include <linux/cpu.h>

@@ -186,7 +187,7 @@ int cpuidle_enter_s2idle(struct cpuidle_driver *drv, struct cpuidle_device *dev)
         * be frozen safely.
         */
        index = find_deepest_state(drv, dev, U64_MAX, 0, true);
-       if (index > 0)
+       if (index > 0 && !current_clr_polling_and_test())
                enter_s2idle_proper(drv, dev, index);
 
        return index;
--

2.17.1

Currently s2idle is a little different from the normal idle path. This patch makes call_s2idle() consistent with call_cpuidle(), and also s2idle_enter() is analogous to cpuidle_enter().

No functional change.

Suggested-by Peter Zijlstra (Intel) <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Len Brown <[email protected]>

Cc Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Chen Yu <[email protected]>

---
 drivers/cpuidle/cpuidle.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

drivers/cpuidle/cpuidle.c +2 -2

index e092789187c6..b2e764d1ac99 100644
@@ -134,8 +134,8 @@ int cpuidle_find_deepest_state(struct cpuidle_driver *drv,
 }
 
 #ifdef CONFIG_SUSPEND
-static void enter_s2idle_proper(struct cpuidle_driver *drv,
-				struct cpuidle_device *dev, int index)
+static void s2idle_enter(struct cpuidle_driver *drv,
+			 struct cpuidle_device *dev, int index)
 {
 	ktime_t time_start, time_end;
 

drivers/cpuidle/cpuidle.c +3 -0

+	return index;
+}
+
 /**
  * cpuidle_enter_s2idle - Enter an idle state suitable for suspend-to-idle.
  * @drv: cpuidle driver for the given CPU.

From: Rafael J. Wysocki <[email protected]>
Subject: [PATCH] cpuidle: Rearrange s2idle-specific idle state entry code

Implement call_cpuidle_s2idle() in analogy with call_cpuidle() for the s2idle-specific idle state entry and invoke it from cpuidle_idle_call() to make the s2idle-specific idle entry code path look more similar to the "regular" idle entry one.

No intentional functional impact.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/cpuidle/cpuidle.c |    6 +++---
 kernel/sched/idle.c       |   15 +++++++++++----
 2 files changed, 14 insertions(+), 7 deletions(-)

Index: linux-pm/kernel/sched/idle.c
===================================================================

kernel/sched/idle.c +9 -0

@@ -96,6 +96,15 @@ void __cpuidle default_idle_call(void)
 	}
 }
 
+static int call_cpuidle_s2idle(struct cpuidle_driver *drv,
+			       struct cpuidle_device *dev)
+{
+	if (current_clr_polling_and_test())
+		return -EBUSY;
+
+	return cpuidle_enter_s2idle(drv, dev);
+}
+
 static int call_cpuidle(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 		      int next_state)
 {

Hi Rafael,

Quoted 40 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Subject: [PATCH] cpuidle: Rearrange s2idle-specific idle state entry code

Implement call_cpuidle_s2idle() in analogy with call_cpuidle() for the s2idle-specific idle state entry and invoke it from cpuidle_idle_call() to make the s2idle-specific idle entry code path look more similar to the "regular" idle entry one.

No intentional functional impact.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/cpuidle/cpuidle.c |    6 +++---
 kernel/sched/idle.c       |   15 +++++++++++----
 2 files changed, 14 insertions(+), 7 deletions(-)

Index: linux-pm/kernel/sched/idle.c
===================================================================

kernel/sched/idle.c +9 -0

@@ -96,6 +96,15 @@ void __cpuidle default_idle_call(void)
      }
 }
 
+static int call_cpuidle_s2idle(struct cpuidle_driver *drv,
+                            struct cpuidle_device *dev)
+{
+     if (current_clr_polling_and_test())
+             return -EBUSY;
+
+     return cpuidle_enter_s2idle(drv, dev);
+}
+
 static int call_cpuidle(struct cpuidle_driver *drv, struct cpuidle_device *dev,
                    int next_state)
 {

@@ -171,11 +180,9 @@ static void cpuidle_idle_call(void)
              if (idle_should_enter_s2idle()) {
                      rcu_idle_enter();
 

kernel/sched/idle.c static void cpuidle_idle_call(void) · line 183

Quoted 4 lines

kernel/sched/idle.c +1 -3

-                     entered_state = cpuidle_enter_s2idle(drv, dev);
-                     if (entered_state > 0) {
-                             local_irq_enable();
+                     entered_state = call_cpuidle_s2idle(drv, dev);

I guess this changes the context a little bit that(comparing to [1/2 patch], after this modification, when we found that TIF_NEED_RESCHED is set we can have a second chance in the following call_cpuidle to do a second s2idle try. However in [1/2 patch], it might exit the s2idle phase directly once when we see TIF_NEED_RESCHED is set(because entered_state is postive we treat it as a successful s2idle). In summary I think the change (patch [2/2]) is more robust.

Acked-by Chen Yu <[email protected]>