[PATCH V4 0/3] Thermal: thermal enhancements for boot and system sleep

[PATCH V4 0/3] Thermal: thermal enhancements for boot and system sleep

[Zhang Rui]

Currently, there are a couple of problems in thermal core framework after boot
and resume from system sleep state, because the thermal zone devices are not
put into a proper state in these cases.

Details of the problems are described in the patch change logs.

In general, altogether they fix three bugs
https://bugzilla.kernel.org/show_bug.cgi?id=78201
https://bugzilla.kernel.org/show_bug.cgi?id=91411
https://bugzilla.kernel.org/show_bug.cgi?id=92431

Bug 78201 needs patch 1/3 and 2/3.
Bug 91411 and 92431 are regressions caused by
commit 19593a1fb1f6718406afca5b867dab184289d406
Author: Aaron Lu <aaron.lu@intel.com>
Date:   Tue Nov 19 16:59:20 2013 +0800

    ACPI / fan: convert to platform driver

    Convert ACPI fan driver to a platform driver for the purpose of phasing
    out ACPI bus.

    Signed-off-by: Aaron Lu <aaron.lu@intel.com>
    Signed-off-by: Zhang Rui <rui.zhang@intel.com>

which is shipped in 3.18.

Bug 91411 needs patch 1/3, 2/3 to fix, while 92431 needs all three patches.

If possible, I'd like to push these patches into 4.0-rc and 3.18/3.19 stable
kernel as it actually fixes a regression in 3.18.

Any comments are welcome.

thanks,
rui
-----
Changes in V4:
	reset THERMAL_TEMP_INVALID to -274000, which means < 0K
Changes in V3:
	accessing in_suspend with locking.
Changes in V2:
	rename no_thermal_update to in_suspend
	rename thermal_notify to thermal_pm_notify
	declare thermal_pm_nb with callback initialized
	unregister thermal_pm_nb in thermal_exit()

[PATCH V4 1/3] Thermal: initialize thermal zone device correctly

[Zhang Rui]

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.

CC: <stable@vger.kernel.org> #3.18+
Tested-by: Manuel Krause <manuelkrause@netscape.net>
Tested-by: szegad <szegadlo@poczta.onet.pl>
Tested-by: prash <prash.n.rao@gmail.com>
Tested-by: amish <ammdispose-arch@yahoo.com>
Tested-by: Matthias <morpheusxyz123@yahoo.de>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/step_wise.c    | 15 +++++++++++++--
 drivers/thermal/thermal_core.c | 19 +++++++++++++++++--
 drivers/thermal/thermal_core.h |  1 +
 include/linux/thermal.h        |  3 +++
 4 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index 5a0f12d..c2bb37c 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -63,6 +63,16 @@ 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;
+	}
+
 	switch (trend) {
 	case THERMAL_TREND_RAISING:
 		if (throttle) {
@@ -149,7 +159,8 @@ 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 +172,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 */
 	}
 
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 174d3bc..9d6f71b 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -469,8 +469,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)
@@ -1574,6 +1588,7 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 	if (!tz->ops->get_temp)
 		thermal_zone_device_set_polling(tz, 0);
 
+	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 0531c75..6d9ffa5 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 5eac316..fb96b15 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -40,6 +40,9 @@
 /* No upper/lower limit requirement */
 #define THERMAL_NO_LIMIT	((u32)~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)
-- 
1.9.1

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

[Zhang Rui]

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 19593a1fb1f6718406afca5b867dab184289d406
Author: Aaron Lu <aaron.lu@intel.com>
Date:   Tue Nov 19 16:59:20 2013 +0800

    ACPI / fan: convert to platform driver

    Convert ACPI fan driver to a platform driver for the purpose of phasing
    out ACPI bus.

    Signed-off-by: Aaron Lu <aaron.lu@intel.com>
    Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Because, with the 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: <stable@vger.kernel.org> #3.18+
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=78201
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=91411
Tested-by: Manuel Krause <manuelkrause@netscape.net>
Tested-by: szegad <szegadlo@poczta.onet.pl>
Tested-by: prash <prash.n.rao@gmail.com>
Tested-by: amish <ammdispose-arch@yahoo.com>
Tested-by: Matthias <morpheusxyz123@yahoo.de>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/thermal_core.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 9d6f71b..875a9bb 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -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)
@@ -491,6 +494,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;
 
@@ -1823,6 +1829,36 @@ static void thermal_unregister_governors(void)
 	thermal_gov_user_space_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;
@@ -1843,6 +1879,8 @@ static int __init thermal_init(void)
 	if (result)
 		goto exit_netlink;
 
+	register_pm_notifier(&thermal_pm_nb);
+
 	return 0;
 
 exit_netlink:
@@ -1862,6 +1900,7 @@ static int __init thermal_init(void)
 
 static void __exit thermal_exit(void)
 {
+	unregister_pm_notifier(&thermal_pm_nb);
 	of_thermal_destroy_zones();
 	genetlink_exit();
 	class_unregister(&thermal_class);
-- 
1.9.1

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

[Zhang Rui]

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.

CC: <stable@vger.kernel.org> #3.18+
Reference:https://bugzilla.kernel.org/show_bug.cgi?id=92431
Tested-by: Manuel Krause <manuelkrause@netscape.net>
Tested-by: szegad <szegadlo@poczta.onet.pl>
Tested-by: prash <prash.n.rao@gmail.com>
Tested-by: amish <ammdispose-arch@yahoo.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/thermal_core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 875a9bb..e37042c 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1140,6 +1140,7 @@ __thermal_cooling_device_register(struct device_node *np,
 				  const struct thermal_cooling_device_ops *ops)
 {
 	struct thermal_cooling_device *cdev;
+	struct thermal_instance *pos, *next;
 	int result;
 
 	if (type && strlen(type) >= THERMAL_NAME_LENGTH)
@@ -1184,6 +1185,15 @@ __thermal_cooling_device_register(struct device_node *np,
 	/* Update binding information for 'this' new cdev */
 	bind_cdev(cdev);
 
+	list_for_each_entry_safe(pos, next, &cdev->thermal_instances, cdev_node) {
+			if (next->cdev_node.next == &cdev->thermal_instances) {
+				thermal_zone_device_update(next->tz);
+				break;
+			}
+			if (pos->tz != next->tz)
+				thermal_zone_device_update(pos->tz);
+	}
+
 	return cdev;
 }
 
-- 
1.9.1

Re: [PATCH V4 1/3] Thermal: initialize thermal zone device correctly

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 5452 bytes --]

On Tue, Apr 07, 2015 at 10:24:34AM +0800, Zhang Rui wrote:
> 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.
> 
> CC: <stable@vger.kernel.org> #3.18+
> Tested-by: Manuel Krause <manuelkrause@netscape.net>
> Tested-by: szegad <szegadlo@poczta.onet.pl>
> Tested-by: prash <prash.n.rao@gmail.com>
> Tested-by: amish <ammdispose-arch@yahoo.com>
> Tested-by: Matthias <morpheusxyz123@yahoo.de>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Can you please consider the comments I made on V3?

Summary:

1. Change initialized to trend_valid
2. return next_target earlier in the get_target_state, so the behavior
is consistent.

> ---
>  drivers/thermal/step_wise.c    | 15 +++++++++++++--
>  drivers/thermal/thermal_core.c | 19 +++++++++++++++++--
>  drivers/thermal/thermal_core.h |  1 +
>  include/linux/thermal.h        |  3 +++
>  4 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> index 5a0f12d..c2bb37c 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -63,6 +63,16 @@ 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;
> +	}
> +
>  	switch (trend) {
>  	case THERMAL_TREND_RAISING:
>  		if (throttle) {
> @@ -149,7 +159,8 @@ 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 +172,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 */
>  	}
>  
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 174d3bc..9d6f71b 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -469,8 +469,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)
> @@ -1574,6 +1588,7 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>  	if (!tz->ops->get_temp)
>  		thermal_zone_device_set_polling(tz, 0);
>  
> +	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 0531c75..6d9ffa5 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 5eac316..fb96b15 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -40,6 +40,9 @@
>  /* No upper/lower limit requirement */
>  #define THERMAL_NO_LIMIT	((u32)~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)
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

RE: [PATCH V4 1/3] Thermal: initialize thermal zone device correctly

[Zhang, Rui]

> -----Original Message-----
> From: Eduardo Valentin [mailto:edubezval@gmail.com]
> Sent: Tuesday, April 7, 2015 10:47 AM
> To: Zhang, Rui
> Cc: linux-pm@vger.kernel.org
> Subject: Re: [PATCH V4 1/3] Thermal: initialize thermal zone device correctly
> Importance: High
> 
> On Tue, Apr 07, 2015 at 10:24:34AM +0800, Zhang Rui wrote:
> > 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.
> >
> > CC: <stable@vger.kernel.org> #3.18+
> > Tested-by: Manuel Krause <manuelkrause@netscape.net>
> > Tested-by: szegad <szegadlo@poczta.onet.pl>
> > Tested-by: prash <prash.n.rao@gmail.com>
> > Tested-by: amish <ammdispose-arch@yahoo.com>
> > Tested-by: Matthias <morpheusxyz123@yahoo.de>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> 
> Can you please consider the comments I made on V3?
> 
Sorry, I missed your previous comment.

> Summary:
> 
> 1. Change initialized to trend_valid

I don't think it is proper to call it "trend_valid" because in the case that cooling device
is registered after thermal zone (the problem pointed out in patch 3/3),
the new created thermal_instance needs to be set properly, and the trend is valid
at this time actually.

IMO, thermal_instance->initialized just means if the thermal instance is evaluated for
the first time or not, and if yes, we need some special handling.

> 2. return next_target earlier in the get_target_state, so the behavior is
> consistent.

Agreed. Will fix in next version.

Thanks,
rui
> 
> > ---
> >  drivers/thermal/step_wise.c    | 15 +++++++++++++--
> >  drivers/thermal/thermal_core.c | 19 +++++++++++++++++--
> > drivers/thermal/thermal_core.h |  1 +
> >  include/linux/thermal.h        |  3 +++
> >  4 files changed, 34 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> > index 5a0f12d..c2bb37c 100644
> > --- a/drivers/thermal/step_wise.c
> > +++ b/drivers/thermal/step_wise.c
> > @@ -63,6 +63,16 @@ 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;
> > +	}
> > +
> >  	switch (trend) {
> >  	case THERMAL_TREND_RAISING:
> >  		if (throttle) {
> > @@ -149,7 +159,8 @@ 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 +172,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 */
> >  	}
> >
> > diff --git a/drivers/thermal/thermal_core.c
> > b/drivers/thermal/thermal_core.c index 174d3bc..9d6f71b 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -469,8 +469,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) @@
> > -1574,6 +1588,7 @@ struct thermal_zone_device
> *thermal_zone_device_register(const char *type,
> >  	if (!tz->ops->get_temp)
> >  		thermal_zone_device_set_polling(tz, 0);
> >
> > +	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 0531c75..6d9ffa5 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
> > 5eac316..fb96b15 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -40,6 +40,9 @@
> >  /* No upper/lower limit requirement */
> >  #define THERMAL_NO_LIMIT	((u32)~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)
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> > the body of a message to majordomo@vger.kernel.org More majordomo info
> > at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 1/3] Thermal: initialize thermal zone device correctly

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 7551 bytes --]

Hello Rui,

On Wed, Apr 08, 2015 at 01:01:08PM +0000, Zhang, Rui wrote:
> 
> 
> > -----Original Message-----
> > From: Eduardo Valentin [mailto:edubezval@gmail.com]
> > Sent: Tuesday, April 7, 2015 10:47 AM
> > To: Zhang, Rui
> > Cc: linux-pm@vger.kernel.org
> > Subject: Re: [PATCH V4 1/3] Thermal: initialize thermal zone device correctly
> > Importance: High
> > 
> > On Tue, Apr 07, 2015 at 10:24:34AM +0800, Zhang Rui wrote:
> > > 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.
> > >
> > > CC: <stable@vger.kernel.org> #3.18+
> > > Tested-by: Manuel Krause <manuelkrause@netscape.net>
> > > Tested-by: szegad <szegadlo@poczta.onet.pl>
> > > Tested-by: prash <prash.n.rao@gmail.com>
> > > Tested-by: amish <ammdispose-arch@yahoo.com>
> > > Tested-by: Matthias <morpheusxyz123@yahoo.de>
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > 
> > Can you please consider the comments I made on V3?
> > 
> Sorry, I missed your previous comment.
> 
> > Summary:
> > 
> > 1. Change initialized to trend_valid
> 
> I don't think it is proper to call it "trend_valid" because in the case that cooling device
> is registered after thermal zone (the problem pointed out in patch 3/3),
> the new created thermal_instance needs to be set properly, and the trend is valid
> at this time actually.

I see your point. But for the case of the problem solved by patch 3/3,
shouldn't the core part call then call thermal_zone_device_update only
for the thermal_zones that the cooling device has a thermal_instance
with ->initialized == false?

I think in this case the thermal_core needs to be more aware of this
property, instead of leaving for governors to deal with it. And the way
it is put today, looks like the property is useful only for step_wise,
that is why it makes me feel like it is trend bounded.


> 
> IMO, thermal_instance->initialized just means if the thermal instance is evaluated for
> the first time or not, and if yes, we need some special handling.

OK. Thanks for clarifying. 

> 
> > 2. return next_target earlier in the get_target_state, so the behavior is
> > consistent.
> 
> Agreed. Will fix in next version.

OK. Thanks.

> 
> Thanks,
> rui
> > 
> > > ---
> > >  drivers/thermal/step_wise.c    | 15 +++++++++++++--
> > >  drivers/thermal/thermal_core.c | 19 +++++++++++++++++--
> > > drivers/thermal/thermal_core.h |  1 +
> > >  include/linux/thermal.h        |  3 +++
> > >  4 files changed, 34 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> > > index 5a0f12d..c2bb37c 100644
> > > --- a/drivers/thermal/step_wise.c
> > > +++ b/drivers/thermal/step_wise.c
> > > @@ -63,6 +63,16 @@ 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;
> > > +	}
> > > +
> > >  	switch (trend) {
> > >  	case THERMAL_TREND_RAISING:
> > >  		if (throttle) {
> > > @@ -149,7 +159,8 @@ 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 +172,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 */
> > >  	}
> > >
> > > diff --git a/drivers/thermal/thermal_core.c
> > > b/drivers/thermal/thermal_core.c index 174d3bc..9d6f71b 100644
> > > --- a/drivers/thermal/thermal_core.c
> > > +++ b/drivers/thermal/thermal_core.c
> > > @@ -469,8 +469,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) @@
> > > -1574,6 +1588,7 @@ struct thermal_zone_device
> > *thermal_zone_device_register(const char *type,
> > >  	if (!tz->ops->get_temp)
> > >  		thermal_zone_device_set_polling(tz, 0);
> > >
> > > +	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 0531c75..6d9ffa5 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
> > > 5eac316..fb96b15 100644
> > > --- a/include/linux/thermal.h
> > > +++ b/include/linux/thermal.h
> > > @@ -40,6 +40,9 @@
> > >  /* No upper/lower limit requirement */
> > >  #define THERMAL_NO_LIMIT	((u32)~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)
> > > --
> > > 1.9.1
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> > > the body of a message to majordomo@vger.kernel.org More majordomo info
> > > at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 2670 bytes --]

Hello Rui,

On Tue, Apr 07, 2015 at 10:24:36AM +0800, Zhang Rui wrote:
> 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.
> 
> CC: <stable@vger.kernel.org> #3.18+
> Reference:https://bugzilla.kernel.org/show_bug.cgi?id=92431
> Tested-by: Manuel Krause <manuelkrause@netscape.net>
> Tested-by: szegad <szegadlo@poczta.onet.pl>
> Tested-by: prash <prash.n.rao@gmail.com>
> Tested-by: amish <ammdispose-arch@yahoo.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/thermal/thermal_core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 875a9bb..e37042c 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1140,6 +1140,7 @@ __thermal_cooling_device_register(struct device_node *np,
>  				  const struct thermal_cooling_device_ops *ops)
>  {
>  	struct thermal_cooling_device *cdev;
> +	struct thermal_instance *pos, *next;
>  	int result;
>  
>  	if (type && strlen(type) >= THERMAL_NAME_LENGTH)
> @@ -1184,6 +1185,15 @@ __thermal_cooling_device_register(struct device_node *np,
>  	/* Update binding information for 'this' new cdev */
>  	bind_cdev(cdev);
>  
> +	list_for_each_entry_safe(pos, next, &cdev->thermal_instances, cdev_node) {
> +			if (next->cdev_node.next == &cdev->thermal_instances) {
> +				thermal_zone_device_update(next->tz);
> +				break;
> +			}
> +			if (pos->tz != next->tz)
> +				thermal_zone_device_update(pos->tz);
> +	}

Maybe the reasoning for not calling only when initialized == false is
because at this point all instances are initialized == false. 

Then why not moving this thermal_zone_device_update(tz) to the
thermal_zone_bind_cooling_device(), where we create the
thermal_instance?

> +

Besides, Can you please elaborate more on why we need to specifically call
thermal_zone_device_update(tz) here and not simply wait until next
tz->poll_queue work is called?

Any particular reason for the problem not be solved byt the call
of thermal_zone_device_update(tz) from the poll_queue of each tz?



>  	return cdev;
>  }
>  
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH V4 1/3] Thermal: initialize thermal zone device correctly

[Javi Merino]

On Wed, Apr 08, 2015 at 02:01:08PM +0100, Zhang, Rui wrote:
> 
> 
> > -----Original Message-----
> > From: Eduardo Valentin [mailto:edubezval@gmail.com]
> > Sent: Tuesday, April 7, 2015 10:47 AM
> > To: Zhang, Rui
> > Cc: linux-pm@vger.kernel.org
> > Subject: Re: [PATCH V4 1/3] Thermal: initialize thermal zone device correctly
> > Importance: High
> > 
> > On Tue, Apr 07, 2015 at 10:24:34AM +0800, Zhang Rui wrote:
> > > 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.
> > >
> > > CC: <stable@vger.kernel.org> #3.18+
> > > Tested-by: Manuel Krause <manuelkrause@netscape.net>
> > > Tested-by: szegad <szegadlo@poczta.onet.pl>
> > > Tested-by: prash <prash.n.rao@gmail.com>
> > > Tested-by: amish <ammdispose-arch@yahoo.com>
> > > Tested-by: Matthias <morpheusxyz123@yahoo.de>
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > 
> > Can you please consider the comments I made on V3?
> > 
> Sorry, I missed your previous comment.
> 
> > Summary:
> > 
> > 1. Change initialized to trend_valid
> 
> I don't think it is proper to call it "trend_valid" because in the case that cooling device
> is registered after thermal zone (the problem pointed out in patch 3/3),
> the new created thermal_instance needs to be set properly, and the trend is valid
> at this time actually.
> 
> IMO, thermal_instance->initialized just means if the thermal instance is evaluated for
> the first time or not, and if yes, we need some special handling.

I think the main source of confusion here is the fact that this is
only needed for step_wise.  Thermal governors that don't care about
trend will always have thermal instances with "initialized" being
false, which seems counter-intuitive.  The only name I can think of is
"not_yet_evaluated_by_step_wise" which is ridiculously long name for a
variable, but can we get something more descriptive than
"initialized"?

Cheers,
Javi

Re: [PATCH V4 1/3] Thermal: initialize thermal zone device correctly

[Javi Merino]

On Tue, Apr 07, 2015 at 03:24:34AM +0100, Zhang Rui wrote:
> 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.
> 
> CC: <stable@vger.kernel.org> #3.18+
> Tested-by: Manuel Krause <manuelkrause@netscape.net>
> Tested-by: szegad <szegadlo@poczta.onet.pl>
> Tested-by: prash <prash.n.rao@gmail.com>
> Tested-by: amish <ammdispose-arch@yahoo.com>
> Tested-by: Matthias <morpheusxyz123@yahoo.de>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/thermal/step_wise.c    | 15 +++++++++++++--
>  drivers/thermal/thermal_core.c | 19 +++++++++++++++++--
>  drivers/thermal/thermal_core.h |  1 +
>  include/linux/thermal.h        |  3 +++
>  4 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> index 5a0f12d..c2bb37c 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -63,6 +63,16 @@ 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;

CodingStyle says that if one branch of an if statement needs braces,
all branches must have braces:

if (condition) {
        do_this();
        do_that();
} else {
        otherwise();
}

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

> +	}
> +

I pointed out before and I think Eduardo has said it as well.  I think
you should "return next_target;" at the end of the "if
(!instance->initialized)"

>  	switch (trend) {
>  	case THERMAL_TREND_RAISING:
>  		if (throttle) {

Cheers,
Javi

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

[Zhang, Rui]

> -----Original Message-----
> From: Eduardo Valentin [mailto:edubezval@gmail.com]
> Sent: Wednesday, April 8, 2015 11:04 PM
> To: Zhang, Rui
> Cc: linux-pm@vger.kernel.org
> Subject: Re: [PATCH V4 3/3] Thermal: do thermal zone update after a cooling
> device registered
> Importance: High
> 
> Hello Rui,
> 
> On Tue, Apr 07, 2015 at 10:24:36AM +0800, Zhang Rui wrote:
> > 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.
> >
> > CC: <stable@vger.kernel.org> #3.18+
> > Reference:https://bugzilla.kernel.org/show_bug.cgi?id=92431
> > Tested-by: Manuel Krause <manuelkrause@netscape.net>
> > Tested-by: szegad <szegadlo@poczta.onet.pl>
> > Tested-by: prash <prash.n.rao@gmail.com>
> > Tested-by: amish <ammdispose-arch@yahoo.com>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/thermal/thermal_core.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/thermal/thermal_core.c
> > b/drivers/thermal/thermal_core.c index 875a9bb..e37042c 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -1140,6 +1140,7 @@ __thermal_cooling_device_register(struct
> device_node *np,
> >  				  const struct thermal_cooling_device_ops *ops)
> {
> >  	struct thermal_cooling_device *cdev;
> > +	struct thermal_instance *pos, *next;
> >  	int result;
> >
> >  	if (type && strlen(type) >= THERMAL_NAME_LENGTH) @@ -1184,6
> +1185,15
> > @@ __thermal_cooling_device_register(struct device_node *np,
> >  	/* Update binding information for 'this' new cdev */
> >  	bind_cdev(cdev);
> >
> > +	list_for_each_entry_safe(pos, next, &cdev->thermal_instances,
> cdev_node) {
> > +			if (next->cdev_node.next == &cdev->thermal_instances)
> {
> > +				thermal_zone_device_update(next->tz);
> > +				break;
> > +			}
> > +			if (pos->tz != next->tz)
> > +				thermal_zone_device_update(pos->tz);
> > +	}
> 
> Maybe the reasoning for not calling only when initialized == false is because at
> this point all instances are initialized == false.
> 
No. 
At this point, only the thermal instances for the current cooling device are initialized=false.
For the cooling devices that have already been registered, their thermal instances are initialized = true.

> Then why not moving this thermal_zone_device_update(tz) to the
> thermal_zone_bind_cooling_device(), where we create the thermal_instance?
> 
The same problem, as there may be multiple thermal instances for the same cooling device and zone, but for different trip point,
If we do thermal_zone_device_update() for every binding, this means that there may be several thermal zone device update when one cooling device is registered.

> > +
> 
> Besides, Can you please elaborate more on why we need to specifically call
> thermal_zone_device_update(tz) here and not simply wait until next
> tz->poll_queue work is called?

If the system boots with low temperature, and the fan devices are turned on in boot phrase by firmware,
there may be not thermal events as the fan is always running in full speed, thus there is no chance to do thermal zone device update.

Actually, on ACPI platform, we have a bug report that the temperature is a bogus value, which is statically 16C,
and the ACPI fan devices are turned on automatically by the acpi_general_pm_domain, the fan device never spins down.
> 
> Any particular reason for the problem not be solved byt the call of
> thermal_zone_device_update(tz) from the poll_queue of each tz?
> 
Because polling delay is not mandatory for every thermal zone, thus some platform thermal driver relies on interrupts instead of polling.

Thanks,
rui
> 
> 
> >  	return cdev;
> >  }
> >
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> > the body of a message to majordomo@vger.kernel.org More majordomo info
> > at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V4 1/3] Thermal: initialize thermal zone device correctly

[Zhang, Rui]

> -----Original Message-----
> From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> owner@vger.kernel.org] On Behalf Of Eduardo Valentin
> Sent: Wednesday, April 8, 2015 11:04 PM
> To: Zhang, Rui
> Cc: linux-pm@vger.kernel.org
> Subject: Re: [PATCH V4 1/3] Thermal: initialize thermal zone device correctly
> Importance: High
> 
> Hello Rui,
> 
> On Wed, Apr 08, 2015 at 01:01:08PM +0000, Zhang, Rui wrote:
> >
> >
> > > -----Original Message-----
> > > From: Eduardo Valentin [mailto:edubezval@gmail.com]
> > > Sent: Tuesday, April 7, 2015 10:47 AM
> > > To: Zhang, Rui
> > > Cc: linux-pm@vger.kernel.org
> > > Subject: Re: [PATCH V4 1/3] Thermal: initialize thermal zone device
> > > correctly
> > > Importance: High
> > >
> > > On Tue, Apr 07, 2015 at 10:24:34AM +0800, Zhang Rui wrote:
> > > > 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.
> > > >
> > > > CC: <stable@vger.kernel.org> #3.18+
> > > > Tested-by: Manuel Krause <manuelkrause@netscape.net>
> > > > Tested-by: szegad <szegadlo@poczta.onet.pl>
> > > > Tested-by: prash <prash.n.rao@gmail.com>
> > > > Tested-by: amish <ammdispose-arch@yahoo.com>
> > > > Tested-by: Matthias <morpheusxyz123@yahoo.de>
> > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > >
> > > Can you please consider the comments I made on V3?
> > >
> > Sorry, I missed your previous comment.
> >
> > > Summary:
> > >
> > > 1. Change initialized to trend_valid
> >
> > I don't think it is proper to call it "trend_valid" because in the
> > case that cooling device is registered after thermal zone (the problem
> > pointed out in patch 3/3), the new created thermal_instance needs to
> > be set properly, and the trend is valid at this time actually.
> 
> I see your point. But for the case of the problem solved by patch 3/3, shouldn't
> the core part call then call thermal_zone_device_update only for the
> thermal_zones that the cooling device has a thermal_instance with ->initialized
> == false?

Yes, we can do it in that way.
But it seems that we need some extra code for this in thermal governor instead of thermal core. Thus I preferred to use the existing thermal API.
Plus, for the fair_share governor, we do need to re-calculate the cooling state for every thermal instance when a new cooling device registered, right?
> 
> I think in this case the thermal_core needs to be more aware of this property,
> instead of leaving for governors to deal with it. And the way it is put today, looks
> like the property is useful only for step_wise, that is why it makes me feel like it
> is trend bounded.

Hmm, IMO, it is hard to do this in thermal core in current design, because .throttle() callback is based on trip points rather than thermal_instance, i.e. it evaluates the thermal instances that shares the same trip point in one time, thus I don't think we can handle this without thermal governor change.
I agree there are some areas worth being improved, and maybe we could get a better solution some time, but for now, as there are two regression reports, and these patches have been tested to work for them, I really want to push these patches as a urgent fix. :)

Thanks,
rui
> 
> 
> >
> > IMO, thermal_instance->initialized just means if the thermal instance
> > is evaluated for the first time or not, and if yes, we need some special handling.
> 
> OK. Thanks for clarifying.
> 
> >
> > > 2. return next_target earlier in the get_target_state, so the
> > > behavior is consistent.
> >
> > Agreed. Will fix in next version.
> 
> OK. Thanks.
> 
> >
> > Thanks,
> > rui
> > >
> > > > ---
> > > >  drivers/thermal/step_wise.c    | 15 +++++++++++++--
> > > >  drivers/thermal/thermal_core.c | 19 +++++++++++++++++--
> > > > drivers/thermal/thermal_core.h |  1 +
> > > >  include/linux/thermal.h        |  3 +++
> > > >  4 files changed, 34 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/thermal/step_wise.c
> > > > b/drivers/thermal/step_wise.c index 5a0f12d..c2bb37c 100644
> > > > --- a/drivers/thermal/step_wise.c
> > > > +++ b/drivers/thermal/step_wise.c
> > > > @@ -63,6 +63,16 @@ 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;
> > > > +	}
> > > > +
> > > >  	switch (trend) {
> > > >  	case THERMAL_TREND_RAISING:
> > > >  		if (throttle) {
> > > > @@ -149,7 +159,8 @@ 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 +172,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 */
> > > >  	}
> > > >
> > > > diff --git a/drivers/thermal/thermal_core.c
> > > > b/drivers/thermal/thermal_core.c index 174d3bc..9d6f71b 100644
> > > > --- a/drivers/thermal/thermal_core.c
> > > > +++ b/drivers/thermal/thermal_core.c
> > > > @@ -469,8 +469,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)
> > > > @@
> > > > -1574,6 +1588,7 @@ struct thermal_zone_device
> > > *thermal_zone_device_register(const char *type,
> > > >  	if (!tz->ops->get_temp)
> > > >  		thermal_zone_device_set_polling(tz, 0);
> > > >
> > > > +	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 0531c75..6d9ffa5 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
> > > > 5eac316..fb96b15 100644
> > > > --- a/include/linux/thermal.h
> > > > +++ b/include/linux/thermal.h
> > > > @@ -40,6 +40,9 @@
> > > >  /* No upper/lower limit requirement */
> > > >  #define THERMAL_NO_LIMIT	((u32)~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)
> > > > --
> > > > 1.9.1
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe
> > > > linux-pm" in the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V4 1/3] Thermal: initialize thermal zone device correctly

[Zhang, Rui]

> -----Original Message-----
> From: Javi Merino [mailto:javi.merino@arm.com]
> Sent: Thursday, April 9, 2015 10:57 PM
> To: Zhang, Rui
> Cc: linux-pm@vger.kernel.org
> Subject: Re: [PATCH V4 1/3] Thermal: initialize thermal zone device correctly
> Importance: High
> 
> On Tue, Apr 07, 2015 at 03:24:34AM +0100, Zhang Rui wrote:
> > 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.
> >
> > CC: <stable@vger.kernel.org> #3.18+
> > Tested-by: Manuel Krause <manuelkrause@netscape.net>
> > Tested-by: szegad <szegadlo@poczta.onet.pl>
> > Tested-by: prash <prash.n.rao@gmail.com>
> > Tested-by: amish <ammdispose-arch@yahoo.com>
> > Tested-by: Matthias <morpheusxyz123@yahoo.de>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/thermal/step_wise.c    | 15 +++++++++++++--
> >  drivers/thermal/thermal_core.c | 19 +++++++++++++++++--
> > drivers/thermal/thermal_core.h |  1 +
> >  include/linux/thermal.h        |  3 +++
> >  4 files changed, 34 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> > index 5a0f12d..c2bb37c 100644
> > --- a/drivers/thermal/step_wise.c
> > +++ b/drivers/thermal/step_wise.c
> > @@ -63,6 +63,16 @@ 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;
> 
> CodingStyle says that if one branch of an if statement needs braces, all branches
> must have braces:
> 
> if (condition) {
>         do_this();
>         do_that();
> } else {
>         otherwise();
> }
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentatio
> n/CodingStyle#n169
> 
Thanks for pointing out, will fix in next version.

> > +	}
> > +
> 
> I pointed out before and I think Eduardo has said it as well.  I think you should
> "return next_target;" at the end of the "if (!instance->initialized)"
> 
Sorry, I missed these emails as my Linux work station was broken, and I'm not used to outlook yet.

Thanks,
rui
> >  	switch (trend) {
> >  	case THERMAL_TREND_RAISING:
> >  		if (throttle) {
> 
> Cheers,
> Javi

RE: [PATCH V4 1/3] Thermal: initialize thermal zone device correctly

[Zhang, Rui]

> -----Original Message-----
> From: Javi Merino [mailto:javi.merino@arm.com]
> Sent: Thursday, April 9, 2015 10:44 PM
> To: Zhang, Rui
> Cc: Eduardo Valentin; linux-pm@vger.kernel.org
> Subject: Re: [PATCH V4 1/3] Thermal: initialize thermal zone device correctly
> Importance: High
> 
> On Wed, Apr 08, 2015 at 02:01:08PM +0100, Zhang, Rui wrote:
> >
> >
> > > -----Original Message-----
> > > From: Eduardo Valentin [mailto:edubezval@gmail.com]
> > > Sent: Tuesday, April 7, 2015 10:47 AM
> > > To: Zhang, Rui
> > > Cc: linux-pm@vger.kernel.org
> > > Subject: Re: [PATCH V4 1/3] Thermal: initialize thermal zone device
> > > correctly
> > > Importance: High
> > >
> > > On Tue, Apr 07, 2015 at 10:24:34AM +0800, Zhang Rui wrote:
> > > > 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.
> > > >
> > > > CC: <stable@vger.kernel.org> #3.18+
> > > > Tested-by: Manuel Krause <manuelkrause@netscape.net>
> > > > Tested-by: szegad <szegadlo@poczta.onet.pl>
> > > > Tested-by: prash <prash.n.rao@gmail.com>
> > > > Tested-by: amish <ammdispose-arch@yahoo.com>
> > > > Tested-by: Matthias <morpheusxyz123@yahoo.de>
> > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > >
> > > Can you please consider the comments I made on V3?
> > >
> > Sorry, I missed your previous comment.
> >
> > > Summary:
> > >
> > > 1. Change initialized to trend_valid
> >
> > I don't think it is proper to call it "trend_valid" because in the
> > case that cooling device is registered after thermal zone (the problem
> > pointed out in patch 3/3), the new created thermal_instance needs to
> > be set properly, and the trend is valid at this time actually.
> >
> > IMO, thermal_instance->initialized just means if the thermal instance
> > is evaluated for the first time or not, and if yes, we need some special handling.
> 
> I think the main source of confusion here is the fact that this is only needed for
> step_wise.  Thermal governors that don't care about trend will always have
> thermal instances with "initialized" being false, which seems counter-intuitive.

Good point.
Faire_share governor  always re-calculate the cooling state of every thermal instances.
User_space governor does not care about thermal instance at all.
Bang_bang governor actually uses THERMAL_NO_TARGET as the sign of a "uninitialized" thermal instance. I can modify bang_bang governor as well if you think this is confusing.

Thanks,
rui
> The only name I can think of is "not_yet_evaluated_by_step_wise" which is
> ridiculously long name for a variable, but can we get something more descriptive
> than "initialized"?


> 
> Cheers,
> Javi