Re: [PATCH/RFC] Acer Aspire One fan control resume fix, improvements
From: Peter Feuerer
Date: Mon Jun 15 2009 - 13:16:05 EST
Hi Andreas, Hi Boris,
I just returned from my holiday trip to italy. Thank you very much for your
changes. I'll merge and review them within next 2 days and send a complete
patch against linus-git.
How long is the merge window for 2.6.31 opened? Do you think we will finally
get the driver merged?
kind regards,
--peter
Borislav Petkov writes:
Hi,
On Fri, Jun 12, 2009 at 4:37 PM, Andreas Mohr<andi@xxxxxxxx> wrote:
Hello all,
tried version 0.5.6, didn't (quite!) like it. ;)
Reason being that when suspending, every couple times there was
a fan state check error on resume, leading to the annoying issue of
kernel mode fan control getting disabled (via a safety measure).
ChangeLog:
- reduce maximum temperature check interval, 20 seconds seems a bit much
Â(we have to take into account _external_ heat sources, too!)
- configure BIOS mode during suspend downtime, since our driver code is
Âhaving a day off (and go back to previous setting after resume)
- update fan state variable during resume, to reflect new state after
Âpowering up
- optimization: add bios_settings pointer to current bios's settings,
Âavoids array indirection
- change cmd_off, cmd_auto to fancmd[2] for streamlined code
- several improved log messages
- reverse fan state printing (every value that doesn't indicate "OFF" should
Âdefinitely lead towards an "AUTO" fan setting!!!!)
good catch!
Â[some functional check was unsafe in this respect, too]
- use LONG_MAX instead of open-coded 0x7fffffffl
Suspend tested multiple times (on 2.6.30-rc8), checkpatch.pl:ed.
Version 0.5.7 was internal release only.
This patch is intended for Peter mainly, he should decide what to do with
that content - better don't commit it yet.
Signed-off-by: Andreas Mohr <andi@xxxxxxxx>
--- linux-2.6.30-rc8.acerhdf/drivers/platform/x86/acerhdf.c.patched_orig    Â2009-06-12 10:39:30.000000000 +0200
+++ linux-2.6.30-rc8.acerhdf/drivers/platform/x86/acerhdf.c   2009-06-12 16:10:58.000000000 +0200
@@ -13,6 +13,7 @@
Â* Â Â Â Â Â Â Â - Carlos Corbacho Âcathectic (at) gmail.com
Â* Âo lkml    - Matthew Garrett
Â* Â Â Â Â Â Â Â - Borislav Petkov
+ * Â Â Â Â Â Â Â - Andreas Mohr
Â*
Â* ÂThis program is free software; you can redistribute it and/or modify
Â* Âit under the terms of the GNU General Public License as published by
@@ -52,7 +53,7 @@
Â*/
Â#undef START_IN_KERNEL_MODE
-#define VERSION "0.5.6"
+#define VERSION "0.5.8"
Â/*
Â* According to the Atom N270 datasheet,
@@ -62,8 +63,8 @@
Â* assume 89ÂC is critical temperature.
Â*/
Â#define ACERHDF_TEMP_CRIT 89
-#define ACERHDF_FAN_AUTO 1
Â#define ACERHDF_FAN_OFF 0
+#define ACERHDF_FAN_AUTO 1
Â/*
Â* No matter what value the user puts into the fanon variable, turn on the fan
@@ -72,17 +73,18 @@
Â#define ACERHDF_MAX_FANON 80
Â/*
- * Maximal interval between two temperature checks is 20 seconds, as the die
- * can get hot really fast under heavy load
+ * Maximum interval between two temperature checks is 15 seconds, as the die
+ * can get hot really fast under heavy load (plus we shouldn't forget about
+ * possible impact of _external_ aggressive sources such as heaters, sun etc.)
Â*/
-#define ACERHDF_MAX_INTERVAL 20
+#define ACERHDF_MAX_INTERVAL 15
Â/*
Â* As temperatures can be negative, zero or positive, the value indicating
Â* an error must be somewhere beyond valid temperature values.
- * 0x7fffffffl - highest possible positive long value should do the job.
+ * LONG_MAX (highest possible positive long value) should do the job.
Â*/
-#define ACERHDF_ERROR 0x7fffffffl
+#define ACERHDF_ERROR LONG_MAX
Â#ifdef START_IN_KERNEL_MODE
@@ -97,7 +99,7 @@
Âstatic unsigned int verbose;
Âstatic unsigned int fanstate = ACERHDF_FAN_AUTO;
Âstatic int disable_kernelmode;
-static int bios_version = -1;
+static int pre_suspend_kernelmode;
ok, this variable is unnecessary:
it is enough to do
if (kernelmode)
acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
on suspend and since the machine starts with the BIOS in control of the
fan, the acerhdf_set_cur_state() thermal callback will figure out what
to do so you don't need to explicitly save the kernelmode setting before
suspend and restore it on resume.
Âstatic char force_bios[16];
Âstatic unsigned int prev_interval;
Âstruct thermal_zone_device *acerhdf_thz_dev;
@@ -123,25 +125,26 @@
   Âconst char *version;
   Âunsigned char fanreg;
   Âunsigned char tempreg;
- Â Â Â unsigned char cmd_off;
- Â Â Â unsigned char cmd_auto;
+ Â Â Â unsigned char fancmd[2]; /* fan off and auto commands */
Â};
Â/* Register addresses and values for different BIOS versions */
-static const struct bios_settings_t bios_settings[] = {
- Â Â Â {"Acer", "v0.3109", 0x55, 0x58, 0x1f, 0x00},
- Â Â Â {"Acer", "v0.3114", 0x55, 0x58, 0x1f, 0x00},
- Â Â Â {"Acer", "v0.3301", 0x55, 0x58, 0xaf, 0x00},
- Â Â Â {"Acer", "v0.3304", 0x55, 0x58, 0xaf, 0x00},
- Â Â Â {"Acer", "v0.3305", 0x55, 0x58, 0xaf, 0x00},
- Â Â Â {"Acer", "v0.3308", 0x55, 0x58, 0x21, 0x00},
- Â Â Â {"Acer", "v0.3309", 0x55, 0x58, 0x21, 0x00},
- Â Â Â {"Acer", "v0.3310", 0x55, 0x58, 0x21, 0x00},
- Â Â Â {"Gateway", "v0.3103", 0x55, 0x58, 0x21, 0x00},
- Â Â Â {"Packard Bell", "v0.3105", 0x55, 0x58, 0x21, 0x00},
- Â Â Â {"", 0, 0, 0, 0, 0}
+static const struct bios_settings_t bios_settings_table[] = {
+ Â Â Â {"Acer", "v0.3109", 0x55, 0x58, {0x1f, 0x00} },
+ Â Â Â {"Acer", "v0.3114", 0x55, 0x58, {0x1f, 0x00} },
+ Â Â Â {"Acer", "v0.3301", 0x55, 0x58, {0xaf, 0x00} },
+ Â Â Â {"Acer", "v0.3304", 0x55, 0x58, {0xaf, 0x00} },
+ Â Â Â {"Acer", "v0.3305", 0x55, 0x58, {0xaf, 0x00} },
+ Â Â Â {"Acer", "v0.3308", 0x55, 0x58, {0x21, 0x00} },
+ Â Â Â {"Acer", "v0.3309", 0x55, 0x58, {0x21, 0x00} },
+ Â Â Â {"Acer", "v0.3310", 0x55, 0x58, {0x21, 0x00} },
+ Â Â Â {"Gateway", "v0.3103", 0x55, 0x58, {0x21, 0x00} },
+ Â Â Â {"Packard Bell", "v0.3105", 0x55, 0x58, {0x21, 0x00} },
+ Â Â Â {"", 0, 0, 0, {0, 0} }
Â};
+static const struct bios_settings_t *bios_settings __read_mostly;
+
Â/* acer ec functions */
Âstatic int acerhdf_get_temp(void)
@@ -149,7 +152,7 @@
   Âu8 temp;
   Â/* read temperature */
- Â Â Â if (!ec_read(bios_settings[bios_version].tempreg, &temp)) {
+ Â Â Â if (!ec_read(bios_settings->tempreg, &temp)) {
       Âif (verbose)
           Âpr_notice("temp %d\n", temp);
       Âreturn temp;
@@ -161,8 +164,8 @@
Â{
   Âu8 fan;
- Â Â Â if (!ec_read(bios_settings[bios_version].fanreg, &fan))
- Â Â Â Â Â Â Â return (fan == bios_settings[bios_version].cmd_off) ?
+ Â Â Â if (!ec_read(bios_settings->fanreg, &fan))
+ Â Â Â Â Â Â Â return (fan == bios_settings->fancmd[ACERHDF_FAN_OFF]) ?
           ÂACERHDF_FAN_OFF : ACERHDF_FAN_AUTO;
   Âreturn ACERHDF_ERROR;
@@ -173,19 +176,19 @@
   Âunsigned char cmd;
   Âif (verbose)
- Â Â Â Â Â Â Â pr_notice("fan %s\n", (state == ACERHDF_FAN_AUTO) ?
- Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "ON" : "OFF");
+ Â Â Â Â Â Â Â pr_notice("fan %s\n", (state == ACERHDF_FAN_OFF) ?
+ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "OFF" : "ON");
- Â Â Â if (state == ACERHDF_FAN_AUTO) {
- Â Â Â Â Â Â Â cmd = bios_settings[bios_version].cmd_auto;
- Â Â Â Â Â Â Â fanstate = ACERHDF_FAN_AUTO;
- Â Â Â } else {
- Â Â Â Â Â Â Â cmd = bios_settings[bios_version].cmd_off;
- Â Â Â Â Â Â Â fanstate = ACERHDF_FAN_OFF;
+ Â Â Â if ((state != ACERHDF_FAN_OFF) && (state != ACERHDF_FAN_AUTO)) {
+ Â Â Â Â Â Â Â pr_err("invalid fan state %d requested, setting to auto!\n",
+ Â Â Â Â Â Â Â Â Â Â Â state);
+ Â Â Â Â Â Â Â state = ACERHDF_FAN_AUTO;
   Â}
- Â Â Â ec_write(bios_settings[bios_version].fanreg, cmd);
+ Â Â Â cmd = bios_settings->fancmd[state];
+ Â Â Â fanstate = state;
+ Â Â Â ec_write(bios_settings->fanreg, cmd);
Â}
Â/* helpers */
@@ -253,6 +256,18 @@
   Âreturn 0;
Â}
+/* provide one central function to set disable_kernelmode
+ * (always set ACERHDF_FAN_AUTO, too!) */
+static inline void acerhdf_revert_to_bios_mode(void)
+{
+ Â Â Â acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
+ Â Â Â /*
+ Â Â Â Â* let the thermal layer disable kernel mode. This ensures that
+ Â Â Â Â* the thermal layer doesn't switch off the fan again
+ Â Â Â Â*/
+ Â Â Â disable_kernelmode = 1;
+}
+
Â/* Âcurrent operation mode - enabled / disabled */
Âstatic int acerhdf_get_mode(struct thermal_zone_device *thermal,
       Âenum thermal_device_mode *mode)
@@ -279,13 +294,8 @@
       Âpr_notice("kernel mode OFF\n");
       Âthermal->polling_delay = 0;
       Âthermal_zone_device_update(thermal);
- Â Â Â Â Â Â Â acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
- Â Â Â Â Â Â Â /*
- Â Â Â Â Â Â Â Â* let the thermal layer disable kernel mode. This ensures that
- Â Â Â Â Â Â Â Â* the thermal layer doesn't switch off the fan again
- Â Â Â Â Â Â Â Â*/
- Â Â Â Â Â Â Â disable_kernelmode = 1;
+ Â Â Â Â Â Â Â acerhdf_revert_to_bios_mode();
   Â} else {
       Âkernelmode = 1;
       Âpr_notice("kernel mode ON\n");
@@ -394,21 +404,19 @@
    */
   Âif (cur_state != fanstate) {
       Âpr_err("failed reading fan state, "
- Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "disabling kernelmode.\n");
+ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "falling back to default BIOS handling.\n");
       Âpr_err("read state: %d expected state: %d\n",
               Âcur_state, fanstate);
- Â Â Â Â Â Â Â acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
- Â Â Â Â Â Â Â disable_kernelmode = 1;
+ Â Â Â Â Â Â Â acerhdf_revert_to_bios_mode();
       Âreturn -EINVAL;
   Â}
   Â/* same with temperature */
   Âif (cur_temp == ACERHDF_ERROR) {
       Âpr_err("failed reading temperature, "
- Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "disabling kernelmode.\n");
+ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "falling back to default BIOS handling.\n");
- Â Â Â Â Â Â Â acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
- Â Â Â Â Â Â Â disable_kernelmode = 1;
+ Â Â Â Â Â Â Â acerhdf_revert_to_bios_mode();
       Âreturn -EINVAL;
   Â}
@@ -437,11 +445,21 @@
       Âpm_message_t state)
Â{
   Â/*
- Â Â Â Â* in kernelmode turn on fan, because the aspire one awakes with
- Â Â Â Â* spinning fan
+ Â Â Â Â* always revert to BIOS mode during suspend/resume activities:
+ Â Â Â Â* a) during suspend our driver is inactive, thus if there's
+ Â Â Â Â* Â Âanything to be done fan-wise, it's the BIOS's job...
+ Â Â Â Â* b) Aspire awakes with spinning fan in BIOS mode,
+ Â Â Â Â* Â Âthus we better do the same (behaviour is preserved if we use BIOS)
    */
- Â Â Â if (kernelmode)
+
+ Â Â Â /* remember previous setting */
+ Â Â Â pre_suspend_kernelmode = kernelmode;
+
+ Â Â Â if (kernelmode) {
       Âacerhdf_change_fanstate(ACERHDF_FAN_AUTO);
+ Â Â Â Â Â Â Â kernelmode = 0;
+ Â Â Â }
+
   Âif (verbose)
       Âpr_notice("going suspend\n");
   Âreturn 0;
@@ -449,6 +467,14 @@
Âstatic int acerhdf_resume(struct platform_device *device)
Â{
+ Â Â Â kernelmode = pre_suspend_kernelmode;
+
+ Â Â Â /* update our fanstate variable to the possibly different
+ Â Â Â Â* post-resume fan state
+ Â Â Â Â* (to prevent a safety check from failing)
+ Â Â Â Â*/
please use kernel-style comments:
/*
* comment text goes here
*/
+ Â Â Â fanstate = acerhdf_get_fanstate();
+
   Âif (verbose)
       Âpr_notice("resuming\n");
   Âreturn 0;
@@ -526,15 +552,16 @@
   Â/* search BIOS version and BIOS vendor in BIOS settings table */
- Â Â Â for (i = 0; bios_settings[i].version[0]; ++i) {
- Â Â Â Â Â Â Â if (!strcmp(bios_settings[i].vendor, vendor) &&
- Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â !strcmp(bios_settings[i].version, version)) {
- Â Â Â Â Â Â Â Â Â Â Â bios_version = i;
+ Â Â Â for (i = 0; bios_settings_table[i].version[0]; ++i) {
+ Â Â Â Â Â Â Â if (!strcmp(bios_settings_table[i].vendor, vendor) &&
+ Â Â Â Â Â Â Â Â Â !strcmp(bios_settings_table[i].version, version)) {
+ Â Â Â Â Â Â Â Â Â Â Â bios_settings = &bios_settings_table[i];
           Âbreak;
       Â}
   Â}
- Â Â Â if (bios_version == -1) {
- Â Â Â Â Â Â Â pr_err("cannot find BIOS version\n");
+ Â Â Â if (!bios_settings) {
+ Â Â Â Â Â Â Â pr_err("unknown (unsupported) BIOS version %s/%s, "
+ Â Â Â Â Â Â Â Â Â Â Â "please report, aborting!\n", vendor, version);
       Âreturn ACERHDF_ERROR;
   Â}
   Âreturn 0;
--
Regards/Gruss,
Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/