Skip to contentSkip to conversation

hwmon: (dell-smm) Disable fan support on Inspiron 7720 and Vostro 3360

PATCH In review Pali Rohár 3 participants 8 messages Jan 27, 2018 – Jan 29, 2018

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover hwmon: (dell-smm) Disable fan support on Inspiron 7720 and Vostro 3360

This patch series disable fan support on two machines on which is BIOS broken. And it changes measurement of SMM calls duration.

Oleksandr Natalenko (1):
hwmon: (dell-smm) Disable fan support for Dell Vostro 3360

Pali Rohár (3):
hwmon: (dell-smm) Enable broken functionality via "force" module param
hwmon: (dell-smm) Disable fan support for Dell Inspiron 7720
hwmon: (dell-smm) Measure time duration of SMM call around inlined asm

 drivers/hwmon/dell-smm-hwmon.c | 68 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 7 deletions(-)
Signature · 1 line
2.11.0
#1 lore raw

PATCH 1/4 hwmon: (dell-smm) Enable broken functionality via "force" module param

Some Dell machines are broken and some functionality is disabled. Show warning into dmesg about this fact and allow user via "force" module param to override brokenness and enable broken functionality.

---
 drivers/hwmon/dell-smm-hwmon.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/hwmon/dell-smm-hwmon.c +5 -2
index c7c9e95e58a8..aef4f8317ae2 100644
@@ -1060,8 +1060,11 @@ static int __init i8k_probe(void)
 			i8k_get_dmi_data(DMI_BIOS_VERSION));
 	}
 
-	if (dmi_check_system(i8k_blacklist_fan_type_dmi_table))
-		disallow_fan_type_call = true;
+	if (dmi_check_system(i8k_blacklist_fan_type_dmi_table)) {
+		pr_warn("broken Dell BIOS detected, disallow fan type call\n");
+		if (!force)
+			disallow_fan_type_call = true;
+	}
 
 	strlcpy(bios_version, i8k_get_dmi_data(DMI_BIOS_VERSION),
 		sizeof(bios_version));
Signature · 1 line
2.11.0
#2 lore raw

PATCH 2/4 hwmon: (dell-smm) Disable fan support for Dell Inspiron 7720

In reply to Pali Rohár

Calling fan related SMM functions implemented by Dell BIOS firmware on Dell Inspiron 7720 freeze kernel for about 500ms. Until Dell fixes it we need to disable fan support for Dell Inspiron 7720 as it makes system unusable.

Via "force" module param fan support can be enabled.

---
 drivers/hwmon/dell-smm-hwmon.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)
drivers/hwmon/dell-smm-hwmon.c +39 -1
index aef4f8317ae2..3f8b4e482b64 100644
@@ -76,6 +76,7 @@ static uint i8k_fan_mult = I8K_FAN_MULT;
 static uint i8k_pwm_mult;
 static uint i8k_fan_max = I8K_FAN_HIGH;
 static bool disallow_fan_type_call;
+static bool disallow_fan_support;
 
 #define I8K_HWMON_HAVE_TEMP1	(1 << 0)
 #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
@@ -242,6 +243,9 @@ static int i8k_get_fan_status(int fan)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, };
 
+	if (disallow_fan_support)
+		return -EINVAL;
+
 	regs.ebx = fan & 0xff;
 	return i8k_smm(&regs) ? : regs.eax & 0xff;
 }
@@ -253,6 +257,9 @@ static int i8k_get_fan_speed(int fan)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, };
 
+	if (disallow_fan_support)
+		return -EINVAL;
+
 	regs.ebx = fan & 0xff;
 	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
 }
@@ -264,7 +271,7 @@ static int _i8k_get_fan_type(int fan)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_TYPE, };
 
-	if (disallow_fan_type_call)
+	if (disallow_fan_support || disallow_fan_type_call)
 		return -EINVAL;
 
 	regs.ebx = fan & 0xff;
@@ -289,6 +296,9 @@ static int i8k_get_fan_nominal_speed(int fan, int speed)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_GET_NOM_SPEED, };
 
+	if (disallow_fan_support)
+		return -EINVAL;
+
 	regs.ebx = (fan & 0xff) | (speed << 8);
 	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
 }
@@ -300,6 +310,9 @@ static int i8k_set_fan(int fan, int speed)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_SET_FAN, };
 
+	if (disallow_fan_support)
+		return -EINVAL;
+
 	speed = (speed < 0) ? 0 : ((speed > i8k_fan_max) ? i8k_fan_max : speed);
 	regs.ebx = (fan & 0xff) | (speed << 8);
 
@@ -772,6 +785,8 @@ static struct attribute *i8k_attrs[] = {
 static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
 			      int index)
 {
+	if (disallow_fan_support && index >= 8)
+		return 0;
 	if (disallow_fan_type_call &&
 	    (index == 9 || index == 12 || index == 15))
 		return 0;
@@ -1039,6 +1054,23 @@ static const struct dmi_system_id i8k_blacklist_fan_type_dmi_table[] __initconst
 };
 
 /*
+ * On some machines all fan related SMM functions implemented by Dell BIOS
+ * firmware freeze kernel for about 500ms. Until Dell fixes these problems fan
+ * support for affected blacklisted Dell machines stay disabled.
+ * See bug: https://bugzilla.kernel.org/show_bug.cgi?id=195751
+ */
+static struct dmi_system_id i8k_blacklist_fan_support_dmi_table[] __initdata = {
+	{
+		.ident = "Dell Inspiron 7720",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Inspiron 7720"),
+		},
+	},
+	{ }
+};
+
+/*
  * Probe for the presence of a supported laptop.
  */
 static int __init i8k_probe(void)
@@ -1060,6 +1092,12 @@ static int __init i8k_probe(void)
 			i8k_get_dmi_data(DMI_BIOS_VERSION));
 	}
 
+	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
+		pr_warn("broken Dell BIOS detected, disallow fan support\n");
+		if (!force)
+			disallow_fan_support = true;
+	}
+
 	if (dmi_check_system(i8k_blacklist_fan_type_dmi_table)) {
 		pr_warn("broken Dell BIOS detected, disallow fan type call\n");
 		if (!force)
Signature · 1 line
2.11.0
#3 lore raw

PATCH 3/4 hwmon: (dell-smm) Disable fan support for Dell Vostro 3360

In reply to Pali Rohár

From: Oleksandr Natalenko <[email protected]>

Calling fan related SMM functions implemented by Dell BIOS firmware on Dell Vostro 3360 freeze kernel for about 500ms.

Unfortunately, it is unlikely for Dell to fix this since the machine is pretty old, so this commit just disables fan support to make the system usable again.

Via "force" module param fan support can be enabled.

---
 drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/hwmon/dell-smm-hwmon.c +7 -0
index 3f8b4e482b64..bf3bb7e1adab 100644
@@ -1067,6 +1067,13 @@ static struct dmi_system_id i8k_blacklist_fan_support_dmi_table[] __initdata = {
 			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Inspiron 7720"),
 		},
 	},
+	{
+		.ident = "Dell Vostro 3360",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Vostro 3360"),
+		},
+	},
 	{ }
 };
 
Signature · 1 line
2.11.0
#4 lore raw

PATCH 4/4 hwmon: (dell-smm) Measure time duration of SMM call around inlined asm

In reply to Pali Rohár

Measure only inlined asm code, not other functions to have as precise as possible measured time.

---
 drivers/hwmon/dell-smm-hwmon.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/hwmon/dell-smm-hwmon.c +10 -4
index bf3bb7e1adab..e001afd53f46 100644
@@ -147,14 +147,16 @@ static int i8k_smm_func(void *par)
 	int ebx = regs->ebx;
 	unsigned long duration;
 	ktime_t calltime, delta, rettime;
-
-	calltime = ktime_get();
 #endif
 
 	/* SMM requires CPU 0 */
 	if (smp_processor_id() != 0)
 		return -EBUSY;
 
+#ifdef DEBUG
+	calltime = ktime_get();
+#endif
+
 #if defined(CONFIG_X86_64)
 	asm volatile("pushq %%rax\n\t"
 		"movl 0(%%rax),%%edx\n\t"
@@ -208,13 +210,17 @@ static int i8k_smm_func(void *par)
 	    :    "a"(regs)
 	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
 #endif
-	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
-		rc = -EINVAL;
 
 #ifdef DEBUG
 	rettime = ktime_get();
 	delta = ktime_sub(rettime, calltime);
 	duration = ktime_to_ns(delta) >> 10;
+#endif
+
+	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
+		rc = -EINVAL;
+
+#ifdef DEBUG
 	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lu usecs)\n", eax, ebx,
 		(rc ? 0xffff : regs->eax & 0xffff), duration);
 #endif
Signature · 1 line
2.11.0
maintainer
#5 lore raw

Re: [4/4] hwmon: (dell-smm) Measure time duration of SMM call around inlined asm

Quoted 38 lines · Pali Rohár

Measure only inlined asm code, not other functions to have as precise as possible measured time.

---
 drivers/hwmon/dell-smm-hwmon.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/hwmon/dell-smm-hwmon.c +4 -4
index bf3bb7e1adab..e001afd53f46 100644
@@ -147,14 +147,16 @@ static int i8k_smm_func(void *par)
 	int ebx = regs->ebx;
 	unsigned long duration;
 	ktime_t calltime, delta, rettime;
-
-	calltime = ktime_get();
 #endif
 
 	/* SMM requires CPU 0 */
 	if (smp_processor_id() != 0)
 		return -EBUSY;
 
+#ifdef DEBUG
+	calltime = ktime_get();
+#endif
+
 #if defined(CONFIG_X86_64)
 	asm volatile("pushq %%rax\n\t"
 		"movl 0(%%rax),%%edx\n\t"
@@ -208,13 +210,17 @@ static int i8k_smm_func(void *par)
 	    :    "a"(regs)
 	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
 #endif
-	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
-		rc = -EINVAL;
 

drivers/hwmon/dell-smm-hwmon.c static int i8k_smm_func(void *par) · line 223

Quoted 10 lines
drivers/hwmon/dell-smm-hwmon.c +6 -0
 #ifdef DEBUG
 	rettime = ktime_get();
 	delta = ktime_sub(rettime, calltime);
 	duration = ktime_to_ns(delta) >> 10;
+#endif
+
+	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
+		rc = -EINVAL;
+
+#ifdef DEBUG

FWIW, the error introduced by dividing nS by 1,024 instead of 1,000 is much worse than the improvements from moving the calls around. Using specific numbers, the current code reports 500 mS as 488,281 uS. So why bother ?

I would have suggested to use ktime_us_delta(ktime_get(), calltime) instead to make the results more accurate. Sure, you get the offset from the additional divide operation, but at least that would be a constant and not a systematic error.

I'll hold this patch off for a bit. Please confirm that you want it applied as-is. I applied the other patches from the series.

Thanks,
Guenter

#6 lore raw

Re: [4/4] hwmon: (dell-smm) Measure time duration of SMM call around inlined asm

Quoted 39 lines · On Saturday 27 January 2018 09:51:45 Guenter Roeck wrote:
Quoted 38 lines · Pali Rohár

Measure only inlined asm code, not other functions to have as precise as possible measured time.

---
 drivers/hwmon/dell-smm-hwmon.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/hwmon/dell-smm-hwmon.c +4 -4
index bf3bb7e1adab..e001afd53f46 100644
@@ -147,14 +147,16 @@ static int i8k_smm_func(void *par)
 	int ebx = regs->ebx;
 	unsigned long duration;
 	ktime_t calltime, delta, rettime;
-
-	calltime = ktime_get();
 #endif
 
 	/* SMM requires CPU 0 */
 	if (smp_processor_id() != 0)
 		return -EBUSY;
 
+#ifdef DEBUG
+	calltime = ktime_get();
+#endif
+
 #if defined(CONFIG_X86_64)
 	asm volatile("pushq %%rax\n\t"
 		"movl 0(%%rax),%%edx\n\t"
@@ -208,13 +210,17 @@ static int i8k_smm_func(void *par)
 	    :    "a"(regs)
 	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
 #endif
-	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
-		rc = -EINVAL;
 
Quoted 15 lines

drivers/hwmon/dell-smm-hwmon.c static int i8k_smm_func(void *par) · line 223

Quoted 10 lines
drivers/hwmon/dell-smm-hwmon.c +6 -0
 #ifdef DEBUG
 	rettime = ktime_get();
 	delta = ktime_sub(rettime, calltime);
 	duration = ktime_to_ns(delta) >> 10;
+#endif
+
+	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
+		rc = -EINVAL;
+
+#ifdef DEBUG

FWIW, the error introduced by dividing nS by 1,024 instead of 1,000 is much worse than the improvements from moving the calls around. Using specific numbers, the current code reports 500 mS as 488,281 uS. So why bother ?

Ah, I have not noticed this yet :-(

Quoted 7 lines

I would have suggested to use ktime_us_delta(ktime_get(), calltime) instead to make the results more accurate. Sure, you get the offset from the additional divide operation, but at least that would be a constant and not a systematic error.

I'll hold this patch off for a bit. Please confirm that you want it applied as-is. I applied the other patches from the series.

Ok, I will fix this patch and resend just new version of this one.

Anyway, if 2/4 is targeting stable, then 3/4 should too. But now I see that I added Cc only to patch 2/4.

Signature · 2 lines
Pali Rohár
[email protected]
#7 lore raw
In reply to Pali Rohár

Hi.

Quoted 6 lines · On sobota 27. ledna 2018 17:22:00 CET Pali Rohár wrote:

This patch series disable fan support on two machines on which is BIOS broken. And it changes measurement of SMM calls duration.

Oleksandr Natalenko (1):
hwmon: (dell-smm) Disable fan support for Dell Vostro 3360

Quoted 8 lines

Pali Rohár (3):
hwmon: (dell-smm) Enable broken functionality via "force" module param
hwmon: (dell-smm) Disable fan support for Dell Inspiron 7720
hwmon: (dell-smm) Measure time duration of SMM call around inlined asm

 drivers/hwmon/dell-smm-hwmon.c | 68

+++++++++++++++++++++++++++++++++++++----- 1 file changed, 61 insertions(+), 7 deletions(-)

Thanks for taking care of them (finally :)).

Regards,
Oleksandr