thermal governor: does it actually work??

thermal governor: does it actually work??

[Andreas Mohr]

For me after having loaded acerhdf the fan never stops (with kernelmode
active), despite staying safely below trip point
(acerhdf_set_cur_state() actually never gets called).
And AFAIR in a 3.2.0 kernel acerhdf fan operation seemed to just work
(i.e., no fan for low temps, from the beginning).
Needless to say 3.2.0 didn't even feature all the modern thermal
governor crapyard yet ;)
(ok, well, it's more complex but it's also a very nice environment capability)

3.8-rc7:
CONFIG_ACPI_THERMAL=m
CONFIG_THERMAL=m
CONFIG_THERMAL_HWMON=y
CONFIG_THERMAL_DEFAULT_GOV_STEP_WISE=y
# CONFIG_THERMAL_DEFAULT_GOV_FAIR_SHARE is not set
# CONFIG_THERMAL_DEFAULT_GOV_USER_SPACE is not set
CONFIG_FAIR_SHARE=y
CONFIG_STEP_WISE=y
# CONFIG_USER_SPACE is not set
# CONFIG_CPU_THERMAL is not set



Terminology in this area seems to be quite a bit off, too, at several
docs places, at least according to my understanding:

e.g. drivers/thermal/step_wise.c has the following comment:

/**
 * step_wise_throttle - throttles devices associated with the given zone
 * @tz - thermal_zone_device
 * @trip - the trip point
 * @trip_type - type of the trip point
 *
 * Throttling Logic: This uses the trend of the thermal zone to
 * throttle.
 * If the thermal zone is 'heating up' this throttles all the cooling
 * devices associated with the zone and its particular trip point, by
 * one
 * step. If the zone is 'cooling down' it brings back the performance of
 * the devices by one step.



if ... heating up ... throttles ...
Sorry, but at least for P4 clockmod stuff (or some such), throttle
states (P1...P8 IIRC) meant that the CPU operation was *reduced*,
i.e. with pause intervals.
And the translation of throttle clearly says that it does go that way
and not the other way...
(yes, you managed to confuse me that much that I even had to look up
things to verify)

... cooling down ... brings back ...
This should certainly be worded "reduces" or some such.

So, any idea why I'm missing callbacks in acerhdf (if that is what I'm
supposed to expect to happen)?
Kernel bug, .config mistake, missing/wrong user-side setup?

Needless to say if kernel bug this ought to be fixed pre-3.8 ideally.

Thanks,

Andreas Mohr

RE: thermal governor: does it actually work??

[Zhang, Rui]

> -----Original Message-----
> From: Andreas Mohr [mailto:andi@lisas.de]
> Sent: Thursday, February 14, 2013 11:33 PM
> To: linux-kernel@vger.kernel.org
> Cc: R, Durgadoss; Zhang, Rui; peter@piie.net
> Subject: thermal governor: does it actually work??
> Importance: High
> 
> For me after having loaded acerhdf the fan never stops (with kernelmode
> active), despite staying safely below trip point
> (acerhdf_set_cur_state() actually never gets called).

Please attach the output of
"grep . /sys/class/thermal/thermal_zone*/cdev*/*"?

> And AFAIR in a 3.2.0 kernel acerhdf fan operation seemed to just work
> (i.e., no fan for low temps, from the beginning).
> Needless to say 3.2.0 didn't even feature all the modern thermal
> governor crapyard yet ;) (ok, well, it's more complex but it's also a
> very nice environment capability)
> 
> 3.8-rc7:
> CONFIG_ACPI_THERMAL=m
> CONFIG_THERMAL=m
> CONFIG_THERMAL_HWMON=y
> CONFIG_THERMAL_DEFAULT_GOV_STEP_WISE=y
> # CONFIG_THERMAL_DEFAULT_GOV_FAIR_SHARE is not set #
> CONFIG_THERMAL_DEFAULT_GOV_USER_SPACE is not set CONFIG_FAIR_SHARE=y
> CONFIG_STEP_WISE=y # CONFIG_USER_SPACE is not set # CONFIG_CPU_THERMAL
> is not set
> 
> 
> 
> Terminology in this area seems to be quite a bit off, too, at several
> docs places, at least according to my understanding:
> 
> e.g. drivers/thermal/step_wise.c has the following comment:
> 
> /**
>  * step_wise_throttle - throttles devices associated with the given
> zone
>  * @tz - thermal_zone_device
>  * @trip - the trip point
>  * @trip_type - type of the trip point
>  *
>  * Throttling Logic: This uses the trend of the thermal zone to
>  * throttle.
>  * If the thermal zone is 'heating up' this throttles all the cooling
>  * devices associated with the zone and its particular trip point, by
>  * one
>  * step. If the zone is 'cooling down' it brings back the performance
> of
>  * the devices by one step.
> 
> 
> 
> if ... heating up ... throttles ...
> Sorry, but at least for P4 clockmod stuff (or some such), throttle
> states (P1...P8 IIRC) meant that the CPU operation was *reduced*, i.e.
> with pause intervals.

For processors, surely you are right.

> And the translation of throttle clearly says that it does go that way
> and not the other way...
> (yes, you managed to confuse me that much that I even had to look up
> things to verify)
> 

The question is that if we can also call it "throttle" when reducing the device performance to generate less heat.

I do not have a clear answer for this as I'm not a native English speaker.
And what you're saying here may be right.
Surely I can generate a patch to rename it if throttle can't be used in that way.

Len,
What's your idea on this?

Thanks,
rui
> ... cooling down ... brings back ...
> This should certainly be worded "reduces" or some such.
> 
> So, any idea why I'm missing callbacks in acerhdf (if that is what I'm
> supposed to expect to happen)?
> Kernel bug, .config mistake, missing/wrong user-side setup?
> 
> Needless to say if kernel bug this ought to be fixed pre-3.8 ideally.
> 
> Thanks,
> 
> Andreas Mohr

Re: thermal governor: does it actually work??

[Andreas Mohr]

Hi,

On Fri, Feb 15, 2013 at 09:47:07AM +0000, Zhang, Rui wrote:
> Please attach the output of
> "grep . /sys/class/thermal/thermal_zone*/cdev*/*"?

# grep . /sys/class/thermal/thermal_zone*/cdev*/*
/sys/class/thermal/thermal_zone0/cdev0/cur_state:1
/sys/class/thermal/thermal_zone0/cdev0/max_state:1
grep: /sys/class/thermal/thermal_zone0/cdev0/power: Is a directory
grep: /sys/class/thermal/thermal_zone0/cdev0/subsystem: Is a directory
/sys/class/thermal/thermal_zone0/cdev0/type:acerhdf-fan

> The question is that if we can also call it "throttle" when reducing the device performance to generate less heat.

I won't continue to elaborate on this separate issue now, given that my time
currently is very limited ;)

Andreas Mohr

Re: thermal governor: does it actually work??

[Alexander Lam]

I wrote a patch to fix this:

http://lkml.org/lkml/2012/12/30/47

But nobody picked it up and since then I have been too busy to respin
the patch for new -rc kernels.

On Fri, Feb 15, 2013 at 10:49 AM, Andreas Mohr <andi@lisas.de> wrote:
>
> Hi,
>
> On Fri, Feb 15, 2013 at 09:47:07AM +0000, Zhang, Rui wrote:
> > Please attach the output of
> > "grep . /sys/class/thermal/thermal_zone*/cdev*/*"?
>
> # grep . /sys/class/thermal/thermal_zone*/cdev*/*
> /sys/class/thermal/thermal_zone0/cdev0/cur_state:1
> /sys/class/thermal/thermal_zone0/cdev0/max_state:1
> grep: /sys/class/thermal/thermal_zone0/cdev0/power: Is a directory
> grep: /sys/class/thermal/thermal_zone0/cdev0/subsystem: Is a directory
> /sys/class/thermal/thermal_zone0/cdev0/type:acerhdf-fan
>
> > The question is that if we can also call it "throttle" when reducing the
> > device performance to generate less heat.
>
> I won't continue to elaborate on this separate issue now, given that my
> time
> currently is very limited ;)
>
> Andreas Mohr
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/




--
Alexander Lam

Re: thermal governor: does it actually work??

[Borislav Petkov]

On Sat, Feb 16, 2013 at 04:08:11PM -0500, Alexander Lam wrote:
> I wrote a patch to fix this:
> 
> http://lkml.org/lkml/2012/12/30/47
> 
> But nobody picked it up and since then I have been too busy to respin
> the patch for new -rc kernels.

Hmm,

that's definitely worth a try - we've been discussing maybe the same
issue with acerhdf recently. Peter, Andreas?

I'll try to test it soon too.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: thermal governor: does it actually work??

[Peter Feuerer]

Hi Boris, Alex, Andreas,

what do you think about this acerhdf patch?
I think it makes things straight and implements the two-point regulation of 
acerhdf to be for correctly handled by the thermal layer:


>From 7b39bd8837de6dc5658ac3e54ac5d4df9d351528 Mon Sep 17 00:00:00 2001
From: Peter Feuerer <peter@piie.net>
Date: Sun, 17 Feb 2013 03:29:19 +0100
Subject: [PATCH] added two more trip points to acerhdf, this allows thermal
 layer to correctly handle the two point regulation of acerhdf. Trip point 0
 will be active from 0 degree to "fanoff" and is marked as passive, then trip
 point 1 is valid from "fanoff" to "fanon" value and is marked as active,
 even if it's only really active in case temperature is going down from trip
 point 2. Trip point 2 will be valid above "fanon" value and is also marked
 as active.

---
 drivers/platform/x86/acerhdf.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index f94467c..c36633b 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -400,6 +400,10 @@ static int acerhdf_get_trip_type(struct thermal_zone_device *thermal, int trip,
 				 enum thermal_trip_type *type)
 {
 	if (trip == 0)
+		*type = THERMAL_TRIP_PASSIVE;
+	if (trip == 1)
+		*type = THERMAL_TRIP_ACTIVE;
+	if (trip == 2)
 		*type = THERMAL_TRIP_ACTIVE;
 
 	return 0;
@@ -409,6 +413,10 @@ static int acerhdf_get_trip_temp(struct thermal_zone_device *thermal, int trip,
 				 unsigned long *temp)
 {
 	if (trip == 0)
+		*temp = 0;
+	if (trip == 1)
+		*temp = fanoff;
+	if (trip == 2)
 		*temp = fanon;
 
 	return 0;
@@ -486,7 +494,8 @@ static int acerhdf_set_cur_state(struct thermal_cooling_device *cdev,
 		    (cur_temp < fanoff))
 			acerhdf_change_fanstate(ACERHDF_FAN_OFF);
 	} else {
-		if (cur_state == ACERHDF_FAN_OFF)
+		if ((cur_state == ACERHDF_FAN_OFF) &&
+		    (cur_temp > fanon))
 			acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
 	}
 	return 0;
@@ -661,8 +670,8 @@ static int acerhdf_register_thermal(void)
 	if (IS_ERR(cl_dev))
 		return -EINVAL;
 
-	thz_dev = thermal_zone_device_register("acerhdf", 1, 0, NULL,
-					      &acerhdf_dev_ops, NULL, 0,
+	thz_dev = thermal_zone_device_register("acerhdf", 3, 0, NULL,
+					      &acerhdf_dev_ops, (kernelmode) ? interval*1000 : 0,
 					      (kernelmode) ? interval*1000 : 0);
 	if (IS_ERR(thz_dev))
 		return -EINVAL;
-- 
1.8.1.3

Re: thermal governor: does it actually work??

[Borislav Petkov]

On Sun, Feb 17, 2013 at 03:43:13AM +0100, Peter Feuerer wrote:
> Hi Boris, Alex, Andreas,
> 
> what do you think about this acerhdf patch?
> I think it makes things straight and implements the two-point regulation of 
> acerhdf to be for correctly handled by the thermal layer:
> 
> 
> From 7b39bd8837de6dc5658ac3e54ac5d4df9d351528 Mon Sep 17 00:00:00 2001
> From: Peter Feuerer <peter@piie.net>
> Date: Sun, 17 Feb 2013 03:29:19 +0100
> Subject: [PATCH] added two more trip points to acerhdf, this allows thermal
>  layer to correctly handle the two point regulation of acerhdf. Trip point 0
>  will be active from 0 degree to "fanoff" and is marked as passive, then trip
>  point 1 is valid from "fanoff" to "fanon" value and is marked as active,
>  even if it's only really active in case temperature is going down from trip
>  point 2. Trip point 2 will be valid above "fanon" value and is also marked
>  as active.

Right, so this is clearly something new in the thermal pile of code. I
still don't understand the big picture all that clearly but whatever...

> ---
>  drivers/platform/x86/acerhdf.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index f94467c..c36633b 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -400,6 +400,10 @@ static int acerhdf_get_trip_type(struct thermal_zone_device *thermal, int trip,
>  				 enum thermal_trip_type *type)
>  {
>  	if (trip == 0)
> +		*type = THERMAL_TRIP_PASSIVE;
> +	if (trip == 1)
> +		*type = THERMAL_TRIP_ACTIVE;
> +	if (trip == 2)
>  		*type = THERMAL_TRIP_ACTIVE;

So, digging deep into thermal_sys.c, those naked numbers which we get
handed down for 'trip' are some sort of trip points. Now, I'd very much
like to know what those are and there are no defines what they mean -
code simply iterates over a number of thermal_zone trips - tz->trips -
and we (try to) act accordingly.

Now this is very fragile, IMO.

/me stares at the code a bit more.

Ok, from the looks of it, I'm guessing each driver has to do its own
mapping of what each trip point is, IIUC. And the thermal_zone doodles
over those and for those which the driver has defined, it asks the
driver itself what it wants done (i.e. ->get_trip_temp) and, in our case
it doesn't do anything...

Also, come to think of it, why don't we have THERMAL_TRIP_CRITICAL and
THERMAL_TRIP_HOT trip types?

>  	return 0;
> @@ -409,6 +413,10 @@ static int acerhdf_get_trip_temp(struct thermal_zone_device *thermal, int trip,
>  				 unsigned long *temp)
>  {
>  	if (trip == 0)
> +		*temp = 0;
> +	if (trip == 1)
> +		*temp = fanoff;
> +	if (trip == 2)
>  		*temp = fanon;

Maybe the critical and hot types need to go here? I.e., 3 and 4?

>  	return 0;
> @@ -486,7 +494,8 @@ static int acerhdf_set_cur_state(struct thermal_cooling_device *cdev,
>  		    (cur_temp < fanoff))
>  			acerhdf_change_fanstate(ACERHDF_FAN_OFF);
>  	} else {
> -		if (cur_state == ACERHDF_FAN_OFF)
> +		if ((cur_state == ACERHDF_FAN_OFF) &&
> +		    (cur_temp > fanon))
>  			acerhdf_change_fanstate(ACERHDF_FAN_AUTO);

... and we hook in into the thermal_cdev_update() call here and do the
correction ourselves.

Oh well. I need to befriend myself with the whole concept of thermal
- still have a bad feeling about it, like a star wars character:
http://www.youtube.com/watch?v=sBknAcTaMiI :-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: thermal governor: does it actually work??

[Peter Feuerer]

Hi,

Borislav Petkov writes:
> On Sun, Feb 17, 2013 at 03:43:13AM +0100, Peter Feuerer wrote:
>> From 7b39bd8837de6dc5658ac3e54ac5d4df9d351528 Mon Sep 17 00:00:00 2001
>> From: Peter Feuerer <peter@piie.net>
>> Date: Sun, 17 Feb 2013 03:29:19 +0100
>> Subject: [PATCH] added two more trip points to acerhdf, this allows thermal
>>  layer to correctly handle the two point regulation of acerhdf. Trip point 0
>>  will be active from 0 degree to "fanoff" and is marked as passive, then trip
>>  point 1 is valid from "fanoff" to "fanon" value and is marked as active,
>>  even if it's only really active in case temperature is going down from trip
>>  point 2. Trip point 2 will be valid above "fanon" value and is also marked
>>  as active.
> 
> Right, so this is clearly something new in the thermal pile of code. I
> still don't understand the big picture all that clearly but whatever...

Don't think so, I think this was already in since 2.6.<something> and I 
assume with this patch applied, acerhdf works from at least this 
2.6.<something> up to new 3.8 and will still work in the future.


>> ---
>>  drivers/platform/x86/acerhdf.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
>> index f94467c..c36633b 100644
>> --- a/drivers/platform/x86/acerhdf.c
>> +++ b/drivers/platform/x86/acerhdf.c
>> @@ -400,6 +400,10 @@ static int acerhdf_get_trip_type(struct thermal_zone_device *thermal, int trip,
>>  				 enum thermal_trip_type *type)
>>  {
>>  	if (trip == 0)
>> +		*type = THERMAL_TRIP_PASSIVE;
>> +	if (trip == 1)
>> +		*type = THERMAL_TRIP_ACTIVE;
>> +	if (trip == 2)
>>  		*type = THERMAL_TRIP_ACTIVE;
> 
> So, digging deep into thermal_sys.c, those naked numbers which we get
> handed down for 'trip' are some sort of trip points. Now, I'd very much
> like to know what those are and there are no defines what they mean -
> code simply iterates over a number of thermal_zone trips - tz->trips -
> and we (try to) act accordingly.

As far as I understand the code (and Documentation/thermal/cpu-cooling-api.txt),
the thermal api finds the appropriate trip point and then set's the fan to 
the corresponding state, defined by the thermal/fan driver. This is nice 
thing, if you can completely control the speed of the fan, because you have 
then a good fan speed to temperature regulation. But we do only have a two 
point regulation (on and off), that's why we have to handle our thresholds 
within the trip=1 on our own to not get an all the time on-off-toggling of 
the fan.


> Now this is very fragile, IMO.

I think this is how the developer of thermal_sys intended drivers to work. 
But he forgot about two-point regulators (most probably because there's no 
one besides acerhdf)


> /me stares at the code a bit more.
> 
> Ok, from the looks of it, I'm guessing each driver has to do its own
> mapping of what each trip point is, IIUC. And the thermal_zone doodles
> over those and for those which the driver has defined, it asks the
> driver itself what it wants done (i.e. ->get_trip_temp) and, in our case
> it doesn't do anything...

I don't understand what you mean by "in our case it doesn't do anything", 
acerhdf is reporting the trip temperatures correctly, when get_trip_temp is 
called.



> Also, come to think of it, why don't we have THERMAL_TRIP_CRITICAL and
> THERMAL_TRIP_HOT trip types?

You are right, we should at least add the THERMAL_TRIP_CRITICAL, so that we 
handle this, but I think we can ignore THERMAL_TRIP_HOT, as it is not really 
serving anything of value in our case.


> 
>>  	return 0;
>> @@ -409,6 +413,10 @@ static int acerhdf_get_trip_temp(struct thermal_zone_device *thermal, int trip,
>>  				 unsigned long *temp)
>>  {
>>  	if (trip == 0)
>> +		*temp = 0;
>> +	if (trip == 1)
>> +		*temp = fanoff;
>> +	if (trip == 2)
>>  		*temp = fanon;
> 
> Maybe the critical and hot types need to go here? I.e., 3 and 4?

Yes, crit has to go there.


> 
>>  	return 0;
>> @@ -486,7 +494,8 @@ static int acerhdf_set_cur_state(struct thermal_cooling_device *cdev,
>>  		    (cur_temp < fanoff))
>>  			acerhdf_change_fanstate(ACERHDF_FAN_OFF);
>>  	} else {
>> -		if (cur_state == ACERHDF_FAN_OFF)
>> +		if ((cur_state == ACERHDF_FAN_OFF) &&
>> +		    (cur_temp > fanon))
>>  			acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
> 
> ... and we hook in into the thermal_cdev_update() call here and do the
> correction ourselves.

As I wrote above, the thermal_sys layer do not serve 2 point regulation per 
se, but with this check, we are able to achieve it. - We've done this check 
already partly before:
/* turn fan off only if below fanoff temperature */
if ((cur_state == ACERHDF_FAN_AUTO) &&        
     (cur_temp < fanoff))                         
acerhdf_change_fanstate(ACERHDF_FAN_OFF);


> Oh well. I need to befriend myself with the whole concept of thermal
> - still have a bad feeling about it, like a star wars character:
> http://www.youtube.com/watch?v=sBknAcTaMiI :-)

I still think it is the right way to go, but maybe we should ask Durgadoss 
R <durgadoss.r () intel.com>. It seems like he took over the thermal 
handling by this commit:

commit 0c01ebbfd3caf1dc132e0d93c8e7e9f742839d94
Author: Durgadoss R <durgadoss.r()intel.com>
Date:   Tue Sep 18 11:05:04 2012 +0530

    Thermal: Remove throttling logic out of thermal_sys.c


have a nice sunday evening,
--peter;

Re: thermal governor: does it actually work??

[Borislav Petkov]

On Sun, Feb 17, 2013 at 04:41:57PM +0100, Peter Feuerer wrote:
> Don't think so, I think this was already in since 2.6.<something> and
> I assume with this patch applied, acerhdf works from at least this
> 2.6.<something> up to new 3.8 and will still work in the future.

Well, it can't be because there wouldn't be breakage otherwise, right?
I've been running 3.5 on the atom for a long time and it was ok but 3.8
is showing the issue. So something *has* changed.

> As far as I understand the code (and
> Documentation/thermal/cpu-cooling-api.txt), the thermal api finds the
> appropriate trip point and then set's the fan to the corresponding
> state, defined by the thermal/fan driver.

Right, but the low level driver defines what to do at each trip point.

> This is nice thing, if you can completely control the speed of the
> fan, because you have then a good fan speed to temperature
> regulation. But we do only have a two point regulation (on and off),
> that's why we have to handle our thresholds within the trip=1 on our
> own to not get an all the time on-off-toggling of the fan.

Ok.

> I think this is how the developer of thermal_sys intended drivers to
> work. But he forgot about two-point regulators (most probably because
> there's no one besides acerhdf)

Fair enough.

> I don't understand what you mean by "in our case it doesn't do
> anything", acerhdf is reporting the trip temperatures correctly, when
> get_trip_temp is called.

Well, we have the thermal_sys.c deal which is at the top-level, then
there's the governor, i.e. step_wise.c in our case. The actual thing
doing the work is ->set_cur_state in acerhdf.c And you've added logic
there too:

> >>+           if ((cur_state == ACERHDF_FAN_OFF) &&
> >>+               (cur_temp > fanon))
> >>                    acerhdf_change_fanstate(ACERHDF_FAN_AUTO);

which I was expecting the thermal layer to do for us and acerhdf to do
only the switching like the cpufreq drivers to, for example. Btw, if we
did this, acerhdf would be even simpler.

But, as you say above, I guess the two-point scheme of acerhdf doesn't
have a governor that fits. So maybe the correct solution is to have an
"on_off" stupid governor which gets used by acerhdf and does simply call
into acerhdf to switch on the fan to auto above a specified trip point.

It could be a lot of overhead for nothing in the end, though.

> You are right, we should at least add the THERMAL_TRIP_CRITICAL, so
> that we handle this, but I think we can ignore THERMAL_TRIP_HOT, as
> it is not really serving anything of value in our case.
> 
> 
> >
> >> 	return 0;
> >>@@ -409,6 +413,10 @@ static int acerhdf_get_trip_temp(struct thermal_zone_device *thermal, int trip,
> >> 				 unsigned long *temp)
> >> {
> >> 	if (trip == 0)
> >>+		*temp = 0;
> >>+	if (trip == 1)
> >>+		*temp = fanoff;
> >>+	if (trip == 2)
> >> 		*temp = fanon;
> >
> >Maybe the critical and hot types need to go here? I.e., 3 and 4?
> 
> Yes, crit has to go there.

Right, I'll wait for that version of the patch to test. :)

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: thermal governor: does it actually work??

[Peter Feuerer]

Hi Boris, Hi Rui,

@Rui, would be great, if you could review the patch and verify, whether
everything I'm telling is the truth, thanks.


Borislav Petkov writes:

> On Sun, Feb 17, 2013 at 04:41:57PM +0100, Peter Feuerer wrote:
>> Don't think so, I think this was already in since 2.6.<something> and
>> I assume with this patch applied, acerhdf works from at least this
>> 2.6.<something> up to new 3.8 and will still work in the future.
> 
> Well, it can't be because there wouldn't be breakage otherwise, right?
> I've been running 3.5 on the atom for a long time and it was ok but 3.8
> is showing the issue. So something *has* changed.

I think previous 3.7 were two methods possible, the method with one trippoint,
on which the state gets switched on and off using this code in thermal_sys.c:
1060                                if (temp >= trip_temp)
1061                                        cdev->ops->set_cur_state(cdev, 1);
1062                                else
1063                                        cdev->ops->set_cur_state(cdev, 0);


And the other method with all the different trip points, which is used after 
applying my patch.
The older way has been removed intentionally or not, I don't know. But as
acerhdf was depending on this old way, it stopped working.


> But, as you say above, I guess the two-point scheme of acerhdf doesn't
> have a governor that fits. So maybe the correct solution is to have an
> "on_off" stupid governor which gets used by acerhdf and does simply call
> into acerhdf to switch on the fan to auto above a specified trip point.
> 
> It could be a lot of overhead for nothing in the end, though.

I think adding a two-point governor would maybe be the somehow cleaner way...
Rui, what do you think, is there a way, you could provide a two-point governor
with upper temperature for turning on the fan and a lower temperature for
turning it off again?


>> >Maybe the critical and hot types need to go here? I.e., 3 and 4?
>> 
>> Yes, crit has to go there.
> 
> Right, I'll wait for that version of the patch to test. :)

I added crit to the patch below.



>From 74af34f3099828d5c7c2b4fd4ccf445edfdc6f1e Mon Sep 17 00:00:00 2001
From: Peter Feuerer <peter@piie.net>
Date: Mon, 18 Feb 2013 17:23:34 +0100
Subject: [PATCH] added three more trip points to acerhdf, this allows thermal
 layer to correctly handle the two point regulation of acerhdf. Trip point 0
 will be active from 0 degree to "fanoff" and is marked as passive, then trip
 point 1 is valid from "fanoff" to "fanon" value and is marked as active, even
 if it's only really active in case temperature is going down from trip point
 2. Trip point 2 will be valid above "fanon" value and is also marked as
 active. Trip point 3 is when hitting the critical temperature.

---
 drivers/platform/x86/acerhdf.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index f94467c..b121449 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -400,7 +400,13 @@ static int acerhdf_get_trip_type(struct thermal_zone_device *thermal, int trip,
 				 enum thermal_trip_type *type)
 {
 	if (trip == 0)
+		*type = THERMAL_TRIP_PASSIVE;
+	if (trip == 1)
 		*type = THERMAL_TRIP_ACTIVE;
+	if (trip == 2)
+		*type = THERMAL_TRIP_ACTIVE;
+	if (trip == 3)
+		*type = THERMAL_TRIP_CRITICAL;
 
 	return 0;
 }
@@ -409,7 +415,13 @@ static int acerhdf_get_trip_temp(struct thermal_zone_device *thermal, int trip,
 				 unsigned long *temp)
 {
 	if (trip == 0)
+		*temp = 0;
+	if (trip == 1)
+		*temp = fanoff;
+	if (trip == 2)
 		*temp = fanon;
+	if (trip == 3)
+		*temp = ACERHDF_TEMP_CRIT;
 
 	return 0;
 }
@@ -431,6 +443,7 @@ static struct thermal_zone_device_ops acerhdf_dev_ops = {
 	.get_trip_type = acerhdf_get_trip_type,
 	.get_trip_temp = acerhdf_get_trip_temp,
 	.get_crit_temp = acerhdf_get_crit_temp,
+	.notify = NULL,
 };
 
 
@@ -486,7 +499,8 @@ static int acerhdf_set_cur_state(struct thermal_cooling_device *cdev,
 		    (cur_temp < fanoff))
 			acerhdf_change_fanstate(ACERHDF_FAN_OFF);
 	} else {
-		if (cur_state == ACERHDF_FAN_OFF)
+		if ((cur_state == ACERHDF_FAN_OFF) &&
+		    (cur_temp > fanon))
 			acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
 	}
 	return 0;
@@ -661,8 +675,9 @@ static int acerhdf_register_thermal(void)
 	if (IS_ERR(cl_dev))
 		return -EINVAL;
 
-	thz_dev = thermal_zone_device_register("acerhdf", 1, 0, NULL,
-					      &acerhdf_dev_ops, NULL, 0,
+	thz_dev = thermal_zone_device_register("acerhdf", 4, 0, NULL,
+					      &acerhdf_dev_ops,
+					      (kernelmode) ? interval*1000 : 0,
 					      (kernelmode) ? interval*1000 : 0);
 	if (IS_ERR(thz_dev))
 		return -EINVAL;
-- 
1.8.1.3

Re: thermal governor: does it actually work??

[Zhang Rui]

Hi, Peter,

On Mon, 2013-02-18 at 17:47 +0100, Peter Feuerer wrote:
> Hi Boris, Hi Rui,
> 
> @Rui, would be great, if you could review the patch and verify, whether
> everything I'm telling is the truth, thanks.
> 
sure.
But first of all, I'd like to see what the problem is.

>From my understanding, this thread is not the fix for the "echo 0
> /sys/class/thermal/cooling_deviceX(acerhdf fan)/cur_state" not working
problem. right?

> 
> Borislav Petkov writes:
> 
> > On Sun, Feb 17, 2013 at 04:41:57PM +0100, Peter Feuerer wrote:
> >> Don't think so, I think this was already in since 2.6.<something> and
> >> I assume with this patch applied, acerhdf works from at least this
> >> 2.6.<something> up to new 3.8 and will still work in the future.
> > 
> > Well, it can't be because there wouldn't be breakage otherwise, right?
> > I've been running 3.5 on the atom for a long time and it was ok but 3.8
> > is showing the issue.

I'd like to know what the issue is.
is it the problem that fan always spinning even when idle?
If yes, I think the patch at http://lkml.org/lkml/2012/12/30/47 is the
right fix.

>  So something *has* changed.
> 
right, thermal governors are introduced. and the governor throttle
algorithm may have some issues.

> I think previous 3.7 were two methods possible, the method with one trippoint,
> on which the state gets switched on and off using this code in thermal_sys.c:
> 1060                                if (temp >= trip_temp)
> 1061                                        cdev->ops->set_cur_state(cdev, 1);
> 1062                                else
> 1063                                        cdev->ops->set_cur_state(cdev, 0);
> 
No, I think this is the code in 3.6.
It was changed in 3.7 and the new code was moved to thermal governors
instead of thermal_sys.c in 3.8.

> 
> And the other method with all the different trip points, which is used after 
> applying my patch.



> The older way has been removed intentionally or not, I don't know. But as
> acerhdf was depending on this old way, it stopped working.
> 
well, in theory, the code change in driver/thermal should be transparent
to acerhdf driver.

> 
> > But, as you say above, I guess the two-point scheme of acerhdf doesn't
> > have a governor that fits. So maybe the correct solution is to have an
> > "on_off" stupid governor which gets used by acerhdf and does simply call
> > into acerhdf to switch on the fan to auto above a specified trip point.
> > 
> > It could be a lot of overhead for nothing in the end, though.
> 
> I think adding a two-point governor would maybe be the somehow cleaner way...
> Rui, what do you think, is there a way, you could provide a two-point governor
> with upper temperature for turning on the fan and a lower temperature for
> turning it off again?
> 
Okay, here I see the problem, say you want to turn on the fan at 60C and
turn it off at 55C, is this what you want?
hmmm, is it possible to do some tricks in acerhdf driver?
say,
only one active trip point as before. but change acerhdf_get_trip_temp
to:
acerhdf_get_trip_temp()
{
	if (fan is on)
		return 55;
	else
		return 60;
}

In this way, when the fan is off and the temperature is raising, the fan
will be turned on at 60C because we have an active trip point of 60C.
And when the fan is turned on and the temperature starts to drop, the
fan will be turned off at 55C.

> 
> >> >Maybe the critical and hot types need to go here? I.e., 3 and 4?
> >> 
> >> Yes, crit has to go there.
> > 
> > Right, I'll wait for that version of the patch to test. :)
> 
> I added crit to the patch below.
> 
surely you can introduce a critical trip point.
> 
> 
> From 74af34f3099828d5c7c2b4fd4ccf445edfdc6f1e Mon Sep 17 00:00:00 2001
> From: Peter Feuerer <peter@piie.net>
> Date: Mon, 18 Feb 2013 17:23:34 +0100
> Subject: [PATCH] added three more trip points to acerhdf, this allows thermal
>  layer to correctly handle the two point regulation of acerhdf. Trip point 0
>  will be active from 0 degree to "fanoff" and is marked as passive, then trip
>  point 1 is valid from "fanoff" to "fanon" value and is marked as active, even
>  if it's only really active in case temperature is going down from trip point
>  2. Trip point 2 will be valid above "fanon" value and is also marked as
>  active. Trip point 3 is when hitting the critical temperature.
> 
Basically I do not think 4 trip points are needed, as stated above.

thanks,
rui
> ---
>  drivers/platform/x86/acerhdf.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index f94467c..b121449 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -400,7 +400,13 @@ static int acerhdf_get_trip_type(struct thermal_zone_device *thermal, int trip,
>  				 enum thermal_trip_type *type)
>  {
>  	if (trip == 0)
> +		*type = THERMAL_TRIP_PASSIVE;
> +	if (trip == 1)
>  		*type = THERMAL_TRIP_ACTIVE;
> +	if (trip == 2)
> +		*type = THERMAL_TRIP_ACTIVE;
> +	if (trip == 3)
> +		*type = THERMAL_TRIP_CRITICAL;
>  
>  	return 0;
>  }
> @@ -409,7 +415,13 @@ static int acerhdf_get_trip_temp(struct thermal_zone_device *thermal, int trip,
>  				 unsigned long *temp)
>  {
>  	if (trip == 0)
> +		*temp = 0;
> +	if (trip == 1)
> +		*temp = fanoff;
> +	if (trip == 2)
>  		*temp = fanon;
> +	if (trip == 3)
> +		*temp = ACERHDF_TEMP_CRIT;
>  
>  	return 0;
>  }
> @@ -431,6 +443,7 @@ static struct thermal_zone_device_ops acerhdf_dev_ops = {
>  	.get_trip_type = acerhdf_get_trip_type,
>  	.get_trip_temp = acerhdf_get_trip_temp,
>  	.get_crit_temp = acerhdf_get_crit_temp,
> +	.notify = NULL,
>  };
>  
> 
> @@ -486,7 +499,8 @@ static int acerhdf_set_cur_state(struct thermal_cooling_device *cdev,
>  		    (cur_temp < fanoff))
>  			acerhdf_change_fanstate(ACERHDF_FAN_OFF);
>  	} else {
> -		if (cur_state == ACERHDF_FAN_OFF)
> +		if ((cur_state == ACERHDF_FAN_OFF) &&
> +		    (cur_temp > fanon))
>  			acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
>  	}
>  	return 0;
> @@ -661,8 +675,9 @@ static int acerhdf_register_thermal(void)
>  	if (IS_ERR(cl_dev))
>  		return -EINVAL;
>  
> -	thz_dev = thermal_zone_device_register("acerhdf", 1, 0, NULL,
> -					      &acerhdf_dev_ops, NULL, 0,
> +	thz_dev = thermal_zone_device_register("acerhdf", 4, 0, NULL,
> +					      &acerhdf_dev_ops,
> +					      (kernelmode) ? interval*1000 : 0,
>  					      (kernelmode) ? interval*1000 : 0);
>  	if (IS_ERR(thz_dev))
>  		return -EINVAL;

Re: thermal governor: does it actually work??

[Borislav Petkov]

On Tue, Feb 19, 2013 at 10:51:04PM +0800, Zhang Rui wrote:
> Okay, here I see the problem, say you want to turn on the fan at 60C and
> turn it off at 55C, is this what you want?
> hmmm, is it possible to do some tricks in acerhdf driver?
> say,
> only one active trip point as before. but change acerhdf_get_trip_temp
> to:
> acerhdf_get_trip_temp()
> {
> 	if (fan is on)
> 		return 55;
> 	else
> 		return 60;
> }
> 
> In this way, when the fan is off and the temperature is raising, the fan
> will be turned on at 60C because we have an active trip point of 60C.
> And when the fan is turned on and the temperature starts to drop, the
> fan will be turned off at 55C.

Makes a sense to me. I was questioning the need for 4 trip points too.
We probably would need a second, critical trip point though, just in
case.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: thermal governor: does it actually work??

[Zhang Rui]

On Tue, 2013-02-19 at 16:05 +0100, Borislav Petkov wrote:
> On Tue, Feb 19, 2013 at 10:51:04PM +0800, Zhang Rui wrote:
> > Okay, here I see the problem, say you want to turn on the fan at 60C and
> > turn it off at 55C, is this what you want?
> > hmmm, is it possible to do some tricks in acerhdf driver?
> > say,
> > only one active trip point as before. but change acerhdf_get_trip_temp
> > to:
> > acerhdf_get_trip_temp()
> > {
> > 	if (fan is on)
> > 		return 55;
> > 	else
> > 		return 60;
> > }
> > 
> > In this way, when the fan is off and the temperature is raising, the fan
> > will be turned on at 60C because we have an active trip point of 60C.
> > And when the fan is turned on and the temperature starts to drop, the
> > fan will be turned off at 55C.
> 
> Makes a sense to me. I was questioning the need for 4 trip points too.

I do not understand why you need 4 trip points for the issue above,
unless I missed another gap here.

> We probably would need a second, critical trip point though, just in
> case.
> 
yeah, it is okay to have a critical trip point.

thanks,
rui

Re: thermal governor: does it actually work??

[Alexander Lam]

On Feb 18, 2013 7:11 AM, "Zhang, Rui" <rui.zhang@intel.com> wrote:
>
> Sorry I missed that patch.
>
> So the problem happens when the acerhdf thermal zone is registered when the fan has already been spinning, right?

Correct, when the acerhdf driver is switched into kernel controlled
mode and the fan is spinning, it stays spinning until the temperature
goes above the fan-on trip point and then below the fan-off trip
point. then the fan shuts off.

I don't have an opinion on how this is fixed, so do whatever you want
with my patch. Maybe you should drop it entirely if we do end up
applying Peter's fix.

>
>
>
> Thanks,
>
> rui
>
>
>
> From: Alexander Lam [mailto:azl@andrew.cmu.edu]
> Sent: Sunday, February 17, 2013 4:56 AM
> To: Andreas Mohr
> Cc: Zhang, Rui; linux-kernel@vger.kernel.org; R, Durgadoss; peter@piie.net; lenb@kernel.org
> Subject: Re: thermal governor: does it actually work??
> Importance: High
>
>
>
> I wrote a patch to fix this:
>
>
> http://lkml.org/lkml/2012/12/30/47
>
> But nobody picked it up and since then I have been too busy to respin the patch for new -rc kernels.
>
> -Alexander Lam
>
> On Fri, Feb 15, 2013 at 10:49 AM, Andreas Mohr <andi@lisas.de> wrote:
>
> Hi,
>
>
> On Fri, Feb 15, 2013 at 09:47:07AM +0000, Zhang, Rui wrote:
> > Please attach the output of
> > "grep . /sys/class/thermal/thermal_zone*/cdev*/*"?
>
> # grep . /sys/class/thermal/thermal_zone*/cdev*/*
> /sys/class/thermal/thermal_zone0/cdev0/cur_state:1
> /sys/class/thermal/thermal_zone0/cdev0/max_state:1
> grep: /sys/class/thermal/thermal_zone0/cdev0/power: Is a directory
> grep: /sys/class/thermal/thermal_zone0/cdev0/subsystem: Is a directory
> /sys/class/thermal/thermal_zone0/cdev0/type:acerhdf-fan
>
>
> > The question is that if we can also call it "throttle" when reducing the device performance to generate less heat.
>
> I won't continue to elaborate on this separate issue now, given that my time
> currently is very limited ;)
>
>
> Andreas Mohr
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>

-Alexander Lam

RE: thermal governor: does it actually work??

[Zhang Rui]

On Mon, 2013-02-18 at 15:26 -0500, Alexander Lam wrote:
> 
> On Feb 18, 2013 7:11 AM, "Zhang, Rui" <rui.zhang@intel.com> wrote:
> >
> > Sorry I missed that patch.
> >
> > So the problem happens when the acerhdf thermal zone is registered
> when the fan has already been spinning, right?
> 
> Correct, when the acerhdf driver is switched into kernel controlled
> mode and the fan is spinning, it stays spinning until the temperature
> goes above the fan-on trip point and then below the fan-off trip
> point. then the fan shuts off.
> 
> I don't have an opinion on how this is fixed, so do whatever you want
> with my patch. Maybe you should drop it entirely if we do end up
> applying Peter's fix.

> >
> >  
> >
> > Thanks,
> >
> > rui
> >
> >  
> >
> > From: Alexander Lam [mailto:azl@andrew.cmu.edu] 
> > Sent: Sunday, February 17, 2013 4:56 AM
> > To: Andreas Mohr
> > Cc: Zhang, Rui; linux-kernel@vger.kernel.org; R, Durgadoss;
> peter@piie.net; lenb@kernel.org
> > Subject: Re: thermal governor: does it actually work??
> > Importance: High
> >
> >  
> >
> > I wrote a patch to fix this:
> >
> >
> > http://lkml.org/lkml/2012/12/30/47

I think this is the right fix, but I still need to check if there is
some extra work needed before applying it.

thanks,
rui
> >
> > But nobody picked it up and since then I have been too busy to
> respin the patch for new -rc kernels.
> >
> > -Alexander Lam
> >
> > On Fri, Feb 15, 2013 at 10:49 AM, Andreas Mohr <andi@lisas.de>
> wrote:
> >
> > Hi,
> >
> >
> > On Fri, Feb 15, 2013 at 09:47:07AM +0000, Zhang, Rui wrote:
> > > Please attach the output of
> > > "grep . /sys/class/thermal/thermal_zone*/cdev*/*"?
> >
> > # grep . /sys/class/thermal/thermal_zone*/cdev*/*
> > /sys/class/thermal/thermal_zone0/cdev0/cur_state:1
> > /sys/class/thermal/thermal_zone0/cdev0/max_state:1
> > grep: /sys/class/thermal/thermal_zone0/cdev0/power: Is a directory
> > grep: /sys/class/thermal/thermal_zone0/cdev0/subsystem: Is a
> directory
> > /sys/class/thermal/thermal_zone0/cdev0/type:acerhdf-fan
> >
> >
> > > The question is that if we can also call it "throttle" when
> reducing the device performance to generate less heat.
> >
> > I won't continue to elaborate on this separate issue now, given that
> my time
> > currently is very limited ;)
> >
> >
> > Andreas Mohr
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >
> >  
> 
> -Alexander Lam
> 

Re: thermal governor: does it actually work??

[Zhang Rui]

On Thu, 2013-02-14 at 16:32 +0100, Andreas Mohr wrote:
> For me after having loaded acerhdf the fan never stops (with kernelmode
> active), despite staying safely below trip point
> (acerhdf_set_cur_state() actually never gets called).

BTW, could you please check if this one fixes the problem for you?
http://git.kernel.org/?p=linux/kernel/git/rzhang/linux.git;a=commit;h=b8bb6cb999858043489c1ddef08eed2127559169

thanks,
rui
> And AFAIR in a 3.2.0 kernel acerhdf fan operation seemed to just work
> (i.e., no fan for low temps, from the beginning).
> Needless to say 3.2.0 didn't even feature all the modern thermal
> governor crapyard yet ;)
> (ok, well, it's more complex but it's also a very nice environment capability)
> 
> 3.8-rc7:
> CONFIG_ACPI_THERMAL=m
> CONFIG_THERMAL=m
> CONFIG_THERMAL_HWMON=y
> CONFIG_THERMAL_DEFAULT_GOV_STEP_WISE=y
> # CONFIG_THERMAL_DEFAULT_GOV_FAIR_SHARE is not set
> # CONFIG_THERMAL_DEFAULT_GOV_USER_SPACE is not set
> CONFIG_FAIR_SHARE=y
> CONFIG_STEP_WISE=y
> # CONFIG_USER_SPACE is not set
> # CONFIG_CPU_THERMAL is not set
> 
> 
> 
> Terminology in this area seems to be quite a bit off, too, at several
> docs places, at least according to my understanding:
> 
> e.g. drivers/thermal/step_wise.c has the following comment:
> 
> /**
>  * step_wise_throttle - throttles devices associated with the given zone
>  * @tz - thermal_zone_device
>  * @trip - the trip point
>  * @trip_type - type of the trip point
>  *
>  * Throttling Logic: This uses the trend of the thermal zone to
>  * throttle.
>  * If the thermal zone is 'heating up' this throttles all the cooling
>  * devices associated with the zone and its particular trip point, by
>  * one
>  * step. If the zone is 'cooling down' it brings back the performance of
>  * the devices by one step.
> 
> 
> 
> if ... heating up ... throttles ...
> Sorry, but at least for P4 clockmod stuff (or some such), throttle
> states (P1...P8 IIRC) meant that the CPU operation was *reduced*,
> i.e. with pause intervals.
> And the translation of throttle clearly says that it does go that way
> and not the other way...
> (yes, you managed to confuse me that much that I even had to look up
> things to verify)
> 
> ... cooling down ... brings back ...
> This should certainly be worded "reduces" or some such.
> 
> So, any idea why I'm missing callbacks in acerhdf (if that is what I'm
> supposed to expect to happen)?
> Kernel bug, .config mistake, missing/wrong user-side setup?
> 
> Needless to say if kernel bug this ought to be fixed pre-3.8 ideally.
> 
> Thanks,
> 
> Andreas Mohr

Re: thermal governor: does it actually work??

[Peter Feuerer]

Adding Boris,

sorry, I can't do anything currently, I'm down with influenza.

kind regards,
--peter;

Zhang Rui writes:

> On Thu, 2013-02-14 at 16:32 +0100, Andreas Mohr wrote:
>> For me after having loaded acerhdf the fan never stops (with kernelmode
>> active), despite staying safely below trip point
>> (acerhdf_set_cur_state() actually never gets called).
> 
> BTW, could you please check if this one fixes the problem for you?
> http://git.kernel.org/?p=linux/kernel/git/rzhang/linux.git;a=commit;h=b8bb6cb999858043489c1ddef08eed2127559169
> 
> thanks,
> rui
>> And AFAIR in a 3.2.0 kernel acerhdf fan operation seemed to just work
>> (i.e., no fan for low temps, from the beginning).
>> Needless to say 3.2.0 didn't even feature all the modern thermal
>> governor crapyard yet ;)
>> (ok, well, it's more complex but it's also a very nice environment capability)
>> 
>> 3.8-rc7:
>> CONFIG_ACPI_THERMAL=m
>> CONFIG_THERMAL=m
>> CONFIG_THERMAL_HWMON=y
>> CONFIG_THERMAL_DEFAULT_GOV_STEP_WISE=y
>> # CONFIG_THERMAL_DEFAULT_GOV_FAIR_SHARE is not set
>> # CONFIG_THERMAL_DEFAULT_GOV_USER_SPACE is not set
>> CONFIG_FAIR_SHARE=y
>> CONFIG_STEP_WISE=y
>> # CONFIG_USER_SPACE is not set
>> # CONFIG_CPU_THERMAL is not set
>> 
>> 
>> 
>> Terminology in this area seems to be quite a bit off, too, at several
>> docs places, at least according to my understanding:
>> 
>> e.g. drivers/thermal/step_wise.c has the following comment:
>> 
>> /**
>>  * step_wise_throttle - throttles devices associated with the given zone
>>  * @tz - thermal_zone_device
>>  * @trip - the trip point
>>  * @trip_type - type of the trip point
>>  *
>>  * Throttling Logic: This uses the trend of the thermal zone to
>>  * throttle.
>>  * If the thermal zone is 'heating up' this throttles all the cooling
>>  * devices associated with the zone and its particular trip point, by
>>  * one
>>  * step. If the zone is 'cooling down' it brings back the performance of
>>  * the devices by one step.
>> 
>> 
>> 
>> if ... heating up ... throttles ...
>> Sorry, but at least for P4 clockmod stuff (or some such), throttle
>> states (P1...P8 IIRC) meant that the CPU operation was *reduced*,
>> i.e. with pause intervals.
>> And the translation of throttle clearly says that it does go that way
>> and not the other way...
>> (yes, you managed to confuse me that much that I even had to look up
>> things to verify)
>> 
>> ... cooling down ... brings back ...
>> This should certainly be worded "reduces" or some such.
>> 
>> So, any idea why I'm missing callbacks in acerhdf (if that is what I'm
>> supposed to expect to happen)?
>> Kernel bug, .config mistake, missing/wrong user-side setup?
>> 
>> Needless to say if kernel bug this ought to be fixed pre-3.8 ideally.
>> 
>> Thanks,
>> 
>> Andreas Mohr
> 
> 

Re: thermal governor: does it actually work??

[Borislav Petkov]

On Fri, Feb 22, 2013 at 06:33:04AM +0100, Peter Feuerer wrote:
> Adding Boris,
> 
> sorry, I can't do anything currently, I'm down with influenza.

Oh, sorry to hear that - gute Besserung! :-)

Sure, I'll take a look, Zhang's idea made sense to me, let me try it
out.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH] acerhdf: Fix fan activation with new thermal governor

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

The new step_wise thermal governor wasn't able to handle the one-trip
point design of acerhdf where we want to turn off the fan if we go under
the 'fanoff' temperature and to turn it on only after exceeding the
'fanon' temperature.

Do that by looking at the current fan state and return the temperature
accordingly.

Suggested-by: Zhang Rui <rui.zhang@intel.com>
Cc: Peter Feuerer <peter@piie.net>
Cc: Andreas Mohr <andi@lisas.de>
Cc: Alexander Lam <azl@andrew.cmu.edu>
Signed-off-by: Borislav Petkov <bp@suse.de>
---

Guys,

this fixes acerhdf to the old behavior. Testing here looks ok - I'd
appreciate if you could verify this too.

Thanks.

 drivers/platform/x86/acerhdf.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index f94467c05225..b44033756909 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -405,13 +405,33 @@ static int acerhdf_get_trip_type(struct thermal_zone_device *thermal, int trip,
 	return 0;
 }
 
+/*
+ * This is more or less a quirky wagging the dog so that a one-trip
+ * point can still work with the step_wise governor. Basically, we cheat
+ * with the trip temperature depending on whether the current state of
+ * the fan (on vs off).
+ */
 static int acerhdf_get_trip_temp(struct thermal_zone_device *thermal, int trip,
 				 unsigned long *temp)
 {
-	if (trip == 0)
+	int err = 0, state;
+
+	err = acerhdf_get_fanstate(&state);
+	if (err) {
+		/*
+		 * We want to be conservative here: in the unlikely event that
+		 * reading the fanstate failed, we want to leave the fan on.
+		 */
 		*temp = fanon;
+		return err;
+	}
 
-	return 0;
+	if (state == ACERHDF_FAN_AUTO)
+		*temp = fanoff;
+	else
+		*temp = fanon;
+
+	return err;
 }
 
 static int acerhdf_get_crit_temp(struct thermal_zone_device *thermal,
-- 
1.8.1.3.535.ga923c31

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Borislav Petkov]

On Sat, Feb 23, 2013 at 08:20:10PM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> The new step_wise thermal governor wasn't able to handle the one-trip
> point design of acerhdf where we want to turn off the fan if we go under
> the 'fanoff' temperature and to turn it on only after exceeding the
> 'fanon' temperature.
> 
> Do that by looking at the current fan state and return the temperature
> accordingly.
> 
> Suggested-by: Zhang Rui <rui.zhang@intel.com>
> Cc: Peter Feuerer <peter@piie.net>
> Cc: Andreas Mohr <andi@lisas.de>
> Cc: Alexander Lam <azl@andrew.cmu.edu>
> Signed-off-by: Borislav Petkov <bp@suse.de>

Ok, ignore this one for now - the suspend/resume path doesn't pan out
somehow, need to do some more handholding here.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Peter Feuerer]

Hi Boris,

thanks for your best wishes in the last mail, I'm feeling little better now.

Borislav Petkov writes:

> On Sat, Feb 23, 2013 at 08:20:10PM +0100, Borislav Petkov wrote:
>> From: Borislav Petkov <bp@suse.de>
>> 
>> The new step_wise thermal governor wasn't able to handle the one-trip
>> point design of acerhdf where we want to turn off the fan if we go under
>> the 'fanoff' temperature and to turn it on only after exceeding the
>> 'fanon' temperature.
>> 
>> Do that by looking at the current fan state and return the temperature
>> accordingly.
>> 
>> Suggested-by: Zhang Rui <rui.zhang@intel.com>
>> Cc: Peter Feuerer <peter@piie.net>
>> Cc: Andreas Mohr <andi@lisas.de>
>> Cc: Alexander Lam <azl@andrew.cmu.edu>
>> Signed-off-by: Borislav Petkov <bp@suse.de>
> 
> Ok, ignore this one for now - the suspend/resume path doesn't pan out
> somehow, need to do some more handholding here.

Please test my last patch with the 4 trip points ;) - even if you don't 
really like it, it is working great! - And to be honest, I still prefer this 
solution!

Anyhow, did you test what Rui asked us to test?

-- 
kind regards,
--peter;

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Borislav Petkov]

On Sun, Feb 24, 2013 at 12:42:55PM +0100, Peter Feuerer wrote:
> Hi Boris,
> 
> thanks for your best wishes in the last mail, I'm feeling little better now.

Nice :)

> Please test my last patch with the 4 trip points ;) - even if you
> don't really like it, it is working great! - And to be honest, I still
> prefer this solution!

Right, but 4 trip points for this simple driver is a bit of a overkill,
don't you think? If we want to be really accurate, we'd only need two,
think of three temperature intervals here:

0 - temp <= fanon
1 - fanon =< temp < crit
2 - temp >= crit

I need to go stare at it a bit more.

> Anyhow, did you test what Rui asked us to test?

Yep.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Borislav Petkov]

On Sun, Feb 24, 2013 at 01:09:52PM +0100, Borislav Petkov wrote:
> On Sun, Feb 24, 2013 at 12:42:55PM +0100, Peter Feuerer wrote:
> > Hi Boris,
> > 
> > thanks for your best wishes in the last mail, I'm feeling little better now.
> 
> Nice :)
> 
> > Please test my last patch with the 4 trip points ;) - even if you
> > don't really like it, it is working great! - And to be honest, I still
> > prefer this solution!
> 
> Right, but 4 trip points for this simple driver is a bit of a overkill,
> don't you think? If we want to be really accurate, we'd only need two,
> think of three temperature intervals here:
> 
> 0 - temp <= fanon
> 1 - fanon =< temp < crit
> 2 - temp >= crit
> 
> I need to go stare at it a bit more.

Oh, and then there's thermal_cdev_update which calls down
->set_cur_state, i.e. acerhdf_set_cur_state() handing us down a state
which we're supposed to take into consideration but we completely ignore
and no our own thing.

And, AFAICT, when this thing gets called, we need to set the fan to
either AUTO or OFF, based on the state it's giving us from above, no?
Because with acerhdf_get_max_state() we tell it that we have two states,
0 and 1.

Hmm.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Zhang Rui]

On Sun, 2013-02-24 at 13:09 +0100, Borislav Petkov wrote:
> On Sun, Feb 24, 2013 at 12:42:55PM +0100, Peter Feuerer wrote:
> > Hi Boris,
> > 
> > thanks for your best wishes in the last mail, I'm feeling little better now.
> 
> Nice :)
> 
> > Please test my last patch with the 4 trip points ;) - even if you
> > don't really like it, it is working great! - And to be honest, I still
> > prefer this solution!
> 
> Right, but 4 trip points for this simple driver is a bit of a overkill,
> don't you think? If we want to be really accurate, we'd only need two,
> think of three temperature intervals here:
> 
> 0 - temp <= fanon
> 1 - fanon =< temp < crit
> 2 - temp >= crit
> 
> I need to go stare at it a bit more.
> 
actually, I still do not understand how the two active trip points
mechanism work.

say,
act1 = 50C, act2=60C

when the temperature goes up to 50C, will the fan be turned on or not?

thanks,
rui

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Borislav Petkov]

On Mon, Feb 25, 2013 at 11:06:40AM +0800, Zhang Rui wrote:
> when the temperature goes up to 50C, will the fan be turned on or not?

Right, let's first clarify this:

so we want the fan to go on when it reaches and/or exceeds the 'fanon'
temperature and to go off after it goes under the 'fanoff' temperature.
It is supposed to remain on in the [fanoff,fanon] interval.

Now, my observation from yesterday was that, if we don't do the
flattening of the abovementioned interval in acerhdf_set_cur_state() but
we simply turn on the fan if we receive state=1 and turn it off when
state=0, the fan gets turned on only for short bursts when the governor
decides it is time to throttle, i.e. the temperature in the zone is
rising and turns it off when the temperature starts to fall again. And
it would turn on the fan at lower temperatures than fanon.

But this is annoying. The main reason why this acerhdf thing was added
in the first place was to do a better fan control on those machines
because the fans are noisy. Basically, enable the fan only when really
needed - and leave it off when not because it is really annoying.

When enabled, however, leave it on longer until a safe, 'fanoff'
temperature is reached.

Am I making more sense now? Peter?

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Peter Feuerer]

Hi,

sorry for not answering to earlier mails, but sitting in front of the 
computer and writing English mails is still very exhausting for me.


Borislav Petkov writes:

> On Mon, Feb 25, 2013 at 11:06:40AM +0800, Zhang Rui wrote:
>> when the temperature goes up to 50C, will the fan be turned on or not?
> 
> Right, let's first clarify this:
> 
> so we want the fan to go on when it reaches and/or exceeds the 'fanon'
> temperature and to go off after it goes under the 'fanoff' temperature.
> It is supposed to remain on in the [fanoff,fanon] interval.
> 
> Now, my observation from yesterday was that, if we don't do the
> flattening of the abovementioned interval in acerhdf_set_cur_state() but
> we simply turn on the fan if we receive state=1 and turn it off when
> state=0, the fan gets turned on only for short bursts when the governor
> decides it is time to throttle, i.e. the temperature in the zone is
> rising and turns it off when the temperature starts to fall again. And
> it would turn on the fan at lower temperatures than fanon.
> 
> But this is annoying. The main reason why this acerhdf thing was added
> in the first place was to do a better fan control on those machines
> because the fans are noisy. Basically, enable the fan only when really
> needed - and leave it off when not because it is really annoying.
> 
> When enabled, however, leave it on longer until a safe, 'fanoff'
> temperature is reached.
> 
> Am I making more sense now? Peter?

This sounds good, couldn't have stated it more clear. Just one thing to add 
here: This is standard idea of a "schmitt trigger", we implemented 
in acerhdf :)
http://en.wikipedia.org/wiki/Schmitt_trigger

-- 
kind regards,
--peter;

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Borislav Petkov]

On Sun, Feb 24, 2013 at 12:42:55PM +0100, Peter Feuerer wrote:
> Please test my last patch with the 4 trip points ;) - even if you
> don't really like it, it is working great! - And to be honest, I still
> prefer this solution!

Ok, I remember everything now - had to add some debug output to see what
the stepwise governor hands us down.

Btw, Zhang, is there any way we can tell the upper layer to not
poll trips and temps for this driver? I mean, ->get_trip_type and
->get_trip_temp get called every polling interval and the data it
receives back each time is static and don't change so polling the same
values each time for this driver doesn't make any sense.

Can we pass trip points and temperature levels upon registration time
instead?

@Peter: I took your patch, removed one trip point and added comments so
that we don't forget why we do what we do. Please take a look - it works
fine here.

--
>From 1827ed71ff1b73a83d81d21f9dd167fe8e0d4198 Mon Sep 17 00:00:00 2001
From: Peter Feuerer <peter@piie.net>
Date: Sun, 24 Feb 2013 15:04:17 +0100
Subject: [PATCH] acerhdf: Fix fan activation with new thermal governor

After recent thermal subsys rework, acerhdf couldn't cope with the
stepwise governor since it had only one trip point and this didn't fit
into the stepwise scheme. Therefore, add two more trip points - an
active one where we turn on the fan, and a critical one.

However, we still need to flatten out peaks of turning the fan on
and off in acerhdf_set_cur_state because stepwise looks also at the
direction the temperature is going and applies respective policy. This
results in short bursts of interchanging on and off which are really
annoying.

So, we keep the old logic where we turn on the fan only if we exceed
'fanon' temperature and leave it on until we go under 'fanoff'. Document
this behavior while at it.

Signed-off-by: Peter Feuerer <peter@piie.net>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 drivers/platform/x86/acerhdf.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index f94467c05225..b06cdb099e6a 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -400,7 +400,13 @@ static int acerhdf_get_trip_type(struct thermal_zone_device *thermal, int trip,
 				 enum thermal_trip_type *type)
 {
 	if (trip == 0)
+		*type = THERMAL_TRIP_PASSIVE;
+	else if (trip == 1)
 		*type = THERMAL_TRIP_ACTIVE;
+	else if (trip == 2)
+		*type = THERMAL_TRIP_CRITICAL;
+	else
+		WARN_ONCE(1, "%s: Funny trip point %d\n", KBUILD_MODNAME, trip);
 
 	return 0;
 }
@@ -409,7 +415,13 @@ static int acerhdf_get_trip_temp(struct thermal_zone_device *thermal, int trip,
 				 unsigned long *temp)
 {
 	if (trip == 0)
+		*temp = 0;
+	else if (trip == 1)
 		*temp = fanon;
+	else if (trip == 2)
+		*temp = ACERHDF_TEMP_CRIT;
+	else
+		WARN_ONCE(1, "%s: Funny trip point %d\n", KBUILD_MODNAME, trip);
 
 	return 0;
 }
@@ -459,7 +471,9 @@ static int acerhdf_get_cur_state(struct thermal_cooling_device *cdev,
 	return 0;
 }
 
-/* change current fan state - is overwritten when running in kernel mode */
+/*
+ * Change current fan state - is overwritten when running in kernel mode
+ */
 static int acerhdf_set_cur_state(struct thermal_cooling_device *cdev,
 				 unsigned long state)
 {
@@ -480,15 +494,23 @@ static int acerhdf_set_cur_state(struct thermal_cooling_device *cdev,
 		goto err_out;
 	}
 
+	/*
+	 * We need to flatten out the on-off peaks we get from the stepwise
+	 * governor into the wider span between fanoff and fanon because
+	 * otherwise we turn on/off the fan in short bursts, everytime the
+	 * thermal zone decides to throttle and this is annoying.
+	 */
 	if (state == 0) {
 		/* turn fan off only if below fanoff temperature */
 		if ((cur_state == ACERHDF_FAN_AUTO) &&
-		    (cur_temp < fanoff))
+		    (cur_temp <= fanoff))
 			acerhdf_change_fanstate(ACERHDF_FAN_OFF);
 	} else {
-		if (cur_state == ACERHDF_FAN_OFF)
+		if ((cur_state == ACERHDF_FAN_OFF) &&
+		    (cur_temp >= fanon))
 			acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
 	}
+
 	return 0;
 
 err_out:
@@ -661,7 +683,7 @@ static int acerhdf_register_thermal(void)
 	if (IS_ERR(cl_dev))
 		return -EINVAL;
 
-	thz_dev = thermal_zone_device_register("acerhdf", 1, 0, NULL,
+	thz_dev = thermal_zone_device_register("acerhdf", 3, 0, NULL,
 					      &acerhdf_dev_ops, NULL, 0,
 					      (kernelmode) ? interval*1000 : 0);
 	if (IS_ERR(thz_dev))
-- 
1.8.1.3.535.ga923c31


-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Zhang Rui]

On Sun, 2013-02-24 at 15:34 +0100, Borislav Petkov wrote:
> On Sun, Feb 24, 2013 at 12:42:55PM +0100, Peter Feuerer wrote:
> > Please test my last patch with the 4 trip points ;) - even if you
> > don't really like it, it is working great! - And to be honest, I still
> > prefer this solution!
> 
> Ok, I remember everything now - had to add some debug output to see what
> the stepwise governor hands us down.
> 
> Btw, Zhang, is there any way we can tell the upper layer to not
> poll trips and temps for this driver? I mean, ->get_trip_type and
> ->get_trip_temp get called every polling interval and the data it
> receives back each time is static and don't change so polling the same
> values each time for this driver doesn't make any sense.
> 
the reason we keep on calling it because the thermal zone trip point
info may be changed at runtime.

> Can we pass trip points and temperature levels upon registration time
> instead?
> 
no, but we can cache it in the thermal layer, and do update when
necessary.

> @Peter: I took your patch, removed one trip point and added comments so
> that we don't forget why we do what we do. Please take a look - it works
> fine here.
> 
> --
> From 1827ed71ff1b73a83d81d21f9dd167fe8e0d4198 Mon Sep 17 00:00:00 2001
> From: Peter Feuerer <peter@piie.net>
> Date: Sun, 24 Feb 2013 15:04:17 +0100
> Subject: [PATCH] acerhdf: Fix fan activation with new thermal governor
> 
> After recent thermal subsys rework, acerhdf couldn't cope with the
> stepwise governor since it had only one trip point and this didn't fit
> into the stepwise scheme.

why? I do not think the stepwise scheme will break it. If it happens, we
should fix stepwise governor instead.

>  Therefore, add two more trip points - an
> active one where we turn on the fan, and a critical one.
> 
I think you add a passive trip point and a critical one here.

> However, we still need to flatten out peaks of turning the fan on
> and off in acerhdf_set_cur_state because stepwise looks also at the
> direction the temperature is going and applies respective policy. This
> results in short bursts of interchanging on and off which are really
> annoying.
> 
> So, we keep the old logic where we turn on the fan only if we exceed
> 'fanon' temperature and leave it on until we go under 'fanoff'. Document
> this behavior while at it.
> 
has anybody checked if the patch at lkml.org/lkml/2012/12/30/47 fixes
the problem, without any other patch?

thanks,
rui
> Signed-off-by: Peter Feuerer <peter@piie.net>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  drivers/platform/x86/acerhdf.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index f94467c05225..b06cdb099e6a 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -400,7 +400,13 @@ static int acerhdf_get_trip_type(struct thermal_zone_device *thermal, int trip,
>  				 enum thermal_trip_type *type)
>  {
>  	if (trip == 0)
> +		*type = THERMAL_TRIP_PASSIVE;
> +	else if (trip == 1)
>  		*type = THERMAL_TRIP_ACTIVE;
> +	else if (trip == 2)
> +		*type = THERMAL_TRIP_CRITICAL;
> +	else
> +		WARN_ONCE(1, "%s: Funny trip point %d\n", KBUILD_MODNAME, trip);
>  
>  	return 0;
>  }
> @@ -409,7 +415,13 @@ static int acerhdf_get_trip_temp(struct thermal_zone_device *thermal, int trip,
>  				 unsigned long *temp)
>  {
>  	if (trip == 0)
> +		*temp = 0;
> +	else if (trip == 1)
>  		*temp = fanon;
> +	else if (trip == 2)
> +		*temp = ACERHDF_TEMP_CRIT;
> +	else
> +		WARN_ONCE(1, "%s: Funny trip point %d\n", KBUILD_MODNAME, trip);
>  
>  	return 0;
>  }
> @@ -459,7 +471,9 @@ static int acerhdf_get_cur_state(struct thermal_cooling_device *cdev,
>  	return 0;
>  }
>  
> -/* change current fan state - is overwritten when running in kernel mode */
> +/*
> + * Change current fan state - is overwritten when running in kernel mode
> + */
>  static int acerhdf_set_cur_state(struct thermal_cooling_device *cdev,
>  				 unsigned long state)
>  {
> @@ -480,15 +494,23 @@ static int acerhdf_set_cur_state(struct thermal_cooling_device *cdev,
>  		goto err_out;
>  	}
>  
> +	/*
> +	 * We need to flatten out the on-off peaks we get from the stepwise
> +	 * governor into the wider span between fanoff and fanon because
> +	 * otherwise we turn on/off the fan in short bursts, everytime the
> +	 * thermal zone decides to throttle and this is annoying.
> +	 */
>  	if (state == 0) {
>  		/* turn fan off only if below fanoff temperature */
>  		if ((cur_state == ACERHDF_FAN_AUTO) &&
> -		    (cur_temp < fanoff))
> +		    (cur_temp <= fanoff))
>  			acerhdf_change_fanstate(ACERHDF_FAN_OFF);
>  	} else {
> -		if (cur_state == ACERHDF_FAN_OFF)
> +		if ((cur_state == ACERHDF_FAN_OFF) &&
> +		    (cur_temp >= fanon))
>  			acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
>  	}
> +
>  	return 0;
>  
>  err_out:
> @@ -661,7 +683,7 @@ static int acerhdf_register_thermal(void)
>  	if (IS_ERR(cl_dev))
>  		return -EINVAL;
>  
> -	thz_dev = thermal_zone_device_register("acerhdf", 1, 0, NULL,
> +	thz_dev = thermal_zone_device_register("acerhdf", 3, 0, NULL,
>  					      &acerhdf_dev_ops, NULL, 0,
>  					      (kernelmode) ? interval*1000 : 0);
>  	if (IS_ERR(thz_dev))
> -- 
> 1.8.1.3.535.ga923c31
> 
> 

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Borislav Petkov]

On Mon, Feb 25, 2013 at 11:21:54AM +0800, Zhang Rui wrote:
> the reason we keep on calling it because the thermal zone trip point
> info may be changed at runtime.
> 
> > Can we pass trip points and temperature levels upon registration time
> > instead?
> > 
> no, but we can cache it in the thermal layer, and do update when
> necessary.

Right, so this would make sense for fan, etc drivers which have static
trip points.

> why? I do not think the stepwise scheme will break it. If it happens, we
> should fix stepwise governor instead.

Ok, see my earlier mail for what acerhdf would need to be able to do.

> >  Therefore, add two more trip points - an
> > active one where we turn on the fan, and a critical one.
> > 
> I think you add a passive trip point and a critical one here.

I got 3 in total:

    if (trip == 0)
+           *type = THERMAL_TRIP_PASSIVE;
+   else if (trip == 1)
            *type = THERMAL_TRIP_ACTIVE;
+   else if (trip == 2)
+           *type = THERMAL_TRIP_CRITICAL;

> has anybody checked if the patch at lkml.org/lkml/2012/12/30/47 fixes
> the problem, without any other patch?

Ok, let me try it out.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Borislav Petkov]

On Mon, Feb 25, 2013 at 11:25:26AM +0100, Borislav Petkov wrote:
> > has anybody checked if the patch at lkml.org/lkml/2012/12/30/47 fixes
> > the problem, without any other patch?
> 
> Ok, let me try it out.

Ok, doing a short test here proves successful. I'll run it the next days
too to make sure - it would be nice if people could confirm my test
results, though.

Tested-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Borislav Petkov]

On Mon, Feb 25, 2013 at 01:16:08PM +0100, Borislav Petkov wrote:
> On Mon, Feb 25, 2013 at 11:25:26AM +0100, Borislav Petkov wrote:
> > > has anybody checked if the patch at lkml.org/lkml/2012/12/30/47 fixes
> > > the problem, without any other patch?
> > 
> > Ok, let me try it out.
> 
> Ok, doing a short test here proves successful. I'll run it the next days
> too to make sure - it would be nice if people could confirm my test
> results, though.
> 
> Tested-by: Borislav Petkov <bp@suse.de>

Pjooiiing!

Andreas, Peter, can you guys give http://lkml.org/lkml/2012/12/30/47 a run?

Thanks.


-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Andreas Mohr]

Hi,

On Mon, Mar 04, 2013 at 02:34:10PM +0100, Borislav Petkov wrote:
> Pjooiiing!

Heh, that was a broken one!
Rather well-deserved though - took too much time, been too busy.

> Andreas, Peter, can you guys give http://lkml.org/lkml/2012/12/30/47 a run?

Will fire up my HDD NOW.
About time to get all that stuff going anyway.
In need of a new less(?) buggy kernel anyway, since I suspect that stock 2.6.32
to be especially quirky with overly aggressive USB (SSD) resume timing...
(keep getting astonishing numbers of -71 timeouts on resume)


BTW, thanks very much to Zhang Rui for his timely tackling of the
acerhdf matter!

Andreas Mohr

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Andreas Mohr]

Hi,

On Mon, Mar 04, 2013 at 02:34:10PM +0100, Borislav Petkov wrote:
> On Mon, Feb 25, 2013 at 01:16:08PM +0100, Borislav Petkov wrote:
> > Ok, doing a short test here proves successful. I'll run it the next days
> > too to make sure - it would be nice if people could confirm my test
> > results, though.
> > 
> > Tested-by: Borislav Petkov <bp@suse.de>
> 
> Pjooiiing!
> 
> Andreas, Peter, can you guys give http://lkml.org/lkml/2012/12/30/47 a run?

I'm now running 3.8 proper with only that patch plus my local
beautification-only (really shouldn't matter)
acerhdf updates (now properly split ;),
and so far fan behaviour seems fine, over various usage conditions.
Haven't really stressed it mischievously yet, though. Will do more testing.

So this now does not have any of the Schmitt trigger equivalent (?)
governor changes that were nicely discussed in this activity -
So what do we want to have committed, and what do we not,
and most importantly, does it all work fatally precisely correctly? ;)
(remember, this is thermal layer, not particularly a place for sloppiness)

Thanks,

Andreas Mohr

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Zhang Rui]

On Tue, 2013-03-05 at 22:59 +0100, Andreas Mohr wrote:
> Hi,
> 
> On Mon, Mar 04, 2013 at 02:34:10PM +0100, Borislav Petkov wrote:
> > On Mon, Feb 25, 2013 at 01:16:08PM +0100, Borislav Petkov wrote:
> > > Ok, doing a short test here proves successful. I'll run it the next days
> > > too to make sure - it would be nice if people could confirm my test
> > > results, though.
> > > 
> > > Tested-by: Borislav Petkov <bp@suse.de>
> > 
> > Pjooiiing!
> > 
> > Andreas, Peter, can you guys give http://lkml.org/lkml/2012/12/30/47 a run?
> 
> I'm now running 3.8 proper with only that patch plus my local
> beautification-only (really shouldn't matter)
> acerhdf updates (now properly split ;),
> and so far fan behaviour seems fine, over various usage conditions.
> Haven't really stressed it mischievously yet, though. Will do more testing.
> 
> So this now does not have any of the Schmitt trigger equivalent (?)
> governor changes that were nicely discussed in this activity -
> So what do we want to have committed, and what do we not,

the problem should have been fixed in 3.9-rc1 by
commit b8bb6cb999858043489c1ddef08eed2127559169
Author: Zhang Rui <rui.zhang@intel.com>
Date:   Thu Nov 22 15:45:02 2012 +0800

    step_wise: Unify the code for both throttle and dethrottle

    Signed-off-by: Zhang Rui <rui.zhang@intel.com>

I'll send Alexander' patch for 3.8 stable.

thanks,
rui
> and most importantly, does it all work fatally precisely correctly? ;)
> (remember, this is thermal layer, not particularly a place for sloppiness)
> 
> Thanks,
> 
> Andreas Mohr

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Borislav Petkov]

On Wed, Mar 06, 2013 at 11:47:20AM +0800, Zhang Rui wrote:
> > So this now does not have any of the Schmitt trigger equivalent (?)
> > governor changes that were nicely discussed in this activity -
> > So what do we want to have committed, and what do we not,
> 
> the problem should have been fixed in 3.9-rc1 by
> commit b8bb6cb999858043489c1ddef08eed2127559169
> Author: Zhang Rui <rui.zhang@intel.com>
> Date:   Thu Nov 22 15:45:02 2012 +0800
> 
>     step_wise: Unify the code for both throttle and dethrottle
> 
>     Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> 
> I'll send Alexander' patch for 3.8 stable.

Hold on, do I understand it correctly that this is yet another patch
that "fixes" it? If so, maybe we should test 3.9-rc1 which contains the
said patch above? Yes, no?

Also, I don't see Alexander's patch applied anywhere so if it isn't
upstream, you can't send it to stable.

Please clarify,

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Zhang Rui]

On Wed, 2013-03-06 at 08:00 +0100, Borislav Petkov wrote:
> On Wed, Mar 06, 2013 at 11:47:20AM +0800, Zhang Rui wrote:
> > > So this now does not have any of the Schmitt trigger equivalent (?)
> > > governor changes that were nicely discussed in this activity -
> > > So what do we want to have committed, and what do we not,
> > 
> > the problem should have been fixed in 3.9-rc1 by
> > commit b8bb6cb999858043489c1ddef08eed2127559169
> > Author: Zhang Rui <rui.zhang@intel.com>
> > Date:   Thu Nov 22 15:45:02 2012 +0800
> > 
> >     step_wise: Unify the code for both throttle and dethrottle
> > 
> >     Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > 
> > I'll send Alexander' patch for 3.8 stable.
> 
> Hold on, do I understand it correctly that this is yet another patch
> that "fixes" it?

sorry I should be clearer.
the problem that alexander' patch fixes, does not exist in 3.9-rc1
because the problem is fixed by this commit
b8bb6cb999858043489c1ddef08eed2127559169.

so what we need is, either 3.8.x kernel + alexander's patch, or clean
3.9-rc1.

>  If so, maybe we should test 3.9-rc1 which contains the
> said patch above? Yes, no?
> 
> Also, I don't see Alexander's patch applied anywhere so if it isn't
> upstream, you can't send it to stable.
> 
then I'll back port b8bb6cb999858043489c1ddef08eed2127559169 for 3.8
stable.

thanks,
rui

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Borislav Petkov]

On Wed, Mar 06, 2013 at 03:30:28PM +0800, Zhang Rui wrote:
> sorry I should be clearer. the problem that alexander' patch fixes,
> does not exist in 3.9-rc1 because the problem is fixed by this commit
> b8bb6cb999858043489c1ddef08eed2127559169.
>
> so what we need is, either 3.8.x kernel + alexander's patch, or clean
> 3.9-rc1.

Ok, understood now. I'll run -rc1 to verify the fix.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Peter Feuerer]

Hi Boris,

Borislav Petkov writes:

> On Mon, Feb 25, 2013 at 01:16:08PM +0100, Borislav Petkov wrote:
>> On Mon, Feb 25, 2013 at 11:25:26AM +0100, Borislav Petkov wrote:
>> > > has anybody checked if the patch at lkml.org/lkml/2012/12/30/47 fixes
>> > > the problem, without any other patch?
>> > 
>> > Ok, let me try it out.
>> 
>> Ok, doing a short test here proves successful. I'll run it the next days
>> too to make sure - it would be nice if people could confirm my test
>> results, though.
>> 
>> Tested-by: Borislav Petkov <bp@suse.de>
> 
> Pjooiiing!
> 
> Andreas, Peter, can you guys give http://lkml.org/lkml/2012/12/30/47 a run?

Finally I found some time to do a test run. I did a git clone on the Linus 
tree. But I was not able to apply this patch, seems like it has already been 
applied on mainline.

Anyhow, with current git state acerhdf works flawlessly for me. I tried 
standard usage of the netbook and suspend.

Thanks.

-- 
kind regards,
--peter;

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Borislav Petkov]

On Thu, Mar 07, 2013 at 09:17:03PM +0100, Peter Feuerer wrote:
> Finally I found some time to do a test run. I did a git clone on the
> Linus tree. But I was not able to apply this patch, seems like it has
> already been applied on mainline.

Yep, patch is in 3.9-rc1.

> Anyhow, with current git state acerhdf works flawlessly for me. I
> tried standard usage of the netbook and suspend.

Good, so we're all fine then, nice.

Thanks for testing.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Zhang Rui]

On Sun, 2013-02-24 at 12:42 +0100, Peter Feuerer wrote:
> Hi Boris,
> 
> thanks for your best wishes in the last mail, I'm feeling little better now.
> 
> Borislav Petkov writes:
> 
> > On Sat, Feb 23, 2013 at 08:20:10PM +0100, Borislav Petkov wrote:
> >> From: Borislav Petkov <bp@suse.de>
> >> 
> >> The new step_wise thermal governor wasn't able to handle the one-trip
> >> point design of acerhdf where we want to turn off the fan if we go under
> >> the 'fanoff' temperature and to turn it on only after exceeding the
> >> 'fanon' temperature.
> >> 
> >> Do that by looking at the current fan state and return the temperature
> >> accordingly.
> >> 
> >> Suggested-by: Zhang Rui <rui.zhang@intel.com>
> >> Cc: Peter Feuerer <peter@piie.net>
> >> Cc: Andreas Mohr <andi@lisas.de>
> >> Cc: Alexander Lam <azl@andrew.cmu.edu>
> >> Signed-off-by: Borislav Petkov <bp@suse.de>
> > 
> > Ok, ignore this one for now - the suspend/resume path doesn't pan out
> > somehow, need to do some more handholding here.
> 
> Please test my last patch with the 4 trip points ;) - even if you don't 
> really like it, it is working great! - And to be honest, I still prefer this 
> solution!
> 
well, the 4 trip points do not make sense, at least the passive trip
point is a no op as we have no passive cooling device binded with it.

thanks,
rui

Re: [PATCH] acerhdf: Fix fan activation with new thermal governor

[Zhang Rui]

On Sun, 2013-02-24 at 12:28 +0100, Borislav Petkov wrote:
> On Sat, Feb 23, 2013 at 08:20:10PM +0100, Borislav Petkov wrote:
> > From: Borislav Petkov <bp@suse.de>
> > 
> > The new step_wise thermal governor wasn't able to handle the one-trip
> > point design of acerhdf where we want to turn off the fan if we go under
> > the 'fanoff' temperature and to turn it on only after exceeding the
> > 'fanon' temperature.
> > 
> > Do that by looking at the current fan state and return the temperature
> > accordingly.
> > 
> > Suggested-by: Zhang Rui <rui.zhang@intel.com>
> > Cc: Peter Feuerer <peter@piie.net>
> > Cc: Andreas Mohr <andi@lisas.de>
> > Cc: Alexander Lam <azl@andrew.cmu.edu>
> > Signed-off-by: Borislav Petkov <bp@suse.de>
> 
> Ok, ignore this one for now - the suspend/resume path doesn't pan out
> somehow, need to do some more handholding here.
> 
hmm,
this reminds me that we should do a thermal_zone_device_update() for
each registered thermal zone when resuming from system suspend.

thanks,
rui