-----Original Message-----
From: Chen, Yu C
Sent: Tuesday, January 12, 2016 2:42 PM
To: Eduardo Valentin <edubezval@gmail.com>; Zhang, Rui
[off-list ref]
Cc: javi.merino@arm.com; linux-pm@vger.kernel.org; linux-
kernel@vger.kernel.org; stable@vger.kernel.org
Subject: RE: [PATCH 1/3][v3] Thermal: initialize thermal zone device correctly
Importance: High

Hi Eduardo,
Thanks for your review and sorry for that I missed your email.

quoted

-----Original Message-----
From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
owner@vger.kernel.org] On Behalf Of Eduardo Valentin
Sent: Friday, January 01, 2016 2:44 AM
To: Chen, Yu C
Cc: Zhang, Rui; javi.merino@arm.com; linux-pm@vger.kernel.org; linux-
kernel@vger.kernel.org; stable@vger.kernel.org
Subject: Re: [PATCH 1/3][v3] Thermal: initialize thermal zone device
correctly

For some reason, I thought Rui had picked this already.

Anyways, here are a couple of comments:

On Fri, Oct 30, 2015 at 04:31:47PM +0800, Chen Yu wrote:

quoted

From: Zhang Rui <rui.zhang@intel.com>

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.

CC: <redacted> #3.18+
Tested-by: Manuel Krause <redacted>
Tested-by: szegad <redacted>
Tested-by: prash <redacted>
Tested-by: amish <redacted>
Tested-by: Matthias <redacted>
Reviewed-by: Javi Merino <redacted>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 drivers/thermal/step_wise.c    | 17 +++++++++++++++--
 drivers/thermal/thermal_core.c | 19 +++++++++++++++++--
drivers/thermal/thermal_core.h |  1 +

I would prefer if you could split this patch in two. One for thermal
core another one for step wise.

[Yu]  It would be better if we can split this patch into two, for stable material.
What do you think, Rui?

[Zhang, Rui] No, first of all, we cannot take one patch for upstream and then split it into two for stable, second, I'd say I'm okay if the code is organized in two patches as Eduardo described, but at the same time, I don't think it's a problem if it's sent within one patch.

quoted

quoted

 include/linux/thermal.h        |  3 +++
 4 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/step_wise.c

b/drivers/thermal/step_wise.c index 2f9f708..ea9366a 100644

--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c

@@ -63,6 +63,19 @@ static unsigned long get_target_state(struct

thermal_instance *instance,

quoted

 	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)

quoted

 		dev_dbg(&instance->cdev->device, "old_target=%d,

target=%d\n",

quoted

 					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

@@

quoted

static void thermal_zone_trip_update(struct thermal_zone_device *tz,
int

trip)

quoted

 			instance->target == THERMAL_NO_TARGET)
 			update_passive_instance(tz, trip_type, -1);

-
+		instance->initialized = true;
 		instance->cdev->updated = false; /* cdev needs update */
 	}

Considering that I understood the problem and your proposal well, I
would say these changes on step wise are the perfect case for setting
up a step_wise.bind_to_tz(). bind_to_tz() is already designed as an
opportunity for governor to check the thermal zone status at the time of

binding.

quoted

Remember that moving to bind_to_tz() covers not only registration
time, but governor switching too (say, user chooses user_space, then

step_wise).

[Yu] The  code change in step_wise. get_target_state is mainly for
suspend/resume scenario, which  is not involved with thermal
zone/governor bindings IMO.

Agreed.

Thanks,
rui

quoted

The above code seams to be correct, but after reviewing the code of
step_wise.throttle(), I would say it is already complicated and
deserves simplification, when possible.

quoted

diff --git a/drivers/thermal/thermal_core.c

b/drivers/thermal/thermal_core.c index d9e525c..682bc1e 100644

--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c

@@ -532,8 +532,22 @@ static void update_temperature(struct

thermal_zone_device *tz)

quoted

 	mutex_unlock(&tz->lock);

 	trace_thermal_temperature(tz);
-	dev_dbg(&tz->device, "last_temperature=%d,

current_temperature=%d\n",

quoted

-				tz->last_temperature, tz->temperature);
+	if (tz->last_temperature == THERMAL_TEMP_INVALID)
+		dev_dbg(&tz->device, "last_temperature N/A,

current_temperature=%d\n",

quoted

+			tz->temperature);
+	else
+		dev_dbg(&tz->device, "last_temperature=%d,

current_temperature=%d\n",

quoted

+			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)

@@

quoted

quoted

-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);

quoted

+	thermal_zone_device_reset(tz);
 	thermal_zone_device_update(tz);

 	return tz;

diff --git a/drivers/thermal/thermal_core.h

b/drivers/thermal/thermal_core.h index d7ac1fc..749d41a 100644

--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h

@@ -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 */

diff --git a/include/linux/thermal.h b/include/linux/thermal.h index

157d366..5bcabc7 100644

--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h

@@ -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

*/

quoted

+#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)
--
1.8.4.2