Upstream patch

[haarp]

Hi,

I saw that you posted this patch on ibm-acpi-devel. Very good!

However, kernel devs generally don't accept patches if they aren't in their preferred format. I would suggest resubmitting to the list, but paying attention to the idiosyncrasies. See e.g. this comment.

On behalf of myself and other Thinkpad users, thank you!

[lhofhansl]

I also saw the post here: https://sourceforge.net/p/ibm-acpi/mailman/message/36892617/ and am very interested in this.

I also have - I believe - a simpler patch. That patch simply represents both fans as one fan. So all tooling (thinkfan, etc) stays the same. I can't see a scenario where anybody would really want to control the fan separately. As for the patch, I suspect the devs will not like the side-effects of setting an external variable to change the internal behavior of the select_fan function.

Let's figure out the best patch and post an actual patch to the mailing list - I'm happy to do the mechanics of that.

It's also important to give proper credit. I can't tell where the initial patch (for the P50) came from.

@civic9

[lhofhansl]

I listed my proposed patch here: https://github.com/vmatare/thinkfan/issues/58#issuecomment-605689370

[civic9]

@lhofhansl @haarp I think this is the source of initial patch https://sourceforge.net/p/ibm-acpi/mailman/message/35005000/ and credits should go to Matthias Hochsteger. Then @voidworker prepared newer version (https://github.com/vmatare/thinkfan/issues/58).

Would be nice if one of you can prepare this patch in a proper way to pass it to the kernel developers. I don't have any experience with that and I don't have too much time now.

About controlling the fan separately. Hmm... I think I heard they were sometimes running separately with hardware control enabled. Maybe we should check it under different cpu and discrete gpu loads, with hardware fan control (pwm_enable=2), monitoring cpu and gpu temperatures and fans speed. On the other side simpler control with just one thinkfan process is also a good idea.

[lhofhansl]

Maybe make it an option? But that might get out of hand a bit. I'll think on a bit. Then propose a patch here (or vmatare/thinkfan#58) and then see if I can propose it for the Kernel.

[lhofhansl]

Did some tests on my x1 extreme (gen2) with the BIOS default fan controlling.

There seems to be no scenario where the two fan are controlled independently, they both rev at exactly the same time to the same level. So I think actually controlling them independently is an unnecessary complication.

[lhofhansl]

@civic9 what do you think of this:

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index da794dcfdd92..8d46b4c2bde8 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8325,11 +8325,20 @@ static int fan_set_level(int level)
 
 	switch (fan_control_access_mode) {
 	case TPACPI_FAN_WR_ACPI_SFAN:
-		if (level >= 0 && level <= 7) {
-			if (!acpi_evalf(sfan_handle, NULL, NULL, "vd", level))
-				return -EIO;
-		} else
+		if (((level < 0) || (level > 7)))
 			return -EINVAL;
+
+		if (tp_features.second_fan) {
+			if (!fan_select_fan2() ||
+			    !acpi_evalf(sfan_handle, NULL, NULL, "vd", level)) {
+				fan_select_fan1();
+				pr_warn("Couldn't set 2nd fan level, disabling support\n");
+				tp_features.second_fan = 0;
+			}
+			fan_select_fan1();
+		}
+		if (!acpi_evalf(sfan_handle, NULL, NULL, "vd", level))
+			return -EIO;
 		break;
 
 	case TPACPI_FAN_WR_ACPI_FANS:
@@ -8346,6 +8355,16 @@ static int fan_set_level(int level)
 		else if (level & TP_EC_FAN_AUTO)
 			level |= 4;	/* safety min speed 4 */
 
+		if (tp_features.second_fan) {
+			if (!fan_select_fan2() ||
+			    !acpi_ec_write(fan_status_offset, level)) {
+				fan_select_fan1();
+				pr_warn("Couldn't set 2nd fan level, disabling support\n");
+				tp_features.second_fan = 0;
+			}
+			fan_select_fan1();
+
+		}
 		if (!acpi_ec_write(fan_status_offset, level))
 			return -EIO;
 		else
@@ -8772,6 +8791,9 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
 	TPACPI_QEC_IBM('7', '0', TPACPI_FAN_Q1),
 	TPACPI_QEC_LNV('7', 'M', TPACPI_FAN_2FAN),
 	TPACPI_Q_LNV('N', '1', TPACPI_FAN_2FAN),
+	TPACPI_Q_LNV3('N', '2', 'C', TPACPI_FAN_2FAN),	/* P52 / P72 */
+	TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2FAN),	/* X1 Extreme (1st gen) */
+	TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2FAN),	/* X1 Extreme (2nd gen) */
 };
 
 static int __init fan_init(struct ibm_init_struct *iibm)
@@ -8813,8 +8835,7 @@ static int __init fan_init(struct ibm_init_struct *iibm)
 				fan_quirk1_setup();
 			if (quirks & TPACPI_FAN_2FAN) {
 				tp_features.second_fan = 1;
-				dbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_FAN,
-					"secondary fan support enabled\n");
+				pr_info("secondary fan support enabled\n");
 			}
 		} else {
 			pr_err("ThinkPad ACPI EC access misbehaving, fan status and control unavailable\n");

Adds some safety, and follows the Kernel coding guidelines.

[lhofhansl]

Posted the patch to the relevant Kernel Mailing List. Zero response so far. Oh well.

[civic9]

@lhofhansl
Version posted on apci-devel doesn't include updated quirks table. Can you update it?

[voidworker]

Don't like the idea of abandoning separate control of fans since I have discrete card. I don't use it often and usually I have only one (cpu) fan active. It's less loud (P50 fans have a bit nasty high-frequency noise).

[civic9]

@voidworker - I agree with you. There was a little discussion on the apci-devel list about it. Looks like we can improve lhofhansl patch to optionally support separate control. See https://sourceforge.net/p/ibm-acpi/mailman/message/36999088/ But I don't have enough time for this now. I still use old path available here based on your work.

Btw, about nasty noise, you can also try this method https://www.reddit.com/r/thinkpad/comments/acesmt/x1_extremes_jet_engine_noise_reduced_after_mesh/ This helped me a lot too.

[voidworker]

I have no experience with it :c Moreover, when I got a shadow of idea of changes of the patch, someone said that it should be implemented in different way. I'd like to, but I don't understand how I can help.

As for the noise, thank you. I don't know if it's ok to remove it (I live in "dirty" environment), but will definitely think on it.

[voidworker]

Meanwhile on a localhost... I continue maintenance of my version with separate fan control for my own use. I don't understand what I do when merge //FAN parts from this file to other ones on every kernel update, but somehow it works. Linux should not be inferior in functionality to windows, after all. That time that microdebate (thinkfan thread) took place, I already had some understanding that nothing lasts longer than a temporary (especially after introducing new userspace behavior and locking bunch of utilites/expectations on it). But I still have some hope, so I put it there. https://gist.github.com/voidworker/266141e0f4e4318e07bd553643f5f68c