[PATCH 0/6] thermal: improvements re. forced passive cooling

[PATCH 0/6] thermal: improvements re. forced passive cooling

[Frans Pop]

This set three new patches in addition to four sent earlier as:
- [PATCH 0/3] acpi: improvements for Documentation/thermal/sysfs-api.txt
- [PATCH] acpi: thermal: display forced passive trip points in proc

There's one new whitespace fix in patch 1/1 from the first set;
patch 3/3 from that set was dropped as it was NACKed by Rui.

The first two patches in this series are purely documentation; the
others enhance functionality.

I've added Matthew in CC as he orginally implemented forced cooling.
Sujith Thomas is only CCed on the first two patches.

 Documentation/thermal/sysfs-api.txt |  393 ++++++++++++++++++-----------------
 drivers/acpi/thermal.c              |    7 +
 drivers/thermal/thermal_sys.c       |   17 +-
 3 files changed, 222 insertions(+), 195 deletions(-)

Cheers,
FJP

[PATCH 1/6] thermal: sysfs-api.txt - reformat for improved readability

[Frans Pop]

The document currently uses large indentations which make the text
too wide for easy readability. Also improve general consistency.

Signed-off-by: Frans Pop <elendil@planet.nl>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Sujith Thomas <sujith.thomas@intel.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index 70d68ce..895337f 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -1,5 +1,5 @@
 Generic Thermal Sysfs driver How To
-=========================
+===================================
 
 Written by Sujith Thomas <sujith.thomas@intel.com>, Zhang Rui <rui.zhang@intel.com>
 
@@ -10,20 +10,20 @@ Copyright (c)  2008 Intel Corporation
 
 0. Introduction
 
-The generic thermal sysfs provides a set of interfaces for thermal zone devices (sensors)
-and thermal cooling devices (fan, processor...) to register with the thermal management
-solution and to be a part of it.
+The generic thermal sysfs provides a set of interfaces for thermal zone
+devices (sensors) and thermal cooling devices (fan, processor...) to register
+with the thermal management solution and to be a part of it.
 
-This how-to focuses on enabling new thermal zone and cooling devices to participate
-in thermal management.
-This solution is platform independent and any type of thermal zone devices and
-cooling devices should be able to make use of the infrastructure.
+This how-to focuses on enabling new thermal zone and cooling devices to
+participate in thermal management.
+This solution is platform independent and any type of thermal zone devices
+and cooling devices should be able to make use of the infrastructure.
 
-The main task of the thermal sysfs driver is to expose thermal zone attributes as well
-as cooling device attributes to the user space.
-An intelligent thermal management application can make decisions based on inputs
-from thermal zone attributes (the current temperature and trip point temperature)
-and throttle appropriate devices.
+The main task of the thermal sysfs driver is to expose thermal zone attributes
+as well as cooling device attributes to the user space.
+An intelligent thermal management application can make decisions based on
+inputs from thermal zone attributes (the current temperature and trip point
+temperature) and throttle appropriate devices.
 
 [0-*]	denotes any positive number starting from 0
 [1-*]	denotes any positive number starting from 1
@@ -31,77 +31,77 @@ and throttle appropriate devices.
 1. thermal sysfs driver interface functions
 
 1.1 thermal zone device interface
-1.1.1 struct thermal_zone_device *thermal_zone_device_register(char *name, int trips,
-				void *devdata, struct thermal_zone_device_ops *ops)
-
-	This interface function adds a new thermal zone device (sensor) to
-	/sys/class/thermal folder as thermal_zone[0-*].
-	It tries to bind all the thermal cooling devices registered at the same time.
-
-	name: the thermal zone name.
-	trips: the total number of trip points this thermal zone supports.
-	devdata: device private data
-	ops: thermal zone device call-backs.
-		.bind: bind the thermal zone device with a thermal cooling device.
-		.unbind: unbind the thermal zone device with a thermal cooling device.
-		.get_temp: get the current temperature of the thermal zone.
-		.get_mode: get the current mode (user/kernel) of the thermal zone.
-			   "kernel" means thermal management is done in kernel.
-			   "user" will prevent kernel thermal driver actions upon trip points
-			   so that user applications can take charge of thermal management.
-		.set_mode: set the mode (user/kernel) of the thermal zone.
-		.get_trip_type: get the type of certain trip point.
-		.get_trip_temp: get the temperature above which the certain trip point
-				will be fired.
+1.1.1 struct thermal_zone_device *thermal_zone_device_register(char *name,
+		int trips, void *devdata, struct thermal_zone_device_ops *ops)
+
+    This interface function adds a new thermal zone device (sensor) to
+    /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
+    thermal cooling devices registered at the same time.
+
+    name: the thermal zone name.
+    trips: the total number of trip points this thermal zone supports.
+    devdata: device private data
+    ops: thermal zone device call-backs.
+	.bind: bind the thermal zone device with a thermal cooling device.
+	.unbind: unbind the thermal zone device with a thermal cooling device.
+	.get_temp: get the current temperature of the thermal zone.
+	.get_mode: get the current mode (user/kernel) of the thermal zone.
+	    - "kernel" means thermal management is done in kernel.
+	    - "user" will prevent kernel thermal driver actions upon trip points
+	      so that user applications can take charge of thermal management.
+	.set_mode: set the mode (user/kernel) of the thermal zone.
+	.get_trip_type: get the type of certain trip point.
+	.get_trip_temp: get the temperature above which the certain trip point
+			will be fired.
 
 1.1.2 void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 
-	This interface function removes the thermal zone device.
-	It deletes the corresponding entry form /sys/class/thermal folder and unbind all
-	the thermal cooling devices it uses.
+    This interface function removes the thermal zone device.
+    It deletes the corresponding entry form /sys/class/thermal folder and
+    unbind all the thermal cooling devices it uses.
 
 1.2 thermal cooling device interface
 1.2.1 struct thermal_cooling_device *thermal_cooling_device_register(char *name,
-					void *devdata, struct thermal_cooling_device_ops *)
-
-	This interface function adds a new thermal cooling device (fan/processor/...) to
-	/sys/class/thermal/ folder as cooling_device[0-*].
-	It tries to bind itself to all the thermal zone devices register at the same time.
-	name: the cooling device name.
-	devdata: device private data.
-	ops: thermal cooling devices call-backs.
-		.get_max_state: get the Maximum throttle state of the cooling device.
-		.get_cur_state: get the Current throttle state of the cooling device.
-		.set_cur_state: set the Current throttle state of the cooling device.
+		void *devdata, struct thermal_cooling_device_ops *)
+
+    This interface function adds a new thermal cooling device (fan/processor/...)
+    to /sys/class/thermal/ folder as cooling_device[0-*]. It tries to bind itself
+    to all the thermal zone devices register at the same time.
+    name: the cooling device name.
+    devdata: device private data.
+    ops: thermal cooling devices call-backs.
+	.get_max_state: get the Maximum throttle state of the cooling device.
+	.get_cur_state: get the Current throttle state of the cooling device.
+	.set_cur_state: set the Current throttle state of the cooling device.
 
 1.2.2 void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
 
-	This interface function remove the thermal cooling device.
-	It deletes the corresponding entry form /sys/class/thermal folder and unbind
-	itself from all	the thermal zone devices using it.
+    This interface function remove the thermal cooling device.
+    It deletes the corresponding entry form /sys/class/thermal folder and
+    unbind itself from all the thermal zone devices using it.
 
 1.3 interface for binding a thermal zone device with a thermal cooling device
 1.3.1 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
-			int trip, struct thermal_cooling_device *cdev);
+		int trip, struct thermal_cooling_device *cdev);
 
-	This interface function bind a thermal cooling device to the certain trip point
-	of a thermal zone device.
-	This function is usually called in the thermal zone device .bind callback.
-	tz: the thermal zone device
-	cdev: thermal cooling device
-	trip: indicates which trip point the cooling devices is associated with
-		 in this thermal zone.
+    This interface function bind a thermal cooling device to the certain trip
+    point of a thermal zone device.
+    This function is usually called in the thermal zone device .bind callback.
+    tz: the thermal zone device
+    cdev: thermal cooling device
+    trip: indicates which trip point the cooling devices is associated with
+	  in this thermal zone.
 
 1.3.2 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *tz,
-				int trip, struct thermal_cooling_device *cdev);
+		int trip, struct thermal_cooling_device *cdev);
 
-	This interface function unbind a thermal cooling device from the certain trip point
-	of a thermal zone device.
-	This function is usually called in the thermal zone device .unbind callback.
-	tz: the thermal zone device
-	cdev: thermal cooling device
-	trip: indicates which trip point the cooling devices is associated with
-		in this thermal zone.
+    This interface function unbind a thermal cooling device from the certain
+    trip point of a thermal zone device. This function is usually called in
+    the thermal zone device .unbind callback.
+    tz: the thermal zone device
+    cdev: thermal cooling device
+    trip: indicates which trip point the cooling devices is associated with
+	  in this thermal zone.
 
 2. sysfs attributes structure
 
@@ -114,153 +114,157 @@ if hwmon is compiled in or built as a module.
 
 Thermal zone device sys I/F, created once it's registered:
 /sys/class/thermal/thermal_zone[0-*]:
-	|-----type:			Type of the thermal zone
-	|-----temp:			Current temperature
-	|-----mode:			Working mode of the thermal zone
-	|-----trip_point_[0-*]_temp:	Trip point temperature
-	|-----trip_point_[0-*]_type:	Trip point type
+    |---type:			Type of the thermal zone
+    |---temp:			Current temperature
+    |---mode:			Working mode of the thermal zone
+    |---trip_point_[0-*]_temp:	Trip point temperature
+    |---trip_point_[0-*]_type:	Trip point type
 
 Thermal cooling device sys I/F, created once it's registered:
 /sys/class/thermal/cooling_device[0-*]:
-	|-----type :			Type of the cooling device(processor/fan/...)
-	|-----max_state:		Maximum cooling state of the cooling device
-	|-----cur_state:		Current cooling state of the cooling device
+    |---type:			Type of the cooling device(processor/fan/...)
+    |---max_state:		Maximum cooling state of the cooling device
+    |---cur_state:		Current cooling state of the cooling device
 
 
-These two dynamic attributes are created/removed in pairs.
-They represent the relationship between a thermal zone and its associated cooling device.
-They are created/removed for each
-thermal_zone_bind_cooling_device/thermal_zone_unbind_cooling_device successful execution.
+Then next two dynamic attributes are created/removed in pairs. They represent
+the relationship between a thermal zone and its associated cooling device.
+They are created/removed for each successful execution of
+thermal_zone_bind_cooling_device/thermal_zone_unbind_cooling_device.
 
-/sys/class/thermal/thermal_zone[0-*]
-	|-----cdev[0-*]:		The [0-*]th cooling device in the current thermal zone
-	|-----cdev[0-*]_trip_point:	Trip point that cdev[0-*] is associated with
+/sys/class/thermal/thermal_zone[0-*]:
+    |---cdev[0-*]:		[0-*]th cooling device in current thermal zone
+    |---cdev[0-*]_trip_point:	Trip point that cdev[0-*] is associated with
 
 Besides the thermal zone device sysfs I/F and cooling device sysfs I/F,
-the generic thermal driver also creates a hwmon sysfs I/F for each _type_ of
-thermal zone device. E.g. the generic thermal driver registers one hwmon class device
-and build the associated hwmon sysfs I/F for all the registered ACPI thermal zones.
+the generic thermal driver also creates a hwmon sysfs I/F for each _type_
+of thermal zone device. E.g. the generic thermal driver registers one hwmon
+class device and build the associated hwmon sysfs I/F for all the registered
+ACPI thermal zones.
+
 /sys/class/hwmon/hwmon[0-*]:
-	|-----name:			The type of the thermal zone devices.
-	|-----temp[1-*]_input:		The current temperature of thermal zone [1-*].
-	|-----temp[1-*]_critical:	The critical trip point of thermal zone [1-*].
+    |---name:			The type of the thermal zone devices
+    |---temp[1-*]_input:	The current temperature of thermal zone [1-*]
+    |---temp[1-*]_critical:	The critical trip point of thermal zone [1-*]
+
 Please read Documentation/hwmon/sysfs-interface for additional information.
 
 ***************************
 * Thermal zone attributes *
 ***************************
 
-type				Strings which represent the thermal zone type.
-				This is given by thermal zone driver as part of registration.
-				Eg: "acpitz" indicates it's an ACPI thermal device.
-				In order to keep it consistent with hwmon sys attribute,
-				this should be a short, lowercase string,
-				not containing spaces nor dashes.
-				RO
-				Required
-
-temp				Current temperature as reported by thermal zone (sensor)
-				Unit: millidegree Celsius
-				RO
-				Required
-
-mode				One of the predefined values in [kernel, user]
-				This file gives information about the algorithm
-				that is currently managing the thermal zone.
-				It can be either default kernel based algorithm
-				or user space application.
-				RW
-				Optional
-				kernel	= Thermal management in kernel thermal zone driver.
-				user	= Preventing kernel thermal zone driver actions upon
-					  trip points so that user application can take full
-					  charge of the thermal management.
-
-trip_point_[0-*]_temp		The temperature above which trip point will be fired
-				Unit: millidegree Celsius
-				RO
-				Optional
-
-trip_point_[0-*]_type 		Strings which indicate the type of the trip point
-				E.g. it can be one of critical, hot, passive,
-				    active[0-*] for ACPI thermal zone.
-				RO
-				Optional
-
-cdev[0-*]			Sysfs link to the thermal cooling device node where the sys I/F
-				for cooling device throttling control represents.
-				RO
-				Optional
-
-cdev[0-*]_trip_point		The trip point with which cdev[0-*] is associated in this thermal zone
-				-1 means the cooling device is not associated with any trip point.
-				RO
-				Optional
-
-******************************
-* Cooling device  attributes *
-******************************
-
-type				String which represents the type of device
-				eg: For generic ACPI: this should be "Fan",
-				"Processor" or "LCD"
-				eg. For memory controller device on intel_menlow platform:
-				this should be "Memory controller"
-				RO
-				Required
-
-max_state			The maximum permissible cooling state of this cooling device.
-				RO
-				Required
-
-cur_state			The current cooling state of this cooling device.
-				the value can any integer numbers between 0 and max_state,
-				cur_state == 0 means no cooling
-				cur_state == max_state means the maximum cooling.
-				RW
-				Required
+type
+	Strings which represent the thermal zone type.
+	This is given by thermal zone driver as part of registration.
+	E.g: "acpitz" indicates it's an ACPI thermal device.
+	In order to keep it consistent with hwmon sys attribute; this should
+	be a short, lowercase string, not containing spaces nor dashes.
+	RO, Required
+
+temp
+	Current temperature as reported by thermal zone (sensor).
+	Unit: millidegree Celsius
+	RO, Required
+
+mode
+	One of the predefined values in [kernel, user].
+	This file gives information about the algorithm that is currently
+	managing the thermal zone. It can be either default kernel based
+	algorithm or user space application.
+	kernel	= Thermal management in kernel thermal zone driver.
+	user	= Preventing kernel thermal zone driver actions upon
+		  trip points so that user application can take full
+		  charge of the thermal management.
+	RW, Optional
+
+trip_point_[0-*]_temp
+	The temperature above which trip point will be fired.
+	Unit: millidegree Celsius
+	RO, Optional
+
+trip_point_[0-*]_type
+	Strings which indicate the type of the trip point.
+	E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
+	thermal zone.
+	RO, Optional
+
+cdev[0-*]
+	Sysfs link to the thermal cooling device node where the sys I/F
+	for cooling device throttling control represents.
+	RO, Optional
+
+cdev[0-*]_trip_point
+	The trip point with which cdev[0-*] is associated in this thermal
+	zone; -1 means the cooling device is not associated with any trip
+	point.
+	RO, Optional
+
+*****************************
+* Cooling device attributes *
+*****************************
+
+type
+	String which represents the type of device, e.g:
+	- for generic ACPI: should be "Fan", "Processor" or "LCD"
+	- for memory controller device on intel_menlow platform:
+	  should be "Memory controller".
+	RO, Required
+
+max_state
+	The maximum permissible cooling state of this cooling device.
+	RO, Required
+
+cur_state
+	The current cooling state of this cooling device.
+	The value can any integer numbers between 0 and max_state:
+	- cur_state == 0 means no cooling
+	- cur_state == max_state means the maximum cooling.
+	RW, Required
 
 3. A simple implementation
 
-ACPI thermal zone may support multiple trip points like critical/hot/passive/active.
-If an ACPI thermal zone supports critical, passive, active[0] and active[1] at the same time,
-it may register itself as a thermal_zone_device (thermal_zone1) with 4 trip points in all.
-It has one processor and one fan, which are both registered as thermal_cooling_device.
-If the processor is listed in _PSL method, and the fan is listed in _AL0 method,
-the sys I/F structure will be built like this:
+ACPI thermal zone may support multiple trip points like critical, hot,
+passive, active. If an ACPI thermal zone supports critical, passive,
+active[0] and active[1] at the same time, it may register itself as a
+thermal_zone_device (thermal_zone1) with 4 trip points in all.
+It has one processor and one fan, which are both registered as
+thermal_cooling_device.
+
+If the processor is listed in _PSL method, and the fan is listed in _AL0
+method, the sys I/F structure will be built like this:
 
 /sys/class/thermal:
 
 |thermal_zone1:
-	|-----type:			acpitz
-	|-----temp:			37000
-	|-----mode:			kernel
-	|-----trip_point_0_temp:	100000
-	|-----trip_point_0_type:	critical
-	|-----trip_point_1_temp:	80000
-	|-----trip_point_1_type:	passive
-	|-----trip_point_2_temp:	70000
-	|-----trip_point_2_type:	active0
-	|-----trip_point_3_temp:	60000
-	|-----trip_point_3_type:	active1
-	|-----cdev0:			--->/sys/class/thermal/cooling_device0
-	|-----cdev0_trip_point:		1	/* cdev0 can be used for passive */
-	|-----cdev1:			--->/sys/class/thermal/cooling_device3
-	|-----cdev1_trip_point:		2	/* cdev1 can be used for active[0]*/
+    |---type:			acpitz
+    |---temp:			37000
+    |---mode:			kernel
+    |---trip_point_0_temp:	100000
+    |---trip_point_0_type:	critical
+    |---trip_point_1_temp:	80000
+    |---trip_point_1_type:	passive
+    |---trip_point_2_temp:	70000
+    |---trip_point_2_type:	active0
+    |---trip_point_3_temp:	60000
+    |---trip_point_3_type:	active1
+    |---cdev0:			--->/sys/class/thermal/cooling_device0
+    |---cdev0_trip_point:	1	/* cdev0 can be used for passive */
+    |---cdev1:			--->/sys/class/thermal/cooling_device3
+    |---cdev1_trip_point:	2	/* cdev1 can be used for active[0]*/
 
 |cooling_device0:
-	|-----type:			Processor
-	|-----max_state:		8
-	|-----cur_state:		0
+    |---type:			Processor
+    |---max_state:		8
+    |---cur_state:		0
 
 |cooling_device3:
-	|-----type:			Fan
-	|-----max_state:		2
-	|-----cur_state:		0
+    |---type:			Fan
+    |---max_state:		2
+    |---cur_state:		0
 
 /sys/class/hwmon:
 
 |hwmon0:
-	|-----name:			acpitz
-	|-----temp1_input:		37000
-	|-----temp1_crit:		100000
+    |---name:			acpitz
+    |---temp1_input:		37000
+    |---temp1_crit:		100000

[PATCH 2/6] thermal: sysfs-api.txt - document passive attribute for thermal zones

[Frans Pop]

Signed-off-by: Frans Pop <elendil@planet.nl>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Sujith Thomas <sujith.thomas@intel.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index 895337f..2a036eb 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -3,7 +3,7 @@ Generic Thermal Sysfs driver How To
 
 Written by Sujith Thomas <sujith.thomas@intel.com>, Zhang Rui <rui.zhang@intel.com>
 
-Updated: 2 January 2008
+Updated: 21 August 2009
 
 Copyright (c)  2008 Intel Corporation
 
@@ -199,6 +199,15 @@ cdev[0-*]_trip_point
 	point.
 	RO, Optional
 
+passive
+	Attribute is only present for zones which do not have a passive
+	cooling policy (_PSV) defined in ACPI. Default is zero and can be
+	set to a temperature (in millidegrees) to enable a passive trip
+	point for the zone. Activation is done by polling with an interval
+	of 1 second.
+	Unit: millidegrees Celsius
+	RW, Optional
+
 *****************************
 * Cooling device attributes *
 *****************************
@@ -230,8 +239,9 @@ thermal_zone_device (thermal_zone1) with 4 trip points in all.
 It has one processor and one fan, which are both registered as
 thermal_cooling_device.
 
-If the processor is listed in _PSL method, and the fan is listed in _AL0
-method, the sys I/F structure will be built like this:
+If the processor is listed in _PSL method, the fan is listed in _AL0
+method, and the zone has a _PSV method, the sys I/F structure will be
+built like this:
 
 /sys/class/thermal:
 

Re: [PATCH 2/6] thermal: sysfs-api.txt - document passive attribute for thermal zones

[Zhang Rui]

On Thu, 2009-08-27 at 00:17 +0800, Frans Pop wrote:
> Signed-off-by: Frans Pop <elendil@planet.nl>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Sujith Thomas <sujith.thomas@intel.com>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> 
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> index 895337f..2a036eb 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -3,7 +3,7 @@ Generic Thermal Sysfs driver How To
>  
>  Written by Sujith Thomas <sujith.thomas@intel.com>, Zhang Rui <rui.zhang@intel.com>
>  
> -Updated: 2 January 2008
> +Updated: 21 August 2009
>  
>  Copyright (c)  2008 Intel Corporation
>  
> @@ -199,6 +199,15 @@ cdev[0-*]_trip_point
>  	point.
>  	RO, Optional
>  
> +passive
> +	Attribute is only present for zones which do not have a passive
> +	cooling policy (_PSV) defined in ACPI.

We should not involve any platform specific stuff in this documentation.
how about "Attribute is only present for zones in which the passive
cooling policy is not supported by native thermal driver"

>  Default is zero and can be
> +	set to a temperature (in millidegrees) to enable a passive trip
> +	point for the zone. Activation is done by polling with an interval
> +	of 1 second.
> +	Unit: millidegrees Celsius
> +	RW, Optional
> +
>  *****************************
>  * Cooling device attributes *
>  *****************************
> @@ -230,8 +239,9 @@ thermal_zone_device (thermal_zone1) with 4 trip points in all.
>  It has one processor and one fan, which are both registered as
>  thermal_cooling_device.
>  
> -If the processor is listed in _PSL method, and the fan is listed in _AL0
> -method, the sys I/F structure will be built like this:
> +If the processor is listed in _PSL method, the fan is listed in _AL0
> +method, and the zone has a _PSV method, the sys I/F structure will be
> +built like this:
>  
For an ACPI thermal zone, supporting passive cooling means that both
_PSL and _PSV are available.
so I don't think we need to change this. :)

thanks,
rui
>  /sys/class/thermal:
>  

Re: [PATCH 2/6] thermal: sysfs-api.txt - document passive attribute for thermal zones

[Frans Pop]

On Monday 31 August 2009, Zhang Rui wrote:
> > +passive
> > +	Attribute is only present for zones which do not have a passive
> > +	cooling policy (_PSV) defined in ACPI.
>
> We should not involve any platform specific stuff in this
> documentation. how about "Attribute is only present for zones in which
> the passive cooling policy is not supported by native thermal driver"
[...]
> > -If the processor is listed in _PSL method, and the fan is listed in
> > _AL0 -method, the sys I/F structure will be built like this:
> > +If the processor is listed in _PSL method, the fan is listed in _AL0
> > +method, and the zone has a _PSV method, the sys I/F structure will
> > be +built like this:
>
> For an ACPI thermal zone, supporting passive cooling means that both
> _PSL and _PSV are available.
> so I don't think we need to change this. :)

OK. Updated patch below.


From: Frans Pop <elendil@planet.nl>
Subject: thermal: sysfs-api.txt - document passive attribute for thermal zones

Signed-off-by: Frans Pop <elendil@planet.nl>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Sujith Thomas <sujith.thomas@intel.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index 895337f..a87dc27 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -199,6 +199,15 @@ cdev[0-*]_trip_point
 	point.
 	RO, Optional
 
+passive
+	Attribute is only present for zones in which the passive cooling
+	policy is not supported by native thermal driver. Default is zero
+	and can be set to a temperature (in millidegrees) to enable a
+	passive trip point for the zone. Activation is done by polling
+	with an interval of 1 second.
+	Unit: millidegrees Celsius
+	RW, Optional
+
 *****************************
 * Cooling device attributes *
 *****************************

Re: [PATCH 2/6] thermal: sysfs-api.txt - document passive attribute for thermal zones

[Zhang Rui]

On Mon, 2009-08-31 at 19:19 +0800, Frans Pop wrote:
> On Monday 31 August 2009, Zhang Rui wrote:
> > > +passive
> > > +	Attribute is only present for zones which do not have a passive
> > > +	cooling policy (_PSV) defined in ACPI.
> >
> > We should not involve any platform specific stuff in this
> > documentation. how about "Attribute is only present for zones in which
> > the passive cooling policy is not supported by native thermal driver"
> [...]
> > > -If the processor is listed in _PSL method, and the fan is listed in
> > > _AL0 -method, the sys I/F structure will be built like this:
> > > +If the processor is listed in _PSL method, the fan is listed in _AL0
> > > +method, and the zone has a _PSV method, the sys I/F structure will
> > > be +built like this:
> >
> > For an ACPI thermal zone, supporting passive cooling means that both
> > _PSL and _PSV are available.
> > so I don't think we need to change this. :)
> 
> OK. Updated patch below.
> 
> 
> From: Frans Pop <elendil@planet.nl>
> Subject: thermal: sysfs-api.txt - document passive attribute for thermal zones
> 
> Signed-off-by: Frans Pop <elendil@planet.nl>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Sujith Thomas <sujith.thomas@intel.com>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> 
Acked-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> index 895337f..a87dc27 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -199,6 +199,15 @@ cdev[0-*]_trip_point
>  	point.
>  	RO, Optional
>  
> +passive
> +	Attribute is only present for zones in which the passive cooling
> +	policy is not supported by native thermal driver. Default is zero
> +	and can be set to a temperature (in millidegrees) to enable a
> +	passive trip point for the zone. Activation is done by polling
> +	with an interval of 1 second.
> +	Unit: millidegrees Celsius
> +	RW, Optional
> +
>  *****************************
>  * Cooling device attributes *
>  *****************************

Re: [PATCH 2/6] thermal: sysfs-api.txt - document passive attribute for thermal zones

[Pavel Machek]

Hi!

> > > We should not involve any platform specific stuff in this
> > > documentation. how about "Attribute is only present for zones in which
> > > the passive cooling policy is not supported by native thermal driver"
> > [...]
> > > > -If the processor is listed in _PSL method, and the fan is listed in
> > > > _AL0 -method, the sys I/F structure will be built like this:
> > > > +If the processor is listed in _PSL method, the fan is listed in _AL0
> > > > +method, and the zone has a _PSV method, the sys I/F structure will
> > > > be +built like this:
> > >
> > > For an ACPI thermal zone, supporting passive cooling means that both
> > > _PSL and _PSV are available.
> > > so I don't think we need to change this. :)
> > 
> > OK. Updated patch below.
> > 
> > 
> > From: Frans Pop <elendil@planet.nl>
> > Subject: thermal: sysfs-api.txt - document passive attribute for thermal zones
> > 
> > Signed-off-by: Frans Pop <elendil@planet.nl>
> > Cc: Zhang Rui <rui.zhang@intel.com>
> > Cc: Sujith Thomas <sujith.thomas@intel.com>
> > Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> > 
> Acked-by: Zhang Rui <rui.zhang@intel.com>
> 
> thanks,
> rui
> > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> > index 895337f..a87dc27 100644
> > --- a/Documentation/thermal/sysfs-api.txt
> > +++ b/Documentation/thermal/sysfs-api.txt
> > @@ -199,6 +199,15 @@ cdev[0-*]_trip_point
> >  	point.
> >  	RO, Optional
> >  
> > +passive
> > +	Attribute is only present for zones in which the passive cooling
> > +	policy is not supported by native thermal driver. Default is zero
> > +	and can be set to a temperature (in millidegrees) to enable a
> > +	passive trip point for the zone. Activation is done by polling
> > +	with an interval of 1 second.
> > +	Unit: millidegrees Celsius
> > +	RW, Optional
> > +

So '0=disabled'? Better say that, because throttling at 0C would be
useless in most of the world.

Are values below zero supported? Maybe -28000mC should mean
'disabled'? :-).

									Pavel 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/6] thermal: sysfs-api.txt - document passive attribute for thermal zones

[Frans Pop]

On Wednesday 02 September 2009, Pavel Machek wrote:
> So '0=disabled'? Better say that, because throttling at 0C would be
> useless in most of the world.

I've addressed this in the new version of patch 4/6 I just sent.

Cheers,
FJP

[PATCH 3/6] acpi: thermal: display forced passive trip points in proc

[Frans Pop]

Users can force passive cooling for a thermal zone that does not
have _PSV defined in ACPI by setting the passive attribute in sysfs.
It's useful to display such trip points in /proc/acpi/thermal_zone.

.../TZ1/cooling_mode:<setting not supported>
.../TZ1/polling_frequency:polling frequency:       10 seconds
.../TZ1/state:state:                   ok
.../TZ1/temperature:temperature:             53 C
.../TZ1/trip_points:critical (S5):           110 C
.../TZ1/trip_points:passive (forced):        95 C

And if not set (passive is 0):
.../TZ1/trip_points:passive (forced):<not set>

Signed-off-by: Frans Pop <elendil@planet.nl>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 564ea14..a4e507b 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -1050,6 +1050,13 @@ static int acpi_thermal_trip_seq_show(struct seq_file *seq, void *offset)
 				   acpi_device_bid(device));
 		}
 		seq_puts(seq, "\n");
+	} else {
+		seq_printf(seq, "passive (forced):");
+		if (tz->thermal_zone->forced_passive)
+			seq_printf(seq, "        %i C\n",
+				   tz->thermal_zone->forced_passive / 1000);
+		else
+			seq_printf(seq, "<not set>\n");
 	}
 
 	for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {

Re: [PATCH 3/6] acpi: thermal: display forced passive trip points in proc

[Zhang Rui]

On Thu, 2009-08-27 at 00:17 +0800, Frans Pop wrote:
> Users can force passive cooling for a thermal zone that does not
> have _PSV defined in ACPI by setting the passive attribute in sysfs.
> It's useful to display such trip points in /proc/acpi/thermal_zone.
> 
> .../TZ1/cooling_mode:<setting not supported>
> .../TZ1/polling_frequency:polling frequency:       10 seconds
> .../TZ1/state:state:                   ok
> .../TZ1/temperature:temperature:             53 C
> .../TZ1/trip_points:critical (S5):           110 C
> .../TZ1/trip_points:passive (forced):        95 C
> 
> And if not set (passive is 0):
> .../TZ1/trip_points:passive (forced):<not set>
> 
> Signed-off-by: Frans Pop <elendil@planet.nl>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> 
Acked-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 564ea14..a4e507b 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -1050,6 +1050,13 @@ static int acpi_thermal_trip_seq_show(struct seq_file *seq, void *offset)
>  				   acpi_device_bid(device));
>  		}
>  		seq_puts(seq, "\n");
> +	} else {
> +		seq_printf(seq, "passive (forced):");
> +		if (tz->thermal_zone->forced_passive)
> +			seq_printf(seq, "        %i C\n",
> +				   tz->thermal_zone->forced_passive / 1000);
> +		else
> +			seq_printf(seq, "<not set>\n");
>  	}
>  
>  	for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {

[PATCH 4/6] thermal: add sanity check for the passive attribute

[Frans Pop]

Values below 40000 milli-celsius (limit is somewhat arbitrary)
don't make sense and can cause the system to go into a thermal
heart attack: the actual temperature will always be lower and
thus the system will be throttled down to its lowest setting.

For values below 1000 an additional problem is that they would
show as 0 in /proc/acpi/thermal/TZx/trip_points:passive.

cat passive
0
echo -n 90000 >passive
cat passive
90000
echo -n 30000 >passive
bash: echo: write error: Invalid argument

Signed-off-by: Frans Pop <elendil@planet.nl>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Zhang Rui <rui.zhang@intel.com>

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index 2a036eb..09d5c88 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -206,6 +206,7 @@ passive
 	point for the zone. Activation is done by polling with an interval
 	of 1 second.
 	Unit: millidegrees Celsius
+	Minimum value: 40000
 	RW, Optional
 
 *****************************
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 0a69672..2d13d0d 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -225,6 +225,12 @@ passive_store(struct device *dev, struct device_attribute *attr,
 	if (!sscanf(buf, "%d\n", &state))
 		return -EINVAL;
 
+	/* sanity check: values below 40000 millicelcius don't make sense
+	 * and can cause the system to go into a thermal heart attack
+	 */
+	if (state && state < 40000)
+		return -EINVAL;
+
 	if (state && !tz->forced_passive) {
 		mutex_lock(&thermal_list_lock);
 		list_for_each_entry(cdev, &thermal_cdev_list, node) {

Re: [PATCH 4/6] thermal: add sanity check for the passive attribute

[Matthew Garrett]

On Wed, Aug 26, 2009 at 06:17:23PM +0200, Frans Pop wrote:
> Values below 40000 milli-celsius (limit is somewhat arbitrary)
> don't make sense and can cause the system to go into a thermal
> heart attack: the actual temperature will always be lower and
> thus the system will be throttled down to its lowest setting.

Not keen on this - it's a pretty arbitrary cutoff, and there are some 
cases where someone might want this value. Policy belongs in userspace, 
and all that.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 4/6] thermal: add sanity check for the passive attribute

[Frans Pop]

On Wednesday 26 August 2009, Matthew Garrett wrote:
> On Wed, Aug 26, 2009 at 06:17:23PM +0200, Frans Pop wrote:
> > Values below 40000 milli-celsius (limit is somewhat arbitrary)
> > don't make sense and can cause the system to go into a thermal
> > heart attack: the actual temperature will always be lower and
> > thus the system will be throttled down to its lowest setting.
>
> Not keen on this - it's a pretty arbitrary cutoff, and there are some
> cases where someone might want this value. Policy belongs in userspace,
> and all that.

What cases do you see? Testing? Or systems that might have to operate at 
such a low temperature? I deliberately chose a value that's at a level 
that's easy to reach.

I agree it is arbitrary, but it will prevent major confusion when someone 
like me echo's 95 directly in sysfs.
Would 1000 (1 °C) perhaps be more acceptable as a limit? I doubt there are 
valid use-cases for below 0 temps :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] thermal: add sanity check for the passive attribute

[Frans Pop]

On Wednesday 26 August 2009, Matthew Garrett wrote:
> On Wed, Aug 26, 2009 at 06:17:23PM +0200, Frans Pop wrote:
> > Values below 40000 milli-celsius (limit is somewhat arbitrary)
> > don't make sense and can cause the system to go into a thermal
> > heart attack: the actual temperature will always be lower and
> > thus the system will be throttled down to its lowest setting.
>
> Not keen on this - it's a pretty arbitrary cutoff, and there are some
> cases where someone might want this value. Policy belongs in userspace,
> and all that.

What cases do you see? Testing? Or systems that might have to operate at 
such a low temperature? I deliberately chose a value that's at a level 
that's easy to reach.

I agree it is arbitrary, but it will prevent major confusion when someone 
like me echo's 95 directly in sysfs.
Would 1000 (1 °C) perhaps be more acceptable as a limit? I doubt there are 
valid use-cases for below 0 temps :-)

Re: [PATCH 4/6] thermal: add sanity check for the passive attribute

[Zhang Rui]

On Thu, 2009-08-27 at 00:48 +0800, Frans Pop wrote:
> On Wednesday 26 August 2009, Matthew Garrett wrote:
> > On Wed, Aug 26, 2009 at 06:17:23PM +0200, Frans Pop wrote:
> > > Values below 40000 milli-celsius (limit is somewhat arbitrary)
> > > don't make sense and can cause the system to go into a thermal
> > > heart attack: the actual temperature will always be lower and
> > > thus the system will be throttled down to its lowest setting.
> >
> > Not keen on this - it's a pretty arbitrary cutoff, and there are some
> > cases where someone might want this value. Policy belongs in userspace,
> > and all that.
> 
> What cases do you see? Testing? Or systems that might have to operate at 
> such a low temperature? I deliberately chose a value that's at a level 
> that's easy to reach.
> 
> I agree it is arbitrary, but it will prevent major confusion when someone 
> like me echo's 95 directly in sysfs.

this is a problem.
how about something like:
#define THERMAL_PASSIVE_WARNING_LEVEL 0x40000

if (state < THERMAL_PASSIVE_WARNING_LEVEL)
   printk(KERN_WARNING PREFIX "Passive trip point too low, this may"
          "slow down your laptop because processors are throttled "
          "whenever the temperature is higher than %dC\n", state/1000);

thanks,
rui

> Would 1000 (1 °C) perhaps be more acceptable as a limit? I doubt there are 
> valid use-cases for below 0 temps :-)

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] thermal: add sanity check for the passive attribute

[Zhang Rui]

On Thu, 2009-08-27 at 00:48 +0800, Frans Pop wrote:
> On Wednesday 26 August 2009, Matthew Garrett wrote:
> > On Wed, Aug 26, 2009 at 06:17:23PM +0200, Frans Pop wrote:
> > > Values below 40000 milli-celsius (limit is somewhat arbitrary)
> > > don't make sense and can cause the system to go into a thermal
> > > heart attack: the actual temperature will always be lower and
> > > thus the system will be throttled down to its lowest setting.
> >
> > Not keen on this - it's a pretty arbitrary cutoff, and there are some
> > cases where someone might want this value. Policy belongs in userspace,
> > and all that.
> 
> What cases do you see? Testing? Or systems that might have to operate at 
> such a low temperature? I deliberately chose a value that's at a level 
> that's easy to reach.
> 
> I agree it is arbitrary, but it will prevent major confusion when someone 
> like me echo's 95 directly in sysfs.

this is a problem.
how about something like:
#define THERMAL_PASSIVE_WARNING_LEVEL 0x40000

if (state < THERMAL_PASSIVE_WARNING_LEVEL)
   printk(KERN_WARNING PREFIX "Passive trip point too low, this may"
          "slow down your laptop because processors are throttled "
          "whenever the temperature is higher than %dC\n", state/1000);

thanks,
rui

> Would 1000 (1 °C) perhaps be more acceptable as a limit? I doubt there are 
> valid use-cases for below 0 temps :-)

Re: [PATCH 4/6] thermal: add sanity check for the passive attribute

[Frans Pop]

On Monday 31 August 2009, Zhang Rui wrote:
> On Thu, 2009-08-27 at 00:48 +0800, Frans Pop wrote:
> > On Wednesday 26 August 2009, Matthew Garrett wrote:
> > > On Wed, Aug 26, 2009 at 06:17:23PM +0200, Frans Pop wrote:
> > > > Values below 40000 milli-celsius (limit is somewhat arbitrary)
> > > > don't make sense and can cause the system to go into a thermal
> > > > heart attack: the actual temperature will always be lower and
> > > > thus the system will be throttled down to its lowest setting.
> > >
> > > Not keen on this - it's a pretty arbitrary cutoff, and there are
> > > some cases where someone might want this value. Policy belongs in
> > > userspace, and all that.
> >
> > What cases do you see? Testing? Or systems that might have to operate
> > at such a low temperature? I deliberately chose a value that's at a
> > level that's easy to reach.
> >
> > I agree it is arbitrary, but it will prevent major confusion when
> > someone like me echo's 95 directly in sysfs.
>
> this is a problem.
> how about something like:
> #define THERMAL_PASSIVE_WARNING_LEVEL 0x40000

Hmmm. 40000 hexadecimal? That seems a bit high ;-)

> if (state < THERMAL_PASSIVE_WARNING_LEVEL)
>    printk(KERN_WARNING PREFIX "Passive trip point too low, this may"
>           "slow down your laptop because processors are throttled "
>           "whenever the temperature is higher than %dC\n", state/1000);

Disadvantage is that users are unlikely to actually see that message at 
the time they set the value, especially if they're working in some xterm. 
They'd have to check dmesg or log files. It also increases the .text size 
of the module for an option that very few people are likely to use.

> > Would 1000 (1 °C) perhaps be more acceptable as a limit? I doubt
> > there are valid use-cases for below 0 temps :-)

I'd prefer this option. Do you see any downside to this?

Cheers,
FJP
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] thermal: add sanity check for the passive attribute

[Frans Pop]

On Monday 31 August 2009, Zhang Rui wrote:
> On Thu, 2009-08-27 at 00:48 +0800, Frans Pop wrote:
> > On Wednesday 26 August 2009, Matthew Garrett wrote:
> > > On Wed, Aug 26, 2009 at 06:17:23PM +0200, Frans Pop wrote:
> > > > Values below 40000 milli-celsius (limit is somewhat arbitrary)
> > > > don't make sense and can cause the system to go into a thermal
> > > > heart attack: the actual temperature will always be lower and
> > > > thus the system will be throttled down to its lowest setting.
> > >
> > > Not keen on this - it's a pretty arbitrary cutoff, and there are
> > > some cases where someone might want this value. Policy belongs in
> > > userspace, and all that.
> >
> > What cases do you see? Testing? Or systems that might have to operate
> > at such a low temperature? I deliberately chose a value that's at a
> > level that's easy to reach.
> >
> > I agree it is arbitrary, but it will prevent major confusion when
> > someone like me echo's 95 directly in sysfs.
>
> this is a problem.
> how about something like:
> #define THERMAL_PASSIVE_WARNING_LEVEL 0x40000

Hmmm. 40000 hexadecimal? That seems a bit high ;-)

> if (state < THERMAL_PASSIVE_WARNING_LEVEL)
>    printk(KERN_WARNING PREFIX "Passive trip point too low, this may"
>           "slow down your laptop because processors are throttled "
>           "whenever the temperature is higher than %dC\n", state/1000);

Disadvantage is that users are unlikely to actually see that message at 
the time they set the value, especially if they're working in some xterm. 
They'd have to check dmesg or log files. It also increases the .text size 
of the module for an option that very few people are likely to use.

> > Would 1000 (1 °C) perhaps be more acceptable as a limit? I doubt
> > there are valid use-cases for below 0 temps :-)

I'd prefer this option. Do you see any downside to this?

Cheers,
FJP

Re: [PATCH 4/6] thermal: add sanity check for the passive attribute

[Zhang Rui]

On Mon, 2009-08-31 at 18:30 +0800, Frans Pop wrote:
> On Monday 31 August 2009, Zhang Rui wrote:
> > On Thu, 2009-08-27 at 00:48 +0800, Frans Pop wrote:
> > > On Wednesday 26 August 2009, Matthew Garrett wrote:
> > > > On Wed, Aug 26, 2009 at 06:17:23PM +0200, Frans Pop wrote:
> > > > > Values below 40000 milli-celsius (limit is somewhat arbitrary)
> > > > > don't make sense and can cause the system to go into a thermal
> > > > > heart attack: the actual temperature will always be lower and
> > > > > thus the system will be throttled down to its lowest setting.
> > > >
> > > > Not keen on this - it's a pretty arbitrary cutoff, and there are
> > > > some cases where someone might want this value. Policy belongs in
> > > > userspace, and all that.
> > >
> > > What cases do you see? Testing? Or systems that might have to operate
> > > at such a low temperature? I deliberately chose a value that's at a
> > > level that's easy to reach.
> > >
> > > I agree it is arbitrary, but it will prevent major confusion when
> > > someone like me echo's 95 directly in sysfs.
> >
> > this is a problem.
> > how about something like:
> > #define THERMAL_PASSIVE_WARNING_LEVEL 0x40000
> 
> Hmmm. 40000 hexadecimal? That seems a bit high ;-)
> 
> > if (state < THERMAL_PASSIVE_WARNING_LEVEL)
> >    printk(KERN_WARNING PREFIX "Passive trip point too low, this may"
> >           "slow down your laptop because processors are throttled "
> >           "whenever the temperature is higher than %dC\n", state/1000);
> 
> Disadvantage is that users are unlikely to actually see that message at 
> the time they set the value, especially if they're working in some xterm. 
> They'd have to check dmesg or log files. It also increases the .text size 
> of the module for an option that very few people are likely to use.
> 
> > > Would 1000 (1 °C) perhaps be more acceptable as a limit? I doubt
> > > there are valid use-cases for below 0 temps :-)
> 
> I'd prefer this option. Do you see any downside to this?
> 
I see many laptops with a passive trip point higher than 90C, so a
passive trip point higher than 100C may be meaningful.
I think we should use a higher value, say 2000?

thanks,
rui


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] thermal: add sanity check for the passive attribute

[Zhang Rui]

On Mon, 2009-08-31 at 18:30 +0800, Frans Pop wrote:
> On Monday 31 August 2009, Zhang Rui wrote:
> > On Thu, 2009-08-27 at 00:48 +0800, Frans Pop wrote:
> > > On Wednesday 26 August 2009, Matthew Garrett wrote:
> > > > On Wed, Aug 26, 2009 at 06:17:23PM +0200, Frans Pop wrote:
> > > > > Values below 40000 milli-celsius (limit is somewhat arbitrary)
> > > > > don't make sense and can cause the system to go into a thermal
> > > > > heart attack: the actual temperature will always be lower and
> > > > > thus the system will be throttled down to its lowest setting.
> > > >
> > > > Not keen on this - it's a pretty arbitrary cutoff, and there are
> > > > some cases where someone might want this value. Policy belongs in
> > > > userspace, and all that.
> > >
> > > What cases do you see? Testing? Or systems that might have to operate
> > > at such a low temperature? I deliberately chose a value that's at a
> > > level that's easy to reach.
> > >
> > > I agree it is arbitrary, but it will prevent major confusion when
> > > someone like me echo's 95 directly in sysfs.
> >
> > this is a problem.
> > how about something like:
> > #define THERMAL_PASSIVE_WARNING_LEVEL 0x40000
> 
> Hmmm. 40000 hexadecimal? That seems a bit high ;-)
> 
> > if (state < THERMAL_PASSIVE_WARNING_LEVEL)
> >    printk(KERN_WARNING PREFIX "Passive trip point too low, this may"
> >           "slow down your laptop because processors are throttled "
> >           "whenever the temperature is higher than %dC\n", state/1000);
> 
> Disadvantage is that users are unlikely to actually see that message at 
> the time they set the value, especially if they're working in some xterm. 
> They'd have to check dmesg or log files. It also increases the .text size 
> of the module for an option that very few people are likely to use.
> 
> > > Would 1000 (1 °C) perhaps be more acceptable as a limit? I doubt
> > > there are valid use-cases for below 0 temps :-)
> 
> I'd prefer this option. Do you see any downside to this?
> 
I see many laptops with a passive trip point higher than 90C, so a
passive trip point higher than 100C may be meaningful.
I think we should use a higher value, say 2000?

thanks,
rui

Re: [PATCH 4/6,v2] thermal: add sanity check for the passive attribute

[Frans Pop]

On Thursday 03 September 2009, Zhang Rui wrote:
> > > > Would 1000 (1 °C) perhaps be more acceptable as a limit? I doubt
> > > > there are valid use-cases for below 0 temps :-)
> >
> > I'd prefer this option. Do you see any downside to this?
>
> I see many laptops with a passive trip point higher than 90C, so a
> passive trip point higher than 100C may be meaningful.
> I think we should use a higher value, say 2000?

I think you misunderstand. I want to return -EINVAL if state < 1000, which
means that they entered a value smaller than 1 degree C. This will prevent
users from entering for example "90" in the expectation that that sets the
trip point to 90 degrees C. See the new patch below.

When they see the error, it will be straightforward to discover that they
should enter 90000 instead, for example because temp is also in
millidegrees.

I don't think there is any reason to test for an upper limit.

Cheers,
FJP


From: Frans Pop <elendil@planet.nl>
Subject: thermal: add sanity check for the passive attribute

Values below 1000 milli-celsius don't make sense and can cause the
system to go into a thermal heart attack: the actual temperature
will always be lower and thus the system will be throttled down to
its lowest setting.

An additional problem is that values below 1000 will show as 0 in
/proc/acpi/thermal/TZx/trip_points:passive.

cat passive
0
echo -n 90 >passive
bash: echo: write error: Invalid argument
echo -n 90000 >passive
cat passive
90000

Signed-off-by: Frans Pop <elendil@planet.nl>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Zhang Rui <rui.zhang@intel.com>

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index a87dc27..cb3d15b 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -206,6 +206,7 @@ passive
 	passive trip point for the zone. Activation is done by polling with
 	an interval of 1 second.
 	Unit: millidegrees Celsius
+	Valid values: 0 (disabled) or greater than 1000
 	RW, Optional
 
 *****************************
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 4e83c29..74d2eb5 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -225,6 +225,12 @@ passive_store(struct device *dev, struct device_attribute *attr,
 	if (!sscanf(buf, "%d\n", &state))
 		return -EINVAL;
 
+	/* sanity check: values below 1000 millicelcius don't make sense
+	 * and can cause the system to go into a thermal heart attack
+	 */
+	if (state && state < 1000)
+		return -EINVAL;
+
 	if (state && !tz->forced_passive) {
 		mutex_lock(&thermal_list_lock);
 		list_for_each_entry(cdev, &thermal_cdev_list, node) {
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6,v2] thermal: add sanity check for the passive attribute

[Frans Pop]

On Thursday 03 September 2009, Zhang Rui wrote:
> > > > Would 1000 (1 °C) perhaps be more acceptable as a limit? I doubt
> > > > there are valid use-cases for below 0 temps :-)
> >
> > I'd prefer this option. Do you see any downside to this?
>
> I see many laptops with a passive trip point higher than 90C, so a
> passive trip point higher than 100C may be meaningful.
> I think we should use a higher value, say 2000?

I think you misunderstand. I want to return -EINVAL if state < 1000, which
means that they entered a value smaller than 1 degree C. This will prevent
users from entering for example "90" in the expectation that that sets the
trip point to 90 degrees C. See the new patch below.

When they see the error, it will be straightforward to discover that they
should enter 90000 instead, for example because temp is also in
millidegrees.

I don't think there is any reason to test for an upper limit.

Cheers,
FJP


From: Frans Pop <elendil@planet.nl>
Subject: thermal: add sanity check for the passive attribute

Values below 1000 milli-celsius don't make sense and can cause the
system to go into a thermal heart attack: the actual temperature
will always be lower and thus the system will be throttled down to
its lowest setting.

An additional problem is that values below 1000 will show as 0 in
/proc/acpi/thermal/TZx/trip_points:passive.

cat passive
0
echo -n 90 >passive
bash: echo: write error: Invalid argument
echo -n 90000 >passive
cat passive
90000

Signed-off-by: Frans Pop <elendil@planet.nl>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Zhang Rui <rui.zhang@intel.com>

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index a87dc27..cb3d15b 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -206,6 +206,7 @@ passive
 	passive trip point for the zone. Activation is done by polling with
 	an interval of 1 second.
 	Unit: millidegrees Celsius
+	Valid values: 0 (disabled) or greater than 1000
 	RW, Optional
 
 *****************************
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 4e83c29..74d2eb5 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -225,6 +225,12 @@ passive_store(struct device *dev, struct device_attribute *attr,
 	if (!sscanf(buf, "%d\n", &state))
 		return -EINVAL;
 
+	/* sanity check: values below 1000 millicelcius don't make sense
+	 * and can cause the system to go into a thermal heart attack
+	 */
+	if (state && state < 1000)
+		return -EINVAL;
+
 	if (state && !tz->forced_passive) {
 		mutex_lock(&thermal_list_lock);
 		list_for_each_entry(cdev, &thermal_cdev_list, node) {

[PATCH 5/6] thermal: Only set passive_delay for forced passive cooling

[Frans Pop]

Setting polling_delay is useless as passive_delay has priority,
so the value shown in proc isn't the actual polling delay. It
also gives the impression to the user that he can change the
polling interval through proc, while in fact he can't.

Also, unset passive_delay when the forced passive trip point is
unbound to allow polling to be disabled.

Signed-off-by: Frans Pop <elendil@planet.nl>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Zhang Rui <rui.zhang@intel.com>
---

I'm not sure why polling_delay was getting set here. Possibly the
intention was to set polling_frequency instead, which is in deci-
seconds and would thus explain the factor 10 between the values.
But even for polling_frequency there is IMO no reason to set it.

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 2d13d0d..ceda0f1 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -241,6 +241,8 @@ passive_store(struct device *dev, struct device_attribute *attr,
 								 cdev);
 		}
 		mutex_unlock(&thermal_list_lock);
+		if (!tz->passive_delay)
+			tz->passive_delay = 1000;
 	} else if (!state && tz->forced_passive) {
 		mutex_lock(&thermal_list_lock);
 		list_for_each_entry(cdev, &thermal_cdev_list, node) {
@@ -251,17 +253,12 @@ passive_store(struct device *dev, struct device_attribute *attr,
 								   cdev);
 		}
 		mutex_unlock(&thermal_list_lock);
+		tz->passive_delay = 0;
 	}
 
 	tz->tc1 = 1;
 	tz->tc2 = 1;
 
-	if (!tz->passive_delay)
-		tz->passive_delay = 1000;
-
-	if (!tz->polling_delay)
-		tz->polling_delay = 10000;
-
 	tz->forced_passive = state;
 
 	thermal_zone_device_update(tz);

Re: [PATCH 5/6] thermal: Only set passive_delay for forced passive cooling

[Matthew Garrett]

On Wed, Aug 26, 2009 at 06:17:24PM +0200, Frans Pop wrote:
> Setting polling_delay is useless as passive_delay has priority,
> so the value shown in proc isn't the actual polling delay. It
> also gives the impression to the user that he can change the
> polling interval through proc, while in fact he can't.
> 
> Also, unset passive_delay when the forced passive trip point is
> unbound to allow polling to be disabled.
> 
> Signed-off-by: Frans Pop <elendil@planet.nl>
> Cc: Matthew Garrett <mjg@redhat.com>
> Cc: Zhang Rui <rui.zhang@intel.com>

I'll look over this - I seem to remember having some reason to set that, 
but it escapes me now.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 5/6] thermal: Only set passive_delay for forced passive cooling

[Frans Pop]

Hi Matthew,

On Wednesday 26 August 2009, Matthew Garrett wrote:
> On Wed, Aug 26, 2009 at 06:17:24PM +0200, Frans Pop wrote:
> > Setting polling_delay is useless as passive_delay has priority,
> > so the value shown in proc isn't the actual polling delay. It
> > also gives the impression to the user that he can change the
> > polling interval through proc, while in fact he can't.
> >
> > Also, unset passive_delay when the forced passive trip point is
> > unbound to allow polling to be disabled.
> >
> > Signed-off-by: Frans Pop <elendil@planet.nl>
> > Cc: Matthew Garrett <mjg@redhat.com>
> > Cc: Zhang Rui <rui.zhang@intel.com>
>
> I'll look over this - I seem to remember having some reason to set
> that, but it escapes me now.

Have you had a chance to check this?

Cheers,
FJP

Re: [PATCH 5/6] thermal: Only set passive_delay for forced passive cooling

[Matthew Garrett]

On Thu, Sep 10, 2009 at 06:07:12PM +0200, Frans Pop wrote:

> Have you had a chance to check this?

I think this was just an artifact of me modifying the existing logic 
rather than it being meaningful, so as long as this works for you:

Acked-by: Matthew Garrett <mjg@redhat.com>

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH 6/6] thermal: disable polling if passive_delay and polling_delay are both unset

[Frans Pop]

Otherwise polling will continue for the thermal zone even when
it is no longer needed, for example because forced passive cooling
was disabled.

Signed-off-by: Frans Pop <elendil@planet.nl>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Zhang Rui <rui.zhang@intel.com>

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index ceda0f1..a59685b 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -1012,6 +1012,8 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
 		thermal_zone_device_set_polling(tz, tz->passive_delay);
 	else if (tz->polling_delay)
 		thermal_zone_device_set_polling(tz, tz->polling_delay);
+	else
+		thermal_zone_device_set_polling(tz, 0);
 	mutex_unlock(&tz->lock);
 }
 EXPORT_SYMBOL(thermal_zone_device_update);

Re: [PATCH 6/6] thermal: disable polling if passive_delay and polling_delay are both unset

[Matthew Garrett]

On Wed, Aug 26, 2009 at 06:17:25PM +0200, Frans Pop wrote:
> Otherwise polling will continue for the thermal zone even when
> it is no longer needed, for example because forced passive cooling
> was disabled.
> 
> Signed-off-by: Frans Pop <elendil@planet.nl>
> Cc: Matthew Garrett <mjg@redhat.com>
> Cc: Zhang Rui <rui.zhang@intel.com>

Acked-by: Matthew Garrett <mjg@redhat.com>

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH 0/6] thermal: improvements re. forced passive cooling

[Frans Pop]

Hello Len,

Please consider this patch set [1] for 2.6.32.

All patches have now been acked by either Rui or Matthew, with the 
exception of:

1/6 thermal: sysfs-api.txt - reformat for improved readability
This is a trivial documentation patch.

4/6 thermal: add sanity check for the passive attribute
There was some discussion on this, but I feel that my version 2 of the 
patch is a reasonable compromise, in that it prevents obviously useless 
values (below 1C), but does not set an arbitrary lower limit. If you feel 
it needs more discussion, it could be dropped from the series for now.

If you'd like me to post the series again, then please let me know.

Thanks,
FJP

[1] http://www.spinics.net/lists/linux-acpi/msg24104.html