| 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: <alpine.LFD.2.00.0811120002420.3828@localhost.localdomain>
Date: Wed, 12 Nov 2008 00:02:47 -0500 (EST)
From: Len Brown <lenb@...nel.org>
To: Henrique de Moraes Holschuh <hmh@....eng.br>
Cc: "Rafael J. Wysocki" <rjw@...k.pl>,
ibm-acpi-devel@...ts.sourceforge.net, linux-acpi@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ACPI: thinkpad-acpi: fix fan sleep/resume path
applied.
thanks,
-Len
On Sun, 9 Nov 2008, Henrique de Moraes Holschuh wrote:
> This fixes a regression from v2.6.27, caused by commit
> 5814f737e1cd2cfa2893badd62189acae3e1e1fd, "ACPI: thinkpad-acpi:
> attempt to preserve fan state on resume".
>
> It is possible for fan_suspend() to fail to properly initialize
> fan_control_desired_level as required by fan_resume(), resulting on
> the fan always being set to level 7 on resume if the user didn't
> touch the fan controller.
>
> In order to get fan sleep/resume handling to work right:
>
> 1. Fix the fan_suspend handling of the T43 firmware quirk. If it is
> still undefined, we didn't touch the fan yet and that means we have no
> business doing it on resume.
>
> 2. Store the fan level on its own variable to avoid any possible
> issues with hijacking fan_control_desired_level (which isn't supposed
> to have anything other than 0-7 in it, anyway).
>
> 3. Change the fan_resume code to me more straightforward to understand
> (although we DO optimize the boolean logic there, otherwise it looks
> disgusting).
>
> 4. Add comments to help understand what the code is supposed to be
> doing.
>
> 5. Change fan_set_level to be less strict about how auto and
> full-speed modes are requested.
>
> Signed-off-by: Henrique de Moraes Holschuh <hmh@....eng.br>
> Reported-by: Tino Keitel <tino.keitel@...ei.de>
> ---
> drivers/misc/thinkpad_acpi.c | 57 +++++++++++++++++++++++++++++++++---------
> 1 files changed, 45 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
> index 4db1cf9..b7e4d47 100644
> --- a/drivers/misc/thinkpad_acpi.c
> +++ b/drivers/misc/thinkpad_acpi.c
> @@ -5309,6 +5309,7 @@ static enum fan_control_commands fan_control_commands;
>
> static u8 fan_control_initial_status;
> static u8 fan_control_desired_level;
> +static u8 fan_control_resume_level;
> static int fan_watchdog_maxinterval;
>
> static struct mutex fan_mutex;
> @@ -5431,8 +5432,8 @@ static int fan_set_level(int level)
>
> case TPACPI_FAN_WR_ACPI_FANS:
> case TPACPI_FAN_WR_TPEC:
> - if ((level != TP_EC_FAN_AUTO) &&
> - (level != TP_EC_FAN_FULLSPEED) &&
> + if (!(level & TP_EC_FAN_AUTO) &&
> + !(level & TP_EC_FAN_FULLSPEED) &&
> ((level < 0) || (level > 7)))
> return -EINVAL;
>
> @@ -5996,38 +5997,67 @@ static void fan_exit(void)
>
> static void fan_suspend(pm_message_t state)
> {
> + int rc;
> +
> if (!fan_control_allowed)
> return;
>
> /* Store fan status in cache */
> - fan_get_status_safe(NULL);
> + fan_control_resume_level = 0;
> + rc = fan_get_status_safe(&fan_control_resume_level);
> + if (rc < 0)
> + printk(TPACPI_NOTICE
> + "failed to read fan level for later "
> + "restore during resume: %d\n", rc);
> +
> + /* if it is undefined, don't attempt to restore it.
> + * KEEP THIS LAST */
> if (tp_features.fan_ctrl_status_undef)
> - fan_control_desired_level = TP_EC_FAN_AUTO;
> + fan_control_resume_level = 0;
> }
>
> static void fan_resume(void)
> {
> - u8 saved_fan_level;
> u8 current_level = 7;
> bool do_set = false;
> + int rc;
>
> /* DSDT *always* updates status on resume */
> tp_features.fan_ctrl_status_undef = 0;
>
> - saved_fan_level = fan_control_desired_level;
> if (!fan_control_allowed ||
> + !fan_control_resume_level ||
> (fan_get_status_safe(¤t_level) < 0))
> return;
>
> switch (fan_control_access_mode) {
> case TPACPI_FAN_WR_ACPI_SFAN:
> - do_set = (saved_fan_level > current_level);
> + /* never decrease fan level */
> + do_set = (fan_control_resume_level > current_level);
> break;
> case TPACPI_FAN_WR_ACPI_FANS:
> case TPACPI_FAN_WR_TPEC:
> - do_set = ((saved_fan_level & TP_EC_FAN_FULLSPEED) ||
> - (saved_fan_level == 7 &&
> - !(current_level & TP_EC_FAN_FULLSPEED)));
> + /* never decrease fan level, scale is:
> + * TP_EC_FAN_FULLSPEED > 7 >= TP_EC_FAN_AUTO
> + *
> + * We expect the firmware to set either 7 or AUTO, but we
> + * handle FULLSPEED out of paranoia.
> + *
> + * So, we can safely only restore FULLSPEED or 7, anything
> + * else could slow the fan. Restoring AUTO is useless, at
> + * best that's exactly what the DSDT already set (it is the
> + * slower it uses).
> + *
> + * Always keep in mind that the DSDT *will* have set the
> + * fans to what the vendor supposes is the best level. We
> + * muck with it only to speed the fan up.
> + */
> + if (fan_control_resume_level != 7 &&
> + !(fan_control_resume_level & TP_EC_FAN_FULLSPEED))
> + return;
> + else
> + do_set = !(current_level & TP_EC_FAN_FULLSPEED) &&
> + (current_level != fan_control_resume_level);
> break;
> default:
> return;
> @@ -6035,8 +6065,11 @@ static void fan_resume(void)
> if (do_set) {
> printk(TPACPI_NOTICE
> "restoring fan level to 0x%02x\n",
> - saved_fan_level);
> - fan_set_level_safe(saved_fan_level);
> + fan_control_resume_level);
> + rc = fan_set_level_safe(fan_control_resume_level);
> + if (rc < 0)
> + printk(TPACPI_NOTICE
> + "failed to restore fan level: %d\n", rc);
> }
> }
>
> --
> 1.5.6.5
>
--
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