[GIT PATCH] thinkpad-acpi regression fix for 2.6.28-rc

[GIT PATCH] thinkpad-acpi regression fix for 2.6.28-rc

[Henrique de Moraes Holschuh]

This patch should fix a reported regression on fan_resume (not
on bugzilla, report on the LKML and ibm-acpi-devel).

Tino, Rafael, please test and reply with a tested-by line.  I have
tested it here, but my T43 has a fan quirk that might get in the
way...

Len, if we get reports that this fixes the regression, it would be
really nice if you could send it to Linus for 2.6.28-rc.  It is a
rather annoying regression.  Otherwise, a revert of the commit who
broke things (listed in the patch commit log) is also acceptable.

Thank you.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

[PATCH] ACPI: thinkpad-acpi: fix fan sleep/resume path

[Henrique de Moraes Holschuh]

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@hmh.eng.br>
Reported-by: Tino Keitel <tino.keitel@tikei.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(&current_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

Re: [ibm-acpi-devel] [GIT PATCH] thinkpad-acpi regression fix for 2.6.28-rc

[Henrique de Moraes Holschuh]

On Sun, 09 Nov 2008, Henrique de Moraes Holschuh wrote:
> This patch should fix a reported regression on fan_resume (not
> on bugzilla, report on the LKML and ibm-acpi-devel).

Spoke too soon.  Bugzilla #11982.  I've sent the patch there as well.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] ACPI: thinkpad-acpi: fix fan sleep/resume path

[Len Brown]

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-N3TV7GIv+o9fyO9Q7EP/yw@public.gmane.org>
> Reported-by: Tino Keitel <tino.keitel-rAwCM5oiXHA@public.gmane.org>
> ---
>  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(&current_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
> 

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] ACPI: thinkpad-acpi: fix fan sleep/resume path

[Henrique de Moraes Holschuh]

Len,

The patch needs to go to Linus...  I don't think anyone cares to have
their names as tested-by:, or they would have replied already.

I have tested the patch and it fixes things here.  It would be better with
more "works for me" replies, but it should be enough testing.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [ibm-acpi-devel] [PATCH] ACPI: thinkpad-acpi: fix fan sleep/resume path

[Tino Keitel]

On Mon, Nov 17, 2008 at 00:14:08 -0200, Henrique de Moraes Holschuh wrote:
> Len,
> 
> The patch needs to go to Linus...  I don't think anyone cares to have
> their names as tested-by:, or they would have replied already.
> 
> I have tested the patch and it fixes things here.  It would be better with
> more "works for me" replies, but it should be enough testing.

Hi,

sorry for the delay. I tested the above patch and the "auto" fan level
was preserved (or better: not touched) at resume, so this regression is
fixed.

Regards,
Tino