Fan level 7 after resume wit 2.6.28-rc3

Fan level 7 after resume wit 2.6.28-rc3

[Tino Keitel]

Hi,

I tried 2.6.28-rc3 (git a75952b) on my ThinkPad X61s, and now the fan
runs at full speed after resume from suspend:

$ cat /proc/acpi/ibm/fan
status:		enabled
speed:		0
level:		auto
commands:	level <level> (<level> is 0-7, auto, disengaged,
full-speed)
commands:	enable, disable
commands:	watchdog <timeout> (<timeout> is 0 (off), 1-120
(seconds))
$ echo mem > /sys/power/state

After resume:

$ cat /proc/acpi/ibm/fan
status:		enabled
speed:		4796
level:		7
commands:	level <level> (<level> is 0-7, auto, disengaged,
full-speed)
commands:	enable, disable
commands:	watchdog <timeout> (<timeout> is 0 (off), 1-120
(seconds))

Setting it back again to "auto" works. After the next suspend cycle, it
is back to level 7.

This doesn't happen with 2.6.27.

Regards,
Tino

Re: [ibm-acpi-devel] Fan level 7 after resume wit 2.6.28-rc3

[Tino Keitel]

On Wed, Nov 05, 2008 at 08:33:08 +0100, Tino Keitel wrote:
> Hi,
> 
> I tried 2.6.28-rc3 (git a75952b) on my ThinkPad X61s, and now the fan
> runs at full speed after resume from suspend:

I forgot to look into the kernel logs for more info. Well, I found this
line:

thinkpad_acpi: restoring fan level to 0x07

So I guess that according to

ACPI: thinkpad-acpi: attempt to preserve fan state on resume

the attempt failed. :-)

Regards,
Tino

Re: [ibm-acpi-devel] Fan level 7 after resume wit 2.6.28-rc3

[Henrique de Moraes Holschuh]

On Wed, 05 Nov 2008, Tino Keitel wrote:
> On Wed, Nov 05, 2008 at 08:33:08 +0100, Tino Keitel wrote:
> > I tried 2.6.28-rc3 (git a75952b) on my ThinkPad X61s, and now the fan
> > runs at full speed after resume from suspend:
> 
> I forgot to look into the kernel logs for more info. Well, I found this
> line:
> 
> thinkpad_acpi: restoring fan level to 0x07
> 
> So I guess that according to
> 
> ACPI: thinkpad-acpi: attempt to preserve fan state on resume
> 
> the attempt failed. :-)

Indeed.  Please send the full kernel output of thinkpad-acpi, showing boot,
suspend and resume messages from the driver, please?

Also, please tell me what is the contents of /proc/acpi/ibm/fan (and if
possible, the contents of /proc/acpi/ibm/ecdump) before you issue the
suspend command.

-- 
  "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] Fan level 7 after resume wit 2.6.28-rc3

[Tino Keitel]

On Wed, Nov 05, 2008 at 10:26:32 -0200, Henrique de Moraes Holschuh wrote:
> On Wed, 05 Nov 2008, Tino Keitel wrote:
> > On Wed, Nov 05, 2008 at 08:33:08 +0100, Tino Keitel wrote:
> > > I tried 2.6.28-rc3 (git a75952b) on my ThinkPad X61s, and now the fan
> > > runs at full speed after resume from suspend:
> > 
> > I forgot to look into the kernel logs for more info. Well, I found this
> > line:
> > 
> > thinkpad_acpi: restoring fan level to 0x07
> > 
> > So I guess that according to
> > 
> > ACPI: thinkpad-acpi: attempt to preserve fan state on resume
> > 
> > the attempt failed. :-)
> 
> Indeed.  Please send the full kernel output of thinkpad-acpi, showing boot,
> suspend and resume messages from the driver, please?



> Also, please tell me what is the contents of /proc/acpi/ibm/fan (and if

See the first mail. I don't touch any fan settings, everything is at
the default value.

Re: [ibm-acpi-devel] Fan level 7 after resume wit 2.6.28-rc3

[Tino Keitel]

On Wed, Nov 05, 2008 at 10:26:32 -0200, Henrique de Moraes Holschuh wrote:
> On Wed, 05 Nov 2008, Tino Keitel wrote:
> > On Wed, Nov 05, 2008 at 08:33:08 +0100, Tino Keitel wrote:
> > > I tried 2.6.28-rc3 (git a75952b) on my ThinkPad X61s, and now the fan
> > > runs at full speed after resume from suspend:
> > 
> > I forgot to look into the kernel logs for more info. Well, I found this
> > line:
> > 
> > thinkpad_acpi: restoring fan level to 0x07
> > 
> > So I guess that according to
> > 
> > ACPI: thinkpad-acpi: attempt to preserve fan state on resume
> > 
> > the attempt failed. :-)
> 
> Indeed.  Please send the full kernel output of thinkpad-acpi, showing boot,
> suspend and resume messages from the driver, please?

See http://tikei.de/kernel.log

> Also, please tell me what is the contents of /proc/acpi/ibm/fan (and if

See the first mail. I don't touch any fan settings, everything is at
the default value.

> possible, the contents of /proc/acpi/ibm/ecdump) before you issue the
> suspend command.

See http://tikei.de/ecdump (however, this was taken with 2.6.27.4).

Regards,
Tino

Re: [ibm-acpi-devel] Fan level 7 after resume wit 2.6.28-rc3

[Henrique de Moraes Holschuh]

On Wed, 05 Nov 2008, Tino Keitel wrote:
> On Wed, Nov 05, 2008 at 10:26:32 -0200, Henrique de Moraes Holschuh wrote:
> > On Wed, 05 Nov 2008, Tino Keitel wrote:
> > > On Wed, Nov 05, 2008 at 08:33:08 +0100, Tino Keitel wrote:
> > > > I tried 2.6.28-rc3 (git a75952b) on my ThinkPad X61s, and now the fan
> > > > runs at full speed after resume from suspend:
> > > 
> > > I forgot to look into the kernel logs for more info. Well, I found this
> > > line:
> > > 
> > > thinkpad_acpi: restoring fan level to 0x07
> > > 
> > > So I guess that according to
> > > 
> > > ACPI: thinkpad-acpi: attempt to preserve fan state on resume
> > > 
> > > the attempt failed. :-)
> > 
> > Indeed.  Please send the full kernel output of thinkpad-acpi, showing boot,
> > suspend and resume messages from the driver, please?
> 
> See http://tikei.de/kernel.log
> 
> > Also, please tell me what is the contents of /proc/acpi/ibm/fan (and if
> 
> See the first mail. I don't touch any fan settings, everything is at
> the default value.
> 
> > possible, the contents of /proc/acpi/ibm/ecdump) before you issue the
> > suspend command.
> 
> See http://tikei.de/ecdump (however, this was taken with 2.6.27.4).

It should not have set the fan to level 7, so it means we will need to add
debugging hints to the driver.  I don't have my T43 with me right now, or I
could try to reproduce the issue.

Are you confortable with adding some printk's to kernel source?  If so, try
to printk the interesting bits in fan_suspend and fan_resume on
drivers/misc/thinkpad_acpi.c.

Otherwise, I can provide you with a patch with the debug printks later.  But
either way, you'd need to recompile your kernel to get the changed
thinkpad-acpi module to test.  I hope that is not a problem?

-- 
  "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] Fan level 7 after resume wit 2.6.28-rc3

[Tino Keitel]

[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]

On Wed, Nov 05, 2008 at 14:24:01 -0200, Henrique de Moraes Holschuh wrote:

[...]

> It should not have set the fan to level 7, so it means we will need to add
> debugging hints to the driver.  I don't have my T43 with me right now, or I
> could try to reproduce the issue.
> 
> Are you confortable with adding some printk's to kernel source?  If so, try
> to printk the interesting bits in fan_suspend and fan_resume on
> drivers/misc/thinkpad_acpi.c.

In fan_init(), fan_control_desired_level is set to 7, and never changed
to anything else. It is not set in fan_suspend() because
tp_features.fan_ctrl_status_undef is 0 (also set in fan_init()).

In fan_resume(), saved_fan_level is taken from
fan_control_desired_level. current_level is read and set to
TP_EC_FAN_AUTO.

Now the following check is true:

saved_fan_level == 7 && !(current_level & TP_EC_FAN_FULLSPEED))

So do_set is set to true and the fan speed is set to 7 at resume.

I tried to write a correct patch, but I got lost in all that
fan_control_desired_level, fan_control_initial_status and
tp_features.fan_ctrl_status_undef stuff.

My brain says that one would just read the current fan settings from
the EC at initialization. Then, after resume, this setting is restored
unconditionally, or at least if it differs from current_level. The
attached patch works for me. However, I don't have all the knowledge
about older models and their specific behaviour.

Correction: I just tested a bit further, and it doesn't work. If I set
fan level to 3, suspend, resume, set fan level to auto, and
resume/suspend again, fan level is restored to 3. This is because
fan_control_desired_level isn't updated by fan_update_desired_level()
if it is set back to auto, but kept at the old value. So, my impression
is that all the values and states should be cleaned up a bit and
simplified. In the current state, there are a lot of strage checks and
quirks that have side effects when other parts are changed.

Regards,
Tino

[-- Attachment #2: thinkpad-acpi_fan-level-restore-fix.diff --]
[-- Type: text/x-diff, Size: 859 bytes --]

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 4db1cf9..fa2b8df 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -5886,6 +5886,7 @@ static int __init fan_init(struct ibm_init_struct *iibm)
 		if (likely(acpi_ec_read(fan_status_offset,
 					&fan_control_initial_status))) {
 			fan_status_access_mode = TPACPI_FAN_RD_TPEC;
+			fan_control_desired_level = fan_control_initial_status;
 
 			/* In some ThinkPads, neither the EC nor the ACPI
 			 * DSDT initialize the fan status, and it ends up
@@ -6025,9 +6026,7 @@ static void fan_resume(void)
 		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)));
+		do_set = 1;
 		break;
 	default:
 		return;

Re: [ibm-acpi-devel] Fan level 7 after resume wit 2.6.28-rc3

[Tino Keitel]

[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]

On Thu, Nov 06, 2008 at 01:35:44 +0100, Tino Keitel wrote:

[...]

> I tried to write a correct patch, but I got lost in all that
> fan_control_desired_level, fan_control_initial_status and
> tp_features.fan_ctrl_status_undef stuff.
> 
> My brain says that one would just read the current fan settings from
> the EC at initialization. Then, after resume, this setting is restored
> unconditionally, or at least if it differs from current_level. The
> attached patch works for me. However, I don't have all the knowledge
> about older models and their specific behaviour.
> 
> Correction: I just tested a bit further, and it doesn't work. If I set
> fan level to 3, suspend, resume, set fan level to auto, and
> resume/suspend again, fan level is restored to 3. This is because
> fan_control_desired_level isn't updated by fan_update_desired_level()
> if it is set back to auto, but kept at the old value. So, my impression
> is that all the values and states should be cleaned up a bit and
> simplified. In the current state, there are a lot of strage checks and
> quirks that have side effects when other parts are changed.

The whole fan level stuff looks a bit complicated to me. Especially the
fan_control_desired_level handling is somethat strange because it
considers values as invalid that are written by fan_set_level() before.
It ignores the TP_EC_FAN_FULLSPEED and TP_EC_FAN_AUTO values. Now, in
fan_resume(), this value is checked against TP_EC_FAN_FULLSPEED which
makes no sense to me.

The attached patch tries to simplify this a bit. It sets
fan_control_desired_level to the current EC value in fan_init(), and
also tries to keep fan_control_desired_level and the real EC value in
sync. I also removed the checks against 7 and TP_EC_FAN_FULLSPEED in
fan_resume() as they made no sense to me. This works as intended on my
X61s after rmmod/insmod and suspend/resume.

Regards,
Tino

[-- Attachment #2: thinkpad-acpi_fan-level-restore-fix_v2.diff --]
[-- Type: text/x-diff, Size: 1366 bytes --]

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 4db1cf9..7f095c4 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -5329,13 +5329,8 @@ TPACPI_HANDLE(sfan, ec, "SFAN",	/* 570 */
  */
 static void fan_update_desired_level(u8 status)
 {
-	if ((status &
-	     (TP_EC_FAN_AUTO | TP_EC_FAN_FULLSPEED)) == 0) {
-		if (status > 7)
-			fan_control_desired_level = 7;
-		else
-			fan_control_desired_level = status;
-	}
+	fan_control_desired_level = status &
+		(TP_EC_FAN_AUTO | TP_EC_FAN_FULLSPEED | 7);
 }
 
 static int fan_get_status(u8 *status)
@@ -5886,6 +5881,7 @@ static int __init fan_init(struct ibm_init_struct *iibm)
 		if (likely(acpi_ec_read(fan_status_offset,
 					&fan_control_initial_status))) {
 			fan_status_access_mode = TPACPI_FAN_RD_TPEC;
+			fan_update_desired_level(fan_control_initial_status);
 
 			/* In some ThinkPads, neither the EC nor the ACPI
 			 * DSDT initialize the fan status, and it ends up
@@ -6025,9 +6021,8 @@ static void fan_resume(void)
 		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)));
+		do_set = (current_level != saved_fan_level) && 
+			!(current_level & TP_EC_FAN_FULLSPEED);
 		break;
 	default:
 		return;

Re: [ibm-acpi-devel] Fan level 7 after resume wit 2.6.28-rc3

[Henrique de Moraes Holschuh]

On Thu, 06 Nov 2008, Tino Keitel wrote:
> The whole fan level stuff looks a bit complicated to me. Especially the

It is.  The sysfs hwmon interface does not map 1:1 with the three possible
thinkpad fan control interfaces.  fan_control_desired_level is used for that
interface mainly, since I have to remember the last state used that was in
the set of 0-7, ignoring AUTO (which is a separate pwm control mode for
hwmon) and full-speed (which is a separate pwm control mode for hwmon).

It is hijacked by fan_suspend/fan_resume to store state between
sleep/resume, because that was convenient.  Too bad I failed to notice it
would not work properly for that.

> The attached patch tries to simplify this a bit. It sets

NAK, it would break a lot of stuff.   See my previous reply on this thread
for *some* of the stuff it would break.

I will have a proper patch out probably within 24h but most certainly before
next Monday.

Meanwhile, I suggest you just remove the calls to fan_suspend and fan_resume
as a workaround.

-- 
  "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] Fan level 7 after resume wit 2.6.28-rc3

[Rafael J. Wysocki]

On Thursday, 6 of November 2008, Henrique de Moraes Holschuh wrote:
> On Thu, 06 Nov 2008, Tino Keitel wrote:
> > The whole fan level stuff looks a bit complicated to me. Especially the
> 
> It is.  The sysfs hwmon interface does not map 1:1 with the three possible
> thinkpad fan control interfaces.  fan_control_desired_level is used for that
> interface mainly, since I have to remember the last state used that was in
> the set of 0-7, ignoring AUTO (which is a separate pwm control mode for
> hwmon) and full-speed (which is a separate pwm control mode for hwmon).
> 
> It is hijacked by fan_suspend/fan_resume to store state between
> sleep/resume, because that was convenient.  Too bad I failed to notice it
> would not work properly for that.
> 
> > The attached patch tries to simplify this a bit. It sets
> 
> NAK, it would break a lot of stuff.   See my previous reply on this thread
> for *some* of the stuff it would break.
> 
> I will have a proper patch out probably within 24h but most certainly before
> next Monday.
> 
> Meanwhile, I suggest you just remove the calls to fan_suspend and fan_resume
> as a workaround.

Speaking of which, last time I looked at fan_suspend and fan_resume, they
were hopelessly broken (I admit that was quite some time ago, though).

IMO, fan_suspend() is not necessary at all and the only thing fan_resume()
could do is to make the kernel's data structures reflect the actual state of
the fan.

Thanks,
Rafael

Re: [ibm-acpi-devel] Fan level 7 after resume wit 2.6.28-rc3

[Henrique de Moraes Holschuh]

On Sat, 08 Nov 2008, Rafael J. Wysocki wrote:
> > Meanwhile, I suggest you just remove the calls to fan_suspend and fan_resume
> > as a workaround.
> 
> Speaking of which, last time I looked at fan_suspend and fan_resume, they
> were hopelessly broken (I admit that was quite some time ago, though).

And they still were. I have the patch fixing it, and it reworked that path
entirely.  Will send it soon.

> IMO, fan_suspend() is not necessary at all and the only thing fan_resume()
> could do is to make the kernel's data structures reflect the actual state of
> the fan.

There are NO kernel data structures to reflect the actual state of the fan.
The fan BELONGS to the EC in the thinkpad.  We don't store any crap about it
in the driver, except to -track- one quirk.  We query the EC for all the
required data (which happens to be a single byte) every time we need the
info.  If the user asks us to do something, we send that to the EC and then
promptly forget about it.

So, I don't have to restore anything for things to just work.  The "feature"
was added because people who set the fan to something different than AUTO
wanted it to retain the state they set across sleep, which the box won't do
by itself (the DSDT resets the fan on the WAK path).

OTOH, if I want to restore anything across sleep/resume, I have to store the
state on sleep.

But there is no way I am slowing down the fan on resume: it could be in
emergency mode due to thermal conditions.

-- 
  "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

[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

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

[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: [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@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: [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

Re: [ibm-acpi-devel] Fan level 7 after resume wit 2.6.28-rc3

[Pavel Machek]

On Sun 2008-11-09 09:30:11, Henrique de Moraes Holschuh wrote:
> On Sat, 08 Nov 2008, Rafael J. Wysocki wrote:
> > > Meanwhile, I suggest you just remove the calls to fan_suspend and fan_resume
> > > as a workaround.
> > 
> > Speaking of which, last time I looked at fan_suspend and fan_resume, they
> > were hopelessly broken (I admit that was quite some time ago, though).
> 
> And they still were. I have the patch fixing it, and it reworked that path
> entirely.  Will send it soon.
> 
> > IMO, fan_suspend() is not necessary at all and the only thing fan_resume()
> > could do is to make the kernel's data structures reflect the actual state of
> > the fan.
> 
> There are NO kernel data structures to reflect the actual state of the fan.
> The fan BELONGS to the EC in the thinkpad.  We don't store any crap about it
> in the driver, except to -track- one quirk.  We query the EC for all the
> required data (which happens to be a single byte) every time we need the
> info.  If the user asks us to do something, we send that to the EC and then
> promptly forget about it.
> 
> So, I don't have to restore anything for things to just work.  The "feature"
> was added because people who set the fan to something different than AUTO
> wanted it to retain the state they set across sleep, which the box won't do
> by itself (the DSDT resets the fan on the WAK path).
> 
> OTOH, if I want to restore anything across sleep/resume, I have to store the
> state on sleep.
> 
> But there is no way I am slowing down the fan on resume: it could be in
> emergency mode due to thermal conditions.

Well... one part  of me says that if user explicitely asked for
overheat he should get that :-). 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [ibm-acpi-devel] Fan level 7 after resume wit 2.6.28-rc3

[Henrique de Moraes Holschuh]

On Thu, 06 Nov 2008, Tino Keitel wrote:
> On Wed, Nov 05, 2008 at 14:24:01 -0200, Henrique de Moraes Holschuh wrote:
> In fan_init(), fan_control_desired_level is set to 7, and never changed
> to anything else. It is not set in fan_suspend() because
> tp_features.fan_ctrl_status_undef is 0 (also set in fan_init()).

fan_suspend calls fan_get_status_safe(NULL).

fan_get_status_safe(NULL) will update fan_control_desired_level if it can
get the fan status without an error... unless it is set to AUTO or FULL
SPEED, and it is VERY likely to be set to AUTO.

So it is borked, indeed.  That explains why it is not being initialized to
something else than 7.  Weird that I didn't notice it here when testing,
must have confused it with the effects of the T43 firmware bug.

> I tried to write a correct patch, but I got lost in all that
> fan_control_desired_level, fan_control_initial_status and
> tp_features.fan_ctrl_status_undef stuff.

Ignore fan_ctrl_status_undef, it is a workaround for a firmware bug you
don't have (it is specific to the T43 ACPI DSDT).  Basically, you cannot
trust a fan_get_status() return of level 7 while fan_ctrl_status_undef is
active, it could be AUTO instead.

> My brain says that one would just read the current fan settings from
> the EC at initialization. Then, after resume, this setting is restored
> unconditionally, or at least if it differs from current_level. The

Never.  I will sooner remove the keep settings across suspend/resume than
touch the fan controller unconditionally.  The ACPI DSDT or the EC itself
may have changed the fan mode while we were sleeping, and for a good reason
(like a thermal emergency).

The fan must NEVER be slowed down by the fan_resume() method.  This pretty
much means it can only restore the fan to full-speed, auto or level 7 on a
new-style fan control like the one found on your thinkpad.  And that it
needs to query the state first, must do nothing if it cannot read the state,
and must set it to the higher of (saved, current) using this rule:

fulll-speed > 7 > AUTO.

> Correction: I just tested a bit further, and it doesn't work. If I set
> fan level to 3, suspend, resume, set fan level to auto, and
> resume/suspend again, fan level is restored to 3. This is because
> fan_control_desired_level isn't updated by fan_update_desired_level()

Fan level should NEVER be restored to 3, it should end up set to auto,
full-speed, or 7 when the box finishes resuming.  If it can be restored to
3, something is hideously broken.

Maybe something in the hwmon class is also trying to keep values across
sleep/suspend?

Could you instrument fan_set_level() and check what levels it is being
called with, and when?

-- 
  "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] Fan level 7 after resume wit 2.6.28-rc3

[Tino Keitel]

On Thu, Nov 06, 2008 at 12:11:21 -0200, Henrique de Moraes Holschuh wrote:

[...]

> fulll-speed > 7 > AUTO.
> 
> > Correction: I just tested a bit further, and it doesn't work. If I set
> > fan level to 3, suspend, resume, set fan level to auto, and
> > resume/suspend again, fan level is restored to 3. This is because
> > fan_control_desired_level isn't updated by fan_update_desired_level()
> 
> Fan level should NEVER be restored to 3, it should end up set to auto,
> full-speed, or 7 when the box finishes resuming.  If it can be restored to
> 3, something is hideously broken.

Then it seems I misunderstood the whole fan state resume stuff. I
thought that it restores the value that was set before suspend, unless
it was set to some emergency value meanwhile. I think I should not look
into this further, as there are too many restrictions and/or side
effects involved. However, I think it's a good idea to document all
that in the source code in the future, so that other people can really
understand this. I think it's also a good idea to collect all the
quirks, safety nets and fallbacks in a central place, so that the
context is clear.


> Maybe something in the hwmon class is also trying to keep values across
> sleep/suspend?

This happened only with my first (broken) patch, so no need to bother
about this.

Regards,
Tino

Re: [ibm-acpi-devel] Fan level 7 after resume wit 2.6.28-rc3

[Henrique de Moraes Holschuh]

On Thu, 06 Nov 2008, Tino Keitel wrote:
> Then it seems I misunderstood the whole fan state resume stuff. I
> thought that it restores the value that was set before suspend, unless
> it was set to some emergency value meanwhile. I think I should not look

No, it basically cannot slow down the fan, no matter what.  And yes, it
needs comments.  I will abuse the opportunity of this regression fix to
insert them.

> > Maybe something in the hwmon class is also trying to keep values across
> > sleep/suspend?
> 
> This happened only with my first (broken) patch, so no need to bother
> about this.

Ok.

-- 
  "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] Fan level 7 after resume wit 2.6.28-rc3

[Tino Keitel]

On Thu, Nov 06, 2008 at 12:11:21 -0200, Henrique de Moraes Holschuh wrote:

[...]

> and must set it to the higher of (saved, current) using this rule:
> 
> fulll-speed > 7 > AUTO.

Maybe the level values should be changed to describe this order.
Currently, it's AUTO (0x80) > full-speed (0x40) > 7. There are already
29(!) functions just for the fan handling, so there should be enough
room for a bit of abstraction. :-) I think it could have the benefit of code
which is easier to read and maintain. Correct me if I'm wrong.

Regards,
Tino

Re: [ibm-acpi-devel] Fan level 7 after resume wit 2.6.28-rc3

[Henrique de Moraes Holschuh]

On Thu, 06 Nov 2008, Tino Keitel wrote:
> Maybe the level values should be changed to describe this order.
> Currently, it's AUTO (0x80) > full-speed (0x40) > 7. There are already
> 29(!) functions just for the fan handling, so there should be enough
> room for a bit of abstraction. :-) I think it could have the benefit of code
> which is easier to read and maintain. Correct me if I'm wrong.

I will think about it.  It wouldn't hurt.

-- 
  "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] Fan level 7 after resume wit 2.6.28-rc3

[Tino Keitel]

On Wed, Nov 05, 2008 at 10:26:32 -0200, Henrique de Moraes Holschuh wrote:
> On Wed, 05 Nov 2008, Tino Keitel wrote:
> > On Wed, Nov 05, 2008 at 08:33:08 +0100, Tino Keitel wrote:
> > > I tried 2.6.28-rc3 (git a75952b) on my ThinkPad X61s, and now the fan
> > > runs at full speed after resume from suspend:
> > 
> > I forgot to look into the kernel logs for more info. Well, I found this
> > line:
> > 
> > thinkpad_acpi: restoring fan level to 0x07
> > 
> > So I guess that according to
> > 
> > ACPI: thinkpad-acpi: attempt to preserve fan state on resume
> > 
> > the attempt failed. :-)
> 
> Indeed.  Please send the full kernel output of thinkpad-acpi, showing boot,
> suspend and resume messages from the driver, please?

See http://tikei.de/kernel.log

> Also, please tell me what is the contents of /proc/acpi/ibm/fan (and if

See the first mail. I don't touch any fan settings, everything is at
the default value.

> possible, the contents of /proc/acpi/ibm/ecdump) before you issue the
> suspend command.

See http://tikei.de/ecdump (however, this was taken with 2.6.27.4).

Regards,
Tino