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

[Frans Pop <elendil@planet.nl>]

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