[PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Pali Rohár]

On more Dell machines (e.g. Dell Precision M3800) I8K_SMM_GET_FAN_TYPE call
is too expensive (CPU is too long in SMM mode) and cause kernel to hang.
This patch cache type for each fan (as it should not change) and change the
way how fan presense is detected. It revert and use function fan_status()
as was before commit f989e55452c7 ("i8k: Add support for fan labels").

Moreover, kernel hangs for 2 - 3 seconds only sometimes and only on some
Dell machines. When kernel hangs fan speed is at max. So it was hard to
debug and bisect where is root of this problem. It looks like this is bug
in Dell BIOS which implement fan type SMM code... and there is no way how
to fix it in kernel.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Reviewed-by: Jean Delvare <jdelvare@suse.de>
Reported-and-tested-by: Tolga Cakir <cevelnet@gmail.com>
Fixes: f989e55452c7 ("i8k: Add support for fan labels")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=112021
Link: https://bugzilla.kernel.org/show_bug.cgi?id=100121
Cc: stable@vger.kernel.org # v4.0+, will need backport
---
 drivers/hwmon/dell-smm-hwmon.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)
---

Hi!

Jan C Peters, Thorsten Leemhuis, David Santamaría Rogado, Peter Saunderson
and others, can you test this patch if it fixes your freeze problem at
boottime and when using "sensors" program?

I need to know if this patch fixes problem on Dell Studio XPS 8000 and
Dell Studio XPS 8100 machines, so we can revert git commits:

6220f4ebd7b4 ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8000")
a4b45b25f18d ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8100")

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index c43318d..577445f 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -235,7 +235,7 @@ static int i8k_get_fan_speed(int fan)
 /*
  * Read the fan type.
  */
-static int i8k_get_fan_type(int fan)
+static int _i8k_get_fan_type(int fan)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_TYPE, };
 
@@ -243,6 +243,17 @@ static int i8k_get_fan_type(int fan)
 	return i8k_smm(&regs) ? : regs.eax & 0xff;
 }
 
+static int i8k_get_fan_type(int fan)
+{
+	/* I8K_SMM_GET_FAN_TYPE SMM call is expensive, so cache values */
+	static int types[2] = { INT_MIN, INT_MIN };
+
+	if (types[fan] == INT_MIN)
+		types[fan] = _i8k_get_fan_type(fan);
+
+	return types[fan];
+}
+
 /*
  * Read the fan nominal rpm for specific fan speed.
  */
@@ -767,13 +778,13 @@ static int __init i8k_init_hwmon(void)
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
 
-	/* First fan attributes, if fan type is OK */
-	err = i8k_get_fan_type(0);
+	/* First fan attributes, if fan status is OK */
+	err = i8k_get_fan_status(0);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN1;
 
-	/* Second fan attributes, if fan type is OK */
-	err = i8k_get_fan_type(1);
+	/* Second fan attributes, if fan status is OK */
+	err = i8k_get_fan_status(1);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN2;
 
-- 
1.7.9.5

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Guenter Roeck]

On 05/21/2016 07:46 AM, Pali Rohár wrote:
> On more Dell machines (e.g. Dell Precision M3800) I8K_SMM_GET_FAN_TYPE call
> is too expensive (CPU is too long in SMM mode) and cause kernel to hang.
> This patch cache type for each fan (as it should not change) and change the
> way how fan presense is detected. It revert and use function fan_status()
> as was before commit f989e55452c7 ("i8k: Add support for fan labels").
>
> Moreover, kernel hangs for 2 - 3 seconds only sometimes and only on some
> Dell machines. When kernel hangs fan speed is at max. So it was hard to
> debug and bisect where is root of this problem. It looks like this is bug
> in Dell BIOS which implement fan type SMM code... and there is no way how
> to fix it in kernel.
>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> Reported-and-tested-by: Tolga Cakir <cevelnet@gmail.com>
> Fixes: f989e55452c7 ("i8k: Add support for fan labels")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=112021
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=100121
> Cc: stable@vger.kernel.org # v4.0+, will need backport

Should this patch be applied, or do you wait for more testing ?

Guenter

> ---
>   drivers/hwmon/dell-smm-hwmon.c |   21 ++++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)
> ---
>
> Hi!
>
> Jan C Peters, Thorsten Leemhuis, David Santamaría Rogado, Peter Saunderson
> and others, can you test this patch if it fixes your freeze problem at
> boottime and when using "sensors" program?
>
> I need to know if this patch fixes problem on Dell Studio XPS 8000 and
> Dell Studio XPS 8100 machines, so we can revert git commits:
>
> 6220f4ebd7b4 ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8000")
> a4b45b25f18d ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8100")
>
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index c43318d..577445f 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -235,7 +235,7 @@ static int i8k_get_fan_speed(int fan)
>   /*
>    * Read the fan type.
>    */
> -static int i8k_get_fan_type(int fan)
> +static int _i8k_get_fan_type(int fan)
>   {
>   	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_TYPE, };
>
> @@ -243,6 +243,17 @@ static int i8k_get_fan_type(int fan)
>   	return i8k_smm(&regs) ? : regs.eax & 0xff;
>   }
>
> +static int i8k_get_fan_type(int fan)
> +{
> +	/* I8K_SMM_GET_FAN_TYPE SMM call is expensive, so cache values */
> +	static int types[2] = { INT_MIN, INT_MIN };
> +
> +	if (types[fan] == INT_MIN)
> +		types[fan] = _i8k_get_fan_type(fan);
> +
> +	return types[fan];
> +}
> +
>   /*
>    * Read the fan nominal rpm for specific fan speed.
>    */
> @@ -767,13 +778,13 @@ static int __init i8k_init_hwmon(void)
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
>
> -	/* First fan attributes, if fan type is OK */
> -	err = i8k_get_fan_type(0);
> +	/* First fan attributes, if fan status is OK */
> +	err = i8k_get_fan_status(0);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN1;
>
> -	/* Second fan attributes, if fan type is OK */
> -	err = i8k_get_fan_type(1);
> +	/* Second fan attributes, if fan status is OK */
> +	err = i8k_get_fan_status(1);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN2;
>
>

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3914 bytes --]

On Sunday 22 May 2016 02:19:48 Guenter Roeck wrote:
> On 05/21/2016 07:46 AM, Pali Rohár wrote:
> > On more Dell machines (e.g. Dell Precision M3800)
> > I8K_SMM_GET_FAN_TYPE call is too expensive (CPU is too long in SMM
> > mode) and cause kernel to hang. This patch cache type for each fan
> > (as it should not change) and change the way how fan presense is
> > detected. It revert and use function fan_status() as was before
> > commit f989e55452c7 ("i8k: Add support for fan labels").
> > 
> > Moreover, kernel hangs for 2 - 3 seconds only sometimes and only on
> > some Dell machines. When kernel hangs fan speed is at max. So it
> > was hard to debug and bisect where is root of this problem. It
> > looks like this is bug in Dell BIOS which implement fan type SMM
> > code... and there is no way how to fix it in kernel.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > Reviewed-by: Jean Delvare <jdelvare@suse.de>
> > Reported-and-tested-by: Tolga Cakir <cevelnet@gmail.com>
> > Fixes: f989e55452c7 ("i8k: Add support for fan labels")
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=112021
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=100121
> > Cc: stable@vger.kernel.org # v4.0+, will need backport
> 
> Should this patch be applied, or do you wait for more testing ?

I would like to hear some confirmation from people with affected machine.

> Guenter
> 
> > ---
> > 
> >   drivers/hwmon/dell-smm-hwmon.c |   21 ++++++++++++++++-----
> >   1 file changed, 16 insertions(+), 5 deletions(-)
> > 
> > ---
> > 
> > Hi!
> > 
> > Jan C Peters, Thorsten Leemhuis, David Santamaría Rogado, Peter
> > Saunderson and others, can you test this patch if it fixes your
> > freeze problem at boottime and when using "sensors" program?
> > 
> > I need to know if this patch fixes problem on Dell Studio XPS 8000
> > and Dell Studio XPS 8100 machines, so we can revert git commits:
> > 
> > 6220f4ebd7b4 ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8000")
> > a4b45b25f18d ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8100")
> > 
> > diff --git a/drivers/hwmon/dell-smm-hwmon.c
> > b/drivers/hwmon/dell-smm-hwmon.c index c43318d..577445f 100644
> > --- a/drivers/hwmon/dell-smm-hwmon.c
> > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > @@ -235,7 +235,7 @@ static int i8k_get_fan_speed(int fan)
> > 
> >   /*
> >   
> >    * Read the fan type.
> >    */
> > 
> > -static int i8k_get_fan_type(int fan)
> > +static int _i8k_get_fan_type(int fan)
> > 
> >   {
> >   
> >   	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_TYPE, };
> > 
> > @@ -243,6 +243,17 @@ static int i8k_get_fan_type(int fan)
> > 
> >   	return i8k_smm(&regs) ? : regs.eax & 0xff;
> >   
> >   }
> > 
> > +static int i8k_get_fan_type(int fan)
> > +{
> > +	/* I8K_SMM_GET_FAN_TYPE SMM call is expensive, so cache values */
> > +	static int types[2] = { INT_MIN, INT_MIN };
> > +
> > +	if (types[fan] == INT_MIN)
> > +		types[fan] = _i8k_get_fan_type(fan);
> > +
> > +	return types[fan];
> > +}
> > +
> > 
> >   /*
> >   
> >    * Read the fan nominal rpm for specific fan speed.
> >    */
> > 
> > @@ -767,13 +778,13 @@ static int __init i8k_init_hwmon(void)
> > 
> >   	if (err >= 0)
> >   	
> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
> > 
> > -	/* First fan attributes, if fan type is OK */
> > -	err = i8k_get_fan_type(0);
> > +	/* First fan attributes, if fan status is OK */
> > +	err = i8k_get_fan_status(0);
> > 
> >   	if (err >= 0)
> >   	
> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN1;
> > 
> > -	/* Second fan attributes, if fan type is OK */
> > -	err = i8k_get_fan_type(1);
> > +	/* Second fan attributes, if fan status is OK */
> > +	err = i8k_get_fan_status(1);
> > 
> >   	if (err >= 0)
> >   	
> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN2;


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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Thorsten Leemhuis]

Lo!

Pali Rohár wrote on 21.05.2016 16:46:
> […] Thorsten […] can you test this patch if it fixes your freeze problem at
> boottime and when using "sensors" program?

FWIW, I never saw either of those problems. I only saw the third issue
that was mentioned: the CPU fan speed is going up and down as described
in https://bugzilla.kernel.org/show_bug.cgi?id=100121

> I need to know if this patch fixes problem on Dell Studio XPS 8000 and
> Dell Studio XPS 8100 machines, so we can revert git commits:
> 6220f4ebd7b4 ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8000")
> a4b45b25f18d ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8100")

Just checked: Sorry, that patch did not fix the CPU fan speed issue on
my XPS 8000, so there is afaics no reason to revert 6220f4ebd7b4 :-/

HTH, CU, knurd

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 767 bytes --]

On Thursday 26 May 2016 17:39:57 Thorsten Leemhuis wrote:
> > I need to know if this patch fixes problem on Dell Studio XPS 8000
> > and Dell Studio XPS 8100 machines, so we can revert git commits:
> > 6220f4ebd7b4 ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8000")
> > a4b45b25f18d ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8100")
> 
> Just checked: Sorry, that patch did not fix the CPU fan speed issue
> on my XPS 8000, so there is afaics no reason to revert 6220f4ebd7b4
> :-/

Please, can you apply patch "dell-smm-hwmon: In debug mode log duration 
of SMM calls" and compile dell-smm-hwmon in debug mode to tell us which 
SMM call takes too long? This is probably they key for that XPS 
problems...

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Thorsten Leemhuis]

Pali Rohár wrote on 26.05.2016 17:51:
> On Thursday 26 May 2016 17:39:57 Thorsten Leemhuis wrote:
>> > I need to know if this patch fixes problem on Dell Studio XPS 8000
>> > and Dell Studio XPS 8100 machines, so we can revert git commits:
>> > 6220f4ebd7b4 ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8000")
>> > a4b45b25f18d ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8100")
>> Just checked: Sorry, that patch did not fix the CPU fan speed issue
>> on my XPS 8000, so there is afaics no reason to revert 6220f4ebd7b4
>> :-/
> Please, can you apply patch "dell-smm-hwmon: In debug mode log duration 
> of SMM calls" 

On top of "Cache fan_type() calls and use fan_status() for fan 
detection" or with that patch reverted? I did the latter and 
tested with 4.6 (just mentioning it here in case it matters).

> and compile dell-smm-hwmon in debug mode to tell us which 
> SMM call takes too long? This is probably they key for that XPS 
> problems...

[    5.941037] dell_smm_hwmon: smm(0xfea3 0x0000) = 0x4147  (took   14470 usecs)
[    5.941139] dell_smm_hwmon: smm(0x11a3 0x0000) = 0xffff  (took      93 usecs)
[    5.941236] dell_smm_hwmon: smm(0x11a3 0x0001) = 0xffff  (took      93 usecs)
[    5.941331] dell_smm_hwmon: smm(0x11a3 0x0002) = 0xffff  (took      92 usecs)
[    5.941427] dell_smm_hwmon: smm(0x11a3 0x0003) = 0xffff  (took      92 usecs)
[    5.941526] dell_smm_hwmon: smm(0x03a3 0x0000) = 0x0000  (took      95 usecs)
[    5.941625] dell_smm_hwmon: smm(0x03a3 0x0001) = 0x0001  (took      95 usecs)

HTH, CU, knurd

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Pali Rohár]

On Friday 27 May 2016 10:00:05 Thorsten Leemhuis wrote:
> Pali Rohár wrote on 26.05.2016 17:51:
> > On Thursday 26 May 2016 17:39:57 Thorsten Leemhuis wrote:
> >> > I need to know if this patch fixes problem on Dell Studio XPS 8000
> >> > and Dell Studio XPS 8100 machines, so we can revert git commits:
> >> > 6220f4ebd7b4 ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8000")
> >> > a4b45b25f18d ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8100")
> >> Just checked: Sorry, that patch did not fix the CPU fan speed issue
> >> on my XPS 8000, so there is afaics no reason to revert 6220f4ebd7b4
> >> :-/
> > Please, can you apply patch "dell-smm-hwmon: In debug mode log duration 
> > of SMM calls" 
> 
> On top of "Cache fan_type() calls and use fan_status() for fan 
> detection" or with that patch reverted? I did the latter and 
> tested with 4.6 (just mentioning it here in case it matters).

Ideally both.

> > and compile dell-smm-hwmon in debug mode to tell us which 
> > SMM call takes too long? This is probably they key for that XPS 
> > problems...
> 
> [    5.941037] dell_smm_hwmon: smm(0xfea3 0x0000) = 0x4147  (took   14470 usecs)
> [    5.941139] dell_smm_hwmon: smm(0x11a3 0x0000) = 0xffff  (took      93 usecs)
> [    5.941236] dell_smm_hwmon: smm(0x11a3 0x0001) = 0xffff  (took      93 usecs)
> [    5.941331] dell_smm_hwmon: smm(0x11a3 0x0002) = 0xffff  (took      92 usecs)
> [    5.941427] dell_smm_hwmon: smm(0x11a3 0x0003) = 0xffff  (took      92 usecs)
> [    5.941526] dell_smm_hwmon: smm(0x03a3 0x0000) = 0x0000  (took      95 usecs)
> [    5.941625] dell_smm_hwmon: smm(0x03a3 0x0001) = 0x0001  (took      95 usecs)
> 
> HTH, CU, knurd

I do not see any long taking SMM call here. Are you really sure that
your machine freeze when loading or using dell-smm-hwmon?

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

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Thorsten Leemhuis]

Pali Rohár wrote on 27.05.2016 11:47:
> On Friday 27 May 2016 10:00:05 Thorsten Leemhuis wrote:
>> Pali Rohár wrote on 26.05.2016 17:51:
>> > On Thursday 26 May 2016 17:39:57 Thorsten Leemhuis wrote:
>> >> > I need to know if this patch fixes problem on Dell Studio XPS 8000
>> >> > and Dell Studio XPS 8100 machines, so we can revert git commits:
>> >> > 6220f4ebd7b4 ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8000")
>> >> > a4b45b25f18d ("hwmon: (dell-smm) Blacklist Dell Studio XPS 8100")
>> >> Just checked: Sorry, that patch did not fix the CPU fan speed issue
>> >> on my XPS 8000, so there is afaics no reason to revert 6220f4ebd7b4
>> >> :-/
>> > Please, can you apply patch "dell-smm-hwmon: In debug mode log duration 
>> > of SMM calls" 
> […] 
> I do not see any long taking SMM call here. Are you really sure that
> your machine freeze when loading or using dell-smm-hwmon?

Uhh, sorry, it seems something got mixed up somewhere. As mentioned
earlier in this thread already: The Studio XPS 8000 I have here never
froze when loading or using dell-smm-hwmon. I only saw its CPU fan speed
going up and down as described in
https://bugzilla.kernel.org/show_bug.cgi?id=100121 From the mail that
started this thread I got the impression that all those problems are all
related somehow, so I went testing the patch as asked.

CU, knurd

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1448 bytes --]

On Friday 27 May 2016 12:01:21 Thorsten Leemhuis wrote:
> Pali Rohár wrote on 27.05.2016 11:47:
> > […]
> > I do not see any long taking SMM call here. Are you really sure
> > that your machine freeze when loading or using dell-smm-hwmon?
> 
> Uhh, sorry, it seems something got mixed up somewhere. As mentioned
> earlier in this thread already: The Studio XPS 8000 I have here never
> froze when loading or using dell-smm-hwmon. I only saw its CPU fan
> speed going up and down as described in
> https://bugzilla.kernel.org/show_bug.cgi?id=100121 From the mail that
> started this thread I got the impression that all those problems are
> all related somehow, so I went testing the patch as asked.

Looks like there are two different problems with dell-smm-hwmon driver:

1) Fan speed going randomly up and down without system freeze

2) System freeze at boot time and at that time fan speed goes up

I though that problem with fan speed is SMM freeze, but from your log I 
see that it is not truth. And my patch seems to fix problem 2).

So for problem 1) I need to know:

* Is it regression? I.e. is there kernel version under which i8k (or 
dell-smm-hwmon) is working?

* If it is regression, then I need somebody to fully fully bisect 
problematic commit. And then starts reverting it on top of new kernel 
versions (to check that only that commit cause problem 1)).

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Thorsten Leemhuis]

Pali Rohár wrote on 27.05.2016 12:45:
> […]
> Looks like there are two different problems with dell-smm-hwmon driver:
> 1) Fan speed going randomly up and down without system freeze
> […]
> So for problem 1) I need to know:
> 
> * Is it regression? […]

Yes, it is known to be a regression from f989e55452, as identified
by Jan in https://bugzilla.kernel.org/show_bug.cgi?id=100121#c13
 
I just verified and reverted that change on top of 4.6; the
problem with the fan speed indeed goes away. So I tried a few things
and came to the conclusion: the problem shows up as soon as 
i8k_get_fan_type() (introduced in f989e55452) is called somewhere.
Find below the minimal patch I could come up to that makes the fan
act normal on the Studio 8000 I have here (it's just meant as a 
reference and not meant to be applied, as it leaves unused functions 
around).

CU, knurd



--- dell-smm-hwmon.c-unmodified	2016-05-27 13:05:58.441654144 +0200
+++ dell-smm-hwmon.c	2016-05-27 14:49:31.896224380 +0200
@@ -686,14 +686,10 @@
 static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
 			  3);
 static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, i8k_hwmon_show_fan, NULL, 0);
-static SENSOR_DEVICE_ATTR(fan1_label, S_IRUGO, i8k_hwmon_show_fan_label, NULL,
-			  0);
 static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, i8k_hwmon_show_pwm,
 			  i8k_hwmon_set_pwm, 0);
 static SENSOR_DEVICE_ATTR(fan2_input, S_IRUGO, i8k_hwmon_show_fan, NULL,
 			  1);
-static SENSOR_DEVICE_ATTR(fan2_label, S_IRUGO, i8k_hwmon_show_fan_label, NULL,
-			  1);
 static SENSOR_DEVICE_ATTR(pwm2, S_IRUGO | S_IWUSR, i8k_hwmon_show_pwm,
 			  i8k_hwmon_set_pwm, 1);
 
@@ -707,11 +703,9 @@
 	&sensor_dev_attr_temp4_input.dev_attr.attr,	/* 6 */
 	&sensor_dev_attr_temp4_label.dev_attr.attr,	/* 7 */
 	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 8 */
-	&sensor_dev_attr_fan1_label.dev_attr.attr,	/* 9 */
-	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 10 */
-	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 11 */
-	&sensor_dev_attr_fan2_label.dev_attr.attr,	/* 12 */
-	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 13 */
+	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 9 */
+	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 10 */
+	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 11 */
 	NULL
 };
 
@@ -730,10 +724,10 @@
 	if (index >= 6 && index <= 7 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP4))
 		return 0;
-	if (index >= 8 && index <= 10 &&
+	if (index >= 8 && index <= 9 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
 		return 0;
-	if (index >= 11 && index <= 13 &&
+	if (index >= 10 && index <= 11 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
 		return 0;
 
@@ -768,12 +762,14 @@
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
 
 	/* First fan attributes, if fan type is OK */
-	err = i8k_get_fan_type(0);
+	// err = i8k_get_fan_type(0);
+	err = i8k_get_fan_status(0);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN1;
 
 	/* Second fan attributes, if fan type is OK */
-	err = i8k_get_fan_type(1);
+	// err = i8k_get_fan_type(1);
+	err = i8k_get_fan_status(1);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN2;
 
@@ -932,17 +928,6 @@
 static struct dmi_system_id i8k_blacklist_dmi_table[] __initdata = {
 	{
 		/*
-		 * CPU fan speed going up and down on Dell Studio XPS 8000
-		 * for unknown reasons.
-		 */
-		.ident = "Dell Studio XPS 8000",
-		.matches = {
-			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Studio XPS 8000"),
-		},
-	},
-	{
-		/*
 		 * CPU fan speed going up and down on Dell Studio XPS 8100
 		 * for unknown reasons.
 		 */

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1716 bytes --]

On Friday 27 May 2016 15:05:54 Thorsten Leemhuis wrote:
> Pali Rohár wrote on 27.05.2016 12:45:
> > […]
> > Looks like there are two different problems with dell-smm-hwmon
> > driver: 1) Fan speed going randomly up and down without system
> > freeze […]
> > So for problem 1) I need to know:
> > 
> > * Is it regression? […]
> 
> Yes, it is known to be a regression from f989e55452, as identified
> by Jan in https://bugzilla.kernel.org/show_bug.cgi?id=100121#c13
> 
> I just verified and reverted that change on top of 4.6; the
> problem with the fan speed indeed goes away.

Ok, thanks for testing!

> So I tried a few things
> and came to the conclusion: the problem shows up as soon as
> i8k_get_fan_type() (introduced in f989e55452) is called somewhere.

So, once kernel call i8k_get_fan_type() function, then fan speed going 
up/down? Even if it was called only at once? Can you confirm it? Caching 
patch cause that for each fan that function is called exactly one time.

If this is problem, we can probably create DMI list of machines which do 
not like i8k_get_fan_type() call and disable it for them.

To make sure that this is root of your problem, can you take some older 
kernel version (where is i8k working fine) and try to patch+call that 
i8k_get_fan_type() function? To check that something else cannot 
interference with it...

> Find below the minimal patch I could come up to that makes the fan
> act normal on the Studio 8000 I have here (it's just meant as a
> reference and not meant to be applied, as it leaves unused functions
> around).

Patch just make sure that i8k_get_fan_type() is never called.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Peter Saunderson]

I have just tested removing i8k_get_fan_type() function from the 
dell-smm-hwmon driver in the kernel on my Dell Inspiron 580 and the fan 
speed problem goes away.  My patch simply replaced fan_type with 
fan_status in i8k_init_hwmon and used the index as the type in 
i8k_hwmon_show_fan_label since index and the type were the same 
numerical value on my machine.

Removing i8k_get_fan_type() function for Dell Inspiron 580 would be a 
very good fix!  Well done for finding it!

On 27/05/16 14:21, Pali Rohár wrote:
> So, once kernel call i8k_get_fan_type() function, then fan speed going
> up/down? Even if it was called only at once? Can you confirm it? Caching
> patch cause that for each fan that function is called exactly one time.
Yes even if the i8k_get_fan_type() function is called once I get the fan 
speed problem.
> If this is problem, we can probably create DMI list of machines which do
> not like i8k_get_fan_type() call and disable it for them.
Please add Dell Inspiron 580 to any blacklist that you create.  The 
DMI_PRODUCT_NAME seems to have a white space at the end:

	{
		/*
		 * CPU fan speed going up and down on Dell Inspiron 580
		 * for unknown reasons.
		 */
		.ident = "Dell Inspiron 580",
		.matches = {
			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Inspiron 580 "),
		},
	},

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Pali Rohár]

On Sunday 29 May 2016 20:27:15 Peter Saunderson wrote:
> I have just tested removing i8k_get_fan_type() function from the
> dell-smm-hwmon driver in the kernel on my Dell Inspiron 580 and the fan
> speed problem goes away.  My patch simply replaced fan_type with fan_status
> in i8k_init_hwmon and used the index as the type in i8k_hwmon_show_fan_label
> since index and the type were the same numerical value on my machine.
> 
> Removing i8k_get_fan_type() function for Dell Inspiron 580 would be a very
> good fix!  Well done for finding it!
> 
> On 27/05/16 14:21, Pali Rohár wrote:
> >So, once kernel call i8k_get_fan_type() function, then fan speed going
> >up/down? Even if it was called only at once? Can you confirm it? Caching
> >patch cause that for each fan that function is called exactly one time.
> Yes even if the i8k_get_fan_type() function is called once I get the fan
> speed problem.
> >If this is problem, we can probably create DMI list of machines which do
> >not like i8k_get_fan_type() call and disable it for them.
> Please add Dell Inspiron 580 to any blacklist that you create.  The
> DMI_PRODUCT_NAME seems to have a white space at the end:
> 
> 	{
> 		/*
> 		 * CPU fan speed going up and down on Dell Inspiron 580
> 		 * for unknown reasons.
> 		 */
> 		.ident = "Dell Inspiron 580",
> 		.matches = {
> 			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Inspiron 580 "),
> 		},
> 	},
> 
> 
> 

Hi Peter! Thank you for information! Are you able to try to call that
function on some old kernel (e.g. 3.12 or 3.14) to verify that it is
caused only and only by that function?

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

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Thorsten Leemhuis]

Pali Rohár wrote on 27.05.2016 15:21:
> On Friday 27 May 2016 15:05:54 Thorsten Leemhuis wrote:
>> Pali Rohár wrote on 27.05.2016 12:45:
>> So I tried a few things
>> and came to the conclusion: the problem shows up as soon as
>> i8k_get_fan_type() (introduced in f989e55452) is called somewhere.
> So, once kernel call i8k_get_fan_type() function, then fan speed going 
> up/down?

Yes.

> To make sure that this is root of your problem, can you take some older 
> kernel version (where is i8k working fine) and try to patch+call that 
> i8k_get_fan_type() function? To check that something else cannot 
> interference with it...

I just tried with 3.19.8 (had to install a older distro first :-/ ),
where the problem does not show up (I verified just to be sure). Then I
applied below patch and voila: the fan speed starts going up/down.

IOW: From what I can see that SMM call that i8k_get_fan_type() triggers
the problem on my Studio 8000

CU, knurd

--- i8k.c-unmodified	2016-05-30 13:04:32.980825405 +0200
+++ i8k.c	2016-05-30 13:25:03.186630115 +0200
@@ -41,6 +41,7 @@
 #define I8K_SMM_SET_FAN		0x01a3
 #define I8K_SMM_GET_FAN		0x00a3
 #define I8K_SMM_GET_SPEED	0x02a3
+#define I8K_SMM_GET_FAN_TYPE   0x03a3
 #define I8K_SMM_GET_TEMP	0x10a3
 #define I8K_SMM_GET_DELL_SIG1	0xfea3
 #define I8K_SMM_GET_DELL_SIG2	0xffa3
@@ -275,6 +276,19 @@
 	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
 }

+
+/*
+ * Read the fan type.
+ */
+static int i8k_get_fan_type(int fan)
+{
+	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_TYPE, };
+
+	regs.ebx = fan & 0xff;
+	return i8k_smm(&regs) ? : regs.eax & 0xff;
+}
+
+
 /*
  * Set the fan speed (off, low, high). Returns the new fan status.
  */
@@ -870,6 +884,9 @@
 	if (err)
 		goto exit_remove_proc;

+	// calling i8k_get_fan_type once
+	printk("testing i8k_get_fan_type (%d)", i8k_get_fan_type(0));
+
 	return 0;

  exit_remove_proc:

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Peter Saunderson]

On 30/05/16 10:36, Pali Rohár wrote:
> Hi Peter! Thank you for information! Are you able to try to call that
> function on some old kernel (e.g. 3.12 or 3.14) to verify that it is
> caused only and only by that function?
>
I have tried to use my old 3.19.0 kernel that did not have the problem 
but now it will not boot.  The boot screen is left with

Loading Linux linux...
Loading initial ramdisk...

No logs etc.. I am guessing that some grub or ram disk change is giving 
me the problem.  I have just upgraded to Ubuntu 16.04 and do not want to 
downgrade so I am a bit stuck at the moment and doubt that I can do this 
test.

Peter.

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Pali Rohár]

On Monday 30 May 2016 17:25:26 Peter Saunderson wrote:
> On 30/05/16 10:36, Pali Rohár wrote:
> >Hi Peter! Thank you for information! Are you able to try to call that
> >function on some old kernel (e.g. 3.12 or 3.14) to verify that it is
> >caused only and only by that function?
> >
> I have tried to use my old 3.19.0 kernel that did not have the problem but
> now it will not boot.  The boot screen is left with
> 
> Loading Linux linux...
> Loading initial ramdisk...
> 
> No logs etc.. I am guessing that some grub or ram disk change is giving me
> the problem.  I have just upgraded to Ubuntu 16.04 and do not want to
> downgrade so I am a bit stuck at the moment and doubt that I can do this
> test.
> 
> Peter.

You can try to regenerate initramfs with ubuntu command:

$ sudo update-initramfs -u -k all

In case you modified or updated some initram files or kernel modules you
need to regenerate it to take effect at boot... -k all tells to
regenerate *all* initramfs images, not only for currently booted kernel.

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

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1337 bytes --]

On Monday 30 May 2016 13:45:51 Thorsten Leemhuis wrote:
> Pali Rohár wrote on 27.05.2016 15:21:
> > On Friday 27 May 2016 15:05:54 Thorsten Leemhuis wrote:
> >> Pali Rohár wrote on 27.05.2016 12:45:
> >> So I tried a few things
> >> and came to the conclusion: the problem shows up as soon as
> >> i8k_get_fan_type() (introduced in f989e55452) is called somewhere.
> > 
> > So, once kernel call i8k_get_fan_type() function, then fan speed
> > going up/down?
> 
> Yes.
> 
> > To make sure that this is root of your problem, can you take some
> > older kernel version (where is i8k working fine) and try to
> > patch+call that i8k_get_fan_type() function? To check that
> > something else cannot interference with it...
> 
> I just tried with 3.19.8 (had to install a older distro first :-/ ),
> where the problem does not show up (I verified just to be sure). Then
> I applied below patch and voila: the fan speed starts going up/down.
> 
> IOW: From what I can see that SMM call that i8k_get_fan_type()
> triggers the problem on my Studio 8000
> 
> CU, knurd
> 

Thank you for your testing! I believe now we definitely know root of 
this problem. It is buggy Dell BIOS/SMM code and we need to avoid 
calling I8K_SMM_GET_FAN_TYPE on affected buggy Dell machines.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2585 bytes --]

On Sunday 22 May 2016 02:28:00 Pali Rohár wrote:
> On Sunday 22 May 2016 02:19:48 Guenter Roeck wrote:
> > On 05/21/2016 07:46 AM, Pali Rohár wrote:
> > > On more Dell machines (e.g. Dell Precision M3800)
> > > I8K_SMM_GET_FAN_TYPE call is too expensive (CPU is too long in
> > > SMM mode) and cause kernel to hang. This patch cache type for
> > > each fan (as it should not change) and change the way how fan
> > > presense is detected. It revert and use function fan_status() as
> > > was before commit f989e55452c7 ("i8k: Add support for fan
> > > labels").
> > > 
> > > Moreover, kernel hangs for 2 - 3 seconds only sometimes and only
> > > on some Dell machines. When kernel hangs fan speed is at max. So
> > > it was hard to debug and bisect where is root of this problem.
> > > It looks like this is bug in Dell BIOS which implement fan type
> > > SMM code... and there is no way how to fix it in kernel.
> > > 
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > Reviewed-by: Jean Delvare <jdelvare@suse.de>
> > > Reported-and-tested-by: Tolga Cakir <cevelnet@gmail.com>
> > > Fixes: f989e55452c7 ("i8k: Add support for fan labels")
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=112021
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=100121
> > > Cc: stable@vger.kernel.org # v4.0+, will need backport
> > 
> > Should this patch be applied, or do you wait for more testing ?
> 
> I would like to hear some confirmation from people with affected
> machine.
> 

Ok, now after testing we know that kernel should prevent calling 
I8K_SMM_GET_FAN_TYPE on affected buggy Dell machines.

Looks like there are two different bugs in Dell SMM with 
I8K_SMM_GET_FAN_TYPE call.

First bug cause that kernel freeze for 2 - 3 seconds when 
I8K_SMM_GET_FAN_TYPE is issued.

Second bug cause that fan goes randomly up and down (that is controlled 
by Dell SMM) when I8K_SMM_GET_FAN_TYPE is issued. Normal behaviour is 
returned after machine reboots.

Some Dell machines are affected by first bug, some by second bug. And 
there are Dell machines without both bugs.

This my patch just partially fix first bug and prevent calling that call 
at boot time. But can be issued by sysfs (+value is cached, so it is 
called only once).

So question is: is my patch enough for fixing first bug?

And second question: how to fix second bug? I see only one option: 
Create machine blacklist with broken Dell SMM firmware and disallow 
calling I8K_SMM_GET_FAN_TYPE for them.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Guenter Roeck]

On 06/13/2016 11:52 AM, Pali Rohár wrote:
> On Sunday 22 May 2016 02:28:00 Pali Rohár wrote:
>> On Sunday 22 May 2016 02:19:48 Guenter Roeck wrote:
>>> On 05/21/2016 07:46 AM, Pali Rohár wrote:
>>>> On more Dell machines (e.g. Dell Precision M3800)
>>>> I8K_SMM_GET_FAN_TYPE call is too expensive (CPU is too long in
>>>> SMM mode) and cause kernel to hang. This patch cache type for
>>>> each fan (as it should not change) and change the way how fan
>>>> presense is detected. It revert and use function fan_status() as
>>>> was before commit f989e55452c7 ("i8k: Add support for fan
>>>> labels").
>>>>
>>>> Moreover, kernel hangs for 2 - 3 seconds only sometimes and only
>>>> on some Dell machines. When kernel hangs fan speed is at max. So
>>>> it was hard to debug and bisect where is root of this problem.
>>>> It looks like this is bug in Dell BIOS which implement fan type
>>>> SMM code... and there is no way how to fix it in kernel.
>>>>
>>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>>>> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>>>> Reported-and-tested-by: Tolga Cakir <cevelnet@gmail.com>
>>>> Fixes: f989e55452c7 ("i8k: Add support for fan labels")
>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=112021
>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=100121
>>>> Cc: stable@vger.kernel.org # v4.0+, will need backport
>>>
>>> Should this patch be applied, or do you wait for more testing ?
>>
>> I would like to hear some confirmation from people with affected
>> machine.
>>
>
> Ok, now after testing we know that kernel should prevent calling
> I8K_SMM_GET_FAN_TYPE on affected buggy Dell machines.
>
> Looks like there are two different bugs in Dell SMM with
> I8K_SMM_GET_FAN_TYPE call.
>
> First bug cause that kernel freeze for 2 - 3 seconds when
> I8K_SMM_GET_FAN_TYPE is issued.
>
> Second bug cause that fan goes randomly up and down (that is controlled
> by Dell SMM) when I8K_SMM_GET_FAN_TYPE is issued. Normal behaviour is
> returned after machine reboots.
>
> Some Dell machines are affected by first bug, some by second bug. And
> there are Dell machines without both bugs.
>
> This my patch just partially fix first bug and prevent calling that call
> at boot time. But can be issued by sysfs (+value is cached, so it is
> called only once).
>
> So question is: is my patch enough for fixing first bug?
>
> And second question: how to fix second bug? I see only one option:
> Create machine blacklist with broken Dell SMM firmware and disallow
> calling I8K_SMM_GET_FAN_TYPE for them.
>
Or maybe a whitelist with known working systems ?

Guenter

Re: [PATCH] dell-smm-hwmon: Cache fan_type() calls and use fan_status() for fan detection

[Pali Rohár]

On Monday 13 June 2016 19:02:14 Guenter Roeck wrote:
> >And second question: how to fix second bug? I see only one option:
> >Create machine blacklist with broken Dell SMM firmware and disallow
> >calling I8K_SMM_GET_FAN_TYPE for them.
> >
> Or maybe a whitelist with known working systems ?
> 
> Guenter

Smaller list is better, so I will try to collect blacklist and if it
will be big we can use whitelist.

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