diff mbox

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

Message ID 01084692-618a-31db-76f8-60176d1cd2cc@leemhuis.info (mailing list archive)
State Not Applicable
Headers show

Commit Message

Thorsten Leemhuis May 27, 2016, 1:05 p.m. UTC
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



--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Pali Rohár May 27, 2016, 1:21 p.m. UTC | #1
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.
Peter Saunderson May 29, 2016, 7:27 p.m. UTC | #2
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 "),
		},
	},



--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pali Rohár May 30, 2016, 9:36 a.m. UTC | #3
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?
Peter Saunderson May 30, 2016, 4:25 p.m. UTC | #4
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pali Rohár June 2, 2016, 1:06 p.m. UTC | #5
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.
diff mbox

Patch

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