Skip to contentSkip to conversation

duplicate test if (level & TP_EC_FAN_FULLSPEED)

drivers/misc/thinkpad_acpi.c Applied Roel Kluin 5 participants 11 messages Feb 4, 2008 – Feb 7, 2008

Subsystems (2)
ACPI Char and misc drivers
lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

in drivers/misc/thinkpad_acpi.c: 4137-4142 it reads:

/* safety net should the EC not support AUTO
* or FULLSPEED mode bits and just ignore them */
if (level & TP_EC_FAN_FULLSPEED)
level |= 7; /* safety min speed 7 */
else if (level & TP_EC_FAN_FULLSPEED)
level |= 4; /* safety min speed 4 */

Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should this be replaced by

                if (level & TP_EC_FAN_FULLSPEED)
                        level |= 7;     /* safety min speed 7 */
                else
                        level |= 4;     /* safety min speed 4 */

or

if (level & TP_EC_FAN_FULLSPEED)
level |= 7; /* safety min speed 7 */
if (level & TP_EC_FAN_FULLSPEED)
level |= 4; /* safety min speed 4 */

?

#1 lore raw
Quoted 9 lines

/* safety net should the EC not support AUTO
* or FULLSPEED mode bits and just ignore them */
if (level & TP_EC_FAN_FULLSPEED)
level |= 7; /* safety min speed 7 */
else if (level & TP_EC_FAN_FULLSPEED)
level |= 4; /* safety min speed 4 */

Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should this be replaced by

Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO (based on the comment).

author
#2 lore raw

PATCH 1/1 duplicate test if (level & TP_EC_FAN_FULLSPEED)

Quoted 7 lines · Roland Dreier
Quoted 7 lines

/* safety net should the EC not support AUTO
* or FULLSPEED mode bits and just ignore them */
if (level & TP_EC_FAN_FULLSPEED)
level |= 7; /* safety min speed 7 */
else if (level & TP_EC_FAN_FULLSPEED)
level |= 4; /* safety min speed 4 */

Quoted 6 lines
Quoted 2 lines

Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should this be replaced by

Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO (based on the comment).

Thanks Roland, for your info

based on the comments in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad, I think this should be modified like below:

ACPI: thinkpad-acpi: add a safety net for TPEC fan control mode The Linux ThinkPad community is not positive that all ThinkPads that do HFSP EC fan control do implement full-speed and auto modes, some of the earlier ones supporting HFSP might not.

If the EC ignores the AUTO or FULL-SPEED bits, it will pay attention to the lower three bits that set the fan level. And as thinkpad-acpi was leaving these set to zero, it would stop(!) the fan, which is Not A Good Thing. So, as a safety net, we now make sure to also set the fan level part of the HFSP register to speed 7 for full-speed, and a minimum of speed 4 for auto mode.
--
second TP_EC_FAN_FULLSPEED should be P_EC_FAN_AUTO

Signed-off-by Roel Kluin <[email protected]>

---

drivers/misc/thinkpad_acpi.c +1 -1
index cf56647..3c323fe 100644
@@ -4138,7 +4138,7 @@ static int fan_set_level(int level)
 		 * or FULLSPEED mode bits and just ignore them */
 		if (level & TP_EC_FAN_FULLSPEED)
 			level |= 7;	/* safety min speed 7 */
-		else if (level & TP_EC_FAN_FULLSPEED)
+		else if (level & TP_EC_FAN_AUTO)
 			level |= 4;	/* safety min speed 4 */
 
 		if (!acpi_ec_write(fan_status_offset, level))
#3 lore raw
Quoted 41 lines · Roel Kluin
Quoted 7 lines · Roland Dreier
Quoted 7 lines

/* safety net should the EC not support AUTO
* or FULLSPEED mode bits and just ignore them */
if (level & TP_EC_FAN_FULLSPEED)
level |= 7; /* safety min speed 7 */
else if (level & TP_EC_FAN_FULLSPEED)
level |= 4; /* safety min speed 4 */

Quoted 5 lines
Quoted 2 lines

Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should this be replaced by

Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO (based on the comment).

Thanks Roland, for your info

based on the comments in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad, I think this should be modified like below:

ACPI: thinkpad-acpi: add a safety net for TPEC fan control mode The Linux ThinkPad community is not positive that all ThinkPads that do HFSP EC fan control do implement full-speed and auto modes, some of the earlier ones supporting HFSP might not.

If the EC ignores the AUTO or FULL-SPEED bits, it will pay attention to the lower three bits that set the fan level. And as thinkpad-acpi was leaving these set to zero, it would stop(!) the fan, which is Not A Good Thing. So, as a safety net, we now make sure to also set the fan level part of the HFSP register to speed 7 for full-speed, and a minimum of speed 4 for auto mode.
--
second TP_EC_FAN_FULLSPEED should be P_EC_FAN_AUTO

Signed-off-by Roel Kluin <[email protected]>

---

drivers/misc/thinkpad_acpi.c +0 -0
index cf56647..3c323fe 100644

drivers/misc/thinkpad_acpi.c static int fan_set_level(int level) · line 4144

Quoted 8 lines
drivers/misc/thinkpad_acpi.c +1 -1
@@ -4138,7 +4138,7 @@ static int fan_set_level(int level)
 		 * or FULLSPEED mode bits and just ignore them */
 		if (level & TP_EC_FAN_FULLSPEED)
 			level |= 7;	/* safety min speed 7 */
-		else if (level & TP_EC_FAN_FULLSPEED)
+		else if (level & TP_EC_FAN_AUTO)
 			level |= 4;	/* safety min speed 4 */
 
 		if (!acpi_ec_write(fan_status_offset, level))

ACK. This needs to be sent to stable as well. I think both 2.6.22 and 2.6.23 need this patch.

Signature · 9 lines
  "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: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
author
#4 lore raw
Quoted 48 lines · Henrique de Moraes Holschuh
Quoted 47 lines · Roel Kluin
Quoted 7 lines · Roland Dreier
Quoted 7 lines

/* safety net should the EC not support AUTO
* or FULLSPEED mode bits and just ignore them */
if (level & TP_EC_FAN_FULLSPEED)
level |= 7; /* safety min speed 7 */
else if (level & TP_EC_FAN_FULLSPEED)
level |= 4; /* safety min speed 4 */

Quoted 5 lines
Quoted 2 lines

Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should this be replaced by

Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO (based on the comment).

Thanks Roland, for your info

based on the comments in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad, I think this should be modified like below:

ACPI: thinkpad-acpi: add a safety net for TPEC fan control mode The Linux ThinkPad community is not positive that all ThinkPads that do HFSP EC fan control do implement full-speed and auto modes, some of the earlier ones supporting HFSP might not.

If the EC ignores the AUTO or FULL-SPEED bits, it will pay attention to the lower three bits that set the fan level. And as thinkpad-acpi was leaving these set to zero, it would stop(!) the fan, which is Not A Good Thing. So, as a safety net, we now make sure to also set the fan level part of the HFSP register to speed 7 for full-speed, and a minimum of speed 4 for auto mode.
--
second TP_EC_FAN_FULLSPEED should be P_EC_FAN_AUTO

Signed-off-by Roel Kluin <[email protected]>

---

drivers/misc/thinkpad_acpi.c +1 -1
index cf56647..3c323fe 100644
@@ -4138,7 +4138,7 @@ static int fan_set_level(int level)
 		 * or FULLSPEED mode bits and just ignore them */
 		if (level & TP_EC_FAN_FULLSPEED)
 			level |= 7;	/* safety min speed 7 */
-		else if (level & TP_EC_FAN_FULLSPEED)
+		else if (level & TP_EC_FAN_AUTO)
 			level |= 4;	/* safety min speed 4 */
Quoted 5 lines

drivers/misc/thinkpad_acpi.c static int fan_set_level(int level) · line 4144

Quoted 1 line
drivers/misc/thinkpad_acpi.c +0 -0
 		if (!acpi_ec_write(fan_status_offset, level))

ACK. This needs to be sent to stable as well. I think both 2.6.22 and 2.6.23 need this patch.

CC [email protected]

#5 lore raw
Quoted 49 lines · Roel Kluin
Quoted 48 lines · Henrique de Moraes Holschuh
Quoted 47 lines · Roel Kluin
Quoted 7 lines · Roland Dreier
Quoted 7 lines

/* safety net should the EC not support AUTO
* or FULLSPEED mode bits and just ignore them */
if (level & TP_EC_FAN_FULLSPEED)
level |= 7; /* safety min speed 7 */
else if (level & TP_EC_FAN_FULLSPEED)
level |= 4; /* safety min speed 4 */

Quoted 5 lines
Quoted 2 lines

Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should this be replaced by

Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO (based on the comment).

Thanks Roland, for your info

based on the comments in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad, I think this should be modified like below:

ACPI: thinkpad-acpi: add a safety net for TPEC fan control mode The Linux ThinkPad community is not positive that all ThinkPads that do HFSP EC fan control do implement full-speed and auto modes, some of the earlier ones supporting HFSP might not.

If the EC ignores the AUTO or FULL-SPEED bits, it will pay attention to the lower three bits that set the fan level. And as thinkpad-acpi was leaving these set to zero, it would stop(!) the fan, which is Not A Good Thing. So, as a safety net, we now make sure to also set the fan level part of the HFSP register to speed 7 for full-speed, and a minimum of speed 4 for auto mode.
--
second TP_EC_FAN_FULLSPEED should be P_EC_FAN_AUTO

Signed-off-by Roel Kluin <[email protected]>

---

drivers/misc/thinkpad_acpi.c +1 -1
index cf56647..3c323fe 100644
@@ -4138,7 +4138,7 @@ static int fan_set_level(int level)
 		 * or FULLSPEED mode bits and just ignore them */
 		if (level & TP_EC_FAN_FULLSPEED)
 			level |= 7;	/* safety min speed 7 */
-		else if (level & TP_EC_FAN_FULLSPEED)
+		else if (level & TP_EC_FAN_AUTO)
 			level |= 4;	/* safety min speed 4 */
Quoted 7 lines
Quoted 5 lines

drivers/misc/thinkpad_acpi.c static int fan_set_level(int level) · line 4144

Quoted 1 line
drivers/misc/thinkpad_acpi.c +0 -0
 		if (!acpi_ec_write(fan_status_offset, level))

ACK. This needs to be sent to stable as well. I think both 2.6.22 and 2.6.23 need this patch.

CC [email protected]

Please send us a real copy of the patch, when it goes into Linus's tree, so we know what to apply, and that it is safe to do so.

thanks,

greg k-h

author
#6 lore raw
Quoted 12 lines · Greg KH
Quoted 10 lines · Roel Kluin
Quoted 9 lines · Henrique de Moraes Holschuh
Quoted 8 lines · Roel Kluin
Quoted 5 lines · Roland Dreier
Quoted 2 lines

Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should this be replaced by

Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO (based on the comment).

Thanks Roland, for your info

Quoted 7 lines
Quoted 4 lines
Quoted 3 lines

ACK. This needs to be sent to stable as well. I think both 2.6.22 and 2.6.23 need this patch.

CC [email protected]

Please send us a real copy of the patch, when it goes into Linus's tree, so we know what to apply, and that it is safe to do so.

It's a oneliner and the patch is from linus' tree.
---
in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad, a safety net for TPEC fan control mode was added. Quoting that patch:

"The Linux ThinkPad community is not positive that all ThinkPads that do HFSP EC fan control do implement full-speed and auto modes, some of the earlier ones supporting HFSP might not.

If the EC ignores the AUTO or FULL-SPEED bits, it will pay attention to the lower three bits that set the fan level. And as thinkpad-acpi was leaving these set to zero, it would stop(!) the fan, which is Not A Good Thing. So, as a safety net, we now make sure to also set the fan level part of the HFSP register to speed 7 for full-speed, and a minimum of speed 4 for auto mode."

However, in the section below the test for the FULL-SPEED bits was not added:
the AUTO bits were tested twice. This patch corrects this and ensures that the fan level part of the HFSP register is set to a minimum of speed 4 for auto mode.

Signed-off-by Roel Kluin <[email protected]>

---

drivers/misc/thinkpad_acpi.c +1 -1
index cf56647..3c323fe 100644
@@ -4138,7 +4138,7 @@ static int fan_set_level(int level)
 		 * or FULLSPEED mode bits and just ignore them */
 		if (level & TP_EC_FAN_FULLSPEED)
 			level |= 7;	/* safety min speed 7 */
-		else if (level & TP_EC_FAN_FULLSPEED)
+		else if (level & TP_EC_FAN_AUTO)
 			level |= 4;	/* safety min speed 4 */
 
 		if (!acpi_ec_write(fan_status_offset, level))
#7 lore raw
Quoted 1 line · Roel Kluin

It's a oneliner and the patch is from linus' tree.

Roel, better to just let me and Len Brown handle it. I will add your patch to my thinkpad-acpi queue and send it to Len soon, along with other simple patches that are pending. Either I or Len will notify [email protected] when it hits mainline.

Signature · 4 lines
  "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
#8 lore raw
Quoted 7 lines · Henrique de Moraes Holschuh
Quoted 1 line · Roel Kluin

It's a oneliner and the patch is from linus' tree.

Roel, better to just let me and Len Brown handle it. I will add your patch to my thinkpad-acpi queue and send it to Len soon, along with other simple patches that are pending. Either I or Len will notify [email protected] when it hits mainline.

Sounds good, I've dropped it from my queue, so please resend it when it hits Linus's tree to [email protected] if you want it there.

thanks,

greg k-h

#9 lore raw
In reply to Roel Kluin
Quoted 18 lines · Roel Kluin
Quoted 17 lines · Greg KH
Quoted 15 lines · Roel Kluin
Quoted 6 lines · Henrique de Moraes Holschuh
Quoted 5 lines · Roel Kluin
Quoted 3 lines · Roland Dreier
Quoted 2 lines

Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should this be replaced by

Quoted 7 lines
Quoted 3 lines
Quoted 2 lines

Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO (based on the comment).

Thanks Roland, for your info

ACK. This needs to be sent to stable as well. I think both 2.6.22 and 2.6.23 need this patch.

CC [email protected]

Quoted 7 lines
Quoted 2 lines

Please send us a real copy of the patch, when it goes into Linus's tree, so we know what to apply, and that it is safe to do so.

It's a oneliner and the patch is from linus' tree.
---
in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad, a safety net for TPEC fan control mode was added. Quoting that patch:

I don't see this patch in Linus's tree, I see the patch you are patching, but not this one :)

Can you tell me the git id of the patch you wish to go into the stable tree, and send the whole thing to us?

thanks,

greg k-h

reviewer Applied
#10 lore raw
In reply to Roel Kluin

Applied.

thanks,
-Len

Quoted 52 lines · Roel Kluin
Quoted 7 lines · Roland Dreier
Quoted 7 lines

/* safety net should the EC not support AUTO
* or FULLSPEED mode bits and just ignore them */
if (level & TP_EC_FAN_FULLSPEED)
level |= 7; /* safety min speed 7 */
else if (level & TP_EC_FAN_FULLSPEED)
level |= 4; /* safety min speed 4 */

Quoted 6 lines
Quoted 2 lines

Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should this be replaced by

Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO (based on the comment).

Thanks Roland, for your info

based on the comments in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad, I think this should be modified like below:

ACPI: thinkpad-acpi: add a safety net for TPEC fan control mode The Linux ThinkPad community is not positive that all ThinkPads that do HFSP EC fan control do implement full-speed and auto modes, some of the earlier ones supporting HFSP might not.

If the EC ignores the AUTO or FULL-SPEED bits, it will pay attention to the lower three bits that set the fan level. And as thinkpad-acpi was leaving these set to zero, it would stop(!) the fan, which is Not A Good Thing. So, as a safety net, we now make sure to also set the fan level part of the HFSP register to speed 7 for full-speed, and a minimum of speed 4 for auto mode.
--
second TP_EC_FAN_FULLSPEED should be P_EC_FAN_AUTO

Signed-off-by Roel Kluin <[email protected]>

---

drivers/misc/thinkpad_acpi.c +1 -2
index cf56647..3c323fe 100644
@@ -4138,7 +4138,7 @@ static int fan_set_level(int level)
 		 * or FULLSPEED mode bits and just ignore them */
 		if (level & TP_EC_FAN_FULLSPEED)
 			level |= 7;	/* safety min speed 7 */
-		else if (level & TP_EC_FAN_FULLSPEED)
+		else if (level & TP_EC_FAN_AUTO)
 			level |= 4;	/* safety min speed 4 */
 
 		if (!acpi_ec_write(fan_status_offset, level))
 
--
Quoted 5 lines

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/