[PATCH] ACPI thermal: do not always return THERMAL_TREND_RAISING for active trip points

[PATCH] ACPI thermal: do not always return THERMAL_TREND_RAISING for active trip points

[Zhang Rui]

Commit 4ae46befb49d4173122e0afa995c4e93d01948a2
introduces a regression that the fan is always on
even if the system is in idle state.

My original idea in that commit is that:
when the current temperature is above the trip point,
keep the fan on, even if the temperature is dropping.
when the current temperature is below the trip point,
turn on the fan when the temperature is raising,
turn off the fan when the temperature is dropping.

But this is what the code actually does:
when the current temperature is above the trip point,
the fan keeps on.
when the current temperature is below the trip point,
the fan is always on because thermal_get_trend()
in driver/acpi/thermal.c returns THERMAL_TREND_RAISING.
Thus the fan keeps running even if the system is idle.

Fix this in drivers/acpi/thermal.c.

https://bugzilla.kernel.org/show_bug.cgi?id=56591
https://bugzilla.kernel.org/show_bug.cgi?id=56601
https://bugzilla.kernel.org/show_bug.cgi?id=50041#c45

Patch should be applied to 3.7 stable kernel and later.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Matthias <morpheusxyz123@yahoo.de>
Tested-by: Ville Syrjälä <syrjala@sci.fi>
Cc: <stable@vger.kernel.org> # v3.7+
---
 drivers/acpi/thermal.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 8470771..a33821c 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -723,9 +723,19 @@ static int thermal_get_trend(struct thermal_zone_device *thermal,
 		return -EINVAL;
 
 	if (type == THERMAL_TRIP_ACTIVE) {
-		/* aggressive active cooling */
-		*trend = THERMAL_TREND_RAISING;
-		return 0;
+		unsigned long trip_temp;
+		unsigned long temp = KELVIN_TO_MILLICELSIUS(tz->temperature,
+							tz->kelvin_offset);
+		if (thermal_get_trip_temp(thermal, trip, &trip_temp))
+			return -EINVAL;
+
+		if (temp > trip_temp) {
+			*trend = THERMAL_TREND_RAISING;
+			return 0;
+		} else {
+			/* Fall back on default trend */
+			return -EINVAL;
+		}
 	}
 
 	/*
-- 
1.7.9.5

Re: [PATCH] ACPI thermal: do not always return THERMAL_TREND_RAISING for active trip points

[Rafael J. Wysocki]

On Friday, April 26, 2013 05:19:53 PM Zhang Rui wrote:
> Commit 4ae46befb49d4173122e0afa995c4e93d01948a2
> introduces a regression that the fan is always on
> even if the system is in idle state.
> 
> My original idea in that commit is that:
> when the current temperature is above the trip point,
> keep the fan on, even if the temperature is dropping.
> when the current temperature is below the trip point,
> turn on the fan when the temperature is raising,
> turn off the fan when the temperature is dropping.
> 
> But this is what the code actually does:
> when the current temperature is above the trip point,
> the fan keeps on.
> when the current temperature is below the trip point,
> the fan is always on because thermal_get_trend()
> in driver/acpi/thermal.c returns THERMAL_TREND_RAISING.
> Thus the fan keeps running even if the system is idle.
> 
> Fix this in drivers/acpi/thermal.c.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=56591
> https://bugzilla.kernel.org/show_bug.cgi?id=56601
> https://bugzilla.kernel.org/show_bug.cgi?id=50041#c45
> 
> Patch should be applied to 3.7 stable kernel and later.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Tested-by: Matthias <morpheusxyz123@yahoo.de>
> Tested-by: Ville Syrjälä <syrjala@sci.fi>
> Cc: <stable@vger.kernel.org> # v3.7+

Applied.

Thanks,
Rafael


> ---
>  drivers/acpi/thermal.c |   16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 8470771..a33821c 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -723,9 +723,19 @@ static int thermal_get_trend(struct thermal_zone_device *thermal,
>  		return -EINVAL;
>  
>  	if (type == THERMAL_TRIP_ACTIVE) {
> -		/* aggressive active cooling */
> -		*trend = THERMAL_TREND_RAISING;
> -		return 0;
> +		unsigned long trip_temp;
> +		unsigned long temp = KELVIN_TO_MILLICELSIUS(tz->temperature,
> +							tz->kelvin_offset);
> +		if (thermal_get_trip_temp(thermal, trip, &trip_temp))
> +			return -EINVAL;
> +
> +		if (temp > trip_temp) {
> +			*trend = THERMAL_TREND_RAISING;
> +			return 0;
> +		} else {
> +			/* Fall back on default trend */
> +			return -EINVAL;
> +		}
>  	}
>  
>  	/*
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI thermal: do not always return THERMAL_TREND_RAISING for active trip points

[Pavel Machek]

Hi!

> Commit 4ae46befb49d4173122e0afa995c4e93d01948a2
> introduces a regression that the fan is always on
> even if the system is in idle state.
> 
> My original idea in that commit is that:
> when the current temperature is above the trip point,
> keep the fan on, even if the temperature is dropping.
> when the current temperature is below the trip point,
> turn on the fan when the temperature is raising,
> turn off the fan when the temperature is dropping.

Is that even right algoritm?

Assume I'm running at very cold room, lets say -10C. Assume idle CPU
will hover around 30C with no fan, or hover around 10C with fan
running, trip point being 50C.

You _could_ leave fan off until 50C, having silent, passively cooled
system.

What it will do instead is annoyingly pulse fan at 10C.

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

Re: [PATCH] ACPI thermal: do not always return THERMAL_TREND_RAISING for active trip points

[Zhang Rui]

On Sat, 2013-04-27 at 12:23 +0200, Pavel Machek wrote:
> Hi!
> 
> > Commit 4ae46befb49d4173122e0afa995c4e93d01948a2
> > introduces a regression that the fan is always on
> > even if the system is in idle state.
> > 
> > My original idea in that commit is that:
> > when the current temperature is above the trip point,
> > keep the fan on, even if the temperature is dropping.
> > when the current temperature is below the trip point,
> > turn on the fan when the temperature is raising,
> > turn off the fan when the temperature is dropping.
> 
> Is that even right algoritm?
> 
sorry the changelog is not clear enough here.
I should say,
"keep the fan on when the temperature is raising"
"turn off the fan when the temperature is dropping"

> Assume I'm running at very cold room, lets say -10C. Assume idle CPU
> will hover around 30C with no fan, or hover around 10C with fan
> running, trip point being 50C.
> 
> You _could_ leave fan off until 50C, having silent, passively cooled
> system.
> 
> What it will do instead is annoyingly pulse fan at 10C.
> 
First, fan will never spins on if the temperature never hits 50C.

let's see how ACPI 3.0 fan (just have on and off state) is supposed to
work after overheat.
1. temperature goes above 50C
2. Fan starts to spin.
3. a) if temperature goes higher (if possible), fan keeps on.
   b) if temperature drops, but still above 50C. the fan keeps on.
   c) if temperature drops below 50C, the last temperature is above 50C,
thus the fan is turned off.
   d) if the temperature goes higher again, the fan will not spin on
until the temperature is higher than the trip point (50C) again.

is this behavior what you want?

Thanks,
rui

Re: [PATCH] ACPI thermal: do not always return THERMAL_TREND_RAISING for active trip points

[Pavel Machek]

Hi!

> > Is that even right algoritm?
> > 
> sorry the changelog is not clear enough here.
> I should say,
> "keep the fan on when the temperature is raising"
> "turn off the fan when the temperature is dropping"
> 
> > Assume I'm running at very cold room, lets say -10C. Assume idle CPU
> > will hover around 30C with no fan, or hover around 10C with fan
> > running, trip point being 50C.
> > 
> > You _could_ leave fan off until 50C, having silent, passively cooled
> > system.
> > 
> > What it will do instead is annoyingly pulse fan at 10C.
> > 
> First, fan will never spins on if the temperature never hits 50C.
> 
> let's see how ACPI 3.0 fan (just have on and off state) is supposed to
> work after overheat.
> 1. temperature goes above 50C
> 2. Fan starts to spin.
> 3. a) if temperature goes higher (if possible), fan keeps on.
>    b) if temperature drops, but still above 50C. the fan keeps on.
>    c) if temperature drops below 50C, the last temperature is above 50C,
> thus the fan is turned off.
>    d) if the temperature goes higher again, the fan will not spin on
> until the temperature is higher than the trip point (50C) again.

So the fan is spinning iff the temperature is over threshold, right?

> is this behavior what you want?

That's better, yes. Even better would be if it had some hysteresis:
turn the fan on at threshold temperature, but turn it off only if it
drops below temperature-5C, or if it is below threshold for 1 minute.

Thanks,
      	    		       	     	      		      Pavel

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