linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [patch V2 01/12] thermal/x86_pkg_temp: Cleanup thermal interrupt handling
  2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
@ 2016-11-22 17:57 ` Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 02/12] thermal/x86_pkg_temp: Remove redundant package search Thomas Gleixner
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2016-11-22 17:57 UTC (permalink / raw)
  To: LKML
  Cc: Rui Zhang, edubezval, Peter Zijlstra, Borislav Petkov, linux-pm,
	x86, rt, Srinivas Pandruvada

[-- Attachment #1: thermalx86_pkg_temp_Cleanup_thermal_interrupt_handling.patch --]
[-- Type: text/plain, Size: 1617 bytes --]

Wenn a package is removed nothing restores the thermal interrupt MSR so
the content will be stale when a CPU of that package becomes online again.

Aside of that the work function reenables interrupts before acknowledging
the current one, which is the wrong order to begin with.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/thermal/x86_pkg_temp_thermal.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -334,7 +334,6 @@ static void pkg_temp_thermal_threshold_w
 	pkg_work_scheduled[phy_id] = 0;
 	spin_unlock_irqrestore(&pkg_work_lock, flags);
 
-	enable_pkg_thres_interrupt();
 	rdmsrl(MSR_IA32_PACKAGE_THERM_STATUS, msr_val);
 	if (msr_val & THERM_LOG_THRESHOLD0) {
 		wrmsrl(MSR_IA32_PACKAGE_THERM_STATUS,
@@ -346,6 +345,9 @@ static void pkg_temp_thermal_threshold_w
 				msr_val & ~THERM_LOG_THRESHOLD1);
 		notify = true;
 	}
+
+	enable_pkg_thres_interrupt();
+
 	if (notify) {
 		pr_debug("thermal_zone_device_update\n");
 		thermal_zone_device_update(phdev->tzone,
@@ -505,6 +507,13 @@ static int pkg_temp_thermal_device_remov
 		list_for_each_entry_safe(phdev, n, &phy_dev_list, list) {
 			if (phdev->phys_proc_id == phys_proc_id) {
 				thermal_zone_device_unregister(phdev->tzone);
+				/*
+				 * Restore original MSR value for package
+				 * thermal interrupt.
+				 */
+				wrmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
+					     phdev->start_pkg_therm_low,
+					     phdev->start_pkg_therm_high);
 				list_del(&phdev->list);
 				kfree(phdev);
 				break;

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking
@ 2016-11-22 17:57 Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 01/12] thermal/x86_pkg_temp: Cleanup thermal interrupt handling Thomas Gleixner
                   ` (13 more replies)
  0 siblings, 14 replies; 16+ messages in thread
From: Thomas Gleixner @ 2016-11-22 17:57 UTC (permalink / raw)
  To: LKML
  Cc: Rui Zhang, edubezval, Peter Zijlstra, Borislav Petkov, linux-pm,
	x86, rt, Srinivas Pandruvada

Changes vs. V1: Fix the package removal wreckage reported by Srinivas

We solely intended to convert that driver to the hotplug state machine and
stumbled over a large pile of insanities, which are all interwoven with the
package management:

 - The work cancelation code, the thermal zone unregistering, the work code
   and the interrupt notification function are racy against each other and
   against cpu hotplug and module exit. The random locking sprinkeled all
   over the place does not help anything and probably exists to make people
   feel good. The resulting issues (mainly use after free) are probably
   hard to trigger, but they clearly exist

 - Work cancelation in the cpu hotplug code can leave the work marked
   scheduled and the package interrupts disabled forever.

 - Storage for a boolean information whether work is scheduled for a
   package is kept in separate allocated storage, which is resized when the
   number of detected packages grows.

 - Delayed work structs are held in a static percpu storage, which makes no
   sense at all because work is strictly per package.

 - Packages are kept in a list, which must be searched over and over.

Fixing the whole pile of races with a few duct tape fixes was pretty much
impossible, so I decided to do a major rewrite to fix all of the
above. Here are the major changes:

 - Rewrite the offline code with proper locking against interrupts and work
   function and make sure that canceled work is rescheduled if there is
   another online cpu in the package.

 - Use the cpu offline callback on module exit to fix the work cancelation
   race.

 - Move the bool which denotes scheduled work into the package struct
   where it belongs.

 - Move the delayed work struct into the package struct, which is the only
   sensible place to have it and schedule the work on the cpu which is the
   target for the sysfs files as this makes the cancellation and
   rescheduling in the cpu offline path simple.

 - Add a large pile of comments documenting the cpu teardown mechanism

 - Code sanitizing, revamp the horrible name choices plus a general coding
   style cleanup.

   Note, that I did the namespace and code cleanup in the middle of the
   series, because staring at that mess just made my eyes bleeding.

 - Store the package pointers in an array which is allocated at init
   time. Sizing of the array is determined from the topology
   information. That makes the package lookup a simple array lookup.

As a last step the driver is converted to the hotplug state machine.

Thanks,
	
	tglx
---
 x86_pkg_temp_thermal.c |  593 ++++++++++++++++++++-----------------------------
 1 file changed, 249 insertions(+), 344 deletions(-)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch V2 02/12] thermal/x86_pkg_temp: Remove redundant package search
  2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 01/12] thermal/x86_pkg_temp: Cleanup thermal interrupt handling Thomas Gleixner
@ 2016-11-22 17:57 ` Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 04/12] thermal/x86_pkg_temp: Sanitize callback (de)initialization Thomas Gleixner
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2016-11-22 17:57 UTC (permalink / raw)
  To: LKML
  Cc: Rui Zhang, edubezval, Peter Zijlstra, Borislav Petkov, linux-pm,
	x86, rt, Srinivas Pandruvada

[-- Attachment #1: thermalx86_pkg_temp_Remove_redundant_package_search.patch --]
[-- Type: text/plain, Size: 2024 bytes --]

In pkg_temp_thermal_device_remove() the package device is searched at the
beginning of the function. When the device refcount becomes zero another
search for the same device is conducted. Remove the pointless loop and use
the device pointer which was retrieved at the beginning of the function.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/thermal/x86_pkg_temp_thermal.c |   33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -479,10 +479,8 @@ static int pkg_temp_thermal_device_add(u
 
 static int pkg_temp_thermal_device_remove(unsigned int cpu)
 {
-	struct phy_dev_entry *n;
+	struct phy_dev_entry *phdev = pkg_temp_thermal_get_phy_entry(cpu);
 	u16 phys_proc_id = topology_physical_package_id(cpu);
-	struct phy_dev_entry *phdev =
-			pkg_temp_thermal_get_phy_entry(cpu);
 
 	if (!phdev)
 		return -ENODEV;
@@ -503,22 +501,19 @@ static int pkg_temp_thermal_device_remov
 	--phdev->ref_cnt;
 	pr_debug("thermal_device_remove: pkg: %d cpu %d ref_cnt %d\n",
 					phys_proc_id, cpu, phdev->ref_cnt);
-	if (!phdev->ref_cnt)
-		list_for_each_entry_safe(phdev, n, &phy_dev_list, list) {
-			if (phdev->phys_proc_id == phys_proc_id) {
-				thermal_zone_device_unregister(phdev->tzone);
-				/*
-				 * Restore original MSR value for package
-				 * thermal interrupt.
-				 */
-				wrmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
-					     phdev->start_pkg_therm_low,
-					     phdev->start_pkg_therm_high);
-				list_del(&phdev->list);
-				kfree(phdev);
-				break;
-			}
-		}
+
+	if (!phdev->ref_cnt) {
+		thermal_zone_device_unregister(phdev->tzone);
+		/*
+		 * Restore original MSR value for package thermal
+		 * interrupt.
+		 */
+		wrmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
+			     phdev->start_pkg_therm_low,
+			     phdev->start_pkg_therm_high);
+		list_del(&phdev->list);
+		kfree(phdev);
+	}
 	mutex_unlock(&phy_dev_list_mutex);
 
 	return 0;

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch V2 04/12] thermal/x86_pkg_temp: Sanitize callback (de)initialization
  2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 01/12] thermal/x86_pkg_temp: Cleanup thermal interrupt handling Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 02/12] thermal/x86_pkg_temp: Remove redundant package search Thomas Gleixner
@ 2016-11-22 17:57 ` Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 03/12] thermal/x86_pkg_temp: Replace open coded cpu search Thomas Gleixner
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2016-11-22 17:57 UTC (permalink / raw)
  To: LKML
  Cc: Rui Zhang, edubezval, Peter Zijlstra, Borislav Petkov, linux-pm,
	x86, rt, Srinivas Pandruvada

[-- Attachment #1: thermalx86_pkg_temp_Sanitize_callback_initialization.patch --]
[-- Type: text/plain, Size: 2721 bytes --]

The threshold callbacks are installed before the initialization of the
online cpus has succeeded and removed after the teardown has been
done. That's both wrong as callbacks might be invoked into a half
initialized or torn down state.

Move them to the proper places: Last in init() and first in exit().

While at it shorten the insane long and horrible named function names.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/thermal/x86_pkg_temp_thermal.c |   19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -281,7 +281,7 @@ static struct thermal_zone_device_ops tz
 	.set_trip_temp = sys_set_trip_temp,
 };
 
-static bool pkg_temp_thermal_platform_thermal_rate_control(void)
+static bool pkg_thermal_rate_control(void)
 {
 	return true;
 }
@@ -355,7 +355,7 @@ static void pkg_temp_thermal_threshold_w
 	}
 }
 
-static int pkg_temp_thermal_platform_thermal_notify(__u64 msr_val)
+static int pkg_thermal_notify(__u64 msr_val)
 {
 	unsigned long flags;
 	int cpu = smp_processor_id();
@@ -579,10 +579,6 @@ static int __init pkg_temp_thermal_init(
 		return -ENODEV;
 
 	spin_lock_init(&pkg_work_lock);
-	platform_thermal_package_notify =
-			pkg_temp_thermal_platform_thermal_notify;
-	platform_thermal_package_rate_control =
-			pkg_temp_thermal_platform_thermal_rate_control;
 
 	cpu_notifier_register_begin();
 	for_each_online_cpu(i)
@@ -591,6 +587,9 @@ static int __init pkg_temp_thermal_init(
 	__register_hotcpu_notifier(&pkg_temp_thermal_notifier);
 	cpu_notifier_register_done();
 
+	platform_thermal_package_notify = pkg_thermal_notify;
+	platform_thermal_package_rate_control = pkg_thermal_rate_control;
+
 	pkg_temp_debugfs_init(); /* Don't care if fails */
 
 	return 0;
@@ -600,9 +599,6 @@ static int __init pkg_temp_thermal_init(
 		put_core_offline(i);
 	cpu_notifier_register_done();
 	kfree(pkg_work_scheduled);
-	platform_thermal_package_notify = NULL;
-	platform_thermal_package_rate_control = NULL;
-
 	return -ENODEV;
 }
 
@@ -611,6 +607,9 @@ static void __exit pkg_temp_thermal_exit
 	struct phy_dev_entry *phdev, *n;
 	int i;
 
+	platform_thermal_package_notify = NULL;
+	platform_thermal_package_rate_control = NULL;
+
 	cpu_notifier_register_begin();
 	__unregister_hotcpu_notifier(&pkg_temp_thermal_notifier);
 	mutex_lock(&phy_dev_list_mutex);
@@ -625,8 +624,6 @@ static void __exit pkg_temp_thermal_exit
 		kfree(phdev);
 	}
 	mutex_unlock(&phy_dev_list_mutex);
-	platform_thermal_package_notify = NULL;
-	platform_thermal_package_rate_control = NULL;
 	for_each_online_cpu(i)
 		cancel_delayed_work_sync(
 			&per_cpu(pkg_temp_thermal_threshold_work, i));

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch V2 03/12] thermal/x86_pkg_temp: Replace open coded cpu search
  2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
                   ` (2 preceding siblings ...)
  2016-11-22 17:57 ` [patch V2 04/12] thermal/x86_pkg_temp: Sanitize callback (de)initialization Thomas Gleixner
@ 2016-11-22 17:57 ` Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 05/12] thermal/x86_pkg_temp: Get rid of ref counting Thomas Gleixner
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2016-11-22 17:57 UTC (permalink / raw)
  To: LKML
  Cc: Rui Zhang, edubezval, Peter Zijlstra, Borislav Petkov, linux-pm,
	x86, rt, Srinivas Pandruvada

[-- Attachment #1: thermalx86_pkg_temp_Replace_open_coded_cpu_search.patch --]
[-- Type: text/plain, Size: 1445 bytes --]

find_next_sibling() iterates over the online cpus and searches for a cpu
with the same package id as the current cpu. This is a pointless exercise
as topology_core_cpumask() allows a simple cpumask search for an online cpu
on the same package.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/thermal/x86_pkg_temp_thermal.c |   19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -384,18 +384,6 @@ static int pkg_temp_thermal_platform_the
 	return 0;
 }
 
-static int find_siblings_cpu(int cpu)
-{
-	int i;
-	int id = topology_physical_package_id(cpu);
-
-	for_each_online_cpu(i)
-		if (i != cpu && topology_physical_package_id(i) == id)
-			return i;
-
-	return 0;
-}
-
 static int pkg_temp_thermal_device_add(unsigned int cpu)
 {
 	int err;
@@ -488,9 +476,10 @@ static int pkg_temp_thermal_device_remov
 	mutex_lock(&phy_dev_list_mutex);
 	/* If we are loosing the first cpu for this package, we need change */
 	if (phdev->first_cpu == cpu) {
-		phdev->first_cpu = find_siblings_cpu(cpu);
-		pr_debug("thermal_device_remove: first cpu switched %d\n",
-					phdev->first_cpu);
+		int target = cpumask_any_but(topology_core_cpumask(cpu), cpu);
+
+		phdev->first_cpu = target;
+		pr_debug("CPU %d down. New first_cpu%d\n", cpu, target);
 	}
 	/*
 	* It is possible that no siblings left as this was the last cpu

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch V2 05/12] thermal/x86_pkg_temp: Get rid of ref counting
  2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
                   ` (3 preceding siblings ...)
  2016-11-22 17:57 ` [patch V2 03/12] thermal/x86_pkg_temp: Replace open coded cpu search Thomas Gleixner
@ 2016-11-22 17:57 ` Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 06/12] thermal/x86_pkg_temp: Cleanup namespace Thomas Gleixner
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2016-11-22 17:57 UTC (permalink / raw)
  To: LKML
  Cc: Rui Zhang, edubezval, Peter Zijlstra, Borislav Petkov, linux-pm,
	x86, rt, Srinivas Pandruvada

[-- Attachment #1: thermalx86_pkg_temp_Get_rid_of_ref_counting.patch --]
[-- Type: text/plain, Size: 5324 bytes --]

There is no point in the whole package data refcounting dance because
topology_core_cpumask tells us whether this is the last cpu in the
package. If yes, then the package can go, if not it stays. It's already
serialized via the hotplug code.

While at it rename the first_cpu member of the package structure to
cpu. The first has absolutely no meaning.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/thermal/x86_pkg_temp_thermal.c |   62 ++++++++++-----------------------
 1 file changed, 20 insertions(+), 42 deletions(-)

--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -60,9 +60,8 @@ MODULE_PARM_DESC(notify_delay_ms,
 struct phy_dev_entry {
 	struct list_head list;
 	u16 phys_proc_id;
-	u16 first_cpu;
+	u16 cpu;
 	u32 tj_max;
-	int ref_cnt;
 	u32 start_pkg_therm_low;
 	u32 start_pkg_therm_high;
 	struct thermal_zone_device *tzone;
@@ -170,8 +169,8 @@ static int sys_get_curr_temp(struct ther
 	struct phy_dev_entry *phy_dev_entry;
 
 	phy_dev_entry = tzd->devdata;
-	rdmsr_on_cpu(phy_dev_entry->first_cpu, MSR_IA32_PACKAGE_THERM_STATUS,
-			&eax, &edx);
+	rdmsr_on_cpu(phy_dev_entry->cpu, MSR_IA32_PACKAGE_THERM_STATUS,
+		     &eax, &edx);
 	if (eax & 0x80000000) {
 		*temp = phy_dev_entry->tj_max -
 				((eax >> 16) & 0x7f) * 1000;
@@ -204,8 +203,8 @@ static int sys_get_trip_temp(struct ther
 		shift = THERM_SHIFT_THRESHOLD0;
 	}
 
-	ret = rdmsr_on_cpu(phy_dev_entry->first_cpu,
-				MSR_IA32_PACKAGE_THERM_INTERRUPT, &eax, &edx);
+	ret = rdmsr_on_cpu(phy_dev_entry->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
+			   &eax, &edx);
 	if (ret < 0)
 		return -EINVAL;
 
@@ -232,9 +231,8 @@ static int sys_set_trip_temp(struct ther
 	if (trip >= MAX_NUMBER_OF_TRIPS || temp >= phy_dev_entry->tj_max)
 		return -EINVAL;
 
-	ret = rdmsr_on_cpu(phy_dev_entry->first_cpu,
-					MSR_IA32_PACKAGE_THERM_INTERRUPT,
-					&l, &h);
+	ret = rdmsr_on_cpu(phy_dev_entry->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
+			   &l, &h);
 	if (ret < 0)
 		return -EINVAL;
 
@@ -259,9 +257,8 @@ static int sys_set_trip_temp(struct ther
 		l |= intr;
 	}
 
-	return wrmsr_on_cpu(phy_dev_entry->first_cpu,
-					MSR_IA32_PACKAGE_THERM_INTERRUPT,
-					l, h);
+	return wrmsr_on_cpu(phy_dev_entry->cpu,	MSR_IA32_PACKAGE_THERM_INTERRUPT,
+			    l, h);
 }
 
 static int sys_get_trip_type(struct thermal_zone_device *thermal,
@@ -431,9 +428,8 @@ static int pkg_temp_thermal_device_add(u
 	spin_unlock_irqrestore(&pkg_work_lock, flags);
 
 	phy_dev_entry->phys_proc_id = topology_physical_package_id(cpu);
-	phy_dev_entry->first_cpu = cpu;
+	phy_dev_entry->cpu = cpu;
 	phy_dev_entry->tj_max = tj_max;
-	phy_dev_entry->ref_cnt = 1;
 	phy_dev_entry->tzone = thermal_zone_device_register("x86_pkg_temp",
 			thres_count,
 			(thres_count == MAX_NUMBER_OF_TRIPS) ?
@@ -468,30 +464,16 @@ static int pkg_temp_thermal_device_add(u
 static int pkg_temp_thermal_device_remove(unsigned int cpu)
 {
 	struct phy_dev_entry *phdev = pkg_temp_thermal_get_phy_entry(cpu);
-	u16 phys_proc_id = topology_physical_package_id(cpu);
+	int target;
 
 	if (!phdev)
 		return -ENODEV;
 
 	mutex_lock(&phy_dev_list_mutex);
-	/* If we are loosing the first cpu for this package, we need change */
-	if (phdev->first_cpu == cpu) {
-		int target = cpumask_any_but(topology_core_cpumask(cpu), cpu);
-
-		phdev->first_cpu = target;
-		pr_debug("CPU %d down. New first_cpu%d\n", cpu, target);
-	}
-	/*
-	* It is possible that no siblings left as this was the last cpu
-	* going offline. We don't need to worry about this assignment
-	* as the phydev entry will be removed in this case and
-	* thermal zone is removed.
-	*/
-	--phdev->ref_cnt;
-	pr_debug("thermal_device_remove: pkg: %d cpu %d ref_cnt %d\n",
-					phys_proc_id, cpu, phdev->ref_cnt);
 
-	if (!phdev->ref_cnt) {
+	target = cpumask_any_but(topology_core_cpumask(cpu), cpu);
+	/* This might be the last cpu in this package */
+	if (target >= nr_cpu_ids) {
 		thermal_zone_device_unregister(phdev->tzone);
 		/*
 		 * Restore original MSR value for package thermal
@@ -502,7 +484,10 @@ static int pkg_temp_thermal_device_remov
 			     phdev->start_pkg_therm_high);
 		list_del(&phdev->list);
 		kfree(phdev);
+	} else if (phdev->cpu == cpu) {
+		phdev->cpu = target;
 	}
+
 	mutex_unlock(&phy_dev_list_mutex);
 
 	return 0;
@@ -520,12 +505,6 @@ static int get_core_online(unsigned int
 			return -ENODEV;
 		if (pkg_temp_thermal_device_add(cpu))
 			return -ENODEV;
-	} else {
-		mutex_lock(&phy_dev_list_mutex);
-		++phdev->ref_cnt;
-		pr_debug("get_core_online: cpu %d ref_cnt %d\n",
-						cpu, phdev->ref_cnt);
-		mutex_unlock(&phy_dev_list_mutex);
 	}
 	INIT_DELAYED_WORK(&per_cpu(pkg_temp_thermal_threshold_work, cpu),
 			pkg_temp_thermal_threshold_work_fn);
@@ -615,10 +594,9 @@ static void __exit pkg_temp_thermal_exit
 	mutex_lock(&phy_dev_list_mutex);
 	list_for_each_entry_safe(phdev, n, &phy_dev_list, list) {
 		/* Retore old MSR value for package thermal interrupt */
-		wrmsr_on_cpu(phdev->first_cpu,
-			MSR_IA32_PACKAGE_THERM_INTERRUPT,
-			phdev->start_pkg_therm_low,
-			phdev->start_pkg_therm_high);
+		wrmsr_on_cpu(phdev->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
+			     phdev->start_pkg_therm_low,
+			     phdev->start_pkg_therm_high);
 		thermal_zone_device_unregister(phdev->tzone);
 		list_del(&phdev->list);
 		kfree(phdev);

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch V2 06/12] thermal/x86_pkg_temp: Cleanup namespace
  2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
                   ` (4 preceding siblings ...)
  2016-11-22 17:57 ` [patch V2 05/12] thermal/x86_pkg_temp: Get rid of ref counting Thomas Gleixner
@ 2016-11-22 17:57 ` Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 07/12] thermal/x86_pkg_temp: Cleanup code some more Thomas Gleixner
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2016-11-22 17:57 UTC (permalink / raw)
  To: LKML
  Cc: Rui Zhang, edubezval, Peter Zijlstra, Borislav Petkov, linux-pm,
	x86, rt, Srinivas Pandruvada

[-- Attachment #1: thermalx86_pkg_temp_Cleanup_namespace.patch --]
[-- Type: text/plain, Size: 11368 bytes --]

Any randomly chosen struct name is more descriptive than phy_dev_entry. 

Rename the whole thing to struct pkg_device, which describes the content
reasonably well and use the same variable name throughout the code so it
gets readable. Rename the msr struct members as well.

No functional change.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/thermal/x86_pkg_temp_thermal.c |  166 +++++++++++++++------------------
 1 file changed, 76 insertions(+), 90 deletions(-)

--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -57,14 +57,14 @@ MODULE_PARM_DESC(notify_delay_ms,
 /* Limit number of package temp zones */
 #define MAX_PKG_TEMP_ZONE_IDS	256
 
-struct phy_dev_entry {
-	struct list_head list;
-	u16 phys_proc_id;
-	u16 cpu;
-	u32 tj_max;
-	u32 start_pkg_therm_low;
-	u32 start_pkg_therm_high;
-	struct thermal_zone_device *tzone;
+struct pkg_device {
+	struct list_head		list;
+	u16				phys_proc_id;
+	u16				cpu;
+	u32				tj_max;
+	u32				msr_pkg_therm_low;
+	u32				msr_pkg_therm_high;
+	struct thermal_zone_device	*tzone;
 };
 
 static struct thermal_zone_params pkg_temp_tz_params = {
@@ -115,18 +115,17 @@ static int pkg_temp_debugfs_init(void)
 	return -ENOENT;
 }
 
-static struct phy_dev_entry
-			*pkg_temp_thermal_get_phy_entry(unsigned int cpu)
+static struct pkg_device *pkg_temp_thermal_get_dev(unsigned int cpu)
 {
 	u16 phys_proc_id = topology_physical_package_id(cpu);
-	struct phy_dev_entry *phy_ptr;
+	struct pkg_device *pkgdev;
 
 	mutex_lock(&phy_dev_list_mutex);
 
-	list_for_each_entry(phy_ptr, &phy_dev_list, list)
-		if (phy_ptr->phys_proc_id == phys_proc_id) {
+	list_for_each_entry(pkgdev, &phy_dev_list, list)
+		if (pkgdev->phys_proc_id == phys_proc_id) {
 			mutex_unlock(&phy_dev_list_mutex);
-			return phy_ptr;
+			return pkgdev;
 		}
 
 	mutex_unlock(&phy_dev_list_mutex);
@@ -165,36 +164,29 @@ static int get_tj_max(int cpu, u32 *tj_m
 
 static int sys_get_curr_temp(struct thermal_zone_device *tzd, int *temp)
 {
+	struct pkg_device *pkgdev = tzd->devdata;
 	u32 eax, edx;
-	struct phy_dev_entry *phy_dev_entry;
 
-	phy_dev_entry = tzd->devdata;
-	rdmsr_on_cpu(phy_dev_entry->cpu, MSR_IA32_PACKAGE_THERM_STATUS,
-		     &eax, &edx);
+	rdmsr_on_cpu(pkgdev->cpu, MSR_IA32_PACKAGE_THERM_STATUS, &eax, &edx);
 	if (eax & 0x80000000) {
-		*temp = phy_dev_entry->tj_max -
-				((eax >> 16) & 0x7f) * 1000;
+		*temp = pkgdev->tj_max - ((eax >> 16) & 0x7f) * 1000;
 		pr_debug("sys_get_curr_temp %d\n", *temp);
 		return 0;
 	}
-
 	return -EINVAL;
 }
 
 static int sys_get_trip_temp(struct thermal_zone_device *tzd,
-		int trip, int *temp)
+			     int trip, int *temp)
 {
-	u32 eax, edx;
-	struct phy_dev_entry *phy_dev_entry;
-	u32 mask, shift;
+	struct pkg_device *pkgdev = tzd->devdata;
 	unsigned long thres_reg_value;
+	u32 mask, shift, eax, edx;
 	int ret;
 
 	if (trip >= MAX_NUMBER_OF_TRIPS)
 		return -EINVAL;
 
-	phy_dev_entry = tzd->devdata;
-
 	if (trip) {
 		mask = THERM_MASK_THRESHOLD1;
 		shift = THERM_SHIFT_THRESHOLD1;
@@ -203,14 +195,14 @@ static int sys_get_trip_temp(struct ther
 		shift = THERM_SHIFT_THRESHOLD0;
 	}
 
-	ret = rdmsr_on_cpu(phy_dev_entry->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
+	ret = rdmsr_on_cpu(pkgdev->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
 			   &eax, &edx);
 	if (ret < 0)
 		return -EINVAL;
 
 	thres_reg_value = (eax & mask) >> shift;
 	if (thres_reg_value)
-		*temp = phy_dev_entry->tj_max - thres_reg_value * 1000;
+		*temp = pkgdev->tj_max - thres_reg_value * 1000;
 	else
 		*temp = 0;
 	pr_debug("sys_get_trip_temp %d\n", *temp);
@@ -218,20 +210,17 @@ static int sys_get_trip_temp(struct ther
 	return 0;
 }
 
-static int sys_set_trip_temp(struct thermal_zone_device *tzd, int trip,
-							int temp)
+static int
+sys_set_trip_temp(struct thermal_zone_device *tzd, int trip, int temp)
 {
-	u32 l, h;
-	struct phy_dev_entry *phy_dev_entry;
-	u32 mask, shift, intr;
+	struct pkg_device *pkgdev = tzd->devdata;
+	u32 l, h, mask, shift, intr;
 	int ret;
 
-	phy_dev_entry = tzd->devdata;
-
-	if (trip >= MAX_NUMBER_OF_TRIPS || temp >= phy_dev_entry->tj_max)
+	if (trip >= MAX_NUMBER_OF_TRIPS || temp >= pkgdev->tj_max)
 		return -EINVAL;
 
-	ret = rdmsr_on_cpu(phy_dev_entry->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
+	ret = rdmsr_on_cpu(pkgdev->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
 			   &l, &h);
 	if (ret < 0)
 		return -EINVAL;
@@ -250,19 +239,18 @@ static int sys_set_trip_temp(struct ther
 	* When users space sets a trip temperature == 0, which is indication
 	* that, it is no longer interested in receiving notifications.
 	*/
-	if (!temp)
+	if (!temp) {
 		l &= ~intr;
-	else {
-		l |= (phy_dev_entry->tj_max - temp)/1000 << shift;
+	} else {
+		l |= (pkgdev->tj_max - temp)/1000 << shift;
 		l |= intr;
 	}
 
-	return wrmsr_on_cpu(phy_dev_entry->cpu,	MSR_IA32_PACKAGE_THERM_INTERRUPT,
-			    l, h);
+	return wrmsr_on_cpu(pkgdev->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT, l, h);
 }
 
-static int sys_get_trip_type(struct thermal_zone_device *thermal,
-		int trip, enum thermal_trip_type *type)
+static int sys_get_trip_type(struct thermal_zone_device *thermal, int trip,
+			     enum thermal_trip_type *type)
 {
 
 	*type = THERMAL_TRIP_PASSIVE;
@@ -315,11 +303,11 @@ static void pkg_temp_thermal_threshold_w
 	__u64 msr_val;
 	int cpu = smp_processor_id();
 	int phy_id = topology_physical_package_id(cpu);
-	struct phy_dev_entry *phdev = pkg_temp_thermal_get_phy_entry(cpu);
+	struct pkg_device *pkgdev = pkg_temp_thermal_get_dev(cpu);
 	bool notify = false;
 	unsigned long flags;
 
-	if (!phdev)
+	if (!pkgdev)
 		return;
 
 	spin_lock_irqsave(&pkg_work_lock, flags);
@@ -347,7 +335,7 @@ static void pkg_temp_thermal_threshold_w
 
 	if (notify) {
 		pr_debug("thermal_zone_device_update\n");
-		thermal_zone_device_update(phdev->tzone,
+		thermal_zone_device_update(pkgdev->tzone,
 					   THERMAL_EVENT_UNSPECIFIED);
 	}
 }
@@ -383,13 +371,11 @@ static int pkg_thermal_notify(__u64 msr_
 
 static int pkg_temp_thermal_device_add(unsigned int cpu)
 {
-	int err;
-	u32 tj_max;
-	struct phy_dev_entry *phy_dev_entry;
-	int thres_count;
-	u32 eax, ebx, ecx, edx;
-	u8 *temp;
+	u32 tj_max, eax, ebx, ecx, edx;
+	struct pkg_device *pkgdev;
+	int thres_count, err;
 	unsigned long flags;
+	u8 *temp;
 
 	cpuid(6, &eax, &ebx, &ecx, &edx);
 	thres_count = ebx & 0x07;
@@ -407,8 +393,8 @@ static int pkg_temp_thermal_device_add(u
 
 	mutex_lock(&phy_dev_list_mutex);
 
-	phy_dev_entry = kzalloc(sizeof(*phy_dev_entry), GFP_KERNEL);
-	if (!phy_dev_entry) {
+	pkgdev = kzalloc(sizeof(*pkgdev), GFP_KERNEL);
+	if (!pkgdev) {
 		err = -ENOMEM;
 		goto err_ret_unlock;
 	}
@@ -427,33 +413,32 @@ static int pkg_temp_thermal_device_add(u
 	pkg_work_scheduled[topology_physical_package_id(cpu)] = 0;
 	spin_unlock_irqrestore(&pkg_work_lock, flags);
 
-	phy_dev_entry->phys_proc_id = topology_physical_package_id(cpu);
-	phy_dev_entry->cpu = cpu;
-	phy_dev_entry->tj_max = tj_max;
-	phy_dev_entry->tzone = thermal_zone_device_register("x86_pkg_temp",
+	pkgdev->phys_proc_id = topology_physical_package_id(cpu);
+	pkgdev->cpu = cpu;
+	pkgdev->tj_max = tj_max;
+	pkgdev->tzone = thermal_zone_device_register("x86_pkg_temp",
 			thres_count,
-			(thres_count == MAX_NUMBER_OF_TRIPS) ?
-				0x03 : 0x01,
-			phy_dev_entry, &tzone_ops, &pkg_temp_tz_params, 0, 0);
-	if (IS_ERR(phy_dev_entry->tzone)) {
-		err = PTR_ERR(phy_dev_entry->tzone);
+			(thres_count == MAX_NUMBER_OF_TRIPS) ? 0x03 : 0x01,
+			pkgdev, &tzone_ops, &pkg_temp_tz_params, 0, 0);
+	if (IS_ERR(pkgdev->tzone)) {
+		err = PTR_ERR(pkgdev->tzone);
 		goto err_ret_free;
 	}
 	/* Store MSR value for package thermal interrupt, to restore at exit */
 	rdmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
-				&phy_dev_entry->start_pkg_therm_low,
-				&phy_dev_entry->start_pkg_therm_high);
+		     &pkgdev->msr_pkg_therm_low,
+		     &pkgdev->msr_pkg_therm_high);
 
-	list_add_tail(&phy_dev_entry->list, &phy_dev_list);
+	list_add_tail(&pkgdev->list, &phy_dev_list);
 	pr_debug("pkg_temp_thermal_device_add :phy_id %d cpu %d\n",
-			phy_dev_entry->phys_proc_id, cpu);
+		 pkgdev->phys_proc_id, cpu);
 
 	mutex_unlock(&phy_dev_list_mutex);
 
 	return 0;
 
 err_ret_free:
-	kfree(phy_dev_entry);
+	kfree(pkgdev);
 err_ret_unlock:
 	mutex_unlock(&phy_dev_list_mutex);
 
@@ -463,10 +448,10 @@ static int pkg_temp_thermal_device_add(u
 
 static int pkg_temp_thermal_device_remove(unsigned int cpu)
 {
-	struct phy_dev_entry *phdev = pkg_temp_thermal_get_phy_entry(cpu);
+	struct pkg_device *pkgdev = pkg_temp_thermal_get_dev(cpu);
 	int target;
 
-	if (!phdev)
+	if (!pkgdev)
 		return -ENODEV;
 
 	mutex_lock(&phy_dev_list_mutex);
@@ -474,18 +459,18 @@ static int pkg_temp_thermal_device_remov
 	target = cpumask_any_but(topology_core_cpumask(cpu), cpu);
 	/* This might be the last cpu in this package */
 	if (target >= nr_cpu_ids) {
-		thermal_zone_device_unregister(phdev->tzone);
+		thermal_zone_device_unregister(pkgdev->tzone);
 		/*
 		 * Restore original MSR value for package thermal
 		 * interrupt.
 		 */
 		wrmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
-			     phdev->start_pkg_therm_low,
-			     phdev->start_pkg_therm_high);
-		list_del(&phdev->list);
-		kfree(phdev);
-	} else if (phdev->cpu == cpu) {
-		phdev->cpu = target;
+			     pkgdev->msr_pkg_therm_low,
+			     pkgdev->msr_pkg_therm_high);
+		list_del(&pkgdev->list);
+		kfree(pkgdev);
+	} else if (pkgdev->cpu == cpu) {
+		pkgdev->cpu = target;
 	}
 
 	mutex_unlock(&phy_dev_list_mutex);
@@ -495,19 +480,20 @@ static int pkg_temp_thermal_device_remov
 
 static int get_core_online(unsigned int cpu)
 {
+	struct pkg_device *pkgdev = pkg_temp_thermal_get_dev(cpu);
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
-	struct phy_dev_entry *phdev = pkg_temp_thermal_get_phy_entry(cpu);
 
 	/* Check if there is already an instance for this package */
-	if (!phdev) {
+	if (!pkgdev) {
 		if (!cpu_has(c, X86_FEATURE_DTHERM) ||
 					!cpu_has(c, X86_FEATURE_PTS))
 			return -ENODEV;
 		if (pkg_temp_thermal_device_add(cpu))
 			return -ENODEV;
 	}
+
 	INIT_DELAYED_WORK(&per_cpu(pkg_temp_thermal_threshold_work, cpu),
-			pkg_temp_thermal_threshold_work_fn);
+			  pkg_temp_thermal_threshold_work_fn);
 
 	pr_debug("get_core_online: cpu %d successful\n", cpu);
 
@@ -583,7 +569,7 @@ static int __init pkg_temp_thermal_init(
 
 static void __exit pkg_temp_thermal_exit(void)
 {
-	struct phy_dev_entry *phdev, *n;
+	struct pkg_device *pkgdev, *n;
 	int i;
 
 	platform_thermal_package_notify = NULL;
@@ -592,14 +578,14 @@ static void __exit pkg_temp_thermal_exit
 	cpu_notifier_register_begin();
 	__unregister_hotcpu_notifier(&pkg_temp_thermal_notifier);
 	mutex_lock(&phy_dev_list_mutex);
-	list_for_each_entry_safe(phdev, n, &phy_dev_list, list) {
+	list_for_each_entry_safe(pkgdev, n, &phy_dev_list, list) {
 		/* Retore old MSR value for package thermal interrupt */
-		wrmsr_on_cpu(phdev->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
-			     phdev->start_pkg_therm_low,
-			     phdev->start_pkg_therm_high);
-		thermal_zone_device_unregister(phdev->tzone);
-		list_del(&phdev->list);
-		kfree(phdev);
+		wrmsr_on_cpu(pkgdev->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
+			     pkgdev->msr_pkg_therm_low,
+			     pkgdev->msr_pkg_therm_high);
+		thermal_zone_device_unregister(pkgdev->tzone);
+		list_del(&pkgdev->list);
+		kfree(pkgdev);
 	}
 	mutex_unlock(&phy_dev_list_mutex);
 	for_each_online_cpu(i)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch V2 07/12] thermal/x86_pkg_temp: Cleanup code some more
  2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
                   ` (5 preceding siblings ...)
  2016-11-22 17:57 ` [patch V2 06/12] thermal/x86_pkg_temp: Cleanup namespace Thomas Gleixner
@ 2016-11-22 17:57 ` Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 08/12] thermal/x86_pkg_temp: Sanitize locking Thomas Gleixner
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2016-11-22 17:57 UTC (permalink / raw)
  To: LKML
  Cc: Rui Zhang, edubezval, Peter Zijlstra, Borislav Petkov, linux-pm,
	x86, rt, Srinivas Pandruvada

[-- Attachment #1: thermalx86_pkg_temp_Cleanup_code_some_more.patch --]
[-- Type: text/plain, Size: 5564 bytes --]

Coding style fixups and replacement of overly complex constructs and random
error codes instead of returning the real ones. This mess makes the eyes bleeding.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/thermal/x86_pkg_temp_thermal.c |   81 ++++++++++++---------------------
 1 file changed, 30 insertions(+), 51 deletions(-)

--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -139,27 +139,17 @@ static struct pkg_device *pkg_temp_therm
 */
 static int get_tj_max(int cpu, u32 *tj_max)
 {
-	u32 eax, edx;
-	u32 val;
+	u32 eax, edx, val;
 	int err;
 
 	err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET, &eax, &edx);
 	if (err)
-		goto err_ret;
-	else {
-		val = (eax >> 16) & 0xff;
-		if (val)
-			*tj_max = val * 1000;
-		else {
-			err = -EINVAL;
-			goto err_ret;
-		}
-	}
+		return err;
 
-	return 0;
-err_ret:
-	*tj_max = 0;
-	return err;
+	val = (eax >> 16) & 0xff;
+	*tj_max = val * 1000;
+
+	return val ? 0 : -EINVAL;
 }
 
 static int sys_get_curr_temp(struct thermal_zone_device *tzd, int *temp)
@@ -198,7 +188,7 @@ static int sys_get_trip_temp(struct ther
 	ret = rdmsr_on_cpu(pkgdev->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
 			   &eax, &edx);
 	if (ret < 0)
-		return -EINVAL;
+		return ret;
 
 	thres_reg_value = (eax & mask) >> shift;
 	if (thres_reg_value)
@@ -223,7 +213,7 @@ sys_set_trip_temp(struct thermal_zone_de
 	ret = rdmsr_on_cpu(pkgdev->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
 			   &l, &h);
 	if (ret < 0)
-		return -EINVAL;
+		return ret;
 
 	if (trip) {
 		mask = THERM_MASK_THRESHOLD1;
@@ -252,9 +242,7 @@ sys_set_trip_temp(struct thermal_zone_de
 static int sys_get_trip_type(struct thermal_zone_device *thermal, int trip,
 			     enum thermal_trip_type *type)
 {
-
 	*type = THERMAL_TRIP_PASSIVE;
-
 	return 0;
 }
 
@@ -274,8 +262,8 @@ static bool pkg_thermal_rate_control(voi
 /* Enable threshold interrupt on local package/cpu */
 static inline void enable_pkg_thres_interrupt(void)
 {
-	u32 l, h;
 	u8 thres_0, thres_1;
+	u32 l, h;
 
 	rdmsr(MSR_IA32_PACKAGE_THERM_INTERRUPT, l, h);
 	/* only enable/disable if it had valid threshold value */
@@ -292,20 +280,21 @@ static inline void enable_pkg_thres_inte
 static inline void disable_pkg_thres_interrupt(void)
 {
 	u32 l, h;
+
 	rdmsr(MSR_IA32_PACKAGE_THERM_INTERRUPT, l, h);
-	wrmsr(MSR_IA32_PACKAGE_THERM_INTERRUPT,
-			l & (~THERM_INT_THRESHOLD0_ENABLE) &
-				(~THERM_INT_THRESHOLD1_ENABLE), h);
+
+	l &= ~(THERM_INT_THRESHOLD0_ENABLE | THERM_INT_THRESHOLD1_ENABLE);
+	wrmsr(MSR_IA32_PACKAGE_THERM_INTERRUPT, l, h);
 }
 
 static void pkg_temp_thermal_threshold_work_fn(struct work_struct *work)
 {
-	__u64 msr_val;
 	int cpu = smp_processor_id();
-	int phy_id = topology_physical_package_id(cpu);
 	struct pkg_device *pkgdev = pkg_temp_thermal_get_dev(cpu);
+	int phy_id = topology_physical_package_id(cpu);
 	bool notify = false;
 	unsigned long flags;
+	u64 msr_val, wr_val;
 
 	if (!pkgdev)
 		return;
@@ -320,14 +309,9 @@ static void pkg_temp_thermal_threshold_w
 	spin_unlock_irqrestore(&pkg_work_lock, flags);
 
 	rdmsrl(MSR_IA32_PACKAGE_THERM_STATUS, msr_val);
-	if (msr_val & THERM_LOG_THRESHOLD0) {
-		wrmsrl(MSR_IA32_PACKAGE_THERM_STATUS,
-				msr_val & ~THERM_LOG_THRESHOLD0);
-		notify = true;
-	}
-	if (msr_val & THERM_LOG_THRESHOLD1) {
-		wrmsrl(MSR_IA32_PACKAGE_THERM_STATUS,
-				msr_val & ~THERM_LOG_THRESHOLD1);
+	wr_val = msr_val & ~(THERM_LOG_THRESHOLD0 | THERM_LOG_THRESHOLD1);
+	if (wr_val != msr_val) {
+		wrmsrl(MSR_IA32_PACKAGE_THERM_STATUS, wr_val);
 		notify = true;
 	}
 
@@ -340,11 +324,11 @@ static void pkg_temp_thermal_threshold_w
 	}
 }
 
-static int pkg_thermal_notify(__u64 msr_val)
+static int pkg_thermal_notify(u64 msr_val)
 {
-	unsigned long flags;
 	int cpu = smp_processor_id();
 	int phy_id = topology_physical_package_id(cpu);
+	unsigned long flags;
 
 	/*
 	* When a package is in interrupted state, all CPU's in that package
@@ -483,21 +467,17 @@ static int get_core_online(unsigned int
 	struct pkg_device *pkgdev = pkg_temp_thermal_get_dev(cpu);
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 
-	/* Check if there is already an instance for this package */
-	if (!pkgdev) {
-		if (!cpu_has(c, X86_FEATURE_DTHERM) ||
-					!cpu_has(c, X86_FEATURE_PTS))
-			return -ENODEV;
-		if (pkg_temp_thermal_device_add(cpu))
-			return -ENODEV;
-	}
+	/* Paranoia check */
+	if (!cpu_has(c, X86_FEATURE_DTHERM) || !cpu_has(c, X86_FEATURE_PTS))
+		return -ENODEV;
 
 	INIT_DELAYED_WORK(&per_cpu(pkg_temp_thermal_threshold_work, cpu),
 			  pkg_temp_thermal_threshold_work_fn);
 
-	pr_debug("get_core_online: cpu %d successful\n", cpu);
-
-	return 0;
+	/* If the package exists, nothing to do */
+	if (pkgdev)
+		return 0;
+	return pkg_temp_thermal_device_add(cpu);
 }
 
 static void put_core_offline(unsigned int cpu)
@@ -555,8 +535,8 @@ static int __init pkg_temp_thermal_init(
 	platform_thermal_package_notify = pkg_thermal_notify;
 	platform_thermal_package_rate_control = pkg_thermal_rate_control;
 
-	pkg_temp_debugfs_init(); /* Don't care if fails */
-
+	 /* Don't care if it fails */
+	pkg_temp_debugfs_init();
 	return 0;
 
 err_ret:
@@ -566,6 +546,7 @@ static int __init pkg_temp_thermal_init(
 	kfree(pkg_work_scheduled);
 	return -ENODEV;
 }
+module_init(pkg_temp_thermal_init)
 
 static void __exit pkg_temp_thermal_exit(void)
 {
@@ -597,8 +578,6 @@ static void __exit pkg_temp_thermal_exit
 
 	debugfs_remove_recursive(debugfs);
 }
-
-module_init(pkg_temp_thermal_init)
 module_exit(pkg_temp_thermal_exit)
 
 MODULE_DESCRIPTION("X86 PKG TEMP Thermal Driver");

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch V2 08/12] thermal/x86_pkg_temp: Sanitize locking
  2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
                   ` (6 preceding siblings ...)
  2016-11-22 17:57 ` [patch V2 07/12] thermal/x86_pkg_temp: Cleanup code some more Thomas Gleixner
@ 2016-11-22 17:57 ` Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 09/12] thermal/x86_pkg_temp: Move work scheduled flag into package struct Thomas Gleixner
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2016-11-22 17:57 UTC (permalink / raw)
  To: LKML
  Cc: Rui Zhang, edubezval, Peter Zijlstra, Borislav Petkov, linux-pm,
	x86, rt, Srinivas Pandruvada

[-- Attachment #1: thermalx86_pkg_temp_Sanitize_locking.patch --]
[-- Type: text/plain, Size: 13025 bytes --]

The work cancellation code, the thermal zone unregistering, the work code
and the interrupt notification function are racy against each other and
against cpu hotplug and module exit. The random locking sprinkeled all
over the place does not help anything and probably exists to make people
feel good. The resulting issues (mainly use after free) are probably
hard to trigger, but they clearly exist

Protect the package list with a spinlock so it can be accessed from the
interrupt notifier and also from the work function. The add/removal code in
the hotplug callbacks take the lock for list manipulation. That makes sure
that on removal neither the interrupt notifier nor the work function can
access the about to be freed package structure anymore.

The thermal zone unregistering is another trainwreck. It's not serialized
against the work function. So unregistering the zone device can race with
the work function and cause havoc.

Protect the thermal zone with a mutex, which is held in the work
function to make sure that the zone device is not being unregistered
concurrently.

To solve the module exit issues, we simply invoke the cpu offline callback
and let it work its magic. For that it's required to keep track of the
participating cpus in a package, because topology_core_mask is not affected
by calling the offline callback for teardown of the driver, so it would
never free the package as there is always a valid target in
topology_core_mask.

Use proper names for the locks so it's clear what they are for and add a
pile of comments to explain the protection rules.

It's amazing that fixing the locking and adding 30 lines of comments
explaining it still removes more lines than it adds.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/thermal/x86_pkg_temp_thermal.c |  222 ++++++++++++++++-----------------
 1 file changed, 110 insertions(+), 112 deletions(-)

--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -65,6 +65,7 @@ struct pkg_device {
 	u32				msr_pkg_therm_low;
 	u32				msr_pkg_therm_high;
 	struct thermal_zone_device	*tzone;
+	struct cpumask			cpumask;
 };
 
 static struct thermal_zone_params pkg_temp_tz_params = {
@@ -73,7 +74,10 @@ static struct thermal_zone_params pkg_te
 
 /* List maintaining number of package instances */
 static LIST_HEAD(phy_dev_list);
-static DEFINE_MUTEX(phy_dev_list_mutex);
+/* Serializes interrupt notification, work and hotplug */
+static DEFINE_SPINLOCK(pkg_temp_lock);
+/* Protects zone operation in the work function against hotplug removal */
+static DEFINE_MUTEX(thermal_zone_mutex);
 
 /* Interrupt to work function schedule queue */
 static DEFINE_PER_CPU(struct delayed_work, pkg_temp_thermal_threshold_work);
@@ -81,8 +85,6 @@ static DEFINE_PER_CPU(struct delayed_wor
 /* To track if the work is already scheduled on a package */
 static u8 *pkg_work_scheduled;
 
-/* Spin lock to prevent races with pkg_work_scheduled */
-static spinlock_t pkg_work_lock;
 static u16 max_phy_id;
 
 /* Debug counters to show using debugfs */
@@ -115,21 +117,23 @@ static int pkg_temp_debugfs_init(void)
 	return -ENOENT;
 }
 
+/*
+ * Protection:
+ *
+ * - cpu hotplug: Read serialized by cpu hotplug lock
+ *		  Write must hold pkg_temp_lock
+ *
+ * - Other callsites: Must hold pkg_temp_lock
+ */
 static struct pkg_device *pkg_temp_thermal_get_dev(unsigned int cpu)
 {
 	u16 phys_proc_id = topology_physical_package_id(cpu);
 	struct pkg_device *pkgdev;
 
-	mutex_lock(&phy_dev_list_mutex);
-
-	list_for_each_entry(pkgdev, &phy_dev_list, list)
-		if (pkgdev->phys_proc_id == phys_proc_id) {
-			mutex_unlock(&phy_dev_list_mutex);
+	list_for_each_entry(pkgdev, &phy_dev_list, list) {
+		if (pkgdev->phys_proc_id == phys_proc_id)
 			return pkgdev;
-		}
-
-	mutex_unlock(&phy_dev_list_mutex);
-
+	}
 	return NULL;
 }
 
@@ -289,67 +293,66 @@ static inline void disable_pkg_thres_int
 
 static void pkg_temp_thermal_threshold_work_fn(struct work_struct *work)
 {
-	int cpu = smp_processor_id();
-	struct pkg_device *pkgdev = pkg_temp_thermal_get_dev(cpu);
-	int phy_id = topology_physical_package_id(cpu);
-	bool notify = false;
-	unsigned long flags;
+	struct thermal_zone_device *tzone = NULL;
+	int phy_id, cpu = smp_processor_id();
+	struct pkg_device *pkgdev;
 	u64 msr_val, wr_val;
 
-	if (!pkgdev)
-		return;
-
-	spin_lock_irqsave(&pkg_work_lock, flags);
+	mutex_lock(&thermal_zone_mutex);
+	spin_lock_irq(&pkg_temp_lock);
 	++pkg_work_cnt;
-	if (unlikely(phy_id > max_phy_id)) {
-		spin_unlock_irqrestore(&pkg_work_lock, flags);
+
+	pkgdev = pkg_temp_thermal_get_dev(cpu);
+	if (!pkgdev) {
+		spin_unlock_irq(&pkg_temp_lock);
+		mutex_unlock(&thermal_zone_mutex);
 		return;
 	}
+
 	pkg_work_scheduled[phy_id] = 0;
-	spin_unlock_irqrestore(&pkg_work_lock, flags);
 
 	rdmsrl(MSR_IA32_PACKAGE_THERM_STATUS, msr_val);
 	wr_val = msr_val & ~(THERM_LOG_THRESHOLD0 | THERM_LOG_THRESHOLD1);
 	if (wr_val != msr_val) {
 		wrmsrl(MSR_IA32_PACKAGE_THERM_STATUS, wr_val);
-		notify = true;
+		tzone = pkgdev->tzone;
 	}
 
 	enable_pkg_thres_interrupt();
+	spin_unlock_irq(&pkg_temp_lock);
 
-	if (notify) {
-		pr_debug("thermal_zone_device_update\n");
-		thermal_zone_device_update(pkgdev->tzone,
-					   THERMAL_EVENT_UNSPECIFIED);
-	}
+	/*
+	 * If tzone is not NULL, then thermal_zone_mutex will prevent the
+	 * concurrent removal in the cpu offline callback.
+	 */
+	if (tzone)
+		thermal_zone_device_update(tzone, THERMAL_EVENT_UNSPECIFIED);
+
+	mutex_unlock(&thermal_zone_mutex);
 }
 
 static int pkg_thermal_notify(u64 msr_val)
 {
 	int cpu = smp_processor_id();
 	int phy_id = topology_physical_package_id(cpu);
+	struct pkg_device *pkgdev;
 	unsigned long flags;
 
-	/*
-	* When a package is in interrupted state, all CPU's in that package
-	* are in the same interrupt state. So scheduling on any one CPU in
-	* the package is enough and simply return for others.
-	*/
-	spin_lock_irqsave(&pkg_work_lock, flags);
+	spin_lock_irqsave(&pkg_temp_lock, flags);
 	++pkg_interrupt_cnt;
-	if (unlikely(phy_id > max_phy_id) || unlikely(!pkg_work_scheduled) ||
-			pkg_work_scheduled[phy_id]) {
-		disable_pkg_thres_interrupt();
-		spin_unlock_irqrestore(&pkg_work_lock, flags);
-		return -EINVAL;
-	}
-	pkg_work_scheduled[phy_id] = 1;
-	spin_unlock_irqrestore(&pkg_work_lock, flags);
 
 	disable_pkg_thres_interrupt();
-	schedule_delayed_work_on(cpu,
+
+	/* Work is per package, so scheduling it once is enough. */
+	pkgdev = pkg_temp_thermal_get_dev(cpu);
+	if (pkgdev && pkg_work_scheduled && !pkg_work_scheduled[phy_id]) {
+		pkg_work_scheduled[phy_id] = 1;
+		schedule_delayed_work_on(cpu,
 				&per_cpu(pkg_temp_thermal_threshold_work, cpu),
 				msecs_to_jiffies(notify_delay_ms));
+	}
+
+	spin_unlock_irqrestore(&pkg_temp_lock, flags);
 	return 0;
 }
 
@@ -373,29 +376,25 @@ static int pkg_temp_thermal_device_add(u
 
 	err = get_tj_max(cpu, &tj_max);
 	if (err)
-		goto err_ret;
-
-	mutex_lock(&phy_dev_list_mutex);
+		return err;
 
 	pkgdev = kzalloc(sizeof(*pkgdev), GFP_KERNEL);
-	if (!pkgdev) {
-		err = -ENOMEM;
-		goto err_ret_unlock;
-	}
+	if (!pkgdev)
+		return -ENOMEM;
 
-	spin_lock_irqsave(&pkg_work_lock, flags);
+	spin_lock_irqsave(&pkg_temp_lock, flags);
 	if (topology_physical_package_id(cpu) > max_phy_id)
 		max_phy_id = topology_physical_package_id(cpu);
 	temp = krealloc(pkg_work_scheduled,
 			(max_phy_id+1) * sizeof(u8), GFP_ATOMIC);
 	if (!temp) {
-		spin_unlock_irqrestore(&pkg_work_lock, flags);
-		err = -ENOMEM;
-		goto err_ret_free;
+		spin_unlock_irqrestore(&pkg_temp_lock, flags);
+		kfree(pkgdev);
+		return -ENOMEM;
 	}
 	pkg_work_scheduled = temp;
 	pkg_work_scheduled[topology_physical_package_id(cpu)] = 0;
-	spin_unlock_irqrestore(&pkg_work_lock, flags);
+	spin_unlock_irqrestore(&pkg_temp_lock, flags);
 
 	pkgdev->phys_proc_id = topology_physical_package_id(cpu);
 	pkgdev->cpu = cpu;
@@ -406,60 +405,81 @@ static int pkg_temp_thermal_device_add(u
 			pkgdev, &tzone_ops, &pkg_temp_tz_params, 0, 0);
 	if (IS_ERR(pkgdev->tzone)) {
 		err = PTR_ERR(pkgdev->tzone);
-		goto err_ret_free;
+		kfree(pkgdev);
+		return err;
 	}
 	/* Store MSR value for package thermal interrupt, to restore at exit */
 	rdmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
 		     &pkgdev->msr_pkg_therm_low,
 		     &pkgdev->msr_pkg_therm_high);
 
+	cpumask_set_cpu(cpu, &pkgdev->cpumask);
+	spin_lock_irq(&pkg_temp_lock);
 	list_add_tail(&pkgdev->list, &phy_dev_list);
-	pr_debug("pkg_temp_thermal_device_add :phy_id %d cpu %d\n",
-		 pkgdev->phys_proc_id, cpu);
-
-	mutex_unlock(&phy_dev_list_mutex);
-
+	spin_unlock_irq(&pkg_temp_lock);
 	return 0;
-
-err_ret_free:
-	kfree(pkgdev);
-err_ret_unlock:
-	mutex_unlock(&phy_dev_list_mutex);
-
-err_ret:
-	return err;
 }
 
-static int pkg_temp_thermal_device_remove(unsigned int cpu)
+static void put_core_offline(unsigned int cpu)
 {
 	struct pkg_device *pkgdev = pkg_temp_thermal_get_dev(cpu);
+	bool lastcpu;
 	int target;
 
 	if (!pkgdev)
-		return -ENODEV;
+		return;
 
-	mutex_lock(&phy_dev_list_mutex);
+	target = cpumask_any_but(&pkgdev->cpumask, cpu);
+	cpumask_clear_cpu(cpu, &pkgdev->cpumask);
+	lastcpu = target >= nr_cpu_ids;
+	/*
+	 * Remove the sysfs files, if this is the last cpu in the package
+	 * before doing further cleanups.
+	 */
+	if (lastcpu) {
+		struct thermal_zone_device *tzone = pkgdev->tzone;
+
+		/*
+		 * We must protect against a work function calling
+		 * thermal_zone_update, after/while unregister. We null out
+		 * the pointer under the zone mutex, so the worker function
+		 * won't try to call.
+		 */
+		mutex_lock(&thermal_zone_mutex);
+		pkgdev->tzone = NULL;
+		mutex_unlock(&thermal_zone_mutex);
 
-	target = cpumask_any_but(topology_core_cpumask(cpu), cpu);
-	/* This might be the last cpu in this package */
-	if (target >= nr_cpu_ids) {
-		thermal_zone_device_unregister(pkgdev->tzone);
+		thermal_zone_device_unregister(tzone);
+	}
+
+	/*
+	 * If this is the last CPU in the package, restore the interrupt
+	 * MSR and remove the package reference from the array.
+	 */
+	if (lastcpu) {
+		/* Protect against work and interrupts */
+		spin_lock_irq(&pkg_temp_lock);
+		list_del(&pkgdev->list);
 		/*
-		 * Restore original MSR value for package thermal
-		 * interrupt.
+		 * After this point nothing touches the MSR anymore. We
+		 * must drop the lock to make the cross cpu call. This goes
+		 * away once we move that code to the hotplug state machine.
 		 */
+		spin_unlock_irq(&pkg_temp_lock);
 		wrmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
 			     pkgdev->msr_pkg_therm_low,
 			     pkgdev->msr_pkg_therm_high);
-		list_del(&pkgdev->list);
 		kfree(pkgdev);
-	} else if (pkgdev->cpu == cpu) {
-		pkgdev->cpu = target;
 	}
 
-	mutex_unlock(&phy_dev_list_mutex);
-
-	return 0;
+	/*
+	 * Note, this is broken when work was really scheduled on the
+	 * outgoing cpu because this will leave the work_scheduled flag set
+	 * and the thermal interrupts disabled. Will be fixed in the next
+	 * step as there is no way to fix it in a sane way with the per cpu
+	 * work nonsense.
+	 */
+	cancel_delayed_work_sync(&per_cpu(pkg_temp_thermal_threshold_work, cpu));
 }
 
 static int get_core_online(unsigned int cpu)
@@ -475,20 +495,13 @@ static int get_core_online(unsigned int
 			  pkg_temp_thermal_threshold_work_fn);
 
 	/* If the package exists, nothing to do */
-	if (pkgdev)
+	if (pkgdev) {
+		cpumask_set_cpu(cpu, &pkgdev->cpumask);
 		return 0;
+	}
 	return pkg_temp_thermal_device_add(cpu);
 }
 
-static void put_core_offline(unsigned int cpu)
-{
-	if (!pkg_temp_thermal_device_remove(cpu))
-		cancel_delayed_work_sync(
-			&per_cpu(pkg_temp_thermal_threshold_work, cpu));
-
-	pr_debug("put_core_offline: cpu %d\n", cpu);
-}
-
 static int pkg_temp_thermal_cpu_callback(struct notifier_block *nfb,
 				 unsigned long action, void *hcpu)
 {
@@ -523,8 +536,6 @@ static int __init pkg_temp_thermal_init(
 	if (!x86_match_cpu(pkg_temp_thermal_ids))
 		return -ENODEV;
 
-	spin_lock_init(&pkg_work_lock);
-
 	cpu_notifier_register_begin();
 	for_each_online_cpu(i)
 		if (get_core_online(i))
@@ -550,7 +561,6 @@ module_init(pkg_temp_thermal_init)
 
 static void __exit pkg_temp_thermal_exit(void)
 {
-	struct pkg_device *pkgdev, *n;
 	int i;
 
 	platform_thermal_package_notify = NULL;
@@ -558,20 +568,8 @@ static void __exit pkg_temp_thermal_exit
 
 	cpu_notifier_register_begin();
 	__unregister_hotcpu_notifier(&pkg_temp_thermal_notifier);
-	mutex_lock(&phy_dev_list_mutex);
-	list_for_each_entry_safe(pkgdev, n, &phy_dev_list, list) {
-		/* Retore old MSR value for package thermal interrupt */
-		wrmsr_on_cpu(pkgdev->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
-			     pkgdev->msr_pkg_therm_low,
-			     pkgdev->msr_pkg_therm_high);
-		thermal_zone_device_unregister(pkgdev->tzone);
-		list_del(&pkgdev->list);
-		kfree(pkgdev);
-	}
-	mutex_unlock(&phy_dev_list_mutex);
 	for_each_online_cpu(i)
-		cancel_delayed_work_sync(
-			&per_cpu(pkg_temp_thermal_threshold_work, i));
+		put_core_offline(i);
 	cpu_notifier_register_done();
 
 	kfree(pkg_work_scheduled);

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch V2 09/12] thermal/x86_pkg_temp: Move work scheduled flag into package struct
  2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
                   ` (7 preceding siblings ...)
  2016-11-22 17:57 ` [patch V2 08/12] thermal/x86_pkg_temp: Sanitize locking Thomas Gleixner
@ 2016-11-22 17:57 ` Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 10/12] thermal/x86_pkg_temp: Move work " Thomas Gleixner
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2016-11-22 17:57 UTC (permalink / raw)
  To: LKML
  Cc: Rui Zhang, edubezval, Peter Zijlstra, Borislav Petkov, linux-pm,
	x86, rt, Srinivas Pandruvada

[-- Attachment #1: thermalx86_pkg_temp_Move_work_scheduled_flag_into_package_struct.patch --]
[-- Type: text/plain, Size: 3937 bytes --]

Storage for a boolean information whether work is scheduled for a package
is kept in separate allocated storage, which is resized when the number of
detected packages grows.

With the proper locking in place this is a completely pointless exercise
because we can simply stick it into the per package struct.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/thermal/x86_pkg_temp_thermal.c |   35 ++++-----------------------------
 1 file changed, 5 insertions(+), 30 deletions(-)

--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -61,6 +61,7 @@ struct pkg_device {
 	struct list_head		list;
 	u16				phys_proc_id;
 	u16				cpu;
+	bool				work_scheduled;
 	u32				tj_max;
 	u32				msr_pkg_therm_low;
 	u32				msr_pkg_therm_high;
@@ -82,11 +83,6 @@ static DEFINE_MUTEX(thermal_zone_mutex);
 /* Interrupt to work function schedule queue */
 static DEFINE_PER_CPU(struct delayed_work, pkg_temp_thermal_threshold_work);
 
-/* To track if the work is already scheduled on a package */
-static u8 *pkg_work_scheduled;
-
-static u16 max_phy_id;
-
 /* Debug counters to show using debugfs */
 static struct dentry *debugfs;
 static unsigned int pkg_interrupt_cnt;
@@ -294,7 +290,7 @@ static inline void disable_pkg_thres_int
 static void pkg_temp_thermal_threshold_work_fn(struct work_struct *work)
 {
 	struct thermal_zone_device *tzone = NULL;
-	int phy_id, cpu = smp_processor_id();
+	int cpu = smp_processor_id();
 	struct pkg_device *pkgdev;
 	u64 msr_val, wr_val;
 
@@ -308,8 +304,7 @@ static void pkg_temp_thermal_threshold_w
 		mutex_unlock(&thermal_zone_mutex);
 		return;
 	}
-
-	pkg_work_scheduled[phy_id] = 0;
+	pkgdev->work_scheduled = false;
 
 	rdmsrl(MSR_IA32_PACKAGE_THERM_STATUS, msr_val);
 	wr_val = msr_val & ~(THERM_LOG_THRESHOLD0 | THERM_LOG_THRESHOLD1);
@@ -334,7 +329,6 @@ static void pkg_temp_thermal_threshold_w
 static int pkg_thermal_notify(u64 msr_val)
 {
 	int cpu = smp_processor_id();
-	int phy_id = topology_physical_package_id(cpu);
 	struct pkg_device *pkgdev;
 	unsigned long flags;
 
@@ -345,8 +339,8 @@ static int pkg_thermal_notify(u64 msr_va
 
 	/* Work is per package, so scheduling it once is enough. */
 	pkgdev = pkg_temp_thermal_get_dev(cpu);
-	if (pkgdev && pkg_work_scheduled && !pkg_work_scheduled[phy_id]) {
-		pkg_work_scheduled[phy_id] = 1;
+	if (pkgdev && !pkgdev->work_scheduled) {
+		pkgdev->work_scheduled = true;
 		schedule_delayed_work_on(cpu,
 				&per_cpu(pkg_temp_thermal_threshold_work, cpu),
 				msecs_to_jiffies(notify_delay_ms));
@@ -361,8 +355,6 @@ static int pkg_temp_thermal_device_add(u
 	u32 tj_max, eax, ebx, ecx, edx;
 	struct pkg_device *pkgdev;
 	int thres_count, err;
-	unsigned long flags;
-	u8 *temp;
 
 	cpuid(6, &eax, &ebx, &ecx, &edx);
 	thres_count = ebx & 0x07;
@@ -382,20 +374,6 @@ static int pkg_temp_thermal_device_add(u
 	if (!pkgdev)
 		return -ENOMEM;
 
-	spin_lock_irqsave(&pkg_temp_lock, flags);
-	if (topology_physical_package_id(cpu) > max_phy_id)
-		max_phy_id = topology_physical_package_id(cpu);
-	temp = krealloc(pkg_work_scheduled,
-			(max_phy_id+1) * sizeof(u8), GFP_ATOMIC);
-	if (!temp) {
-		spin_unlock_irqrestore(&pkg_temp_lock, flags);
-		kfree(pkgdev);
-		return -ENOMEM;
-	}
-	pkg_work_scheduled = temp;
-	pkg_work_scheduled[topology_physical_package_id(cpu)] = 0;
-	spin_unlock_irqrestore(&pkg_temp_lock, flags);
-
 	pkgdev->phys_proc_id = topology_physical_package_id(cpu);
 	pkgdev->cpu = cpu;
 	pkgdev->tj_max = tj_max;
@@ -554,7 +532,6 @@ static int __init pkg_temp_thermal_init(
 	for_each_online_cpu(i)
 		put_core_offline(i);
 	cpu_notifier_register_done();
-	kfree(pkg_work_scheduled);
 	return -ENODEV;
 }
 module_init(pkg_temp_thermal_init)
@@ -572,8 +549,6 @@ static void __exit pkg_temp_thermal_exit
 		put_core_offline(i);
 	cpu_notifier_register_done();
 
-	kfree(pkg_work_scheduled);
-
 	debugfs_remove_recursive(debugfs);
 }
 module_exit(pkg_temp_thermal_exit)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch V2 10/12] thermal/x86_pkg_temp: Move work into package struct
  2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
                   ` (8 preceding siblings ...)
  2016-11-22 17:57 ` [patch V2 09/12] thermal/x86_pkg_temp: Move work scheduled flag into package struct Thomas Gleixner
@ 2016-11-22 17:57 ` Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 11/12] thermal/x86_pkg_temp: Sanitize package management Thomas Gleixner
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2016-11-22 17:57 UTC (permalink / raw)
  To: LKML
  Cc: Rui Zhang, edubezval, Peter Zijlstra, Borislav Petkov, linux-pm,
	x86, rt, Srinivas Pandruvada

[-- Attachment #1: thermalx86_pkg_temp_Move_work_into_package_struct.patch --]
[-- Type: text/plain, Size: 5946 bytes --]

Delayed work structs are held in a static percpu storage, which makes no
sense at all because work is strictly per package and we never schedule
more than one work per package.

Aside of that the work cancelation in the hotplug is broken when the work
is queued on the outgoing cpu and canceled. Nothing reschedules the work on
another online cpu in the package, so the interrupts stay disabled and the
work_scheduled flag stays active.

Move the delayed work struct into the package struct, which is the only
sensible place to have it.

To simplify the cancelation logic schedule the work always on the cpu which
is the target for the sysfs files. This is required so the cancelation
logic in the cpu offline path cancels only when the outgoing cpu is the
current target and reschedule the work when there is still a online
CPU in the package.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/thermal/x86_pkg_temp_thermal.c |   73 +++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 21 deletions(-)

--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -65,6 +65,7 @@ struct pkg_device {
 	u32				tj_max;
 	u32				msr_pkg_therm_low;
 	u32				msr_pkg_therm_high;
+	struct delayed_work		work;
 	struct thermal_zone_device	*tzone;
 	struct cpumask			cpumask;
 };
@@ -80,9 +81,6 @@ static DEFINE_SPINLOCK(pkg_temp_lock);
 /* Protects zone operation in the work function against hotplug removal */
 static DEFINE_MUTEX(thermal_zone_mutex);
 
-/* Interrupt to work function schedule queue */
-static DEFINE_PER_CPU(struct delayed_work, pkg_temp_thermal_threshold_work);
-
 /* Debug counters to show using debugfs */
 static struct dentry *debugfs;
 static unsigned int pkg_interrupt_cnt;
@@ -326,6 +324,13 @@ static void pkg_temp_thermal_threshold_w
 	mutex_unlock(&thermal_zone_mutex);
 }
 
+static void pkg_thermal_schedule_work(int cpu, struct delayed_work *work)
+{
+	unsigned long ms = msecs_to_jiffies(notify_delay_ms);
+
+	schedule_delayed_work_on(cpu, work, ms);
+}
+
 static int pkg_thermal_notify(u64 msr_val)
 {
 	int cpu = smp_processor_id();
@@ -341,9 +346,7 @@ static int pkg_thermal_notify(u64 msr_va
 	pkgdev = pkg_temp_thermal_get_dev(cpu);
 	if (pkgdev && !pkgdev->work_scheduled) {
 		pkgdev->work_scheduled = true;
-		schedule_delayed_work_on(cpu,
-				&per_cpu(pkg_temp_thermal_threshold_work, cpu),
-				msecs_to_jiffies(notify_delay_ms));
+		pkg_thermal_schedule_work(pkgdev->cpu, &pkgdev->work);
 	}
 
 	spin_unlock_irqrestore(&pkg_temp_lock, flags);
@@ -374,6 +377,7 @@ static int pkg_temp_thermal_device_add(u
 	if (!pkgdev)
 		return -ENOMEM;
 
+	INIT_DELAYED_WORK(&pkgdev->work, pkg_temp_thermal_threshold_work_fn);
 	pkgdev->phys_proc_id = topology_physical_package_id(cpu);
 	pkgdev->cpu = cpu;
 	pkgdev->tj_max = tj_max;
@@ -401,7 +405,7 @@ static int pkg_temp_thermal_device_add(u
 static void put_core_offline(unsigned int cpu)
 {
 	struct pkg_device *pkgdev = pkg_temp_thermal_get_dev(cpu);
-	bool lastcpu;
+	bool lastcpu, was_target;
 	int target;
 
 	if (!pkgdev)
@@ -430,13 +434,24 @@ static void put_core_offline(unsigned in
 		thermal_zone_device_unregister(tzone);
 	}
 
+	/* Protect against work and interrupts */
+	spin_lock_irq(&pkg_temp_lock);
+
 	/*
-	 * If this is the last CPU in the package, restore the interrupt
-	 * MSR and remove the package reference from the array.
+	 * Check whether this cpu was the current target and store the new
+	 * one. When we drop the lock, then the interrupt notify function
+	 * will see the new target.
+	 */
+	was_target = pkgdev->cpu == cpu;
+	pkgdev->cpu = target;
+
+	/*
+	 * If this is the last CPU in the package remove the package
+	 * reference from the list and restore the interrupt MSR. When we
+	 * drop the lock neither the interrupt notify function nor the
+	 * worker will see the package anymore.
 	 */
 	if (lastcpu) {
-		/* Protect against work and interrupts */
-		spin_lock_irq(&pkg_temp_lock);
 		list_del(&pkgdev->list);
 		/*
 		 * After this point nothing touches the MSR anymore. We
@@ -447,17 +462,36 @@ static void put_core_offline(unsigned in
 		wrmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
 			     pkgdev->msr_pkg_therm_low,
 			     pkgdev->msr_pkg_therm_high);
-		kfree(pkgdev);
+		spin_lock_irq(&pkg_temp_lock);
 	}
 
 	/*
-	 * Note, this is broken when work was really scheduled on the
-	 * outgoing cpu because this will leave the work_scheduled flag set
-	 * and the thermal interrupts disabled. Will be fixed in the next
-	 * step as there is no way to fix it in a sane way with the per cpu
-	 * work nonsense.
+	 * Check whether there is work scheduled and whether the work is
+	 * targeted at the outgoing CPU.
 	 */
-	cancel_delayed_work_sync(&per_cpu(pkg_temp_thermal_threshold_work, cpu));
+	if (pkgdev->work_scheduled && was_target) {
+		/*
+		 * To cancel the work we need to drop the lock, otherwise
+		 * we might deadlock if the work needs to be flushed.
+		 */
+		spin_unlock_irq(&pkg_temp_lock);
+		cancel_delayed_work_sync(&pkgdev->work);
+		spin_lock_irq(&pkg_temp_lock);
+		/*
+		 * If this is not the last cpu in the package and the work
+		 * did not run after we dropped the lock above, then we
+		 * need to reschedule the work, otherwise the interrupt
+		 * stays disabled forever.
+		 */
+		if (!lastcpu && pkgdev->work_scheduled)
+			pkg_thermal_schedule_work(target, &pkgdev->work);
+	}
+
+	spin_unlock_irq(&pkg_temp_lock);
+
+	/* Final cleanup if this is the last cpu */
+	if (lastcpu)
+		kfree(pkgdev);
 }
 
 static int get_core_online(unsigned int cpu)
@@ -469,9 +503,6 @@ static int get_core_online(unsigned int
 	if (!cpu_has(c, X86_FEATURE_DTHERM) || !cpu_has(c, X86_FEATURE_PTS))
 		return -ENODEV;
 
-	INIT_DELAYED_WORK(&per_cpu(pkg_temp_thermal_threshold_work, cpu),
-			  pkg_temp_thermal_threshold_work_fn);
-
 	/* If the package exists, nothing to do */
 	if (pkgdev) {
 		cpumask_set_cpu(cpu, &pkgdev->cpumask);

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch V2 11/12] thermal/x86_pkg_temp: Sanitize package management
  2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
                   ` (9 preceding siblings ...)
  2016-11-22 17:57 ` [patch V2 10/12] thermal/x86_pkg_temp: Move work " Thomas Gleixner
@ 2016-11-22 17:57 ` Thomas Gleixner
  2016-11-22 17:57 ` [patch V2 12/12] thermal/x86 pkg temp: Convert to hotplug state machine Thomas Gleixner
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2016-11-22 17:57 UTC (permalink / raw)
  To: LKML
  Cc: Rui Zhang, edubezval, Peter Zijlstra, Borislav Petkov, linux-pm,
	x86, rt, Srinivas Pandruvada

[-- Attachment #1: thermalx86_pkg_temp_Sanitize_package_management.patch --]
[-- Type: text/plain, Size: 4573 bytes --]

Packages are kept in a list, which must be searched over and over.

We can be smarter than that and just store the package pointers in an array
which is allocated at init time. Sizing of the array is determined from the
topology information. That makes the package search a simple array lookup.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/thermal/x86_pkg_temp_thermal.c |   42 +++++++++++++++++----------------
 1 file changed, 22 insertions(+), 20 deletions(-)

--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -54,13 +54,9 @@ MODULE_PARM_DESC(notify_delay_ms,
 * is some wrong values returned by cpuid for number of thresholds.
 */
 #define MAX_NUMBER_OF_TRIPS	2
-/* Limit number of package temp zones */
-#define MAX_PKG_TEMP_ZONE_IDS	256
 
 struct pkg_device {
-	struct list_head		list;
-	u16				phys_proc_id;
-	u16				cpu;
+	int				cpu;
 	bool				work_scheduled;
 	u32				tj_max;
 	u32				msr_pkg_therm_low;
@@ -74,8 +70,10 @@ static struct thermal_zone_params pkg_te
 	.no_hwmon	= true,
 };
 
-/* List maintaining number of package instances */
-static LIST_HEAD(phy_dev_list);
+/* Keep track of how many package pointers we allocated in init() */
+static int max_packages __read_mostly;
+/* Array of package pointers */
+static struct pkg_device **packages;
 /* Serializes interrupt notification, work and hotplug */
 static DEFINE_SPINLOCK(pkg_temp_lock);
 /* Protects zone operation in the work function against hotplug removal */
@@ -121,13 +119,10 @@ static int pkg_temp_debugfs_init(void)
  */
 static struct pkg_device *pkg_temp_thermal_get_dev(unsigned int cpu)
 {
-	u16 phys_proc_id = topology_physical_package_id(cpu);
-	struct pkg_device *pkgdev;
+	int pkgid = topology_logical_package_id(cpu);
 
-	list_for_each_entry(pkgdev, &phy_dev_list, list) {
-		if (pkgdev->phys_proc_id == phys_proc_id)
-			return pkgdev;
-	}
+	if (pkgid >= 0 && pkgid < max_packages)
+		return packages[pkgid];
 	return NULL;
 }
 
@@ -355,18 +350,19 @@ static int pkg_thermal_notify(u64 msr_va
 
 static int pkg_temp_thermal_device_add(unsigned int cpu)
 {
+	int pkgid = topology_logical_package_id(cpu);
 	u32 tj_max, eax, ebx, ecx, edx;
 	struct pkg_device *pkgdev;
 	int thres_count, err;
 
+	if (pkgid >= max_packages)
+		return -ENOMEM;
+
 	cpuid(6, &eax, &ebx, &ecx, &edx);
 	thres_count = ebx & 0x07;
 	if (!thres_count)
 		return -ENODEV;
 
-	if (topology_physical_package_id(cpu) > MAX_PKG_TEMP_ZONE_IDS)
-		return -ENODEV;
-
 	thres_count = clamp_val(thres_count, 0, MAX_NUMBER_OF_TRIPS);
 
 	err = get_tj_max(cpu, &tj_max);
@@ -378,7 +374,6 @@ static int pkg_temp_thermal_device_add(u
 		return -ENOMEM;
 
 	INIT_DELAYED_WORK(&pkgdev->work, pkg_temp_thermal_threshold_work_fn);
-	pkgdev->phys_proc_id = topology_physical_package_id(cpu);
 	pkgdev->cpu = cpu;
 	pkgdev->tj_max = tj_max;
 	pkgdev->tzone = thermal_zone_device_register("x86_pkg_temp",
@@ -397,7 +392,7 @@ static int pkg_temp_thermal_device_add(u
 
 	cpumask_set_cpu(cpu, &pkgdev->cpumask);
 	spin_lock_irq(&pkg_temp_lock);
-	list_add_tail(&pkgdev->list, &phy_dev_list);
+	packages[pkgid] = pkgdev;
 	spin_unlock_irq(&pkg_temp_lock);
 	return 0;
 }
@@ -447,12 +442,12 @@ static void put_core_offline(unsigned in
 
 	/*
 	 * If this is the last CPU in the package remove the package
-	 * reference from the list and restore the interrupt MSR. When we
+	 * reference from the array and restore the interrupt MSR. When we
 	 * drop the lock neither the interrupt notify function nor the
 	 * worker will see the package anymore.
 	 */
 	if (lastcpu) {
-		list_del(&pkgdev->list);
+		packages[topology_logical_package_id(cpu)] = NULL;
 		/*
 		 * After this point nothing touches the MSR anymore. We
 		 * must drop the lock to make the cross cpu call. This goes
@@ -545,6 +540,11 @@ static int __init pkg_temp_thermal_init(
 	if (!x86_match_cpu(pkg_temp_thermal_ids))
 		return -ENODEV;
 
+	max_packages = topology_max_packages();
+	packages = kzalloc(max_packages * sizeof(struct pkg_device *), GFP_KERNEL);
+	if (!packages)
+		return -ENOMEM;
+
 	cpu_notifier_register_begin();
 	for_each_online_cpu(i)
 		if (get_core_online(i))
@@ -563,6 +563,7 @@ static int __init pkg_temp_thermal_init(
 	for_each_online_cpu(i)
 		put_core_offline(i);
 	cpu_notifier_register_done();
+	kfree(packages);
 	return -ENODEV;
 }
 module_init(pkg_temp_thermal_init)
@@ -581,6 +582,7 @@ static void __exit pkg_temp_thermal_exit
 	cpu_notifier_register_done();
 
 	debugfs_remove_recursive(debugfs);
+	kfree(packages);
 }
 module_exit(pkg_temp_thermal_exit)
 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch V2 12/12] thermal/x86 pkg temp: Convert to hotplug state machine
  2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
                   ` (10 preceding siblings ...)
  2016-11-22 17:57 ` [patch V2 11/12] thermal/x86_pkg_temp: Sanitize package management Thomas Gleixner
@ 2016-11-22 17:57 ` Thomas Gleixner
  2016-11-22 19:51 ` [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Pandruvada, Srinivas
  2016-11-30  5:27 ` Eduardo Valentin
  13 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2016-11-22 17:57 UTC (permalink / raw)
  To: LKML
  Cc: Rui Zhang, edubezval, Peter Zijlstra, Borislav Petkov, linux-pm,
	x86, rt, Srinivas Pandruvada, Sebastian Andrzej Siewior

[-- Attachment #1: thermalx86_pkg_temp_Convert_to_hotplug_state_machine.patch --]
[-- Type: text/plain, Size: 5359 bytes --]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Install the callbacks via the state machine and let the core invoke
the callbacks on the already online CPUs.

Replace the wrmsr/rdmrs_on_cpu() calls in the hotplug callbacks as they are
guaranteed to be invoked on the incoming/outgoing cpu.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/thermal/x86_pkg_temp_thermal.c |   80 +++++++++------------------------
 1 file changed, 23 insertions(+), 57 deletions(-)

--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -79,6 +79,9 @@ static DEFINE_SPINLOCK(pkg_temp_lock);
 /* Protects zone operation in the work function against hotplug removal */
 static DEFINE_MUTEX(thermal_zone_mutex);
 
+/* The dynamically assigned cpu hotplug state for module_exit() */
+static enum cpuhp_state pkg_thermal_hp_state __read_mostly;
+
 /* Debug counters to show using debugfs */
 static struct dentry *debugfs;
 static unsigned int pkg_interrupt_cnt;
@@ -386,9 +389,8 @@ static int pkg_temp_thermal_device_add(u
 		return err;
 	}
 	/* Store MSR value for package thermal interrupt, to restore at exit */
-	rdmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
-		     &pkgdev->msr_pkg_therm_low,
-		     &pkgdev->msr_pkg_therm_high);
+	rdmsr(MSR_IA32_PACKAGE_THERM_INTERRUPT, pkgdev->msr_pkg_therm_low,
+	      pkgdev->msr_pkg_therm_high);
 
 	cpumask_set_cpu(cpu, &pkgdev->cpumask);
 	spin_lock_irq(&pkg_temp_lock);
@@ -397,14 +399,14 @@ static int pkg_temp_thermal_device_add(u
 	return 0;
 }
 
-static void put_core_offline(unsigned int cpu)
+static int pkg_thermal_cpu_offline(unsigned int cpu)
 {
 	struct pkg_device *pkgdev = pkg_temp_thermal_get_dev(cpu);
 	bool lastcpu, was_target;
 	int target;
 
 	if (!pkgdev)
-		return;
+		return 0;
 
 	target = cpumask_any_but(&pkgdev->cpumask, cpu);
 	cpumask_clear_cpu(cpu, &pkgdev->cpumask);
@@ -448,16 +450,9 @@ static void put_core_offline(unsigned in
 	 */
 	if (lastcpu) {
 		packages[topology_logical_package_id(cpu)] = NULL;
-		/*
-		 * After this point nothing touches the MSR anymore. We
-		 * must drop the lock to make the cross cpu call. This goes
-		 * away once we move that code to the hotplug state machine.
-		 */
-		spin_unlock_irq(&pkg_temp_lock);
-		wrmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
-			     pkgdev->msr_pkg_therm_low,
-			     pkgdev->msr_pkg_therm_high);
-		spin_lock_irq(&pkg_temp_lock);
+		/* After this point nothing touches the MSR anymore. */
+		wrmsr(MSR_IA32_PACKAGE_THERM_INTERRUPT,
+		      pkgdev->msr_pkg_therm_low, pkgdev->msr_pkg_therm_high);
 	}
 
 	/*
@@ -487,9 +482,10 @@ static void put_core_offline(unsigned in
 	/* Final cleanup if this is the last cpu */
 	if (lastcpu)
 		kfree(pkgdev);
+	return 0;
 }
 
-static int get_core_online(unsigned int cpu)
+static int pkg_thermal_cpu_online(unsigned int cpu)
 {
 	struct pkg_device *pkgdev = pkg_temp_thermal_get_dev(cpu);
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
@@ -506,27 +502,6 @@ static int get_core_online(unsigned int
 	return pkg_temp_thermal_device_add(cpu);
 }
 
-static int pkg_temp_thermal_cpu_callback(struct notifier_block *nfb,
-				 unsigned long action, void *hcpu)
-{
-	unsigned int cpu = (unsigned long) hcpu;
-
-	switch (action & ~CPU_TASKS_FROZEN) {
-	case CPU_ONLINE:
-	case CPU_DOWN_FAILED:
-		get_core_online(cpu);
-		break;
-	case CPU_DOWN_PREPARE:
-		put_core_offline(cpu);
-		break;
-	}
-	return NOTIFY_OK;
-}
-
-static struct notifier_block pkg_temp_thermal_notifier __refdata = {
-	.notifier_call = pkg_temp_thermal_cpu_callback,
-};
-
 static const struct x86_cpu_id __initconst pkg_temp_thermal_ids[] = {
 	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_PTS },
 	{}
@@ -535,7 +510,7 @@ MODULE_DEVICE_TABLE(x86cpu, pkg_temp_the
 
 static int __init pkg_temp_thermal_init(void)
 {
-	int i;
+	int ret;
 
 	if (!x86_match_cpu(pkg_temp_thermal_ids))
 		return -ENODEV;
@@ -545,12 +520,13 @@ static int __init pkg_temp_thermal_init(
 	if (!packages)
 		return -ENOMEM;
 
-	cpu_notifier_register_begin();
-	for_each_online_cpu(i)
-		if (get_core_online(i))
-			goto err_ret;
-	__register_hotcpu_notifier(&pkg_temp_thermal_notifier);
-	cpu_notifier_register_done();
+	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "thermal/x86_pkg:online",
+				pkg_thermal_cpu_online,	pkg_thermal_cpu_offline);
+	if (ret < 0)
+		goto err;
+
+	/* Store the state for module exit */
+	pkg_thermal_hp_state = ret;
 
 	platform_thermal_package_notify = pkg_thermal_notify;
 	platform_thermal_package_rate_control = pkg_thermal_rate_control;
@@ -559,28 +535,18 @@ static int __init pkg_temp_thermal_init(
 	pkg_temp_debugfs_init();
 	return 0;
 
-err_ret:
-	for_each_online_cpu(i)
-		put_core_offline(i);
-	cpu_notifier_register_done();
+err:
 	kfree(packages);
-	return -ENODEV;
+	return ret;
 }
 module_init(pkg_temp_thermal_init)
 
 static void __exit pkg_temp_thermal_exit(void)
 {
-	int i;
-
 	platform_thermal_package_notify = NULL;
 	platform_thermal_package_rate_control = NULL;
 
-	cpu_notifier_register_begin();
-	__unregister_hotcpu_notifier(&pkg_temp_thermal_notifier);
-	for_each_online_cpu(i)
-		put_core_offline(i);
-	cpu_notifier_register_done();
-
+	cpuhp_remove_state(pkg_thermal_hp_state);
 	debugfs_remove_recursive(debugfs);
 	kfree(packages);
 }

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking
  2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
                   ` (11 preceding siblings ...)
  2016-11-22 17:57 ` [patch V2 12/12] thermal/x86 pkg temp: Convert to hotplug state machine Thomas Gleixner
@ 2016-11-22 19:51 ` Pandruvada, Srinivas
  2016-11-30 12:30   ` Zhang Rui
  2016-11-30  5:27 ` Eduardo Valentin
  13 siblings, 1 reply; 16+ messages in thread
From: Pandruvada, Srinivas @ 2016-11-22 19:51 UTC (permalink / raw)
  To: tglx, linux-kernel; +Cc: Zhang, Rui, edubezval, peterz, rt, linux-pm, bp, x86

On Tue, 2016-11-22 at 17:57 +0000, Thomas Gleixner wrote:
> Changes vs. V1: Fix the package removal wreckage reported by Srinivas
> 

I haven't looked at individual patch but tested the series as a whole. 

So Rui, you can add

Tested-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Thanks,
Srinivas

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking
  2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
                   ` (12 preceding siblings ...)
  2016-11-22 19:51 ` [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Pandruvada, Srinivas
@ 2016-11-30  5:27 ` Eduardo Valentin
  13 siblings, 0 replies; 16+ messages in thread
From: Eduardo Valentin @ 2016-11-30  5:27 UTC (permalink / raw)
  To: Thomas Gleixner, Rui Zhang
  Cc: LKML, Peter Zijlstra, Borislav Petkov, linux-pm, x86, rt,
	Srinivas Pandruvada

Rui,

On Tue, Nov 22, 2016 at 05:57:04PM -0000, Thomas Gleixner wrote:

<cut>

> ---
>  x86_pkg_temp_thermal.c |  593 ++++++++++++++++++++-----------------------------
>  1 file changed, 249 insertions(+), 344 deletions(-)

I am assuming you are handling this one in your tree directly. In any
case, you can add my:

Acked-by: Eduardo Valentin <edubezval@gmail>

for the series.

BR,

Eduardo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking
  2016-11-22 19:51 ` [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Pandruvada, Srinivas
@ 2016-11-30 12:30   ` Zhang Rui
  0 siblings, 0 replies; 16+ messages in thread
From: Zhang Rui @ 2016-11-30 12:30 UTC (permalink / raw)
  To: Pandruvada, Srinivas, tglx, linux-kernel
  Cc: edubezval, peterz, rt, linux-pm, bp, x86

On Tue, 2016-11-22 at 12:51 -0700, Pandruvada, Srinivas wrote:
> On Tue, 2016-11-22 at 17:57 +0000, Thomas Gleixner wrote:
> > 
> > Changes vs. V1: Fix the package removal wreckage reported by
> > Srinivas
> > 
> I haven't looked at individual patch but tested the series as a
> whole. 
> 
> So Rui, you can add
> 
> Tested-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> 

applied.

thanks,
rui

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2016-11-30 12:30 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-22 17:57 [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner
2016-11-22 17:57 ` [patch V2 01/12] thermal/x86_pkg_temp: Cleanup thermal interrupt handling Thomas Gleixner
2016-11-22 17:57 ` [patch V2 02/12] thermal/x86_pkg_temp: Remove redundant package search Thomas Gleixner
2016-11-22 17:57 ` [patch V2 04/12] thermal/x86_pkg_temp: Sanitize callback (de)initialization Thomas Gleixner
2016-11-22 17:57 ` [patch V2 03/12] thermal/x86_pkg_temp: Replace open coded cpu search Thomas Gleixner
2016-11-22 17:57 ` [patch V2 05/12] thermal/x86_pkg_temp: Get rid of ref counting Thomas Gleixner
2016-11-22 17:57 ` [patch V2 06/12] thermal/x86_pkg_temp: Cleanup namespace Thomas Gleixner
2016-11-22 17:57 ` [patch V2 07/12] thermal/x86_pkg_temp: Cleanup code some more Thomas Gleixner
2016-11-22 17:57 ` [patch V2 08/12] thermal/x86_pkg_temp: Sanitize locking Thomas Gleixner
2016-11-22 17:57 ` [patch V2 09/12] thermal/x86_pkg_temp: Move work scheduled flag into package struct Thomas Gleixner
2016-11-22 17:57 ` [patch V2 10/12] thermal/x86_pkg_temp: Move work " Thomas Gleixner
2016-11-22 17:57 ` [patch V2 11/12] thermal/x86_pkg_temp: Sanitize package management Thomas Gleixner
2016-11-22 17:57 ` [patch V2 12/12] thermal/x86 pkg temp: Convert to hotplug state machine Thomas Gleixner
2016-11-22 19:51 ` [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Pandruvada, Srinivas
2016-11-30 12:30   ` Zhang Rui
2016-11-30  5:27 ` Eduardo Valentin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).