[3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver
diff mbox series

Message ID 1418155621-21644-4-git-send-email-pali.rohar@gmail.com
State New, archived
Headers show
Series
  • i8k: Rework fan_mult and fan_max code
Related show

Commit Message

Pali Rohár Dec. 9, 2014, 8:07 p.m. UTC
Now we have autodetection code for fan multiplier and maximal fan speed so we do
not need to have those constants for each laptop in kernel driver code.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
!!!Please do not apply this patch until all affected machines will be tested!!!

I tested autodetection code only on Dell Latitude E6440 (where it worked).
Other machines which needs to be tested:

Dell Latitude D520
Dell Latitude E6540
Dell Precision WorkStation 490
Dell Studio
Dell XPS M140 (MXC051)
---
 drivers/char/i8k.c |   88 +---------------------------------------------------
 1 file changed, 1 insertion(+), 87 deletions(-)

Comments

Pali Rohár Dec. 10, 2014, 11:51 a.m. UTC | #1
On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> Now we have autodetection code for fan multiplier and maximal
> fan speed so we do not need to have those constants for each
> laptop in kernel driver code.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
> !!!Please do not apply this patch until all affected machines
> will be tested!!!
> 
> I tested autodetection code only on Dell Latitude E6440 (where
> it worked). Other machines which needs to be tested:
> 
> Dell Latitude D520
> Dell Latitude E6540
> Dell Precision WorkStation 490
> Dell Studio
> Dell XPS M140 (MXC051)
> ---

Can somebody else with dell laptops test this patch series?
Gabriele Mazzotta Dec. 10, 2014, 1:32 p.m. UTC | #2
On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> > Now we have autodetection code for fan multiplier and maximal
> > fan speed so we do not need to have those constants for each
> > laptop in kernel driver code.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> > !!!Please do not apply this patch until all affected machines
> > will be tested!!!
> > 
> > I tested autodetection code only on Dell Latitude E6440 (where
> > it worked). Other machines which needs to be tested:
> > 
> > Dell Latitude D520
> > Dell Latitude E6540
> > Dell Precision WorkStation 490
> > Dell Studio
> > Dell XPS M140 (MXC051)
> > ---
> 
> Can somebody else with dell laptops test this patch series?
> 
> 

i8k_get_fan_nominal_rpm() returns -22 on my XPS13, so nothing changed
with this patch series applied.

Gabriele
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Gabriele Mazzotta Dec. 10, 2014, 1:41 p.m. UTC | #3
On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> Now we have autodetection code for fan multiplier and maximal fan speed so we do
> not need to have those constants for each laptop in kernel driver code.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
> !!!Please do not apply this patch until all affected machines will be tested!!!
> 
> I tested autodetection code only on Dell Latitude E6440 (where it worked).
> Other machines which needs to be tested:
> 
> Dell Latitude D520
> Dell Latitude E6540
> Dell Precision WorkStation 490
> Dell Studio
> Dell XPS M140 (MXC051)

Just a note: "or in dmi" has to be removed from a couple of comments.

Gabriele

> ---
>  drivers/char/i8k.c |   88 +---------------------------------------------------
>  1 file changed, 1 insertion(+), 87 deletions(-)
> 
> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> index 8bdbed2..bf74644 100644
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -725,42 +725,6 @@ static int __init i8k_init_hwmon(void)
>  	return 0;
>  }
>  
> -struct i8k_config_data {
> -	int fan_mult;
> -	int fan_max;
> -};
> -
> -enum i8k_configs {
> -	DELL_LATITUDE_D520,
> -	DELL_LATITUDE_E6540,
> -	DELL_PRECISION_490,
> -	DELL_STUDIO,
> -	DELL_XPS_M140,
> -};
> -
> -static const struct i8k_config_data i8k_config_data[] = {
> -	[DELL_LATITUDE_D520] = {
> -		.fan_mult = 1,
> -		.fan_max = I8K_FAN_TURBO,
> -	},
> -	[DELL_LATITUDE_E6540] = {
> -		.fan_mult = 1,
> -		.fan_max = I8K_FAN_HIGH,
> -	},
> -	[DELL_PRECISION_490] = {
> -		.fan_mult = 1,
> -		.fan_max = I8K_FAN_TURBO,
> -	},
> -	[DELL_STUDIO] = {
> -		.fan_mult = 1,
> -		.fan_max = I8K_FAN_HIGH,
> -	},
> -	[DELL_XPS_M140] = {
> -		.fan_mult = 1,
> -		.fan_max = I8K_FAN_HIGH,
> -	},
> -};
> -
>  static struct dmi_system_id i8k_dmi_table[] __initdata = {
>  	{
>  		.ident = "Dell Inspiron",
> @@ -784,30 +748,6 @@ static struct dmi_system_id i8k_dmi_table[] __initdata = {
>  		},
>  	},
>  	{
> -		.ident = "Dell Latitude D520",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> -			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D520"),
> -		},
> -		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_D520],
> -	},
> -	{
> -		.ident = "Dell Latitude E6440",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> -			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
> -		},
> -		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_E6540],
> -	},
> -	{
> -		.ident = "Dell Latitude E6540",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> -			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
> -		},
> -		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_E6540],
> -	},
> -	{
>  		.ident = "Dell Latitude 2",
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> @@ -822,22 +762,13 @@ static struct dmi_system_id i8k_dmi_table[] __initdata = {
>  		},
>  	},
>  	{
> -		.ident = "Dell Inspiron 3",
> +		.ident = "Dell Inspiron 4",
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "MP061"),
>  		},
>  	},
>  	{
> -		.ident = "Dell Precision 490",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> -			DMI_MATCH(DMI_PRODUCT_NAME,
> -				  "Precision WorkStation 490"),
> -		},
> -		.driver_data = (void *)&i8k_config_data[DELL_PRECISION_490],
> -	},
> -	{
>  		.ident = "Dell Precision",
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> @@ -864,7 +795,6 @@ static struct dmi_system_id i8k_dmi_table[] __initdata = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "Studio"),
>  		},
> -		.driver_data = (void *)&i8k_config_data[DELL_STUDIO],
>  	},
>  	{
>  		.ident = "Dell XPS M140",
> @@ -872,7 +802,6 @@ static struct dmi_system_id i8k_dmi_table[] __initdata = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "MXC051"),
>  		},
> -		.driver_data = (void *)&i8k_config_data[DELL_XPS_M140],
>  	},
>  	{ }
>  };
> @@ -884,8 +813,6 @@ MODULE_DEVICE_TABLE(dmi, i8k_dmi_table);
>   */
>  static int __init i8k_probe(void)
>  {
> -	const struct i8k_config_data *conf;
> -	const struct dmi_system_id *id;
>  	int fan, val, ret;
>  
>  	/*
> @@ -915,19 +842,6 @@ static int __init i8k_probe(void)
>  			return -ENODEV;
>  	}
>  
> -	/*
> -	 * Autodetect fan multiplier and maximal fan speed from dmi config
> -	 * Values specified in module parameters override values from dmi
> -	 */
> -	id = dmi_first_match(i8k_dmi_table);
> -	if (id && id->driver_data) {
> -		conf = id->driver_data;
> -		if (fan_mult <= 0 && conf->fan_mult > 0)
> -			fan_mult = conf->fan_mult;
> -		if (fan_max <= 0 && conf->fan_max > 0)
> -			fan_max = conf->fan_max;
> -	}
> -
>  	if (fan_mult <= 0) {
>  		/*
>  		 * Autodetect fan multiplier for each fan based on nominal rpm
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pali Rohár Dec. 17, 2014, 5:54 p.m. UTC | #4
On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> > Now we have autodetection code for fan multiplier and
> > maximal fan speed so we do not need to have those constants
> > for each laptop in kernel driver code.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> > !!!Please do not apply this patch until all affected
> > machines will be tested!!!
> > 
> > I tested autodetection code only on Dell Latitude E6440
> > (where it worked). Other machines which needs to be tested:
> > 
> > Dell Latitude D520
> > Dell Latitude E6540
> > Dell Precision WorkStation 490
> > Dell Studio
> > Dell XPS M140 (MXC051)
> > ---
> 
> Can somebody else with dell laptops test this patch series?
 
Steven Honeyman, can you test this autodetection patch on your 
Latitude E6540?
Steven Honeyman Dec. 17, 2014, 6:20 p.m. UTC | #5
On 17 December 2014 at 17:54, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
>> On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
>> > Now we have autodetection code for fan multiplier and
>> > maximal fan speed so we do not need to have those constants
>> > for each laptop in kernel driver code.
>> >
>> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>> > ---
>> > !!!Please do not apply this patch until all affected
>> > machines will be tested!!!
>> >
>> > I tested autodetection code only on Dell Latitude E6440
>> > (where it worked). Other machines which needs to be tested:
>> >
>> > Dell Latitude D520
>> > Dell Latitude E6540
>> > Dell Precision WorkStation 490
>> > Dell Studio
>> > Dell XPS M140 (MXC051)
>> > ---
>>
>> Can somebody else with dell laptops test this patch series?
>
> Steven Honeyman, can you test this autodetection patch on your
> Latitude E6540?

Sure. You picked the best possible time of day to ask (seriously!)
Just applied patches 1,2,3 and so far seems exactly as before so it
looks OK to me.

While I was spamming "sensors" I did notice this (output trimmed for
neatness) but it is unusual my laptop fan is ever off so it might have
always done this and I would have never noticed:
Each line is about 0.5 - 1 second apart:

fan2:        3186 RPM
fan2:        3231 RPM
<temp is low enough, fan stops>
fan2:        10000 RPM
fan2:        9544 RPM
fan2:        10000 RPM
fan2:           0 RPM
(stays at 0 while fan is off)
<temp back up, fan starts>
fan2:        3236 RPM
fan2:        3597 RPM

Ever seen this before? I'd roll back and test again but this room and
laptop are too warm for the fan to stop completely now.


Thanks,
Steven
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Valdis Kl ē tnieks Dec. 18, 2014, 9:02 a.m. UTC | #6
On Wed, 17 Dec 2014 18:20:21 +0000, Steven Honeyman said:
> On 17 December 2014 at 17:54, Pali Rohár <pali.rohar@gmail.com> wrote:

> >> > Dell Latitude E6540

> >> Can somebody else with dell laptops test this patch series?
> >
> > Steven Honeyman, can you test this autodetection patch on your
> > Latitude E6540?

I'm not Steven, but I have a E6530.  linux-next 20141208
reports fan wonkyness in 'sensors':

i8k-virtual-0
Adapter: Virtual device
fan2:        83070 RPM

I'll have to see if the patch series fixes that.
Pali Rohár Dec. 18, 2014, 11:08 a.m. UTC | #7
On Wednesday 10 December 2014 14:32:16 Gabriele Mazzotta wrote:
> On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> > On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> > > Now we have autodetection code for fan multiplier and
> > > maximal fan speed so we do not need to have those
> > > constants for each laptop in kernel driver code.
> > > 
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > ---
> > > !!!Please do not apply this patch until all affected
> > > machines will be tested!!!
> > > 
> > > I tested autodetection code only on Dell Latitude E6440
> > > (where it worked). Other machines which needs to be
> > > tested:
> > > 
> > > Dell Latitude D520
> > > Dell Latitude E6540
> > > Dell Precision WorkStation 490
> > > Dell Studio
> > > Dell XPS M140 (MXC051)
> > > ---
> > 
> > Can somebody else with dell laptops test this patch series?
> 
> i8k_get_fan_nominal_rpm() returns -22 on my XPS13, so nothing
> changed with this patch series applied.
> 
> Gabriele

So your BIOS cannot report nominal_rpm and because your machine 
is not in dmi list, all 3 patches do nothing for your machine.

But you need to set multiplier to 1, right?

What about this patch? (on top of 3/3)

--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -850,6 +850,10 @@ static int __init i8k_probe(void)
 		 */
 		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
 			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
+			if (i8k_get_fan_rpm(fan) > I8K_FAN_MAX_RPM) {
+				i8k_fan_mult[fan] = 1;
+				continue;
+			}
 			for (val = 0; val < 256; ++val) {
 				ret = i8k_get_fan_nominal_rpm(fan, val);
 				if (ret > I8K_FAN_MAX_RPM) {
Pali Rohár Dec. 18, 2014, 11:11 a.m. UTC | #8
On Wednesday 17 December 2014 19:20:21 Steven Honeyman wrote:
> On 17 December 2014 at 17:54, Pali Rohár <pali.rohar@gmail.com> 
wrote:
> > On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> >> On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> >> > Now we have autodetection code for fan multiplier and
> >> > maximal fan speed so we do not need to have those
> >> > constants for each laptop in kernel driver code.
> >> > 
> >> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> >> > ---
> >> > !!!Please do not apply this patch until all affected
> >> > machines will be tested!!!
> >> > 
> >> > I tested autodetection code only on Dell Latitude E6440
> >> > (where it worked). Other machines which needs to be
> >> > tested:
> >> > 
> >> > Dell Latitude D520
> >> > Dell Latitude E6540
> >> > Dell Precision WorkStation 490
> >> > Dell Studio
> >> > Dell XPS M140 (MXC051)
> >> > ---
> >> 
> >> Can somebody else with dell laptops test this patch series?
> > 
> > Steven Honeyman, can you test this autodetection patch on
> > your Latitude E6540?
> 
> Sure. You picked the best possible time of day to ask
> (seriously!) Just applied patches 1,2,3 and so far seems
> exactly as before so it looks OK to me.
> 

Good, thanks for testing. Now we know that my patches detects 
correct multiplier for E6440 and E6540 machines which are in dmi 
list.

> While I was spamming "sensors" I did notice this (output
> trimmed for neatness) but it is unusual my laptop fan is ever
> off so it might have always done this and I would have never
> noticed:
> Each line is about 0.5 - 1 second apart:
> 
> fan2:        3186 RPM
> fan2:        3231 RPM
> <temp is low enough, fan stops>
> fan2:        10000 RPM
> fan2:        9544 RPM
> fan2:        10000 RPM
> fan2:           0 RPM
> (stays at 0 while fan is off)
> <temp back up, fan starts>
> fan2:        3236 RPM
> fan2:        3597 RPM
> 
> Ever seen this before? I'd roll back and test again but this
> room and laptop are too warm for the fan to stop completely
> now.
> 
> 
> Thanks,
> Steven

On my E6440 I have never seen 10000 RPM.
Valdis Kl ē tnieks Dec. 18, 2014, 3:08 p.m. UTC | #9
On Thu, 18 Dec 2014 12:08:58 +0100, Pali Rohár said:

> So your BIOS cannot report nominal_rpm and because your machine=20
> is not in dmi list, all 3 patches do nothing for your machine.
> 
> But you need to set multiplier to 1, right?
> 
> What about this patch? (on top of 3/3)
> 
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -850,6 +850,10 @@ static int __init i8k_probe(void)
>  		 */
>  		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
>  			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> +			if (i8k_get_fan_rpm(fan) > I8K_FAN_MAX_RPM) {
> +				i8k_fan_mult[fan] = 1;
> +				continue;
> +			}
>  			for (val = 0; val < 256; ++val) {


Dell Latitude E6530, linux-next 20141208 plus the 3 patches you
posted on Nov 30 (support labels), plus the 4 of 3 patches in
this go around, and sensors reports:

i8k-virtual-0
Adapter: Virtual device
fan2:        2774 RPM

and under CPU load, that number rises to 3200 or so, so it seems to be
working....

So you probably should fold this into the current set of patches, and
feel free to put this in there:

Tested-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>
Pali Rohár Dec. 18, 2014, 4:34 p.m. UTC | #10
On Thursday 18 December 2014 16:08:51 Valdis.Kletnieks@vt.edu 
wrote:
> On Thu, 18 Dec 2014 12:08:58 +0100, Pali Rohár said:
> > So your BIOS cannot report nominal_rpm and because your
> > machine=20 is not in dmi list, all 3 patches do nothing for
> > your machine.
> > 
> > But you need to set multiplier to 1, right?
> > 
> > What about this patch? (on top of 3/3)
> > 
> > --- a/drivers/char/i8k.c
> > +++ b/drivers/char/i8k.c
> > @@ -850,6 +850,10 @@ static int __init i8k_probe(void)
> > 
> >  		 */
> >  		
> >  		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
> >  		
> >  			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> > 
> > +			if (i8k_get_fan_rpm(fan) > I8K_FAN_MAX_RPM) {
> > +				i8k_fan_mult[fan] = 1;
> > +				continue;
> > +			}
> > 
> >  			for (val = 0; val < 256; ++val) {
> 
> Dell Latitude E6530, linux-next 20141208 plus the 3 patches
> you posted on Nov 30 (support labels), plus the 4 of 3
> patches in this go around, and sensors reports:
> 
> i8k-virtual-0
> Adapter: Virtual device
> fan2:        2774 RPM
> 
> and under CPU load, that number rises to 3200 or so, so it
> seems to be working....
> 
> So you probably should fold this into the current set of
> patches, and feel free to put this in there:
> 
> Tested-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>

Thanks for testing. Anyway I would like to know if your dell 
machine supports i8k_get_fan_nominal_rpm(). Can you test without 
above 4 lines patch?
Valdis Kl ē tnieks Dec. 18, 2014, 4:44 p.m. UTC | #11
On Thu, 18 Dec 2014 17:34:56 +0100, Pali Rohár said:

> Thanks for testing. Anyway I would like to know if your dell
> machine supports i8k_get_fan_nominal_rpm(). Can you test without
> above 4 lines patch?

I'll give that a try this evening..
Gabriele Mazzotta Dec. 25, 2014, 9:54 p.m. UTC | #12
On Thursday 18 December 2014 12:08:58 Pali Rohár wrote:
> On Wednesday 10 December 2014 14:32:16 Gabriele Mazzotta wrote:
> > On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> > > On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> > > > Now we have autodetection code for fan multiplier and
> > > > maximal fan speed so we do not need to have those
> > > > constants for each laptop in kernel driver code.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > ---
> > > > !!!Please do not apply this patch until all affected
> > > > machines will be tested!!!
> > > > 
> > > > I tested autodetection code only on Dell Latitude E6440
> > > > (where it worked). Other machines which needs to be
> > > > tested:
> > > > 
> > > > Dell Latitude D520
> > > > Dell Latitude E6540
> > > > Dell Precision WorkStation 490
> > > > Dell Studio
> > > > Dell XPS M140 (MXC051)
> > > > ---
> > > 
> > > Can somebody else with dell laptops test this patch series?
> > 
> > i8k_get_fan_nominal_rpm() returns -22 on my XPS13, so nothing
> > changed with this patch series applied.
> > 
> > Gabriele
> 
> So your BIOS cannot report nominal_rpm and because your machine 
> is not in dmi list, all 3 patches do nothing for your machine.
> 
> But you need to set multiplier to 1, right?
> 
> What about this patch? (on top of 3/3)
> 
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -850,6 +850,10 @@ static int __init i8k_probe(void)
>  		 */
>  		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
>  			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> +			if (i8k_get_fan_rpm(fan) > I8K_FAN_MAX_RPM) {
> +				i8k_fan_mult[fan] = 1;
> +				continue;
> +			}
>  			for (val = 0; val < 256; ++val) {
>  				ret = i8k_get_fan_nominal_rpm(fan, val);
>  				if (ret > I8K_FAN_MAX_RPM) {
> 
> 

Hi,

I'm sorry for replying only now, but I couldn't follow this thread
closely and I'm a bit lost now. I haven't tested the suggested
change, but I don't think it would work in a reliable way. It's not
rare for the fan to be completely stopped, especially on boot. You
are right though, 1 is the right multiplier.


Guenter, while I was trying to catch up, I noticed that the support
for the XPS 13 [1] will be added. May I ask you which revision of
the laptop was tested? I own the 9333 one and I'm not sure that
having i8k automatically loaded is a good idea. The reason is that
reading and setting the speed of the right (and only) fan causes a
freeze of the laptop of some milliseconds, enough to be annoying and
noticeable. I'm worried of the effect this might have in the everyday
use, users might start noticing random freezes because of one
application that all the sudden is able to read the speed of the fan.
I don't if the same happens on all the other Dell systems, this is
the first and only Dell laptop I owned.

If the tested laptop wasn't the XPS13 9333 and this problem does not
exists, would it be possible to make the DMI_PRODUCT_NAME string such
that it matches only the tested revision? The string that identifies
my laptop is "XPS13 9333".

Gabriele

[1] http://www.spinics.net/lists/kernel/msg1878801.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Gabriele Mazzotta Dec. 27, 2014, 2:13 p.m. UTC | #13
On Thursday 25 December 2014 22:54:34 Gabriele Mazzotta wrote:
> On Thursday 18 December 2014 12:08:58 Pali Rohár wrote:
> > On Wednesday 10 December 2014 14:32:16 Gabriele Mazzotta wrote:
> > > On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> > > > On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> > > > > Now we have autodetection code for fan multiplier and
> > > > > maximal fan speed so we do not need to have those
> > > > > constants for each laptop in kernel driver code.
> > > > > 
> > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > ---
> > > > > !!!Please do not apply this patch until all affected
> > > > > machines will be tested!!!
> > > > > 
> > > > > I tested autodetection code only on Dell Latitude E6440
> > > > > (where it worked). Other machines which needs to be
> > > > > tested:
> > > > > 
> > > > > Dell Latitude D520
> > > > > Dell Latitude E6540
> > > > > Dell Precision WorkStation 490
> > > > > Dell Studio
> > > > > Dell XPS M140 (MXC051)
> > > > > ---
> > > > 
> > > > Can somebody else with dell laptops test this patch series?
> > > 
> > > i8k_get_fan_nominal_rpm() returns -22 on my XPS13, so nothing
> > > changed with this patch series applied.
> > > 
> > > Gabriele
> > 
> > So your BIOS cannot report nominal_rpm and because your machine 
> > is not in dmi list, all 3 patches do nothing for your machine.
> > 
> > But you need to set multiplier to 1, right?
> > 
> > What about this patch? (on top of 3/3)
> > 
> > --- a/drivers/char/i8k.c
> > +++ b/drivers/char/i8k.c
> > @@ -850,6 +850,10 @@ static int __init i8k_probe(void)
> >  		 */
> >  		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
> >  			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> > +			if (i8k_get_fan_rpm(fan) > I8K_FAN_MAX_RPM) {
> > +				i8k_fan_mult[fan] = 1;
> > +				continue;
> > +			}
> >  			for (val = 0; val < 256; ++val) {
> >  				ret = i8k_get_fan_nominal_rpm(fan, val);
> >  				if (ret > I8K_FAN_MAX_RPM) {
> > 
> > 
> 
> Hi,
> 
> I'm sorry for replying only now, but I couldn't follow this thread
> closely and I'm a bit lost now. I haven't tested the suggested
> change, but I don't think it would work in a reliable way. It's not
> rare for the fan to be completely stopped, especially on boot. You
> are right though, 1 is the right multiplier.

I took a better look at the code and my laptop is indeed able to report
the nominal speed. The problem with the original patch was that the
first call of i8k_get_fan_nominal_rpm() correctly returned -22 (the fan
doesn't exist) and the loop was terminated because of that. I tested
the following patch http://www.spinics.net/lists/kernel/msg1892101.html
and the fan multiplier auto detection worked.

I confirm that the suggested change in case the nominal speed can't
be retrieved is not OK as its outcome depends on the current state of
the fans.

> Guenter, while I was trying to catch up, I noticed that the support
> for the XPS 13 [1] will be added. May I ask you which revision of
> the laptop was tested? I own the 9333 one and I'm not sure that
> having i8k automatically loaded is a good idea. The reason is that
> reading and setting the speed of the right (and only) fan causes a
> freeze of the laptop of some milliseconds, enough to be annoying and
> noticeable. I'm worried of the effect this might have in the everyday
> use, users might start noticing random freezes because of one
> application that all the sudden is able to read the speed of the fan.
> I don't if the same happens on all the other Dell systems, this is
> the first and only Dell laptop I owned.
> 
> If the tested laptop wasn't the XPS13 9333 and this problem does not
> exists, would it be possible to make the DMI_PRODUCT_NAME string such
> that it matches only the tested revision? The string that identifies
> my laptop is "XPS13 9333".
> 
> Gabriele
> 
> [1] http://www.spinics.net/lists/kernel/msg1878801.html

I modified i8k so that it prints the time required to execute the
assembly code in i8k_smm(). Here what I got:

[ 4697.485130] i8k_smm asm: 1965800 ns    #pwm1
[ 4697.487383] i8k_smm asm: 1375057 ns    #temp3_input
[ 4697.489477] i8k_smm asm: 1112493 ns    #temp3_label
[ 4697.991687] i8k_smm asm: 500796086 ns  #fan1_input
[ 4697.998245] i8k_smm asm: 1957365 ns    #fan1_label
[ 4698.009190] i8k_smm asm: 1770247 ns    #temp1_input
[ 4698.014416] i8k_smm asm: 749734 ns     #temp1_label
[ 4698.019103] i8k_smm asm: 2503962 ns    #temp2_input
[ 4698.023490] i8k_smm asm: 1738982 ns    #temp2_label

As you can see, the time required to read the speed of the fan is
substantially higher than the combined time of all the other reads.

Is the difference so big for the other systems that have been tested?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pali Rohár Dec. 28, 2014, 8:22 a.m. UTC | #14
On Saturday 27 December 2014 15:13:28 Gabriele Mazzotta wrote:
> On Thursday 25 December 2014 22:54:34 Gabriele Mazzotta wrote:
> > On Thursday 18 December 2014 12:08:58 Pali Rohár wrote:
> > > On Wednesday 10 December 2014 14:32:16 Gabriele Mazzotta 
wrote:
> > > > On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> > > > > On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> > > > > > Now we have autodetection code for fan multiplier
> > > > > > and maximal fan speed so we do not need to have
> > > > > > those constants for each laptop in kernel driver
> > > > > > code.
> > > > > > 
> > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > > ---
> > > > > > !!!Please do not apply this patch until all affected
> > > > > > machines will be tested!!!
> > > > > > 
> > > > > > I tested autodetection code only on Dell Latitude
> > > > > > E6440 (where it worked). Other machines which needs
> > > > > > to be tested:
> > > > > > 
> > > > > > Dell Latitude D520
> > > > > > Dell Latitude E6540
> > > > > > Dell Precision WorkStation 490
> > > > > > Dell Studio
> > > > > > Dell XPS M140 (MXC051)
> > > > > > ---
> > > > > 
> > > > > Can somebody else with dell laptops test this patch
> > > > > series?
> > > > 
> > > > i8k_get_fan_nominal_rpm() returns -22 on my XPS13, so
> > > > nothing changed with this patch series applied.
> > > > 
> > > > Gabriele
> > > 
> > > So your BIOS cannot report nominal_rpm and because your
> > > machine is not in dmi list, all 3 patches do nothing for
> > > your machine.
> > > 
> > > But you need to set multiplier to 1, right?
> > > 
> > > What about this patch? (on top of 3/3)
> > > 
> > > --- a/drivers/char/i8k.c
> > > +++ b/drivers/char/i8k.c
> > > @@ -850,6 +850,10 @@ static int __init i8k_probe(void)
> > > 
> > >  		 */
> > >  		
> > >  		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
> > >  		
> > >  			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> > > 
> > > +			if (i8k_get_fan_rpm(fan) > I8K_FAN_MAX_RPM) {
> > > +				i8k_fan_mult[fan] = 1;
> > > +				continue;
> > > +			}
> > > 
> > >  			for (val = 0; val < 256; ++val) {
> > >  			
> > >  				ret = i8k_get_fan_nominal_rpm(fan, val);
> > >  				if (ret > I8K_FAN_MAX_RPM) {
> > 
> > Hi,
> > 
> > I'm sorry for replying only now, but I couldn't follow this
> > thread closely and I'm a bit lost now. I haven't tested the
> > suggested change, but I don't think it would work in a
> > reliable way. It's not rare for the fan to be completely
> > stopped, especially on boot. You are right though, 1 is the
> > right multiplier.
> 
> I took a better look at the code and my laptop is indeed able
> to report the nominal speed. The problem with the original
> patch was that the first call of i8k_get_fan_nominal_rpm()
> correctly returned -22 (the fan doesn't exist) and the loop
> was terminated because of that. I tested the following patch
> http://www.spinics.net/lists/kernel/msg1892101.html and the
> fan multiplier auto detection worked.
> 
> I confirm that the suggested change in case the nominal speed
> can't be retrieved is not OK as its outcome depends on the
> current state of the fans.
> 
> > Guenter, while I was trying to catch up, I noticed that the
> > support for the XPS 13 [1] will be added. May I ask you
> > which revision of the laptop was tested? I own the 9333 one
> > and I'm not sure that having i8k automatically loaded is a
> > good idea. The reason is that reading and setting the speed
> > of the right (and only) fan causes a freeze of the laptop
> > of some milliseconds, enough to be annoying and noticeable.
> > I'm worried of the effect this might have in the everyday
> > use, users might start noticing random freezes because of
> > one application that all the sudden is able to read the
> > speed of the fan. I don't if the same happens on all the
> > other Dell systems, this is the first and only Dell laptop
> > I owned.
> > 
> > If the tested laptop wasn't the XPS13 9333 and this problem
> > does not exists, would it be possible to make the
> > DMI_PRODUCT_NAME string such that it matches only the
> > tested revision? The string that identifies my laptop is
> > "XPS13 9333".
> > 
> > Gabriele
> > 
> > [1] http://www.spinics.net/lists/kernel/msg1878801.html
> 
> I modified i8k so that it prints the time required to execute
> the assembly code in i8k_smm(). Here what I got:
> 
> [ 4697.485130] i8k_smm asm: 1965800 ns    #pwm1
> [ 4697.487383] i8k_smm asm: 1375057 ns    #temp3_input
> [ 4697.489477] i8k_smm asm: 1112493 ns    #temp3_label
> [ 4697.991687] i8k_smm asm: 500796086 ns  #fan1_input
> [ 4697.998245] i8k_smm asm: 1957365 ns    #fan1_label
> [ 4698.009190] i8k_smm asm: 1770247 ns    #temp1_input
> [ 4698.014416] i8k_smm asm: 749734 ns     #temp1_label
> [ 4698.019103] i8k_smm asm: 2503962 ns    #temp2_input
> [ 4698.023490] i8k_smm asm: 1738982 ns    #temp2_label
> 
> As you can see, the time required to read the speed of the fan
> is substantially higher than the combined time of all the
> other reads.
> 
> Is the difference so big for the other systems that have been
> tested?

It looks like we should not read fan rpm at startup for 
multiplier autodetection... and only fan nominal speed.
Guenter Roeck Dec. 28, 2014, 8:28 a.m. UTC | #15
On Sun, Dec 28, 2014 at 09:22:29AM +0100, Pali Rohár wrote:
> On Saturday 27 December 2014 15:13:28 Gabriele Mazzotta wrote:
> > On Thursday 25 December 2014 22:54:34 Gabriele Mazzotta wrote:
> > > On Thursday 18 December 2014 12:08:58 Pali Rohár wrote:
> > > > On Wednesday 10 December 2014 14:32:16 Gabriele Mazzotta 
> wrote:
> > > > > On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> > > > > > On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> > > > > > > Now we have autodetection code for fan multiplier
> > > > > > > and maximal fan speed so we do not need to have
> > > > > > > those constants for each laptop in kernel driver
> > > > > > > code.
> > > > > > > 
> > > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > > > ---
> > > > > > > !!!Please do not apply this patch until all affected
> > > > > > > machines will be tested!!!
> > > > > > > 
> > > > > > > I tested autodetection code only on Dell Latitude
> > > > > > > E6440 (where it worked). Other machines which needs
> > > > > > > to be tested:
> > > > > > > 
> > > > > > > Dell Latitude D520
> > > > > > > Dell Latitude E6540
> > > > > > > Dell Precision WorkStation 490
> > > > > > > Dell Studio
> > > > > > > Dell XPS M140 (MXC051)
> > > > > > > ---
> > > > > > 
> > > > > > Can somebody else with dell laptops test this patch
> > > > > > series?
> > > > > 
> > > > > i8k_get_fan_nominal_rpm() returns -22 on my XPS13, so
> > > > > nothing changed with this patch series applied.
> > > > > 
> > > > > Gabriele
> > > > 
> > > > So your BIOS cannot report nominal_rpm and because your
> > > > machine is not in dmi list, all 3 patches do nothing for
> > > > your machine.
> > > > 
> > > > But you need to set multiplier to 1, right?
> > > > 
> > > > What about this patch? (on top of 3/3)
> > > > 
> > > > --- a/drivers/char/i8k.c
> > > > +++ b/drivers/char/i8k.c
> > > > @@ -850,6 +850,10 @@ static int __init i8k_probe(void)
> > > > 
> > > >  		 */
> > > >  		
> > > >  		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
> > > >  		
> > > >  			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> > > > 
> > > > +			if (i8k_get_fan_rpm(fan) > I8K_FAN_MAX_RPM) {
> > > > +				i8k_fan_mult[fan] = 1;
> > > > +				continue;
> > > > +			}
> > > > 
> > > >  			for (val = 0; val < 256; ++val) {
> > > >  			
> > > >  				ret = i8k_get_fan_nominal_rpm(fan, val);
> > > >  				if (ret > I8K_FAN_MAX_RPM) {
> > > 
> > > Hi,
> > > 
> > > I'm sorry for replying only now, but I couldn't follow this
> > > thread closely and I'm a bit lost now. I haven't tested the
> > > suggested change, but I don't think it would work in a
> > > reliable way. It's not rare for the fan to be completely
> > > stopped, especially on boot. You are right though, 1 is the
> > > right multiplier.
> > 
> > I took a better look at the code and my laptop is indeed able
> > to report the nominal speed. The problem with the original
> > patch was that the first call of i8k_get_fan_nominal_rpm()
> > correctly returned -22 (the fan doesn't exist) and the loop
> > was terminated because of that. I tested the following patch
> > http://www.spinics.net/lists/kernel/msg1892101.html and the
> > fan multiplier auto detection worked.
> > 
> > I confirm that the suggested change in case the nominal speed
> > can't be retrieved is not OK as its outcome depends on the
> > current state of the fans.
> > 
> > > Guenter, while I was trying to catch up, I noticed that the
> > > support for the XPS 13 [1] will be added. May I ask you
> > > which revision of the laptop was tested? I own the 9333 one
> > > and I'm not sure that having i8k automatically loaded is a
> > > good idea. The reason is that reading and setting the speed
> > > of the right (and only) fan causes a freeze of the laptop
> > > of some milliseconds, enough to be annoying and noticeable.
> > > I'm worried of the effect this might have in the everyday
> > > use, users might start noticing random freezes because of
> > > one application that all the sudden is able to read the
> > > speed of the fan. I don't if the same happens on all the
> > > other Dell systems, this is the first and only Dell laptop
> > > I owned.
> > > 
> > > If the tested laptop wasn't the XPS13 9333 and this problem
> > > does not exists, would it be possible to make the
> > > DMI_PRODUCT_NAME string such that it matches only the
> > > tested revision? The string that identifies my laptop is
> > > "XPS13 9333".
> > > 

I am several thousand miles away from the XPS13 right now, so I can not check
it. As far as I recall, it was a 9333.

> > > Gabriele
> > > 
> > > [1] http://www.spinics.net/lists/kernel/msg1878801.html
> > 
> > I modified i8k so that it prints the time required to execute
> > the assembly code in i8k_smm(). Here what I got:
> > 
> > [ 4697.485130] i8k_smm asm: 1965800 ns    #pwm1
> > [ 4697.487383] i8k_smm asm: 1375057 ns    #temp3_input
> > [ 4697.489477] i8k_smm asm: 1112493 ns    #temp3_label
> > [ 4697.991687] i8k_smm asm: 500796086 ns  #fan1_input
> > [ 4697.998245] i8k_smm asm: 1957365 ns    #fan1_label
> > [ 4698.009190] i8k_smm asm: 1770247 ns    #temp1_input
> > [ 4698.014416] i8k_smm asm: 749734 ns     #temp1_label
> > [ 4698.019103] i8k_smm asm: 2503962 ns    #temp2_input
> > [ 4698.023490] i8k_smm asm: 1738982 ns    #temp2_label
> > 
> > As you can see, the time required to read the speed of the fan
> > is substantially higher than the combined time of all the
> > other reads.
> > 
> > Is the difference so big for the other systems that have been
> > tested?
> 
> It looks like we should not read fan rpm at startup for 
> multiplier autodetection... and only fan nominal speed.
> 
Yes, agreed. After all, we still have the module parameter to override it
if reading the nominal speed (and thus auto-detection) is not successful.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pali Rohár Dec. 28, 2014, 8:46 a.m. UTC | #16
Ok, here are new patches for testing... Those you are still reading this email thread and have your Dell 
machines near, can you test them (ideally with disabling dmi config data)?

diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
index 8ec4c37..d6e8a26 100644
--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -64,9 +64,9 @@ static DEFINE_MUTEX(i8k_mutex);
 static char bios_version[4];
 static struct device *i8k_hwmon_dev;
 static u32 i8k_hwmon_flags;
-static int i8k_fan_mult;
-static int i8k_pwm_mult;
-static int i8k_fan_max = I8K_FAN_HIGH;
+static uint i8k_fan_mult;
+static uint i8k_pwm_mult;
+static uint i8k_fan_max = I8K_FAN_HIGH;
 
 #define I8K_HWMON_HAVE_TEMP1	(1 << 0)
 #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
@@ -95,12 +95,12 @@ static bool power_status;
 module_param(power_status, bool, 0600);
 MODULE_PARM_DESC(power_status, "Report power status in /proc/i8k");
 
-static int fan_mult = I8K_FAN_MULT;
-module_param(fan_mult, int, 0);
+static uint fan_mult = I8K_FAN_MULT;
+module_param(fan_mult, uint, 0);
 MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with");
 
-static int fan_max = I8K_FAN_HIGH;
-module_param(fan_max, int, 0);
+static uint fan_max = I8K_FAN_HIGH;
+module_param(fan_max, uint, 0);
 MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed");
 
 static int i8k_open_fs(struct inode *inode, struct file *file);
@@ -696,8 +696,8 @@ static int __init i8k_init_hwmon(void)
 }
 
 struct i8k_config_data {
-	int fan_mult;
-	int fan_max;
+	uint fan_mult;
+	uint fan_max;
 };
 
 enum i8k_configs {



diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
index d6e8a26..6ad0872 100644
--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -6,6 +6,7 @@
  * Hwmon integration:
  * Copyright (C) 2011  Jean Delvare <jdelvare@suse.de>
  * Copyright (C) 2013, 2014  Guenter Roeck <linux@roeck-us.net>
+ * Copyright (C) 2014  Pali Rohár <pali.rohar@gmail.com>
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License as published by the
@@ -42,12 +43,14 @@
 #define I8K_SMM_SET_FAN		0x01a3
 #define I8K_SMM_GET_FAN		0x00a3
 #define I8K_SMM_GET_SPEED	0x02a3
+#define I8K_SMM_GET_NOM_SPEED	0x04a3
 #define I8K_SMM_GET_TEMP	0x10a3
 #define I8K_SMM_GET_TEMP_TYPE	0x11a3
 #define I8K_SMM_GET_DELL_SIG1	0xfea3
 #define I8K_SMM_GET_DELL_SIG2	0xffa3
 
 #define I8K_FAN_MULT		30
+#define I8K_FAN_MAX_RPM		30000
 #define I8K_MAX_TEMP		127
 
 #define I8K_FN_NONE		0x00
@@ -64,7 +67,7 @@ static DEFINE_MUTEX(i8k_mutex);
 static char bios_version[4];
 static struct device *i8k_hwmon_dev;
 static u32 i8k_hwmon_flags;
-static uint i8k_fan_mult;
+static uint i8k_fan_mult = I8K_FAN_MULT;
 static uint i8k_pwm_mult;
 static uint i8k_fan_max = I8K_FAN_HIGH;
 
@@ -95,13 +98,13 @@ static bool power_status;
 module_param(power_status, bool, 0600);
 MODULE_PARM_DESC(power_status, "Report power status in /proc/i8k");
 
-static uint fan_mult = I8K_FAN_MULT;
+static uint fan_mult;
 module_param(fan_mult, uint, 0);
-MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with");
+MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with (default: autodetect)");
 
-static uint fan_max = I8K_FAN_HIGH;
+static uint fan_max;
 module_param(fan_max, uint, 0);
-MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed");
+MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");
 
 static int i8k_open_fs(struct inode *inode, struct file *file);
 static long i8k_ioctl(struct file *, unsigned int, unsigned long);
@@ -276,6 +279,17 @@ static int i8k_get_fan_speed(int fan)
 }
 
 /*
+ * Read the fan nominal rpm for specific fan speed.
+ */
+static int i8k_get_fan_nominal_speed(int fan, int speed)
+{
+	struct smm_regs regs = { .eax = I8K_SMM_GET_NOM_SPEED, };
+
+	regs.ebx = (fan & 0xff) | (speed << 8);
+	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
+}
+
+/*
  * Set the fan speed (off, low, high). Returns the new fan status.
  */
 static int i8k_set_fan(int fan, int speed)
@@ -863,6 +877,7 @@ MODULE_DEVICE_TABLE(dmi, i8k_dmi_table);
 static int __init i8k_probe(void)
 {
 	const struct dmi_system_id *id;
+	int fan, ret;
 
 	/*
 	 * Get DMI information
@@ -891,19 +906,40 @@ static int __init i8k_probe(void)
 			return -ENODEV;
 	}
 
-	i8k_fan_mult = fan_mult;
-	i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
+	/*
+	 * Set fan multiplier and maximal fan speed from dmi config
+	 * Values specified in module parameters override values from dmi
+	 */
 	id = dmi_first_match(i8k_dmi_table);
 	if (id && id->driver_data) {
 		const struct i8k_config_data *conf = id->driver_data;
-
-		if (fan_mult == I8K_FAN_MULT && conf->fan_mult)
-			i8k_fan_mult = conf->fan_mult;
-		if (fan_max == I8K_FAN_HIGH && conf->fan_max)
-			i8k_fan_max = conf->fan_max;
+		if (!fan_mult && conf->fan_mult)
+			fan_mult = conf->fan_mult;
+		if (!fan_max && conf->fan_max)
+			fan_max = conf->fan_max;
 	}
+
+	i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
 	i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
 
+	if (!fan_mult) {
+		/*
+		 * Autodetect fan multiplier based on nominal rpm
+		 * If fan reports rpm value too high then set multiplier to 1
+		 */
+		for (fan = 0; fan < 2; ++fan) {
+			ret = i8k_get_fan_nominal_speed(fan, i8k_fan_max);
+			if (ret < 0)
+				continue;
+			if (ret > I8K_FAN_MAX_RPM)
+				i8k_fan_mult = 1;
+			break;
+		}
+	} else {
+		/* Fan multiplier was specified in module param or in dmi */
+		i8k_fan_mult = fan_mult;
+	}
+
 	return 0;
 }
Gabriele Mazzotta Dec. 28, 2014, 3:25 p.m. UTC | #17
On Sunday 28 December 2014 09:46:19 Pali Rohár wrote:
> Ok, here are new patches for testing... Those you are still reading this email thread and have your Dell 
> machines near, can you test them (ideally with disabling dmi config data)?

Could you please tell me exactly against what should I apply these
patches? I can see that they depend on other patches in this thread,
but I can't find any tree that includes them.

> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> index 8ec4c37..d6e8a26 100644
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -64,9 +64,9 @@ static DEFINE_MUTEX(i8k_mutex);
>  static char bios_version[4];
>  static struct device *i8k_hwmon_dev;
>  static u32 i8k_hwmon_flags;
> -static int i8k_fan_mult;
> -static int i8k_pwm_mult;
> -static int i8k_fan_max = I8K_FAN_HIGH;
> +static uint i8k_fan_mult;
> +static uint i8k_pwm_mult;
> +static uint i8k_fan_max = I8K_FAN_HIGH;
>  
>  #define I8K_HWMON_HAVE_TEMP1	(1 << 0)
>  #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
> @@ -95,12 +95,12 @@ static bool power_status;
>  module_param(power_status, bool, 0600);
>  MODULE_PARM_DESC(power_status, "Report power status in /proc/i8k");
>  
> -static int fan_mult = I8K_FAN_MULT;
> -module_param(fan_mult, int, 0);
> +static uint fan_mult = I8K_FAN_MULT;
> +module_param(fan_mult, uint, 0);
>  MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with");
>  
> -static int fan_max = I8K_FAN_HIGH;
> -module_param(fan_max, int, 0);
> +static uint fan_max = I8K_FAN_HIGH;
> +module_param(fan_max, uint, 0);
>  MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed");
>  
>  static int i8k_open_fs(struct inode *inode, struct file *file);
> @@ -696,8 +696,8 @@ static int __init i8k_init_hwmon(void)
>  }
>  
>  struct i8k_config_data {
> -	int fan_mult;
> -	int fan_max;
> +	uint fan_mult;
> +	uint fan_max;
>  };
>  
>  enum i8k_configs {
> 
> 
> 
> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> index d6e8a26..6ad0872 100644
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -6,6 +6,7 @@
>   * Hwmon integration:
>   * Copyright (C) 2011  Jean Delvare <jdelvare@suse.de>
>   * Copyright (C) 2013, 2014  Guenter Roeck <linux@roeck-us.net>
> + * Copyright (C) 2014  Pali Rohár <pali.rohar@gmail.com>
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms of the GNU General Public License as published by the
> @@ -42,12 +43,14 @@
>  #define I8K_SMM_SET_FAN		0x01a3
>  #define I8K_SMM_GET_FAN		0x00a3
>  #define I8K_SMM_GET_SPEED	0x02a3
> +#define I8K_SMM_GET_NOM_SPEED	0x04a3
>  #define I8K_SMM_GET_TEMP	0x10a3
>  #define I8K_SMM_GET_TEMP_TYPE	0x11a3
>  #define I8K_SMM_GET_DELL_SIG1	0xfea3
>  #define I8K_SMM_GET_DELL_SIG2	0xffa3
>  
>  #define I8K_FAN_MULT		30
> +#define I8K_FAN_MAX_RPM		30000
>  #define I8K_MAX_TEMP		127
>  
>  #define I8K_FN_NONE		0x00
> @@ -64,7 +67,7 @@ static DEFINE_MUTEX(i8k_mutex);
>  static char bios_version[4];
>  static struct device *i8k_hwmon_dev;
>  static u32 i8k_hwmon_flags;
> -static uint i8k_fan_mult;
> +static uint i8k_fan_mult = I8K_FAN_MULT;
>  static uint i8k_pwm_mult;
>  static uint i8k_fan_max = I8K_FAN_HIGH;
>  
> @@ -95,13 +98,13 @@ static bool power_status;
>  module_param(power_status, bool, 0600);
>  MODULE_PARM_DESC(power_status, "Report power status in /proc/i8k");
>  
> -static uint fan_mult = I8K_FAN_MULT;
> +static uint fan_mult;
>  module_param(fan_mult, uint, 0);
> -MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with");
> +MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with (default: autodetect)");
>  
> -static uint fan_max = I8K_FAN_HIGH;
> +static uint fan_max;
>  module_param(fan_max, uint, 0);
> -MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed");
> +MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");
>  
>  static int i8k_open_fs(struct inode *inode, struct file *file);
>  static long i8k_ioctl(struct file *, unsigned int, unsigned long);
> @@ -276,6 +279,17 @@ static int i8k_get_fan_speed(int fan)
>  }
>  
>  /*
> + * Read the fan nominal rpm for specific fan speed.
> + */
> +static int i8k_get_fan_nominal_speed(int fan, int speed)
> +{
> +	struct smm_regs regs = { .eax = I8K_SMM_GET_NOM_SPEED, };
> +
> +	regs.ebx = (fan & 0xff) | (speed << 8);
> +	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
> +}
> +
> +/*
>   * Set the fan speed (off, low, high). Returns the new fan status.
>   */
>  static int i8k_set_fan(int fan, int speed)
> @@ -863,6 +877,7 @@ MODULE_DEVICE_TABLE(dmi, i8k_dmi_table);
>  static int __init i8k_probe(void)
>  {
>  	const struct dmi_system_id *id;
> +	int fan, ret;
>  
>  	/*
>  	 * Get DMI information
> @@ -891,19 +906,40 @@ static int __init i8k_probe(void)
>  			return -ENODEV;
>  	}
>  
> -	i8k_fan_mult = fan_mult;
> -	i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
> +	/*
> +	 * Set fan multiplier and maximal fan speed from dmi config
> +	 * Values specified in module parameters override values from dmi
> +	 */
>  	id = dmi_first_match(i8k_dmi_table);
>  	if (id && id->driver_data) {
>  		const struct i8k_config_data *conf = id->driver_data;
> -
> -		if (fan_mult == I8K_FAN_MULT && conf->fan_mult)
> -			i8k_fan_mult = conf->fan_mult;
> -		if (fan_max == I8K_FAN_HIGH && conf->fan_max)
> -			i8k_fan_max = conf->fan_max;
> +		if (!fan_mult && conf->fan_mult)
> +			fan_mult = conf->fan_mult;
> +		if (!fan_max && conf->fan_max)
> +			fan_max = conf->fan_max;
>  	}
> +
> +	i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
>  	i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
>  
> +	if (!fan_mult) {
> +		/*
> +		 * Autodetect fan multiplier based on nominal rpm
> +		 * If fan reports rpm value too high then set multiplier to 1
> +		 */
> +		for (fan = 0; fan < 2; ++fan) {
> +			ret = i8k_get_fan_nominal_speed(fan, i8k_fan_max);
> +			if (ret < 0)
> +				continue;
> +			if (ret > I8K_FAN_MAX_RPM)
> +				i8k_fan_mult = 1;
> +			break;
> +		}
> +	} else {
> +		/* Fan multiplier was specified in module param or in dmi */
> +		i8k_fan_mult = fan_mult;
> +	}
> +
>  	return 0;
>  }
>  
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pali Rohár Dec. 28, 2014, 3:48 p.m. UTC | #18
On Sunday 28 December 2014 16:25:21 Gabriele Mazzotta wrote:
> On Sunday 28 December 2014 09:46:19 Pali Rohár wrote:
> > Ok, here are new patches for testing... Those you are still
> > reading this email thread and have your Dell machines near,
> > can you test them (ideally with disabling dmi config data)?
> 
> Could you please tell me exactly against what should I apply
> these patches? I can see that they depend on other patches in
> this thread, but I can't find any tree that includes them.
> 

Two patches in previous email could apply on 3.19-rc1 tree.
Gabriele Mazzotta Dec. 28, 2014, 4:02 p.m. UTC | #19
On Sunday 28 December 2014 16:48:54 Pali Rohár wrote:
> On Sunday 28 December 2014 16:25:21 Gabriele Mazzotta wrote:
> > On Sunday 28 December 2014 09:46:19 Pali Rohár wrote:
> > > Ok, here are new patches for testing... Those you are still
> > > reading this email thread and have your Dell machines near,
> > > can you test them (ideally with disabling dmi config data)?
> > 
> > Could you please tell me exactly against what should I apply
> > these patches? I can see that they depend on other patches in
> > this thread, but I can't find any tree that includes them.
> > 
> 
> Two patches in previous email could apply on 3.19-rc1 tree.

I see that the second patch you've just sent depends on the followings:
https://patchwork.kernel.org/patch/5524791/ (I8K_SMM_GET_TEMP_TYPE)
https://patchwork.kernel.org/patch/5524731/ (Copyright)

If I'm not wrong, they are not included in 3.19-rc1.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pali Rohár Dec. 28, 2014, 4:07 p.m. UTC | #20
On Sunday 28 December 2014 17:02:35 Gabriele Mazzotta wrote:
> On Sunday 28 December 2014 16:48:54 Pali Rohár wrote:
> > On Sunday 28 December 2014 16:25:21 Gabriele Mazzotta wrote:
> > > On Sunday 28 December 2014 09:46:19 Pali Rohár wrote:
> > > > Ok, here are new patches for testing... Those you are
> > > > still reading this email thread and have your Dell
> > > > machines near, can you test them (ideally with
> > > > disabling dmi config data)?
> > > 
> > > Could you please tell me exactly against what should I
> > > apply these patches? I can see that they depend on other
> > > patches in this thread, but I can't find any tree that
> > > includes them.
> > 
> > Two patches in previous email could apply on 3.19-rc1 tree.
> 
> I see that the second patch you've just sent depends on the
> followings: https://patchwork.kernel.org/patch/5524791/
> (I8K_SMM_GET_TEMP_TYPE)
> https://patchwork.kernel.org/patch/5524731/ (Copyright)
> 
> If I'm not wrong, they are not included in 3.19-rc1.

Yes, they are not included in 3.19-rc1 but patches from previous 
mail should work without them.
Gabriele Mazzotta Dec. 28, 2014, 4:17 p.m. UTC | #21
On Sunday 28 December 2014 17:07:43 Pali Rohár wrote:
> On Sunday 28 December 2014 17:02:35 Gabriele Mazzotta wrote:
> > On Sunday 28 December 2014 16:48:54 Pali Rohár wrote:
> > > On Sunday 28 December 2014 16:25:21 Gabriele Mazzotta wrote:
> > > > On Sunday 28 December 2014 09:46:19 Pali Rohár wrote:
> > > > > Ok, here are new patches for testing... Those you are
> > > > > still reading this email thread and have your Dell
> > > > > machines near, can you test them (ideally with
> > > > > disabling dmi config data)?
> > > > 
> > > > Could you please tell me exactly against what should I
> > > > apply these patches? I can see that they depend on other
> > > > patches in this thread, but I can't find any tree that
> > > > includes them.
> > > 
> > > Two patches in previous email could apply on 3.19-rc1 tree.
> > 
> > I see that the second patch you've just sent depends on the
> > followings: https://patchwork.kernel.org/patch/5524791/
> > (I8K_SMM_GET_TEMP_TYPE)
> > https://patchwork.kernel.org/patch/5524731/ (Copyright)
> > 
> > If I'm not wrong, they are not included in 3.19-rc1.
> 
> Yes, they are not included in 3.19-rc1 but patches from previous 
> mail should work without them.

OK, I wanted to double check in case I had something missing.

Patches tested on my XPS13: the correct values for fan_mult and fan_man
are automatically selected.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pali Rohár Dec. 29, 2014, 12:22 p.m. UTC | #22
On Sunday 28 December 2014 17:17:14 Gabriele Mazzotta wrote:
> OK, I wanted to double check in case I had something missing.
> 
> Patches tested on my XPS13: the correct values for fan_mult
> and fan_man are automatically selected.

Great. Are there any other problems? Now probe time when loading 
module should be lower and could not freeze system, right?
Gabriele Mazzotta Dec. 29, 2014, 12:50 p.m. UTC | #23
On Monday 29 December 2014 13:22:52 Pali Rohár wrote:
> On Sunday 28 December 2014 17:17:14 Gabriele Mazzotta wrote:
> > OK, I wanted to double check in case I had something missing.
> > 
> > Patches tested on my XPS13: the correct values for fan_mult
> > and fan_man are automatically selected.
> 
> Great. Are there any other problems? Now probe time when loading 
> module should be lower and could not freeze system, right?

Yes, the system doesn't hang on module load. The only problem I have
right night now is that the system temporarily hangs when I read or set
the speed of the fan.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Guenter Roeck Dec. 30, 2014, 7:35 a.m. UTC | #24
On Mon, Dec 29, 2014 at 01:50:22PM +0100, Gabriele Mazzotta wrote:
> On Monday 29 December 2014 13:22:52 Pali Rohár wrote:
> > On Sunday 28 December 2014 17:17:14 Gabriele Mazzotta wrote:
> > > OK, I wanted to double check in case I had something missing.
> > > 
> > > Patches tested on my XPS13: the correct values for fan_mult
> > > and fan_man are automatically selected.
> > 
> > Great. Are there any other problems? Now probe time when loading 
> > module should be lower and could not freeze system, right?
> 
> Yes, the system doesn't hang on module load. The only problem I have
> right night now is that the system temporarily hangs when I read or set
> the speed of the fan.

This is a known problem on some Dell laptops; one of mine has the same problem.
Nothing we can do about that, unfortunately. Only thing you can do is
to not read the fan speed.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
index 8bdbed2..bf74644 100644
--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -725,42 +725,6 @@  static int __init i8k_init_hwmon(void)
 	return 0;
 }
 
-struct i8k_config_data {
-	int fan_mult;
-	int fan_max;
-};
-
-enum i8k_configs {
-	DELL_LATITUDE_D520,
-	DELL_LATITUDE_E6540,
-	DELL_PRECISION_490,
-	DELL_STUDIO,
-	DELL_XPS_M140,
-};
-
-static const struct i8k_config_data i8k_config_data[] = {
-	[DELL_LATITUDE_D520] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_TURBO,
-	},
-	[DELL_LATITUDE_E6540] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_HIGH,
-	},
-	[DELL_PRECISION_490] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_TURBO,
-	},
-	[DELL_STUDIO] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_HIGH,
-	},
-	[DELL_XPS_M140] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_HIGH,
-	},
-};
-
 static struct dmi_system_id i8k_dmi_table[] __initdata = {
 	{
 		.ident = "Dell Inspiron",
@@ -784,30 +748,6 @@  static struct dmi_system_id i8k_dmi_table[] __initdata = {
 		},
 	},
 	{
-		.ident = "Dell Latitude D520",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D520"),
-		},
-		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_D520],
-	},
-	{
-		.ident = "Dell Latitude E6440",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
-		},
-		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_E6540],
-	},
-	{
-		.ident = "Dell Latitude E6540",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
-		},
-		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_E6540],
-	},
-	{
 		.ident = "Dell Latitude 2",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
@@ -822,22 +762,13 @@  static struct dmi_system_id i8k_dmi_table[] __initdata = {
 		},
 	},
 	{
-		.ident = "Dell Inspiron 3",
+		.ident = "Dell Inspiron 4",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
 			DMI_MATCH(DMI_PRODUCT_NAME, "MP061"),
 		},
 	},
 	{
-		.ident = "Dell Precision 490",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME,
-				  "Precision WorkStation 490"),
-		},
-		.driver_data = (void *)&i8k_config_data[DELL_PRECISION_490],
-	},
-	{
 		.ident = "Dell Precision",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
@@ -864,7 +795,6 @@  static struct dmi_system_id i8k_dmi_table[] __initdata = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
 			DMI_MATCH(DMI_PRODUCT_NAME, "Studio"),
 		},
-		.driver_data = (void *)&i8k_config_data[DELL_STUDIO],
 	},
 	{
 		.ident = "Dell XPS M140",
@@ -872,7 +802,6 @@  static struct dmi_system_id i8k_dmi_table[] __initdata = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
 			DMI_MATCH(DMI_PRODUCT_NAME, "MXC051"),
 		},
-		.driver_data = (void *)&i8k_config_data[DELL_XPS_M140],
 	},
 	{ }
 };
@@ -884,8 +813,6 @@  MODULE_DEVICE_TABLE(dmi, i8k_dmi_table);
  */
 static int __init i8k_probe(void)
 {
-	const struct i8k_config_data *conf;
-	const struct dmi_system_id *id;
 	int fan, val, ret;
 
 	/*
@@ -915,19 +842,6 @@  static int __init i8k_probe(void)
 			return -ENODEV;
 	}
 
-	/*
-	 * Autodetect fan multiplier and maximal fan speed from dmi config
-	 * Values specified in module parameters override values from dmi
-	 */
-	id = dmi_first_match(i8k_dmi_table);
-	if (id && id->driver_data) {
-		conf = id->driver_data;
-		if (fan_mult <= 0 && conf->fan_mult > 0)
-			fan_mult = conf->fan_mult;
-		if (fan_max <= 0 && conf->fan_max > 0)
-			fan_max = conf->fan_max;
-	}
-
 	if (fan_mult <= 0) {
 		/*
 		 * Autodetect fan multiplier for each fan based on nominal rpm