lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 17 Feb 2013 16:41:57 +0100 From: Peter Feuerer <peter@...e.net> To: Borislav Petkov <bp@...en8.de> Cc: Alexander Lam <azl@...rew.cmu.edu>, linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>, Andreas Mohr <andi@...as.de> Subject: Re: thermal governor: does it actually work?? 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@...e.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; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists