[PATCH 0/4] hwmon: (dell-smm) Disable fan support on Inspiron 7720 and Vostro 3360

[PATCH 0/4] hwmon: (dell-smm) Disable fan support on Inspiron 7720 and Vostro 3360

[Pali Rohár]

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

-- 
2.11.0

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

[Pali Rohár]

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.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/hwmon/dell-smm-hwmon.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index c7c9e95e58a8..aef4f8317ae2 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -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));
-- 
2.11.0

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

[Pali Rohár]

From: Oleksandr Natalenko <oleksandr@natalenko.name>

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.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=195751
Link: http://lkml.iu.edu/hypermail/linux/kernel/1711.2/06083.html
Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
Signed-off-by: Oleksandr Natalenko <oleksandr@natalenko.name>
---
 drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 3f8b4e482b64..bf3bb7e1adab 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -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"),
+		},
+	},
 	{ }
 };
 
-- 
2.11.0

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

[Pali Rohár]

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

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/hwmon/dell-smm-hwmon.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index bf3bb7e1adab..e001afd53f46 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -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
-- 
2.11.0

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

[Guenter Roeck]

On Sat, Jan 27, 2018 at 05:23:51PM +0100, Pali Rohár wrote:
> Measure only inlined asm code, not other functions to have as precise as
> possible measured time.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index bf3bb7e1adab..e001afd53f46 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -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

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

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

[Pali Rohár]

On Saturday 27 January 2018 09:51:45 Guenter Roeck wrote:
> On Sat, Jan 27, 2018 at 05:23:51PM +0100, Pali Rohár wrote:
> > Measure only inlined asm code, not other functions to have as precise as
> > possible measured time.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> >  drivers/hwmon/dell-smm-hwmon.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > index bf3bb7e1adab..e001afd53f46 100644
> > --- a/drivers/hwmon/dell-smm-hwmon.c
> > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > @@ -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
> 
> 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 :-(

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

-- 
Pali Rohár
pali.rohar@gmail.com

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

[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.

Reported-by: vova7890@mail.ru
Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=195751
Cc: stable@vger.kernel.org # v4.0+, will need backport
---
 drivers/hwmon/dell-smm-hwmon.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index aef4f8317ae2..3f8b4e482b64 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -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)
-- 
2.11.0

Re: [PATCH 0/4] hwmon: (dell-smm) Disable fan support on Inspiron 7720 and Vostro 3360

[Oleksandr Natalenko]

Hi.

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