Skip to contentSkip to conversation

Fix thermal problems during suspend/bootup

PATCH Superseded Chen Yu 2 participants 7 messages Oct 26, 2015 – Oct 30, 2015

Subsystems (2)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Chen Yu

author
#0 lore raw

v2 cover Fix thermal problems during suspend/bootup

This patch set fixes two problems when system is trying to suspend and boot up:
1.After system is woken up from suspend, the thermal framework uses
the dirty 'cached' thermal variables before suspend, which might cause expected behavior.
2.If a cooling device is registered after the thermal zone's registration,
current thermal framework forgets to update the thermal_zone's status, which might bring expected behavior under special cases.

Chen Yu (3):
Thermal: initialize thermal zone device correctly
Thermal: handle thermal zone device properly during system sleep
Thermal: do thermal zone update after a cooling device registered

 drivers/thermal/step_wise.c    | 17 +++++++--
 drivers/thermal/thermal_core.c | 78 +++++++++++++++++++++++++++++++++++++++---
 drivers/thermal/thermal_core.h |  1 +
 include/linux/thermal.h        |  4 +++
 4 files changed, 94 insertions(+), 6 deletions(-)
Signature · 1 line
1.8.4.2
author
#1 lore raw

PATCH v2 1/3 Thermal: initialize thermal zone device correctly

From: Zhang Rui <[email protected]>

After thermal zone device registered, as we have not read any temperature before, thus tz->temperature should not be 0, which actually means 0C, and thermal trend is not available. In this case, we need specially handling for the first thermal_zone_device_update().

Both thermal core framework and step_wise governor is enhanced to handle this. And since the step_wise governor is the only one that uses trends, so it's the only thermal governor that needs to be updated.

---
 drivers/thermal/step_wise.c    | 17 +++++++++++++++--
 drivers/thermal/thermal_core.c | 19 +++++++++++++++++--
 drivers/thermal/thermal_core.h |  1 +
 include/linux/thermal.h        |  3 +++
 4 files changed, 36 insertions(+), 4 deletions(-)
drivers/thermal/step_wise.c +15 -2
index 2f9f708..ea9366a 100644
@@ -63,6 +63,19 @@ static unsigned long get_target_state(struct thermal_instance *instance,
 	next_target = instance->target;
 	dev_dbg(&cdev->device, "cur_state=%ld\n", cur_state);
 
+	if (!instance->initialized) {
+		if (throttle) {
+			next_target = (cur_state + 1) >= instance->upper ?
+					instance->upper :
+					((cur_state + 1) < instance->lower ?
+					instance->lower : (cur_state + 1));
+		} else {
+			next_target = THERMAL_NO_TARGET;
+		}
+
+		return next_target;
+	}
+
 	switch (trend) {
 	case THERMAL_TREND_RAISING:
 		if (throttle) {
@@ -149,7 +162,7 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 		dev_dbg(&instance->cdev->device, "old_target=%d, target=%d\n",
 					old_target, (int)instance->target);
 
-		if (old_target == instance->target)
+		if (instance->initialized && old_target == instance->target)
 			continue;
 
 		/* Activate a passive thermal instance */
@@ -161,7 +174,7 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 			instance->target == THERMAL_NO_TARGET)
 			update_passive_instance(tz, trip_type, -1);
 
-
+		instance->initialized = true;
 		instance->cdev->updated = false; /* cdev needs update */
 	}
 
drivers/thermal/thermal_core.c +17 -2
index d9e525c..682bc1e 100644
@@ -532,8 +532,22 @@ static void update_temperature(struct thermal_zone_device *tz)
 	mutex_unlock(&tz->lock);
 
 	trace_thermal_temperature(tz);
-	dev_dbg(&tz->device, "last_temperature=%d, current_temperature=%d\n",
-				tz->last_temperature, tz->temperature);
+	if (tz->last_temperature == THERMAL_TEMP_INVALID)
+		dev_dbg(&tz->device, "last_temperature N/A, current_temperature=%d\n",
+			tz->temperature);
+	else
+		dev_dbg(&tz->device, "last_temperature=%d, current_temperature=%d\n",
+			tz->last_temperature, tz->temperature);
+}
+
+static void thermal_zone_device_reset(struct thermal_zone_device *tz)
+{
+	struct thermal_instance *pos;
+
+	tz->temperature = THERMAL_TEMP_INVALID;
+	tz->passive = 0;
+	list_for_each_entry(pos, &tz->thermal_instances, tz_node)
+		pos->initialized = false;
 }
 
 void thermal_zone_device_update(struct thermal_zone_device *tz)
@@ -1900,6 +1914,7 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 
 	INIT_DELAYED_WORK(&(tz->poll_queue), thermal_zone_device_check);
 
+	thermal_zone_device_reset(tz);
 	thermal_zone_device_update(tz);
 
 	return tz;
drivers/thermal/thermal_core.h +1 -0
index d7ac1fc..749d41a 100644
@@ -41,6 +41,7 @@ struct thermal_instance {
 	struct thermal_zone_device *tz;
 	struct thermal_cooling_device *cdev;
 	int trip;
+	bool initialized;
 	unsigned long upper;	/* Highest cooling state for this trip point */
 	unsigned long lower;	/* Lowest cooling state for this trip point */
 	unsigned long target;	/* expected cooling state */
include/linux/thermal.h +3 -0
index 157d366..5bcabc7 100644
@@ -43,6 +43,9 @@
 /* Default weight of a bound cooling device */
 #define THERMAL_WEIGHT_DEFAULT 0
 
+/* use value, which < 0K, to indicate an invalid/uninitialized temperature */
+#define THERMAL_TEMP_INVALID	-274000
+
 /* Unit conversion macros */
 #define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
 				((long)t-2732+5)/10 : ((long)t-2732-5)/10)
Signature · 1 line
1.8.4.2
author
#2 lore raw

PATCH v2 2/3 Thermal: handle thermal zone device properly during system sleep

In reply to Chen Yu

From: Zhang Rui <[email protected]>

Current thermal code does not handle system sleep well because
1. the cooling device cooling state may be changed during suspend
2. the previous temperature reading becomes invalid after resumed because it is got before system sleep
3. updating thermal zone device during suspending/resuming
is wrong because some devices may have already been suspended or may have not been resumed.

Thus, the proper way to do this is to cancel all thermal zone device update requirements during suspend/resume, and after all the devices have been resumed, reset and update every registered thermal zone devices.

This also fixes a regression introduced by:
Commit 19593a1fb1f6 ("ACPI / fan: convert to platform driver") Because, with above commit applied, all the fan devices are attached to the acpi_general_pm_domain, and they are turned on by the pm_domain automatically after resume, without the awareness of thermal core.

CC: <[email protected]> #3.18+
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=78201
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=91411

---
 drivers/thermal/thermal_core.c | 45 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)
drivers/thermal/thermal_core.c +44 -1
index 682bc1e..abeb995 100644
@@ -37,6 +37,7 @@
 #include <linux/of.h>
 #include <net/netlink.h>
 #include <net/genetlink.h>
+#include <linux/suspend.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/thermal.h>
@@ -59,6 +60,8 @@ static LIST_HEAD(thermal_governor_list);
 static DEFINE_MUTEX(thermal_list_lock);
 static DEFINE_MUTEX(thermal_governor_lock);
 
+static atomic_t in_suspend;
+
 static struct thermal_governor *def_governor;
 
 static struct thermal_governor *__find_governor(const char *name)
@@ -554,6 +557,9 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
 {
 	int count;
 
+	if (atomic_read(&in_suspend))
+		return;
+
 	if (!tz->ops->get_temp)
 		return;
 
@@ -2155,9 +2161,39 @@ static void thermal_unregister_governors(void)
 	thermal_gov_power_allocator_unregister();
 }
 
+static int thermal_pm_notify(struct notifier_block *nb,
+				unsigned long mode, void *_unused)
+{
+	struct thermal_zone_device *tz;
+
+	switch (mode) {
+	case PM_HIBERNATION_PREPARE:
+	case PM_RESTORE_PREPARE:
+	case PM_SUSPEND_PREPARE:
+		atomic_set(&in_suspend, 1);
+		break;
+	case PM_POST_HIBERNATION:
+	case PM_POST_RESTORE:
+	case PM_POST_SUSPEND:
+		atomic_set(&in_suspend, 0);
+		list_for_each_entry(tz, &thermal_tz_list, node) {
+			thermal_zone_device_reset(tz);
+			thermal_zone_device_update(tz);
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static struct notifier_block thermal_pm_nb = {
+	.notifier_call = thermal_pm_notify,
+};
+
 static int __init thermal_init(void)
 {
-	int result;
+	int result, notifier_result;
 
 	result = thermal_register_governors();
 	if (result)
@@ -2175,6 +2211,12 @@ static int __init thermal_init(void)
 	if (result)
 		goto exit_netlink;
 
+	notifier_result = register_pm_notifier(&thermal_pm_nb);
+	if (notifier_result)
+		pr_err("Thermal: Can not register suspend notifier"
+			"for thermal framework, return %d\n",
+			notifier_result);
+
 	return 0;
 
 exit_netlink:
@@ -2194,6 +2236,7 @@ error:
 
 static void __exit thermal_exit(void)
 {
+	unregister_pm_notifier(&thermal_pm_nb);
 	of_thermal_destroy_zones();
 	genetlink_exit();
 	class_unregister(&thermal_class);
Signature · 1 line
1.8.4.2
Reviewed-by on v2 2/3
#3 lore raw

Re: [PATCH 2/3][v2] Thermal: handle thermal zone device properly during system sleep

Hi Yu,

some minor nits below. Other than that, you can add my

Quoted 108 lines · Chen Yu

From: Zhang Rui <[email protected]>

Current thermal code does not handle system sleep well because
1. the cooling device cooling state may be changed during suspend
2. the previous temperature reading becomes invalid after resumed because it is got before system sleep
3. updating thermal zone device during suspending/resuming
is wrong because some devices may have already been suspended or may have not been resumed.

Thus, the proper way to do this is to cancel all thermal zone device update requirements during suspend/resume, and after all the devices have been resumed, reset and update every registered thermal zone devices.

This also fixes a regression introduced by:
Commit 19593a1fb1f6 ("ACPI / fan: convert to platform driver") Because, with above commit applied, all the fan devices are attached to the acpi_general_pm_domain, and they are turned on by the pm_domain automatically after resume, without the awareness of thermal core.

CC: <[email protected]> #3.18+
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=78201
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=91411

---
 drivers/thermal/thermal_core.c | 45 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)
drivers/thermal/thermal_core.c +37 -1
index 682bc1e..abeb995 100644
@@ -37,6 +37,7 @@
 #include <linux/of.h>
 #include <net/netlink.h>
 #include <net/genetlink.h>
+#include <linux/suspend.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/thermal.h>
@@ -59,6 +60,8 @@ static LIST_HEAD(thermal_governor_list);
 static DEFINE_MUTEX(thermal_list_lock);
 static DEFINE_MUTEX(thermal_governor_lock);
 
+static atomic_t in_suspend;
+
 static struct thermal_governor *def_governor;
 
 static struct thermal_governor *__find_governor(const char *name)
@@ -554,6 +557,9 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
 {
 	int count;
 
+	if (atomic_read(&in_suspend))
+		return;
+
 	if (!tz->ops->get_temp)
 		return;
 
@@ -2155,9 +2161,39 @@ static void thermal_unregister_governors(void)
 	thermal_gov_power_allocator_unregister();
 }
 
+static int thermal_pm_notify(struct notifier_block *nb,
+				unsigned long mode, void *_unused)
+{
+	struct thermal_zone_device *tz;
+
+	switch (mode) {
+	case PM_HIBERNATION_PREPARE:
+	case PM_RESTORE_PREPARE:
+	case PM_SUSPEND_PREPARE:
+		atomic_set(&in_suspend, 1);
+		break;
+	case PM_POST_HIBERNATION:
+	case PM_POST_RESTORE:
+	case PM_POST_SUSPEND:
+		atomic_set(&in_suspend, 0);
+		list_for_each_entry(tz, &thermal_tz_list, node) {
+			thermal_zone_device_reset(tz);
+			thermal_zone_device_update(tz);
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static struct notifier_block thermal_pm_nb = {
+	.notifier_call = thermal_pm_notify,
+};
+
 static int __init thermal_init(void)
 {
-	int result;
+	int result, notifier_result;
 
 	result = thermal_register_governors();
 	if (result)

drivers/thermal/thermal_core.c static int __init thermal_init(void) · line 2214

Quoted 4 lines
drivers/thermal/thermal_core.c +1 -0
@@ -2175,6 +2211,12 @@ static int __init thermal_init(void)
 	if (result)
 		goto exit_netlink;
 
+	notifier_result = register_pm_notifier(&thermal_pm_nb);

No need to define a new variable, you can use result.

drivers/thermal/thermal_core.c static int __init thermal_init(void) · line 2216

Quoted 2 lines
drivers/thermal/thermal_core.c +2 -0
+	if (notifier_result)
+		pr_err("Thermal: Can not register suspend notifier"

pr_warn ?

drivers/thermal/thermal_core.c static int __init thermal_init(void) · line 2217

Quoted 1 line
drivers/thermal/thermal_core.c +1 -0
+			"for thermal framework, return %d\n",

Please keep the string in the same line, don't break it. I know that checkpatch.pl complains, but the CodingStyle says don't break them:

"However, never break user-visible strings such as printk messages, because that breaks the ability to grep for them"

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle#n86

Cheers,
Javi

Quoted 16 lines
drivers/thermal/thermal_core.c +3 -0
+			notifier_result);
+
 	return 0;
 
 exit_netlink:
@@ -2194,6 +2236,7 @@ error:
 
 static void __exit thermal_exit(void)
 {
+	unregister_pm_notifier(&thermal_pm_nb);
 	of_thermal_destroy_zones();
 	genetlink_exit();
 	class_unregister(&thermal_class);
Signature · 1 line
1.8.4.2
author
#4 lore raw

PATCH v2 3/3 Thermal: do thermal zone update after a cooling device registered

In reply to Chen Yu

When a new cooling device is registered, we need to update the thermal zone to set the new registered cooling device to a proper state.

This fixes a problem that the system is cool, while the fan devices are left running on full speed after boot, if fan device is registered after thermal zone device.

Here is the history of why current patch looks like this:
https://patchwork.kernel.org/patch/7273041/

CC: <[email protected]> #3.18+ Reference:https://bugzilla.kernel.org/show_bug.cgi?id=92431

---
 drivers/thermal/thermal_core.c | 14 +++++++++++++-
 include/linux/thermal.h        |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)
drivers/thermal/thermal_core.c +13 -1
index abeb995..f36d0bd 100644
@@ -1341,6 +1341,7 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
 	if (!result) {
 		list_add_tail(&dev->tz_node, &tz->thermal_instances);
 		list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
+		atomic_set(&tz->need_update, 1);
 	}
 	mutex_unlock(&cdev->lock);
 	mutex_unlock(&tz->lock);
@@ -1450,6 +1451,7 @@ __thermal_cooling_device_register(struct device_node *np,
 				  const struct thermal_cooling_device_ops *ops)
 {
 	struct thermal_cooling_device *cdev;
+	struct thermal_zone_device *pos = NULL;
 	int result;
 
 	if (type && strlen(type) >= THERMAL_NAME_LENGTH)
@@ -1494,6 +1496,12 @@ __thermal_cooling_device_register(struct device_node *np,
 	/* Update binding information for 'this' new cdev */
 	bind_cdev(cdev);
 
+	mutex_lock(&thermal_list_lock);
+	list_for_each_entry(pos, &thermal_tz_list, node)
+		if (atomic_cmpxchg(&pos->need_update, 1, 0))
+			thermal_zone_device_update(pos);
+	mutex_unlock(&thermal_list_lock);
+
 	return cdev;
 }
 
@@ -1826,6 +1834,8 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 	tz->trips = trips;
 	tz->passive_delay = passive_delay;
 	tz->polling_delay = polling_delay;
+	/* A new thermal zone needs to be updated anyway. */
+	atomic_set(&tz->need_update, 1);
 
 	dev_set_name(&tz->device, "thermal_zone%d", tz->id);
 	result = device_register(&tz->device);
@@ -1921,7 +1931,9 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 	INIT_DELAYED_WORK(&(tz->poll_queue), thermal_zone_device_check);
 
 	thermal_zone_device_reset(tz);
-	thermal_zone_device_update(tz);
+	/* Update the new thermal zone and mark it as already updated. */
+	if (atomic_cmpxchg(&tz->need_update, 1, 0))
+		thermal_zone_device_update(tz);
 
 	return tz;
 
include/linux/thermal.h +1 -0
index 5bcabc7..4298418 100644
@@ -195,6 +195,7 @@ struct thermal_zone_device {
 	int emul_temperature;
 	int passive;
 	unsigned int forced_passive;
+	atomic_t need_update;
 	struct thermal_zone_device_ops *ops;
 	struct thermal_zone_params *tzp;
 	struct thermal_governor *governor;
Signature · 1 line
1.8.4.2
Reviewed-by on v2 3/3
#5 lore raw

Re: [PATCH 3/3][v2] Thermal: do thermal zone update after a cooling device registered

Hi Yu,

One minor nit below. Other than that you can add my

Quoted 82 lines · Chen Yu

When a new cooling device is registered, we need to update the thermal zone to set the new registered cooling device to a proper state.

This fixes a problem that the system is cool, while the fan devices are left running on full speed after boot, if fan device is registered after thermal zone device.

Here is the history of why current patch looks like this:
https://patchwork.kernel.org/patch/7273041/

CC: <[email protected]> #3.18+ Reference:https://bugzilla.kernel.org/show_bug.cgi?id=92431

---
 drivers/thermal/thermal_core.c | 14 +++++++++++++-
 include/linux/thermal.h        |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)
drivers/thermal/thermal_core.c +13 -1
index abeb995..f36d0bd 100644
@@ -1341,6 +1341,7 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
 	if (!result) {
 		list_add_tail(&dev->tz_node, &tz->thermal_instances);
 		list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
+		atomic_set(&tz->need_update, 1);
 	}
 	mutex_unlock(&cdev->lock);
 	mutex_unlock(&tz->lock);
@@ -1450,6 +1451,7 @@ __thermal_cooling_device_register(struct device_node *np,
 				  const struct thermal_cooling_device_ops *ops)
 {
 	struct thermal_cooling_device *cdev;
+	struct thermal_zone_device *pos = NULL;
 	int result;
 
 	if (type && strlen(type) >= THERMAL_NAME_LENGTH)
@@ -1494,6 +1496,12 @@ __thermal_cooling_device_register(struct device_node *np,
 	/* Update binding information for 'this' new cdev */
 	bind_cdev(cdev);
 
+	mutex_lock(&thermal_list_lock);
+	list_for_each_entry(pos, &thermal_tz_list, node)
+		if (atomic_cmpxchg(&pos->need_update, 1, 0))
+			thermal_zone_device_update(pos);
+	mutex_unlock(&thermal_list_lock);
+
 	return cdev;
 }
 
@@ -1826,6 +1834,8 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 	tz->trips = trips;
 	tz->passive_delay = passive_delay;
 	tz->polling_delay = polling_delay;
+	/* A new thermal zone needs to be updated anyway. */
+	atomic_set(&tz->need_update, 1);
 
 	dev_set_name(&tz->device, "thermal_zone%d", tz->id);
 	result = device_register(&tz->device);
@@ -1921,7 +1931,9 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 	INIT_DELAYED_WORK(&(tz->poll_queue), thermal_zone_device_check);
 
 	thermal_zone_device_reset(tz);
-	thermal_zone_device_update(tz);
+	/* Update the new thermal zone and mark it as already updated. */
+	if (atomic_cmpxchg(&tz->need_update, 1, 0))
+		thermal_zone_device_update(tz);
 
 	return tz;
 
include/linux/thermal.h +0 -0
index 5bcabc7..4298418 100644

include/linux/thermal.h struct thermal_zone_device { · line 198

Quoted 4 lines
include/linux/thermal.h +1 -0
@@ -195,6 +195,7 @@ struct thermal_zone_device {
 	int emul_temperature;
 	int passive;
 	unsigned int forced_passive;
+	atomic_t need_update;

Please document this field in the kernel-doc comment for struct thermal_zone_device.

Cheers,
Javi

Quoted 6 lines
include/linux/thermal.h +0 -0
 	struct thermal_zone_device_ops *ops;
 	struct thermal_zone_params *tzp;
 	struct thermal_governor *governor;
Signature · 1 line
1.8.4.2
author
#6 lore raw

RE: [PATCH 3/3][v2] Thermal: do thermal zone update after a cooling device registered

Hi, Javi,
thanks for your review,
will send a version 3 out soon.

Quoted 39 lines

-----Original Message-----
From: Javi Merino [mailto:[email protected]]
Sent: Wednesday, October 28, 2015 6:40 PM
To: Chen, Yu C
Cc: Zhang, Rui; [email protected]; [email protected]; linux- [email protected]; [email protected]
Subject: Re: [PATCH 3/3][v2] Thermal: do thermal zone update after a cooling device registered

Hi Yu,

One minor nit below. Other than that you can add my

Quoted 23 lines · Chen Yu

When a new cooling device is registered, we need to update the thermal zone to set the new registered cooling device to a proper state.

This fixes a problem that the system is cool, while the fan devices are left running on full speed after boot, if fan device is registered after thermal zone device.

Here is the history of why current patch looks like this:
https://patchwork.kernel.org/patch/7273041/

CC: <[email protected]> #3.18+ Reference:https://bugzilla.kernel.org/show_bug.cgi?id=92431

---

include/linux/thermal.h index renamed from include/linux/thermal.h +0 -0

5bcabc7..4298418 100644

include/linux/thermal.h +0 -0
Quoted 9 lines

include/linux/thermal.h struct thermal_zone_device { · line 198

Quoted 5 lines
include/linux/thermal.h +1 -0
@@ -195,6 +195,7 @@ struct thermal_zone_device {
 	int emul_temperature;
 	int passive;
 	unsigned int forced_passive;
+	atomic_t need_update;

Please document this field in the kernel-doc comment for struct thermal_zone_device.

Best Regards,
Yu