lkml.org 
[lkml]   [2009]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH/RFC] Acer Aspire One fan control resume fix, improvements
From
Hi,

On Fri, Jun 12, 2009 at 4:37 PM, Andreas Mohr<andi@lisas.de> 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@lisas.de>
>
> --- 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2009-06-12 17:41    [W:0.106 / U:0.740 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site