Fan level 7 after resume wit 2.6.28-rc3

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:

Quoted 2 lines · Tino Keitel

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. :-)

Quoted 13 lines · Tino Keitel

Quoted 2 lines · Tino Keitel

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

Quoted 1 line

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

Quoted 9 lines · Tino Keitel

Quoted 2 lines · Tino Keitel

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

Quoted 5 lines

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.

Quoted 10 lines · Henrique de Moraes Holschuh

Quoted 9 lines · Tino Keitel

Quoted 2 lines · Tino Keitel

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

Quoted 8 lines

Quoted 5 lines

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

Quoted 1 line

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.

Quoted 2 lines

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.

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

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

Quoted 1 line · Tino Keitel

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.

Quoted 1 line

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.

Quoted 2 lines

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.

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

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/[email protected]>

Reported-by Tino Keitel <[email protected]>

---
 drivers/misc/thinkpad_acpi.c |   57 +++++++++++++++++++++++++++++++++---------
 1 files changed, 45 insertions(+), 12 deletions(-)

drivers/misc/thinkpad_acpi.c +45 -12

index 4db1cf9..b7e4d47 100644
@@ -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);
 	}
 }
 

Signature · 1 line

1.5.6.5

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.

Quoted 5 lines · Rafael J. Wysocki

Quoted 2 lines

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.

Quoted 3 lines

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.

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()).

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

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()

fulll-speed > 7 > AUTO.

Quoted 4 lines

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?

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

Quoted 2 lines

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.

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.

Quoted 9 lines · Tino Keitel

Quoted 2 lines · Tino Keitel

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

Quoted 5 lines

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.