Skip to contentSkip to conversation

intel_idle patches ready for upstream

PATCH New Len Brown 1 participant 14 messages Apr 6, 2016

Subsystems (2)
x86 Intel idle driver
lore
Share & tools
lore

Len Brown

author reviewer
#0 lore raw

cover intel_idle patches ready for upstream

Bug fixes, plus add a few more table entires for new model #'s.

Some of the BXT table entries will be replaced by values read from processor configuration registers at boot-time.

Let me know if you see troubles with any of these patches.

They are all available on this git branch:

git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git cpuidle

thanks,
-Len Brown, Intel Open Source Technology Center

author reviewer
#1 lore raw

PATCH 1/13 intel_idle: add BXT support

From: Len Brown <[email protected]>

Broxton has all the HSW C-states, except C3.
BXT C-state timing is slightly different.

Here we trust the IRTL MSRs as authority
on maximum C-state latency, and override the driver's tables with the values found in the associated IRTL MSRs. Further we set the target_residency to 1x maximum latency, trusting the hardware demotion logic.

Signed-off-by Len Brown <[email protected]>
---
 arch/x86/include/asm/msr-index.h |   8 +++
 drivers/idle/intel_idle.c        | 137 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 145 insertions(+)
arch/x86/include/asm/msr-index.h +8 -0
index 2da46ac..1e81927 100644
@@ -167,6 +167,14 @@
 #define MSR_PKG_C9_RESIDENCY		0x00000631
 #define MSR_PKG_C10_RESIDENCY		0x00000632
 
+/* Interrupt Response Limit */
+#define MSR_PKGC3_IRTL			0x0000060a
+#define MSR_PKGC6_IRTL			0x0000060b
+#define MSR_PKGC7_IRTL			0x0000060c
+#define MSR_PKGC8_IRTL			0x00000633
+#define MSR_PKGC9_IRTL			0x00000634
+#define MSR_PKGC10_IRTL			0x00000635
+
 /* Run Time Average Power Limiting (RAPL) Interface */
 
 #define MSR_RAPL_POWER_UNIT		0x00000606
drivers/idle/intel_idle.c +137 -0

167 lines not on this page · raw message

Signature · 1 line
2.8.0.rc4.16.g56331f8
author reviewer
#2 lore raw

PATCH 2/13 intel_idle: remove useless return from void function.

In reply to Len Brown

From: Richard Cochran <[email protected]>

Signed-off-by Richard Cochran <[email protected]>
Signed-off-by Len Brown <[email protected]>
---
 drivers/idle/intel_idle.c | 2 --
 1 file changed, 2 deletions(-)
drivers/idle/intel_idle.c +0 -2
index 012980a..6638a80 100644
@@ -1397,8 +1397,6 @@ static void __exit intel_idle_exit(void)
 	__unregister_cpu_notifier(&cpu_hotplug_notifier);
 
 	cpu_notifier_register_done();
-
-	return;
 }
 
 module_init(intel_idle_init);
Signature · 1 line
2.8.0.rc4.16.g56331f8
author reviewer
#3 lore raw

PATCH 3/13 intel_idle: Fix a helper function's return value.

In reply to Len Brown

From: Richard Cochran <[email protected]>

The function, intel_idle_cpuidle_driver_init, delivers no error codes at all. This patch changes the function to return 'void' instead of returning zero.

Signed-off-by Richard Cochran <[email protected]>
Signed-off-by Len Brown <[email protected]>
---
 drivers/idle/intel_idle.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/idle/intel_idle.c +1 -3
index 6638a80..d2e7baf 100644
@@ -1248,7 +1248,7 @@ static void intel_idle_state_table_update(void)
  * intel_idle_cpuidle_driver_init()
  * allocate, initialize cpuidle_states
  */
-static int __init intel_idle_cpuidle_driver_init(void)
+static void __init intel_idle_cpuidle_driver_init(void)
 {
 	int cstate;
 	struct cpuidle_driver *drv = &intel_idle_driver;

@@ -1310,8 +1310,6 @@ static int __init intel_idle_cpuidle_driver_init(void)
 
 	if (icpu->disable_promotion_to_c1e)	/* each-cpu is redundant */
 		on_each_cpu(c1e_promotion_disable, NULL, 1);
-
-	return 0;
 }
Signature · 1 line
2.8.0.rc4.16.g56331f8
author reviewer
#4 lore raw

PATCH 4/13 intel_idle: Remove redundant initialization calls.

In reply to Len Brown

From: Richard Cochran <[email protected]>

The function, intel_idle_cpuidle_driver_init, makes calls on each CPU to auto_demotion_disable() and c1e_promotion_disable(). These calls are redundant, as intel_idle_cpu_init() does the same calls just a bit later on. They are also premature, as the driver registration may yet fail.

This patch removes the redundant code.

Signed-off-by Richard Cochran <[email protected]>
Signed-off-by Len Brown <[email protected]>
---
 drivers/idle/intel_idle.c | 6 ------
 1 file changed, 6 deletions(-)
drivers/idle/intel_idle.c +0 -6
index d2e7baf..c50859d 100644
@@ -1300,16 +1300,10 @@ static void __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);
 }
Signature · 1 line
2.8.0.rc4.16.g56331f8
author reviewer
#5 lore raw

PATCH 5/13 intel_idle: Fix deallocation order on the driver exit path.

In reply to Len Brown

From: Richard Cochran <[email protected]>

In the module_exit() method, this driver first frees its per-CPU pointer, then unregisters a callback making use of the pointer. Furthermore, the function, intel_idle_cpuidle_devices_uninit, is racy against CPU hot plugging as it calls for_each_online_cpu().

This patch corrects the issues by unregistering first on the exit path while holding the hot plug lock.

Signed-off-by Richard Cochran <[email protected]>
Signed-off-by Len Brown <[email protected]>
---
 drivers/idle/intel_idle.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/idle/intel_idle.c +3 -3
index c50859d..5b309e5 100644
@@ -1379,16 +1379,16 @@ static int __init intel_idle_init(void)
 
 static void __exit intel_idle_exit(void)
 {
-	intel_idle_cpuidle_devices_uninit();
-	cpuidle_unregister_driver(&intel_idle_driver);
-
 	cpu_notifier_register_begin();
 
 	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
 		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
 	__unregister_cpu_notifier(&cpu_hotplug_notifier);
+	intel_idle_cpuidle_devices_uninit();
 
 	cpu_notifier_register_done();
+
+	cpuidle_unregister_driver(&intel_idle_driver);
 }
 
 module_init(intel_idle_init);
Signature · 1 line
2.8.0.rc4.16.g56331f8
author reviewer
#6 lore raw

PATCH 6/13 intel_idle: Fix dangling registration on error path.

In reply to Len Brown

From: Richard Cochran <[email protected]>

In the module_init() method, if the per-CPU allocation fails, then the active cpuidle registration is not cleaned up. This patch fixes the issue by attempting the allocation before registration, and then cleaning it up again on registration failure.

Signed-off-by Richard Cochran <[email protected]>
Signed-off-by Len Brown <[email protected]>
---
 drivers/idle/intel_idle.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/idle/intel_idle.c +5 -4
index 5b309e5..085538a 100644
@@ -1347,19 +1347,20 @@ static int __init intel_idle_init(void)
 	if (retval)
 		return retval;
 
+	intel_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+	if (intel_idle_cpuidle_devices == NULL)
+		return -ENOMEM;
+
 	intel_idle_cpuidle_driver_init();
 	retval = cpuidle_register_driver(&intel_idle_driver);
 	if (retval) {
 		struct cpuidle_driver *drv = cpuidle_get_driver();
 		printk(KERN_DEBUG PREFIX "intel_idle yielding to %s",
 			drv ? drv->name : "none");
+		free_percpu(intel_idle_cpuidle_devices);
 		return retval;
 	}
 
-	intel_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device);
-	if (intel_idle_cpuidle_devices == NULL)
-		return -ENOMEM;
-
 	cpu_notifier_register_begin();
 
 	for_each_online_cpu(i) {
Signature · 1 line
2.8.0.rc4.16.g56331f8
author reviewer
#7 lore raw

PATCH 7/13 intel_idle: Avoid a double free of the per-CPU data.

In reply to Len Brown

From: Richard Cochran <[email protected]>

The helper function, intel_idle_cpuidle_devices_uninit, frees the globally allocated per-CPU data. However, this function is invoked from the hot plug notifier callback at a time when freeing that data is not safe.

If the call to cpuidle_register_driver() should fail (say, due to lack of memory), then the driver will free its per-CPU region. On the *next* CPU_ONLINE event, the driver will happily use the region again and even free it again if the failure repeats.

This patch fixes the issue by moving the call to free_percpu() outside of the helper function at the two call sites that actually need to free the per-CPU data.

Signed-off-by Richard Cochran <[email protected]>
Signed-off-by Len Brown <[email protected]>
---
 drivers/idle/intel_idle.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/idle/intel_idle.c +3 -4
index 085538a..04fe4df 100644
@@ -1069,7 +1069,7 @@ static int __init intel_idle_probe(void)
 
 /*
  * intel_idle_cpuidle_devices_uninit()
- * unregister, free cpuidle_devices
+ * Unregisters the cpuidle devices.
  */
 static void intel_idle_cpuidle_devices_uninit(void)
 {

@@ -1080,9 +1080,6 @@ static void intel_idle_cpuidle_devices_uninit(void)
 		dev = per_cpu_ptr(intel_idle_cpuidle_devices, i);
 		cpuidle_unregister_device(dev);
 	}
-
-	free_percpu(intel_idle_cpuidle_devices);
-	return;
 }
 
 /*

@@ -1368,6 +1365,7 @@ static int __init intel_idle_init(void)
 		if (retval) {
 			cpu_notifier_register_done();
 			cpuidle_unregister_driver(&intel_idle_driver);
+			free_percpu(intel_idle_cpuidle_devices);
 			return retval;
 		}
 	}

@@ -1390,6 +1388,7 @@ static void __exit intel_idle_exit(void)
 	cpu_notifier_register_done();
 
 	cpuidle_unregister_driver(&intel_idle_driver);
+	free_percpu(intel_idle_cpuidle_devices);
 }
 
 module_init(intel_idle_init);
Signature · 1 line
2.8.0.rc4.16.g56331f8
author reviewer
#8 lore raw

PATCH 8/13 intel_idle: Setup the timer broadcast only on successful driver load.

In reply to Len Brown

From: Richard Cochran <[email protected]>

This driver sets the broadcast tick quite early on during probe and does not clean up again in cast of failure. This patch moves the setup call after the registration, placing the on_each_cpu() calls within the global CPU lock region.

Signed-off-by Richard Cochran <[email protected]>
Signed-off-by Len Brown <[email protected]>
---
 drivers/idle/intel_idle.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
drivers/idle/intel_idle.c +8 -7
index 04fe4df..b5e92dd 100644
@@ -1054,16 +1054,9 @@ static int __init intel_idle_probe(void)
 	icpu = (const struct idle_cpu *)id->driver_data;
 	cpuidle_state_table = icpu->state_table;
 
-	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
-		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
-	else
-		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
-
 	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
 		" model 0x%X\n", boot_cpu_data.x86_model);
 
-	pr_debug(PREFIX "lapic_timer_reliable_states 0x%x\n",
-		lapic_timer_reliable_states);
 	return 0;
 }
 

@@ -1371,8 +1364,16 @@ static int __init intel_idle_init(void)
 	}
 	__register_cpu_notifier(&cpu_hotplug_notifier);
 
+	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
+		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
+	else
+		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
+
 	cpu_notifier_register_done();
 
+	pr_debug(PREFIX "lapic_timer_reliable_states 0x%x\n",
+		lapic_timer_reliable_states);
+
 	return 0;
 }
Signature · 1 line
2.8.0.rc4.16.g56331f8
author reviewer
#9 lore raw

PATCH 9/13 intel_idle: Don't overreact to a cpuidle registration failure.

In reply to Len Brown

From: Richard Cochran <[email protected]>

The helper function, intel_idle_cpu_init, registers one new device with the cpuidle layer. If the registration should fail, that function immediately calls intel_idle_cpuidle_devices_uninit() to unregister every last CPU's device. However, it makes no sense to do so, when called from the hot plug notifier callback.

This patch moves the call to intel_idle_cpuidle_devices_uninit() outside of the helper function to the one call site that actually needs to perform the de-registrations.

Signed-off-by Richard Cochran <[email protected]>
Signed-off-by Len Brown <[email protected]>
---
 drivers/idle/intel_idle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/idle/intel_idle.c +1 -1
index b5e92dd..807be29 100644
@@ -1312,7 +1312,6 @@ static int intel_idle_cpu_init(int cpu)
 
 	if (cpuidle_register_device(dev)) {
 		pr_debug(PREFIX "cpuidle_register_device %d failed!\n", cpu);
-		intel_idle_cpuidle_devices_uninit();
 		return -EIO;
 	}
 

@@ -1356,6 +1355,7 @@ static int __init intel_idle_init(void)
 	for_each_online_cpu(i) {
 		retval = intel_idle_cpu_init(i);
 		if (retval) {
+			intel_idle_cpuidle_devices_uninit();
 			cpu_notifier_register_done();
 			cpuidle_unregister_driver(&intel_idle_driver);
 			free_percpu(intel_idle_cpuidle_devices);
Signature · 1 line
2.8.0.rc4.16.g56331f8
author reviewer
#10 lore raw

PATCH 10/13 intel_idle: Propagate hot plug errors.

In reply to Len Brown

From: Richard Cochran <[email protected]>

If a cpuidle registration error occurs during the hot plug notifier callback, we should really inform the hot plug machinery instead of just ignoring the error. This patch changes the callback to properly return on error.

Signed-off-by Richard Cochran <[email protected]>
Signed-off-by Len Brown <[email protected]>
---
 drivers/idle/intel_idle.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/idle/intel_idle.c +5 -2
index 807be29..45e93f4 100644
@@ -879,8 +879,11 @@ static int cpu_hotplug_notify(struct notifier_block *n,
 		 * driver in this case
 		 */
 		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
-		if (!dev->registered)
-			intel_idle_cpu_init(hotcpu);
+		if (dev->registered)
+			break;
+
+		if (intel_idle_cpu_init(hotcpu))
+			return NOTIFY_BAD;
 
 		break;
 	}
Signature · 1 line
2.8.0.rc4.16.g56331f8
author reviewer
#11 lore raw

PATCH 11/13 intel_idle: Clean up all registered devices on exit.

In reply to Len Brown

From: Richard Cochran <[email protected]>

This driver registers cpuidle devices when a CPU comes online, but it leaves the registrations in place when a CPU goes offline. The module exit code only unregisters the currently online CPUs, leaving the devices for offline CPUs dangling.

This patch changes the driver to clean up all registrations on exit, even those from CPUs that are offline.

Signed-off-by Richard Cochran <[email protected]>
Signed-off-by Len Brown <[email protected]>
---
 drivers/idle/intel_idle.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/idle/intel_idle.c +8 -1
index 45e93f4..7575699 100644
@@ -1382,12 +1382,19 @@ static int __init intel_idle_init(void)
 
 static void __exit intel_idle_exit(void)
 {
+	struct cpuidle_device *dev;
+	int i;
+
 	cpu_notifier_register_begin();
 
 	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
 		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
 	__unregister_cpu_notifier(&cpu_hotplug_notifier);
-	intel_idle_cpuidle_devices_uninit();
+
+	for_each_possible_cpu(i) {
+		dev = per_cpu_ptr(intel_idle_cpuidle_devices, i);
+		cpuidle_unregister_device(dev);
+	}
 
 	cpu_notifier_register_done();
Signature · 1 line
2.8.0.rc4.16.g56331f8
author reviewer
#12 lore raw

PATCH 12/13 intel_idle: Add SKX support

In reply to Len Brown

From: Len Brown <[email protected]>

SKX is similar to BDX

Signed-off-by Len Brown <[email protected]>
---
 drivers/idle/intel_idle.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
drivers/idle/intel_idle.c +34 -0
index 7575699..d0ec343 100644
@@ -660,6 +660,35 @@ static struct cpuidle_state skl_cstates[] = {
 		.enter = NULL }
 };
 
+static struct cpuidle_state skx_cstates[] = {
+	{
+		.name = "C1-SKX",
+		.desc = "MWAIT 0x00",
+		.flags = MWAIT2flg(0x00),
+		.exit_latency = 2,
+		.target_residency = 2,
+		.enter = &intel_idle,
+		.enter_freeze = intel_idle_freeze, },
+	{
+		.name = "C1E-SKX",
+		.desc = "MWAIT 0x01",
+		.flags = MWAIT2flg(0x01),
+		.exit_latency = 10,
+		.target_residency = 20,
+		.enter = &intel_idle,
+		.enter_freeze = intel_idle_freeze, },
+	{
+		.name = "C6-SKX",
+		.desc = "MWAIT 0x20",
+		.flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
+		.exit_latency = 133,
+		.target_residency = 600,
+		.enter = &intel_idle,
+		.enter_freeze = intel_idle_freeze, },
+	{
+		.enter = NULL }
+};
+
 static struct cpuidle_state atom_cstates[] = {
 	{
 		.name = "C1E-ATM",

@@ -968,6 +997,10 @@ static const struct idle_cpu idle_cpu_skl = {
 	.disable_promotion_to_c1e = true,
 };
 
+static const struct idle_cpu idle_cpu_skx = {
+	.state_table = skx_cstates,
+	.disable_promotion_to_c1e = true,
+};
 
 static const struct idle_cpu idle_cpu_avn = {
 	.state_table = avn_cstates,

@@ -1014,6 +1047,7 @@ static const struct x86_cpu_id intel_idle_ids[] __initconst = {
 	ICPU(0x56, idle_cpu_bdw),
 	ICPU(0x4e, idle_cpu_skl),
 	ICPU(0x5e, idle_cpu_skl),
+	ICPU(0x55, idle_cpu_skx),
 	ICPU(0x57, idle_cpu_knl),
 	ICPU(0x5c, idle_cpu_bxt),
 	{}
Signature · 1 line
2.8.0.rc4.16.g56331f8
author reviewer
#13 lore raw

PATCH 13/13 intel_idle: Add KBL support

In reply to Len Brown

From: Len Brown <[email protected]>

KBL is similar to SKL

Signed-off-by Len Brown <[email protected]>
---
 drivers/idle/intel_idle.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/idle/intel_idle.c +2 -0
index d0ec343..c966492 100644
@@ -1047,6 +1047,8 @@ static const struct x86_cpu_id intel_idle_ids[] __initconst = {
 	ICPU(0x56, idle_cpu_bdw),
 	ICPU(0x4e, idle_cpu_skl),
 	ICPU(0x5e, idle_cpu_skl),
+	ICPU(0x8e, idle_cpu_skl),
+	ICPU(0x9e, idle_cpu_skl),
 	ICPU(0x55, idle_cpu_skx),
 	ICPU(0x57, idle_cpu_knl),
 	ICPU(0x5c, idle_cpu_bxt),
Signature · 1 line
2.8.0.rc4.16.g56331f8