[PATCH 00/10] intel_idle: Fix hot plug handling.

[PATCH 00/10] intel_idle: Fix hot plug handling.

[Richard Cochran]

This driver has one serious and one mild bug in its hot plug handling.

First, whenever a new CPU goes on line, if the call to
cpuidle_register_driver() should fail (say, due to lack of memory),
then the driver frees 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.

Second, for each new on line CPU, a device is registered with the
cpuidle layer.  However, when a CPU goes down, its device is never
unregistered, even if the module exits.

Although this driver may not (yet?) be a built as a module, still this
patch series cleans up the exit path in order to make the resource
allocations clear.


Richard Cochran (10):
  intel_idle: remove useless return from void function.
  intel_idle: Fix a helper function's return value.
  intel_idle: Remove redundant initialization calls.
  intel_idle: Fix deallocation order on the driver exit path.
  intel_idle: Fix dangling registration on error path.
  intel_idle: Avoid a double free of the per-CPU data.
  intel_idle: Setup the timer broadcast only on successful driver load.
  intel_idle: Don't overreact to a cpuidle registration failure.
  intel_idle: Propagate hot plug errors.
  intel_idle: Clean up all registered devices on exit.

 drivers/idle/intel_idle.c | 61 ++++++++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 30 deletions(-)

-- 
2.1.4

[PATCH 01/10] intel_idle: remove useless return from void function.

[Richard Cochran]

Cc: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Richard Cochran <rcochran@linutronix.de>
---
 drivers/idle/intel_idle.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index ba947df..9d5ed32 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -1260,8 +1260,6 @@ static void __exit intel_idle_exit(void)
 	__unregister_cpu_notifier(&cpu_hotplug_notifier);
 
 	cpu_notifier_register_done();
-
-	return;
 }
 
 module_init(intel_idle_init);
-- 
2.1.4

[PATCH 02/10] intel_idle: Fix a helper function's return value.

[Richard Cochran]

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.

Cc: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Richard Cochran <rcochran@linutronix.de>
---
 drivers/idle/intel_idle.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 9d5ed32..dfa9055 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -1111,7 +1111,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;
@@ -1173,8 +1173,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;
 }
 
 
-- 
2.1.4

[PATCH 03/10] intel_idle: Remove redundant initialization calls.

[Richard Cochran]

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.

Cc: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Richard Cochran <rcochran@linutronix.de>
---
 drivers/idle/intel_idle.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index dfa9055..cb85c4c 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -1163,16 +1163,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);
 }
 
 
-- 
2.1.4

[PATCH 04/10] intel_idle: Fix deallocation order on the driver exit path.

[Richard Cochran]

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.

Cc: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Richard Cochran <rcochran@linutronix.de>
---
 drivers/idle/intel_idle.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index cb85c4c..4a1de3d 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -1242,16 +1242,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);
-- 
2.1.4

[PATCH 05/10] intel_idle: Fix dangling registration on error path.

[Richard Cochran]

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.

Cc: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Richard Cochran <rcochran@linutronix.de>
---
 drivers/idle/intel_idle.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 4a1de3d..5dd741f 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -1210,19 +1210,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) {
-- 
2.1.4

[PATCH 06/10] intel_idle: Avoid a double free of the per-CPU data.

[Richard Cochran]

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.

Cc: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Richard Cochran <rcochran@linutronix.de>
---
 drivers/idle/intel_idle.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 5dd741f..0b56872 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -1002,7 +1002,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)
 {
@@ -1013,9 +1013,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;
 }
 
 /*
@@ -1231,6 +1228,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;
 		}
 	}
@@ -1253,6 +1251,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);
-- 
2.1.4

[PATCH 07/10] intel_idle: Setup the timer broadcast only on successful driver load.

[Richard Cochran]

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.

Cc: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Richard Cochran <rcochran@linutronix.de>
---
 drivers/idle/intel_idle.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 0b56872..ab34cd8 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -987,16 +987,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;
 }
 
@@ -1234,8 +1227,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;
 }
 
-- 
2.1.4

[PATCH 08/10] intel_idle: Don't overreact to a cpuidle registration failure.

[Richard Cochran]

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.

Cc: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Richard Cochran <rcochran@linutronix.de>
---
 drivers/idle/intel_idle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index ab34cd8..4418cfa 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -1175,7 +1175,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;
 	}
 
@@ -1219,6 +1218,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);
-- 
2.1.4

[PATCH 09/10] intel_idle: Propagate hot plug errors.

[Richard Cochran]

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.

Cc: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Richard Cochran <rcochran@linutronix.de>
---
 drivers/idle/intel_idle.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 4418cfa..8420ba1 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -818,8 +818,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;
 	}
-- 
2.1.4

[PATCH 10/10] intel_idle: Clean up all registered devices on exit.

[Richard Cochran]

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.

Cc: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Richard Cochran <rcochran@linutronix.de>
---
 drivers/idle/intel_idle.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 8420ba1..862346b 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -1245,12 +1245,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();
 
-- 
2.1.4

Re: [PATCH 10/10] intel_idle: Clean up all registered devices on exit.

[Len Brown]

1-10 Applied.

Thanks Richard!

Len Brown, Intel Open Source Technology Center