Thermal managment broken

Thermal managment broken

[Pavel Machek]

Hi!

Thermal managment forgets previous state, so it can never ever get to
slower state than T1.

Second patch allows user to set trip points. This is neccessary on XE3
-- BIOS claims passive cooling at 83C and critical at 100C. Yet it
powers down at ~85C. This allows user to override.

First is strict bugfix, please take. Second is needed, please take it,
too.

								Pavel 

--- clean/drivers/acpi/processor.c	2002-07-29 20:02:23.000000000 +0200
+++ linux/drivers/acpi/processor.c	2002-09-16 17:54:01.000000000 +0200
@@ -1468,6 +1468,9 @@
 	 * performance state.
 	 */
 
+	px = pr->limit.thermal.px;
+	tx = pr->limit.thermal.tx;
+
 	switch (type) {
 
 	case ACPI_PROCESSOR_LIMIT_NONE:
--- clean/drivers/acpi/thermal.c	2002-08-28 22:38:43.000000000 +0200
+++ linux/drivers/acpi/thermal.c	2002-09-16 17:43:37.000000000 +0200
@@ -60,6 +60,7 @@
 #define ACPI_THERMAL_MAX_ACTIVE	10
 
 #define KELVIN_TO_CELSIUS(t)	((t-2732+5)/10)
+#define CELSIUS_TO_KELVIN(t)	((t+273)*10)
 
 static int acpi_thermal_add (struct acpi_device *device);
 static int acpi_thermal_remove (struct acpi_device *device, int type);
@@ -854,6 +868,46 @@
 
 
 static int
+acpi_thermal_write_trip_points (
+        struct file		*file,
+        const char		*buffer,
+        unsigned long		count,
+        void			*data)
+{
+	struct acpi_thermal	*tz = (struct acpi_thermal *) data;
+	char			limit_string[25] = {'\0'};
+	int			critical, hot, passive, active0, active1;
+
+	ACPI_FUNCTION_TRACE("acpi_thermal_write_trip_points");
+
+	if (!tz || (count > sizeof(limit_string) - 1)) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid argument\n"));
+		return_VALUE(-EINVAL);
+	}
+	
+	if (copy_from_user(limit_string, buffer, count)) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid data\n"));
+		return_VALUE(-EFAULT);
+	}
+	
+	limit_string[count] = '\0';
+
+	if (sscanf(limit_string, "%d:%d:%d:%d:%d", &critical, &hot, &passive, &active0, &active1) != 5) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid data format\n"));
+		return_VALUE(-EINVAL);
+	}
+
+	tz->trips.critical.temperature = CELSIUS_TO_KELVIN(critical);
+	tz->trips.hot.temperature = CELSIUS_TO_KELVIN(hot);
+	tz->trips.passive.temperature = CELSIUS_TO_KELVIN(passive);
+	tz->trips.active[0].temperature = CELSIUS_TO_KELVIN(active0);
+	tz->trips.active[1].temperature = CELSIUS_TO_KELVIN(active1);
+	
+	return_VALUE(count);
+}
+
+
+static int
 acpi_thermal_read_cooling_mode (
 	char			*page,
 	char			**start,
@@ -1042,15 +1096,16 @@
 		entry->data = acpi_driver_data(device);
 	}
 
-	/* 'trip_points' [R] */
+	/* 'trip_points' [R/W] */
 	entry = create_proc_entry(ACPI_THERMAL_FILE_TRIP_POINTS,
-		S_IRUGO, acpi_device_dir(device));
+		S_IFREG|S_IRUGO|S_IWUSR, acpi_device_dir(device));
 	if (!entry)
 		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
 			"Unable to create '%s' fs entry\n",
 			ACPI_THERMAL_FILE_POLLING_FREQ));
 	else {
 		entry->read_proc = acpi_thermal_read_trip_points;
+		entry->write_proc = acpi_thermal_write_trip_points;
 		entry->data = acpi_driver_data(device);
 	}
 

-- 
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: Thermal managment broken

[Dominik Brodowski]

Hi Pavel, Andrew, list;

> First is strict bugfix, please take. Second is needed, please take it,
> too.
While I'd "vote" for the second patch "in general", there are two aspects
which should be mentioned first:


> +	tz->trips.critical.temperature = CELSIUS_TO_KELVIN(critical);
> +	tz->trips.hot.temperature = CELSIUS_TO_KELVIN(hot);
> +	tz->trips.passive.temperature = CELSIUS_TO_KELVIN(passive);
> +	tz->trips.active[0].temperature = CELSIUS_TO_KELVIN(active0);
> +	tz->trips.active[1].temperature = CELSIUS_TO_KELVIN(active1);

You explicitely want to support _two_ active trip points here. While this
seems to be the amount of active trip points seen on most systems, I'd
prefer you'd do a more flexible interface [zero to ACPI_THERMAL_MAX_ACTIVE
would be best]. 

Additionally, it might be better to specify a "starting" and
"stopping" trip point - so that you don't have a fan going
on-off-on-off-on-off repeatedly. (IIRC someone complained about this a few
months ago on this list). But even without this "feature", I'd like to see
the user-specified trip points in the acpi patch.

	Dominik




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: Thermal managment broken

[Pavel Machek]

Hi!

> > First is strict bugfix, please take. Second is needed, please take it,
> > too.
> While I'd "vote" for the second patch "in general", there are two aspects
> which should be mentioned first:
> 
> 
> > +	tz->trips.critical.temperature = CELSIUS_TO_KELVIN(critical);
> > +	tz->trips.hot.temperature = CELSIUS_TO_KELVIN(hot);
> > +	tz->trips.passive.temperature = CELSIUS_TO_KELVIN(passive);
> > +	tz->trips.active[0].temperature = CELSIUS_TO_KELVIN(active0);
> > +	tz->trips.active[1].temperature = CELSIUS_TO_KELVIN(active1);
> 
> You explicitely want to support _two_ active trip points here. While this
> seems to be the amount of active trip points seen on most systems, I'd
> prefer you'd do a more flexible interface [zero to ACPI_THERMAL_MAX_ACTIVE
> would be best]. 

Okay.

[Andrew, do you want to take this one and then followup which adds
many active points, or do you want whole new patch?]

> Additionally, it might be better to specify a "starting" and
> "stopping" trip point - so that you don't have a fan going
> on-off-on-off-on-off repeatedly. (IIRC someone complained about this a few
> months ago on this list). But even without this "feature", I'd like to see
> the user-specified trip points in the acpi patch.

I believe this is orthogonal. You need to do two thresholds even
without user help, so it needs to be solved some other way.

								Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.


-------------------------------------------------------
This SF.NET email is sponsored by: AMD - Your access to the experts
on Hammer Technology! Open Source & Linux Developers, register now
for the AMD Developer Symposium. Code: EX8664
http://www.developwithamd.com/developerlab

Re: Thermal managment broken

[Dominik Brodowski]

Hi Pavel,

On Wed, Sep 18, 2002 at 12:25:55PM +0200, Pavel Machek wrote:
> > Additionally, it might be better to specify a "starting" and
> > "stopping" trip point - so that you don't have a fan going
> > on-off-on-off-on-off repeatedly. (IIRC someone complained about this a few
> > months ago on this list). But even without this "feature", I'd like to see
> > the user-specified trip points in the acpi patch.
> 
> I believe this is orthogonal. You need to do two thresholds even
> without user help, so it needs to be solved some other way.

IIRC, that's something the DSDT has to provide - the trip points should
change (right in the DSDT) when the fan is turned on or off, respectively.
Many DSDTs don't do this, and this is an annoyance, but except some basic
"sanity checking" (for example, having a minimum fan-on period of x
seconds), such a policy interface should either provide such a two-threshold
policy itself (turn-off==user-value - 5°), or get direct user-input for it.

	Dominik


-------------------------------------------------------
This SF.NET email is sponsored by: AMD - Your access to the experts
on Hammer Technology! Open Source & Linux Developers, register now
for the AMD Developer Symposium. Code: EX8664
http://www.developwithamd.com/developerlab

RE: Thermal managment broken

[Herbert Nachtnebel]

> You explicitely want to support _two_ active trip points here. While this
> seems to be the amount of active trip points seen on most systems, I'd
> prefer you'd do a more flexible interface [zero to ACPI_THERMAL_MAX_ACTIVE
> would be best].
 
At least the compaq e500 has three trip points, one should be able to change each trip which is specified by the DSDT.
 
Greatings,
Herbert.


-------------------------------------------------------
Sponsored by: AMD - Your access to the experts on Hammer Technology!
Open Source & Linux Developers, register now for the AMD Developer
Symposium. Code: EX8664 http://www.developwithamd.com/developerlab