C1E auto-promotion suspend/resume

Hi-

By BIOS (1.2.3 on a Dell XPS 13 9350) seems to want to enable C1E auto-promotion (ugh!), which results in this difference across suspend/resume according to turbostat:

-cpu3: MSR_IA32_POWER_CTL: 0x0024005d (C1E auto-promotion: DISabled) +cpu3: MSR_IA32_POWER_CTL: 0x0024005f (C1E auto-promotion: ENabled)

Should intel_idle learn to re-disable idle promotion on resume?

--Andy

Yes, it seems that way.

Go ahead and send a patch, or file a bug at bugzilla.kernel.org and we'll get to it.

thanks,
-len

I can't usefully test the fourth patch. I'm reasonably confident that the other three work, though, and I tested them on a laptop that doesn't preserve the C1E auto-promotion flag across suspend/resume.

Andy Lutomirski (4):
intel_idle: Consolidate auto-promotion/auto-demotion fixups
intel_idle: Remove a broadcast MSR fixup at boot
intel_idle: Fix MSRs after resume
intel_idle: Move BYT/CHT auto-demotion fixup into fix_this_cpu

 drivers/idle/intel_idle.c | 56 ++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

2.5.0

This eliminates some duplicate code and will make it easier add fixup calls in places where they're currently missing.

---
 drivers/idle/intel_idle.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

index cd4510a63375..32b3e6049994 100644
@@ -810,21 +810,21 @@ static struct notifier_block cpu_hotplug_notifier = {
 	.notifier_call = cpu_hotplug_notify,
 };
 
-static void auto_demotion_disable(void *dummy)
+static void fix_this_cpu(void *dummy)
 {
 	unsigned long long msr_bits;
 
-	rdmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
-	msr_bits &= ~(icpu->auto_demotion_disable_flags);
-	wrmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
-}
-static void c1e_promotion_disable(void *dummy)
-{
-	unsigned long long msr_bits;
+	if (icpu->auto_demotion_disable_flags) {
+		rdmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
+		msr_bits &= ~(icpu->auto_demotion_disable_flags);
+		wrmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
+	}
 
-	rdmsrl(MSR_IA32_POWER_CTL, msr_bits);
-	msr_bits &= ~0x2;
-	wrmsrl(MSR_IA32_POWER_CTL, msr_bits);
+	if (icpu->disable_promotion_to_c1e) {
+		rdmsrl(MSR_IA32_POWER_CTL, msr_bits);
+		msr_bits &= ~0x2;
+		wrmsrl(MSR_IA32_POWER_CTL, msr_bits);
+	}
 }
 
 static const struct idle_cpu idle_cpu_nehalem = {

@@ -1074,16 +1074,12 @@ static int __init intel_idle_cpuidle_driver_init(void)
 		drv->state_count += 1;
 	}
 
-	if (icpu->auto_demotion_disable_flags)
-		on_each_cpu(auto_demotion_disable, NULL, 1);
-
 	if (icpu->byt_auto_demotion_disable_flag) {
 		wrmsrl(MSR_CC6_DEMOTION_POLICY_CONFIG, 0);
 		wrmsrl(MSR_MC6_DEMOTION_POLICY_CONFIG, 0);
 	}
 
-	if (icpu->disable_promotion_to_c1e)	/* each-cpu is redundant */
-		on_each_cpu(c1e_promotion_disable, NULL, 1);
+	on_each_cpu(fix_this_cpu, NULL, 1);
 
 	return 0;
 }

@@ -1108,11 +1104,7 @@ static int intel_idle_cpu_init(int cpu)
 		return -EIO;
 	}
 
-	if (icpu->auto_demotion_disable_flags)
-		smp_call_function_single(cpu, auto_demotion_disable, NULL, 1);
-
-	if (icpu->disable_promotion_to_c1e)
-		smp_call_function_single(cpu, c1e_promotion_disable, NULL, 1);
+	smp_call_function_single(cpu, fix_this_cpu, NULL, 1);
 
 	return 0;
 }

2.5.0

intel_idle already fixes MSRs on each CPU when it registers for that CPU, so it doesn't need to separately broadcast to all CPUs on startup.

---
 drivers/idle/intel_idle.c | 2 --
 1 file changed, 2 deletions(-)

index 32b3e6049994..338df09ad60b 100644
@@ -1079,8 +1079,6 @@ static int __init intel_idle_cpuidle_driver_init(void)
 		wrmsrl(MSR_MC6_DEMOTION_POLICY_CONFIG, 0);
 	}
 
-	on_each_cpu(fix_this_cpu, NULL, 1);
-
 	return 0;
 }
 

2.5.0

Firmware that enables auto-promotion / auto-demotion flags we don't like will probably re-enable them after suspend/resume. Disable them again after resume so they stay fixed.

I've seen this on my Dell XPS 13 9350.

---
 drivers/idle/intel_idle.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

index 338df09ad60b..e3d7d8bbc843 100644
@@ -61,6 +61,7 @@
 #include <linux/notifier.h>
 #include <linux/cpu.h>
 #include <linux/module.h>
+#include <linux/syscore_ops.h>
 #include <asm/cpu_device_id.h>
 #include <asm/mwait.h>
 #include <asm/msr.h>

@@ -1026,6 +1027,15 @@ void intel_idle_state_table_update(void)
 	return;
 }
 
+static void intel_idle_resume(void)
+{
+	on_each_cpu(fix_this_cpu, NULL, 1);
+}
+
+static struct syscore_ops intel_idle_syscore_ops = {
+	.resume = intel_idle_resume,
+};
+
 /*
  * intel_idle_cpuidle_driver_init()
  * allocate, initialize cpuidle_states

@@ -1119,6 +1129,7 @@ static int __init intel_idle_init(void)
 	if (retval)
 		return retval;
 
+	register_syscore_ops(&intel_idle_syscore_ops);
 	intel_idle_cpuidle_driver_init();
 	retval = cpuidle_register_driver(&intel_idle_driver);
 	if (retval) {

@@ -1153,6 +1164,7 @@ static void __exit intel_idle_exit(void)
 {
 	intel_idle_cpuidle_devices_uninit();
 	cpuidle_unregister_driver(&intel_idle_driver);
+	unregister_syscore_ops(&intel_idle_syscore_ops);
 
 	cpu_notifier_register_begin();
 

2.5.0

This isn't right -- syscore's resume hook is called too early for smp_call_function_many. I think the right way is through a device's dev_pm_ops (SET_SYSTEM_SLEEP_PM_OPS), but there don't seem to be any devices associated with intel_idle.

--Andy

Andy Lutomirski
AMA Capital Management, LLC

The demotion policy config registers are per-package and these CPUs only exist in single-package variants AFAIK, so it's not stricly necessary to fix these MSRs on each core, but fixing them on resume is still likely to be helpful, so move them into fix_this_cpu.

---
 drivers/idle/intel_idle.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index e3d7d8bbc843..cbb02d28dc48 100644
@@ -826,6 +826,11 @@ static void fix_this_cpu(void *dummy)
 		msr_bits &= ~0x2;
 		wrmsrl(MSR_IA32_POWER_CTL, msr_bits);
 	}
+
+	if (icpu->byt_auto_demotion_disable_flag) {
+		wrmsrl(MSR_CC6_DEMOTION_POLICY_CONFIG, 0);
+		wrmsrl(MSR_MC6_DEMOTION_POLICY_CONFIG, 0);
+	}
 }
 
 static const struct idle_cpu idle_cpu_nehalem = {

@@ -1084,11 +1089,6 @@ static int __init intel_idle_cpuidle_driver_init(void)
 		drv->state_count += 1;
 	}
 
-	if (icpu->byt_auto_demotion_disable_flag) {
-		wrmsrl(MSR_CC6_DEMOTION_POLICY_CONFIG, 0);
-		wrmsrl(MSR_MC6_DEMOTION_POLICY_CONFIG, 0);
-	}
-
 	return 0;
 }
 

2.5.0

Sent. The only other differences I see across suspend/resume in turbostat --debug (with taskset -c 0 to suppress spurious junk) are:

@@ -24,8 +24,8 @@
 cpu0: MSR_NHM_SNB_PKG_CST_CFG_CTL: 0x1e008006 (UNdemote-C3,

UNdemote-C1, demote-C3, demote-C1, locked: pkg-cstate-limit=6: pc8)
cpu0: MSR_PM_ENABLE: 0x00000001 (HWP)
cpu0: MSR_HWP_CAPABILITIES: 0x0108171c (high 0x1c guar 0x17 eff 0x8 low 0x1)
-cpu0: MSR_HWP_REQUEST: 0x80001c04 (min 0x4 max 0x1c des 0x0 epp 0x80 window 0x0 pkg 0x0)
-cpu0: MSR_HWP_INTERRUPT: 0x00000001 (EN_Guaranteed_Perf_Change, Dis_Excursion_Min)
+cpu0: MSR_HWP_REQUEST: 0x8000ff01 (min 0x1 max 0xff des 0x0 epp 0x80 window 0x0 pkg 0x0)
+cpu0: MSR_HWP_INTERRUPT: 0x00000000 (Dis_Guaranteed_Perf_Change, Dis_Excursion_Min)
cpu0: MSR_HWP_STATUS: 0x00000000 (No-Guaranteed_Perf_Change, No-Excursion_Min)
cpu0: MSR_IA32_ENERGY_PERF_BIAS: 0x00000006 (balanced)
cpu0: MSR_RAPL_POWER_UNIT: 0x000a0e03 (0.125000 Watts, 0.000061
Joules, 0.000977 sec.)
@@ -36,6 +36,6 @@
cpu0: MSR_DRAM_POWER_LIMIT: 0x5400de00000000 (UNlocked)
cpu0: DRAM Limit: DISabled (0.000000 Watts, 0.000977 sec, clamp DISabled)
cpu0: MSR_IA32_TEMPERATURE_TARGET: 0x00640000 (100 C)
-cpu0: MSR_IA32_PACKAGE_THERM_STATUS: 0x88360000 (46 C)
-cpu0: MSR_IA32_THERM_STATUS: 0x88360000 (46 C +/- 1)
-cpu1: MSR_IA32_THERM_STATUS: 0x88360000 (46 C +/- 1)
+cpu0: MSR_IA32_PACKAGE_THERM_STATUS: 0x883c0000 (40 C)
+cpu0: MSR_IA32_THERM_STATUS: 0x883c0000 (40 C +/- 1)
+cpu1: MSR_IA32_THERM_STATUS: 0x88400000 (36 C +/- 1)

Are either of those potentially interesting?