| 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
| ||
|
Message-ID: <cone.1245086149.988051.6046.1000@deskpiie>
Date: Mon, 15 Jun 2009 19:15:49 +0200
From: Peter Feuerer <peter@...e.net>
To: Borislav Petkov <petkovbb@...glemail.com>
Cc: Andreas Mohr <andi@...as.de>,
Andrew Morton <akpm@...ux-foundation.org>,
Len Brown <len.brown@...el.com>,
Matthew Garrett <mjg59@...f.ucam.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH/RFC] Acer Aspire One fan control resume fix,
improvements
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@...as.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@...as.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@...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