[PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[Michele Sorcinelli]

Allow the module to be loaded on Dell XPS 9570, without having to use
the "force=1" option. The module loads without problems, and reports
correct fan values:

    $ time cat /proc/i8k
    1.0 1.5 -1 35 0 0 0 0 -1 -22
    cat /proc/i8k  0.00s user 0.00s system 7% cpu 0.033 total

However, the call may freeze the kernel for a very small time due to
code running in the SSM layer. This is a known issue with the driver, and
can be reproduced with other supported models. Average execution
time is 33 ms.

The command line tools from i8kutils can properly set the fan speed,
although the firmware will override it, unless automatic fan
control is disabled with the proper SSM call.

Average fans speed (when firwmare automatic control is off):

STATE -> RPM
0 0 -> 0 0
1 1 -> 2500 2500
2 2 -> 5100 5100
3 3 -> same as 2 2
---
 drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 9d3ef879d..367a8a617 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1017,6 +1017,13 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9560"),
 		},
 	},
+	{
+		.ident = "Dell XPS 15 9570",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9570"),
+		},
+	},
 	{ }
 };
 
-- 
2.19.2

Re: [PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[Pali Rohár]

On Tuesday 27 November 2018 23:06:37 Michele Sorcinelli wrote:
> Allow the module to be loaded on Dell XPS 9570, without having to use
> the "force=1" option. The module loads without problems, and reports
> correct fan values:
> 
>     $ time cat /proc/i8k
>     1.0 1.5 -1 35 0 0 0 0 -1 -22
>     cat /proc/i8k  0.00s user 0.00s system 7% cpu 0.033 total

Hi! This interface is deprecated and should not be used in new project
or software...

> However, the call may freeze the kernel for a very small time due to
> code running in the SSM layer. This is a known issue with the driver, and
> can be reproduced with other supported models. Average execution
> time is 33 ms.
> 
> The command line tools from i8kutils can properly set the fan speed,
> although the firmware will override it, unless automatic fan
> control is disabled with the proper SSM call.

i8kutils uses /proc/i8k too.

Can you check standard hwmon interface via lm-sensors / sensors command
is also working correctly? If you do not have sensors command you can
also look into /sys/class/hwmon/

> Average fans speed (when firwmare automatic control is off):
> 
> STATE -> RPM
> 0 0 -> 0 0
> 1 1 -> 2500 2500
> 2 2 -> 5100 5100
> 3 3 -> same as 2 2
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 9d3ef879d..367a8a617 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -1017,6 +1017,13 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9560"),
>  		},
>  	},
> +	{
> +		.ident = "Dell XPS 15 9570",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9570"),
> +		},
> +	},
>  	{ }
>  };
>  

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

Re: [PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[Michele Sorcinelli]

The output of sensors related to the is:

dell_smm-virtual-0
Adapter: Virtual device
fan1:           0 RPM
fan2:           0 RPM

This suggests that temperature sensors are not discovered.

Speeds reported in the hwmon interface are consistent with the others.


On 11/27/18 11:06 PM, Michele Sorcinelli wrote:
> Allow the module to be loaded on Dell XPS 9570, without having to use
> the "force=1" option. The module loads without problems, and reports
> correct fan values:
> 
>      $ time cat /proc/i8k
>      1.0 1.5 -1 35 0 0 0 0 -1 -22
>      cat /proc/i8k  0.00s user 0.00s system 7% cpu 0.033 total
> 
> However, the call may freeze the kernel for a very small time due to
> code running in the SSM layer. This is a known issue with the driver, and
> can be reproduced with other supported models. Average execution
> time is 33 ms.
> 
> The command line tools from i8kutils can properly set the fan speed,
> although the firmware will override it, unless automatic fan
> control is disabled with the proper SSM call.
> 
> Average fans speed (when firwmare automatic control is off):
> 
> STATE -> RPM
> 0 0 -> 0 0
> 1 1 -> 2500 2500
> 2 2 -> 5100 5100
> 3 3 -> same as 2 2
> ---
>   drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 9d3ef879d..367a8a617 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -1017,6 +1017,13 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {
>   			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9560"),
>   		},
>   	},
> +	{
> +		.ident = "Dell XPS 15 9570",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9570"),
> +		},
> +	},
>   	{ }
>   };
>   
> 

Re: [PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[Pali Rohár]

On Wednesday 28 November 2018 12:30:31 Michele Sorcinelli wrote:
> The output of sensors related to the is:
> 
> dell_smm-virtual-0
> Adapter: Virtual device
> fan1:           0 RPM
> fan2:           0 RPM

Ok, this means that both fans are turned off.

When you start fans, have both number meaningful/correct value?

> This suggests that temperature sensors are not discovered.

Yes, probably they are unsupported or not discovered. But this is not
a problem.

> Speeds reported in the hwmon interface are consistent with the others.
> 
> 
> On 11/27/18 11:06 PM, Michele Sorcinelli wrote:
> > Allow the module to be loaded on Dell XPS 9570, without having to use
> > the "force=1" option. The module loads without problems, and reports
> > correct fan values:
> > 
> >      $ time cat /proc/i8k
> >      1.0 1.5 -1 35 0 0 0 0 -1 -22
> >      cat /proc/i8k  0.00s user 0.00s system 7% cpu 0.033 total
> > 
> > However, the call may freeze the kernel for a very small time due to
> > code running in the SSM layer. This is a known issue with the driver, and
> > can be reproduced with other supported models. Average execution
> > time is 33 ms.
> > 
> > The command line tools from i8kutils can properly set the fan speed,
> > although the firmware will override it, unless automatic fan
> > control is disabled with the proper SSM call.
> > 
> > Average fans speed (when firwmare automatic control is off):
> > 
> > STATE -> RPM
> > 0 0 -> 0 0
> > 1 1 -> 2500 2500
> > 2 2 -> 5100 5100
> > 3 3 -> same as 2 2
> > ---
> >   drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > index 9d3ef879d..367a8a617 100644
> > --- a/drivers/hwmon/dell-smm-hwmon.c
> > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > @@ -1017,6 +1017,13 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {
> >   			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9560"),
> >   		},
> >   	},
> > +	{
> > +		.ident = "Dell XPS 15 9570",
> > +		.matches = {
> > +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9570"),
> > +		},
> > +	},
> >   	{ }
> >   };
> > 

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

Re: [PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[Michele Sorcinelli]

Yes, values are reported correctly when the fan are on. I tested it
by disabling the firmware automatic control using 
https://github.com/TomFreudenberg/dell-bios-fan-control and then running 
i8kfan 1 1 and i8kfan 2 2, which brought the fans to 2500 and 5100 
respectively.

When automatic control is on I can see intermediate speeds as well.

Is there anything that can be done to help the driver to discover
the temperature sensors?

On 11/28/18 12:56 PM, Pali Rohár wrote:
> On Wednesday 28 November 2018 12:30:31 Michele Sorcinelli wrote:
>> The output of sensors related to the is:
>>
>> dell_smm-virtual-0
>> Adapter: Virtual device
>> fan1:           0 RPM
>> fan2:           0 RPM
> 
> Ok, this means that both fans are turned off.
> 
> When you start fans, have both number meaningful/correct value?
> 
>> This suggests that temperature sensors are not discovered.
> 
> Yes, probably they are unsupported or not discovered. But this is not
> a problem.
> 
>> Speeds reported in the hwmon interface are consistent with the others.
>>
>>
>> On 11/27/18 11:06 PM, Michele Sorcinelli wrote:
>>> Allow the module to be loaded on Dell XPS 9570, without having to use
>>> the "force=1" option. The module loads without problems, and reports
>>> correct fan values:
>>>
>>>       $ time cat /proc/i8k
>>>       1.0 1.5 -1 35 0 0 0 0 -1 -22
>>>       cat /proc/i8k  0.00s user 0.00s system 7% cpu 0.033 total
>>>
>>> However, the call may freeze the kernel for a very small time due to
>>> code running in the SSM layer. This is a known issue with the driver, and
>>> can be reproduced with other supported models. Average execution
>>> time is 33 ms.
>>>
>>> The command line tools from i8kutils can properly set the fan speed,
>>> although the firmware will override it, unless automatic fan
>>> control is disabled with the proper SSM call.
>>>
>>> Average fans speed (when firwmare automatic control is off):
>>>
>>> STATE -> RPM
>>> 0 0 -> 0 0
>>> 1 1 -> 2500 2500
>>> 2 2 -> 5100 5100
>>> 3 3 -> same as 2 2
>>> ---
>>>    drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>>> index 9d3ef879d..367a8a617 100644
>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>> @@ -1017,6 +1017,13 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {
>>>    			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9560"),
>>>    		},
>>>    	},
>>> +	{
>>> +		.ident = "Dell XPS 15 9570",
>>> +		.matches = {
>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9570"),
>>> +		},
>>> +	},
>>>    	{ }
>>>    };
>>>
> 

Re: [PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[Michele Sorcinelli]

I tried to debug the temperature sensor probing with some printk()
around the code... it looks like the ssm calls are all returning -22
as result:

[   90.565793] Calling i8k_get_temp_type(0)
[   90.565795] Inside i8k_get_temp_type() with sensors = 0
[   90.565795] Performing i8k_smm call
[   90.567708] Result of the i8k_smm call: -22
[   90.567708] Return value of i8k_get_temp_type: -22
[   90.567709] Result of i8k_get_temp_type(0): -22
[   90.567709] Calling i8k_get_temp_type(1)
[   90.567710] Inside i8k_get_temp_type() with sensors = 1
[   90.567710] Performing i8k_smm call
[   90.568567] Result of the i8k_smm call: -22
[   90.568567] Return value of i8k_get_temp_type: -22
[   90.568568] Result of i8k_get_temp_type(0): -22
[   90.568568] Calling i8k_get_temp_type(2)
[   90.568568] Inside i8k_get_temp_type() with sensors = 2
[   90.568569] Performing i8k_smm call
[   90.569441] Result of the i8k_smm call: -22
[   90.569441] Return value of i8k_get_temp_type: -22
[   90.569442] Result of i8k_get_temp_type(0): -22
[   90.569442] Calling i8k_get_temp_type(3)
[   90.569442] Inside i8k_get_temp_type() with sensors = 3
[   90.569443] Performing i8k_smm call
[   90.570321] Result of the i8k_smm call: -22
[   90.570322] Return value of i8k_get_temp_type: -22
[   90.570322] Result of i8k_get_temp_type(0): -22

So this must be the reason for which sensors aren't detected.

AFAIK, SSM code is much of a black-box and finding how to fix
this will probably require some reverse engineering.

However, apart from this and the fact that kernel hangs during
the SSM call (that is reasonable and again not much can't be done),
everything looks fine.

Let me know if you want me to include more information in the commit 
message for the patch or want me to do more testing.

On 11/28/18 1:01 PM, Michele Sorcinelli wrote:
> Yes, values are reported correctly when the fan are on. I tested it
> by disabling the firmware automatic control using 
> https://github.com/TomFreudenberg/dell-bios-fan-control and then running 
> i8kfan 1 1 and i8kfan 2 2, which brought the fans to 2500 and 5100 
> respectively.
> 
> When automatic control is on I can see intermediate speeds as well.
> 
> Is there anything that can be done to help the driver to discover
> the temperature sensors?
> 
> On 11/28/18 12:56 PM, Pali Rohár wrote:
>> On Wednesday 28 November 2018 12:30:31 Michele Sorcinelli wrote:
>>> The output of sensors related to the is:
>>>
>>> dell_smm-virtual-0
>>> Adapter: Virtual device
>>> fan1:           0 RPM
>>> fan2:           0 RPM
>>
>> Ok, this means that both fans are turned off.
>>
>> When you start fans, have both number meaningful/correct value?
>>
>>> This suggests that temperature sensors are not discovered.
>>
>> Yes, probably they are unsupported or not discovered. But this is not
>> a problem.
>>
>>> Speeds reported in the hwmon interface are consistent with the others.
>>>
>>>
>>> On 11/27/18 11:06 PM, Michele Sorcinelli wrote:
>>>> Allow the module to be loaded on Dell XPS 9570, without having to use
>>>> the "force=1" option. The module loads without problems, and reports
>>>> correct fan values:
>>>>
>>>>       $ time cat /proc/i8k
>>>>       1.0 1.5 -1 35 0 0 0 0 -1 -22
>>>>       cat /proc/i8k  0.00s user 0.00s system 7% cpu 0.033 total
>>>>
>>>> However, the call may freeze the kernel for a very small time due to
>>>> code running in the SSM layer. This is a known issue with the 
>>>> driver, and
>>>> can be reproduced with other supported models. Average execution
>>>> time is 33 ms.
>>>>
>>>> The command line tools from i8kutils can properly set the fan speed,
>>>> although the firmware will override it, unless automatic fan
>>>> control is disabled with the proper SSM call.
>>>>
>>>> Average fans speed (when firwmare automatic control is off):
>>>>
>>>> STATE -> RPM
>>>> 0 0 -> 0 0
>>>> 1 1 -> 2500 2500
>>>> 2 2 -> 5100 5100
>>>> 3 3 -> same as 2 2
>>>> ---
>>>>    drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
>>>>    1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c 
>>>> b/drivers/hwmon/dell-smm-hwmon.c
>>>> index 9d3ef879d..367a8a617 100644
>>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>>> @@ -1017,6 +1017,13 @@ static const struct dmi_system_id 
>>>> i8k_dmi_table[] __initconst = {
>>>>                DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9560"),
>>>>            },
>>>>        },
>>>> +    {
>>>> +        .ident = "Dell XPS 15 9570",
>>>> +        .matches = {
>>>> +            DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +            DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9570"),
>>>> +        },
>>>> +    },
>>>>        { }
>>>>    };
>>>>
>>

Re: [PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[Pali Rohár]

On Wednesday 28 November 2018 23:23:07 Michele Sorcinelli wrote:
> I tried to debug the temperature sensor probing with some printk()
> around the code... it looks like the ssm calls are all returning -22
> as result:
> 
> [   90.565793] Calling i8k_get_temp_type(0)
> [   90.565795] Inside i8k_get_temp_type() with sensors = 0
> [   90.565795] Performing i8k_smm call
> [   90.567708] Result of the i8k_smm call: -22
> [   90.567708] Return value of i8k_get_temp_type: -22
> [   90.567709] Result of i8k_get_temp_type(0): -22
> [   90.567709] Calling i8k_get_temp_type(1)
> [   90.567710] Inside i8k_get_temp_type() with sensors = 1
> [   90.567710] Performing i8k_smm call
> [   90.568567] Result of the i8k_smm call: -22
> [   90.568567] Return value of i8k_get_temp_type: -22
> [   90.568568] Result of i8k_get_temp_type(0): -22
> [   90.568568] Calling i8k_get_temp_type(2)
> [   90.568568] Inside i8k_get_temp_type() with sensors = 2
> [   90.568569] Performing i8k_smm call
> [   90.569441] Result of the i8k_smm call: -22
> [   90.569441] Return value of i8k_get_temp_type: -22
> [   90.569442] Result of i8k_get_temp_type(0): -22
> [   90.569442] Calling i8k_get_temp_type(3)
> [   90.569442] Inside i8k_get_temp_type() with sensors = 3
> [   90.569443] Performing i8k_smm call
> [   90.570321] Result of the i8k_smm call: -22
> [   90.570322] Return value of i8k_get_temp_type: -22
> [   90.570322] Result of i8k_get_temp_type(0): -22
> 
> So this must be the reason for which sensors aren't detected.

Yes. SMM just say "I do not support this temp sensor". Also it is
possible that Dell's SMM does not implement it at all.

You can try to play with it more and check if temp sensor does not have
higher index.

But I cannot help more. As you said it is RPC black-box.

> AFAIK, SSM code is much of a black-box and finding how to fix
> this will probably require some reverse engineering.
> 
> However, apart from this and the fact that kernel hangs during
> the SSM call (that is reasonable and again not much can't be done),
> everything looks fine.
> 
> Let me know if you want me to include more information in the commit message
> for the patch or want me to do more testing.

That is enough. You can add my

Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

and hwmon maintainers could pick up this patch.

> On 11/28/18 1:01 PM, Michele Sorcinelli wrote:
> > Yes, values are reported correctly when the fan are on. I tested it
> > by disabling the firmware automatic control using
> > https://github.com/TomFreudenberg/dell-bios-fan-control and then running
> > i8kfan 1 1 and i8kfan 2 2, which brought the fans to 2500 and 5100
> > respectively.
> > 
> > When automatic control is on I can see intermediate speeds as well.
> > 
> > Is there anything that can be done to help the driver to discover
> > the temperature sensors?
> > 
> > On 11/28/18 12:56 PM, Pali Rohár wrote:
> > > On Wednesday 28 November 2018 12:30:31 Michele Sorcinelli wrote:
> > > > The output of sensors related to the is:
> > > > 
> > > > dell_smm-virtual-0
> > > > Adapter: Virtual device
> > > > fan1:           0 RPM
> > > > fan2:           0 RPM
> > > 
> > > Ok, this means that both fans are turned off.
> > > 
> > > When you start fans, have both number meaningful/correct value?
> > > 
> > > > This suggests that temperature sensors are not discovered.
> > > 
> > > Yes, probably they are unsupported or not discovered. But this is not
> > > a problem.
> > > 
> > > > Speeds reported in the hwmon interface are consistent with the others.
> > > > 
> > > > 
> > > > On 11/27/18 11:06 PM, Michele Sorcinelli wrote:
> > > > > Allow the module to be loaded on Dell XPS 9570, without having to use
> > > > > the "force=1" option. The module loads without problems, and reports
> > > > > correct fan values:
> > > > > 
> > > > >       $ time cat /proc/i8k
> > > > >       1.0 1.5 -1 35 0 0 0 0 -1 -22
> > > > >       cat /proc/i8k  0.00s user 0.00s system 7% cpu 0.033 total
> > > > > 
> > > > > However, the call may freeze the kernel for a very small time due to
> > > > > code running in the SSM layer. This is a known issue with
> > > > > the driver, and
> > > > > can be reproduced with other supported models. Average execution
> > > > > time is 33 ms.
> > > > > 
> > > > > The command line tools from i8kutils can properly set the fan speed,
> > > > > although the firmware will override it, unless automatic fan
> > > > > control is disabled with the proper SSM call.
> > > > > 
> > > > > Average fans speed (when firwmare automatic control is off):
> > > > > 
> > > > > STATE -> RPM
> > > > > 0 0 -> 0 0
> > > > > 1 1 -> 2500 2500
> > > > > 2 2 -> 5100 5100
> > > > > 3 3 -> same as 2 2
> > > > > ---
> > > > >    drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
> > > > >    1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/hwmon/dell-smm-hwmon.c
> > > > > b/drivers/hwmon/dell-smm-hwmon.c
> > > > > index 9d3ef879d..367a8a617 100644
> > > > > --- a/drivers/hwmon/dell-smm-hwmon.c
> > > > > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > > > > @@ -1017,6 +1017,13 @@ static const struct dmi_system_id
> > > > > i8k_dmi_table[] __initconst = {
> > > > >                DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9560"),
> > > > >            },
> > > > >        },
> > > > > +    {
> > > > > +        .ident = "Dell XPS 15 9570",
> > > > > +        .matches = {
> > > > > +            DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > > > > +            DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9570"),
> > > > > +        },
> > > > > +    },
> > > > >        { }
> > > > >    };
> > > > > 
> > > 

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

Re: [PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[Michele Sorcinelli]

I've been playing with smm.c from a old version of i8kutils, that
is just an userspace utility to trigger the SMM RPC:

http://sprunge.us/llxG7R

I used that in combination with the following test script:

http://sprunge.us/izJmQK

I managed to run that on a XPS 9560 and 9570 and compare the results.

XPS 9560: http://sprunge.us/xCLNSy
XPS 9570: http://sprunge.us/wSFTKs

The main difference between 9560 and 9570 is that the calls to get
the type of either fan or temperature sensor are not working (they all 
return 0xfff in the eax register), where in the 9560 they're returning
proper values.

However, it looks like the call for reading values (fan speed or 
temperatures) are all returning proper values in 9570 as well.

I'm assuming the following:

- API should be the same between 9560 and 9670 (why should Dell change
   it just now, and just for a single feature?);
- in the 9670 the type calls are not implemented correctly;
- the sensor indexes should be correct (because they work when used to
   read the temperature directly)

I also tried using higher indexes but I always get 0xffff.

We could either wait for Dell to eventually release a firmware update
that implements those calls correctly or we could force the reading of
the temperatures even if the type is not returned properly (although
I didn't know how to map the sensor with the proper label).

I'm not keen to try other unknown procedures as I don't know what could
happen, but I've found that other documented procedures are working 
properly (eg. set fan speed, read firmware version, disable fan control,
enable fan control, etc.), so again I think the API shouldn't have changed.

Let me know your thoughts.

Thanks,
Michele.

On 11/29/18 9:48 AM, Pali Rohár wrote:
> On Wednesday 28 November 2018 23:23:07 Michele Sorcinelli wrote:
>> I tried to debug the temperature sensor probing with some printk()
>> around the code... it looks like the ssm calls are all returning -22
>> as result:
>>
>> [   90.565793] Calling i8k_get_temp_type(0)
>> [   90.565795] Inside i8k_get_temp_type() with sensors = 0
>> [   90.565795] Performing i8k_smm call
>> [   90.567708] Result of the i8k_smm call: -22
>> [   90.567708] Return value of i8k_get_temp_type: -22
>> [   90.567709] Result of i8k_get_temp_type(0): -22
>> [   90.567709] Calling i8k_get_temp_type(1)
>> [   90.567710] Inside i8k_get_temp_type() with sensors = 1
>> [   90.567710] Performing i8k_smm call
>> [   90.568567] Result of the i8k_smm call: -22
>> [   90.568567] Return value of i8k_get_temp_type: -22
>> [   90.568568] Result of i8k_get_temp_type(0): -22
>> [   90.568568] Calling i8k_get_temp_type(2)
>> [   90.568568] Inside i8k_get_temp_type() with sensors = 2
>> [   90.568569] Performing i8k_smm call
>> [   90.569441] Result of the i8k_smm call: -22
>> [   90.569441] Return value of i8k_get_temp_type: -22
>> [   90.569442] Result of i8k_get_temp_type(0): -22
>> [   90.569442] Calling i8k_get_temp_type(3)
>> [   90.569442] Inside i8k_get_temp_type() with sensors = 3
>> [   90.569443] Performing i8k_smm call
>> [   90.570321] Result of the i8k_smm call: -22
>> [   90.570322] Return value of i8k_get_temp_type: -22
>> [   90.570322] Result of i8k_get_temp_type(0): -22
>>
>> So this must be the reason for which sensors aren't detected.
> 
> Yes. SMM just say "I do not support this temp sensor". Also it is
> possible that Dell's SMM does not implement it at all.
> 
> You can try to play with it more and check if temp sensor does not have
> higher index.
> 
> But I cannot help more. As you said it is RPC black-box.
> 
>> AFAIK, SSM code is much of a black-box and finding how to fix
>> this will probably require some reverse engineering.
>>
>> However, apart from this and the fact that kernel hangs during
>> the SSM call (that is reasonable and again not much can't be done),
>> everything looks fine.
>>
>> Let me know if you want me to include more information in the commit message
>> for the patch or want me to do more testing.
> 
> That is enough. You can add my
> 
> Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
> 
> and hwmon maintainers could pick up this patch.
> 
>> On 11/28/18 1:01 PM, Michele Sorcinelli wrote:
>>> Yes, values are reported correctly when the fan are on. I tested it
>>> by disabling the firmware automatic control using
>>> https://github.com/TomFreudenberg/dell-bios-fan-control and then running
>>> i8kfan 1 1 and i8kfan 2 2, which brought the fans to 2500 and 5100
>>> respectively.
>>>
>>> When automatic control is on I can see intermediate speeds as well.
>>>
>>> Is there anything that can be done to help the driver to discover
>>> the temperature sensors?
>>>
>>> On 11/28/18 12:56 PM, Pali Rohár wrote:
>>>> On Wednesday 28 November 2018 12:30:31 Michele Sorcinelli wrote:
>>>>> The output of sensors related to the is:
>>>>>
>>>>> dell_smm-virtual-0
>>>>> Adapter: Virtual device
>>>>> fan1:           0 RPM
>>>>> fan2:           0 RPM
>>>>
>>>> Ok, this means that both fans are turned off.
>>>>
>>>> When you start fans, have both number meaningful/correct value?
>>>>
>>>>> This suggests that temperature sensors are not discovered.
>>>>
>>>> Yes, probably they are unsupported or not discovered. But this is not
>>>> a problem.
>>>>
>>>>> Speeds reported in the hwmon interface are consistent with the others.
>>>>>
>>>>>
>>>>> On 11/27/18 11:06 PM, Michele Sorcinelli wrote:
>>>>>> Allow the module to be loaded on Dell XPS 9570, without having to use
>>>>>> the "force=1" option. The module loads without problems, and reports
>>>>>> correct fan values:
>>>>>>
>>>>>>        $ time cat /proc/i8k
>>>>>>        1.0 1.5 -1 35 0 0 0 0 -1 -22
>>>>>>        cat /proc/i8k  0.00s user 0.00s system 7% cpu 0.033 total
>>>>>>
>>>>>> However, the call may freeze the kernel for a very small time due to
>>>>>> code running in the SSM layer. This is a known issue with
>>>>>> the driver, and
>>>>>> can be reproduced with other supported models. Average execution
>>>>>> time is 33 ms.
>>>>>>
>>>>>> The command line tools from i8kutils can properly set the fan speed,
>>>>>> although the firmware will override it, unless automatic fan
>>>>>> control is disabled with the proper SSM call.
>>>>>>
>>>>>> Average fans speed (when firwmare automatic control is off):
>>>>>>
>>>>>> STATE -> RPM
>>>>>> 0 0 -> 0 0
>>>>>> 1 1 -> 2500 2500
>>>>>> 2 2 -> 5100 5100
>>>>>> 3 3 -> same as 2 2
>>>>>> ---
>>>>>>     drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
>>>>>>     1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c
>>>>>> b/drivers/hwmon/dell-smm-hwmon.c
>>>>>> index 9d3ef879d..367a8a617 100644
>>>>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>>>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>>>>> @@ -1017,6 +1017,13 @@ static const struct dmi_system_id
>>>>>> i8k_dmi_table[] __initconst = {
>>>>>>                 DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9560"),
>>>>>>             },
>>>>>>         },
>>>>>> +    {
>>>>>> +        .ident = "Dell XPS 15 9570",
>>>>>> +        .matches = {
>>>>>> +            DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>>>> +            DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9570"),
>>>>>> +        },
>>>>>> +    },
>>>>>>         { }
>>>>>>     };
>>>>>>
>>>>
> 

Re: [PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[Pali Rohár]

Hi!

On Thursday 29 November 2018 21:42:54 Michele Sorcinelli wrote:
> I've been playing with smm.c from a old version of i8kutils, that
> is just an userspace utility to trigger the SMM RPC:
> 
> http://sprunge.us/llxG7R
> 
> I used that in combination with the following test script:
> 
> http://sprunge.us/izJmQK
> 
> I managed to run that on a XPS 9560 and 9570 and compare the results.
> 
> XPS 9560: http://sprunge.us/xCLNSy
> XPS 9570: http://sprunge.us/wSFTKs
> 
> The main difference between 9560 and 9570 is that the calls to get
> the type of either fan or temperature sensor are not working (they all
> return 0xfff in the eax register), where in the 9560 they're returning
> proper values.

So it looks like that SMM on you machine does not support call to get
type of temperature sensor. And dell-smm-hwmon.c uses type of sensor to
detect if sensor is present or not.

Also from that output it looks like that you have 8 temperature sensors
present, but dell-smm-hwmon.c currently supports only first 4.

If you want to have support for temperature sensors on your machine, you
can look at code in i8k_init_hwmon() how fans are initialized. Code
there first check if fan status does not produce error OR fan type does
not produce error. And you can use same logic for temperature sensors.

And if you want to support all 8 temp sensors, you need to extend
dell-smm-hwmon.c code to support more then 4 temp sensors.

This means that you would not see temperature sensors names as your SMM
does not provide them. But it is a problem at all?

> However, it looks like the call for reading values (fan speed or
> temperatures) are all returning proper values in 9570 as well.
> 
> I'm assuming the following:
> 
> - API should be the same between 9560 and 9670 (why should Dell change
>   it just now, and just for a single feature?);

I think API was not changed. Just one RPC over SMM function is not
supported.

> - in the 9670 the type calls are not implemented correctly;
> - the sensor indexes should be correct (because they work when used to
>   read the temperature directly)
> 
> I also tried using higher indexes but I always get 0xffff.
> 
> We could either wait for Dell to eventually release a firmware update
> that implements those calls correctly or we could force the reading of
> the temperatures even if the type is not returned properly (although
> I didn't know how to map the sensor with the proper label).

Have you reported bug to Dell? If not, then Dell probably do not know
about it and so cannot fix anything.

> I'm not keen to try other unknown procedures as I don't know what could
> happen, but I've found that other documented procedures are working properly
> (eg. set fan speed, read firmware version, disable fan control,
> enable fan control, etc.), so again I think the API shouldn't have changed.
> 
> Let me know your thoughts.
> 
> Thanks,
> Michele.
> 
> On 11/29/18 9:48 AM, Pali Rohár wrote:
> > On Wednesday 28 November 2018 23:23:07 Michele Sorcinelli wrote:
> > > I tried to debug the temperature sensor probing with some printk()
> > > around the code... it looks like the ssm calls are all returning -22
> > > as result:
> > > 
> > > [   90.565793] Calling i8k_get_temp_type(0)
> > > [   90.565795] Inside i8k_get_temp_type() with sensors = 0
> > > [   90.565795] Performing i8k_smm call
> > > [   90.567708] Result of the i8k_smm call: -22
> > > [   90.567708] Return value of i8k_get_temp_type: -22
> > > [   90.567709] Result of i8k_get_temp_type(0): -22
> > > [   90.567709] Calling i8k_get_temp_type(1)
> > > [   90.567710] Inside i8k_get_temp_type() with sensors = 1
> > > [   90.567710] Performing i8k_smm call
> > > [   90.568567] Result of the i8k_smm call: -22
> > > [   90.568567] Return value of i8k_get_temp_type: -22
> > > [   90.568568] Result of i8k_get_temp_type(0): -22
> > > [   90.568568] Calling i8k_get_temp_type(2)
> > > [   90.568568] Inside i8k_get_temp_type() with sensors = 2
> > > [   90.568569] Performing i8k_smm call
> > > [   90.569441] Result of the i8k_smm call: -22
> > > [   90.569441] Return value of i8k_get_temp_type: -22
> > > [   90.569442] Result of i8k_get_temp_type(0): -22
> > > [   90.569442] Calling i8k_get_temp_type(3)
> > > [   90.569442] Inside i8k_get_temp_type() with sensors = 3
> > > [   90.569443] Performing i8k_smm call
> > > [   90.570321] Result of the i8k_smm call: -22
> > > [   90.570322] Return value of i8k_get_temp_type: -22
> > > [   90.570322] Result of i8k_get_temp_type(0): -22
> > > 
> > > So this must be the reason for which sensors aren't detected.
> > 
> > Yes. SMM just say "I do not support this temp sensor". Also it is
> > possible that Dell's SMM does not implement it at all.
> > 
> > You can try to play with it more and check if temp sensor does not have
> > higher index.
> > 
> > But I cannot help more. As you said it is RPC black-box.
> > 
> > > AFAIK, SSM code is much of a black-box and finding how to fix
> > > this will probably require some reverse engineering.
> > > 
> > > However, apart from this and the fact that kernel hangs during
> > > the SSM call (that is reasonable and again not much can't be done),
> > > everything looks fine.
> > > 
> > > Let me know if you want me to include more information in the commit message
> > > for the patch or want me to do more testing.
> > 
> > That is enough. You can add my
> > 
> > Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
> > 
> > and hwmon maintainers could pick up this patch.
> > 
> > > On 11/28/18 1:01 PM, Michele Sorcinelli wrote:
> > > > Yes, values are reported correctly when the fan are on. I tested it
> > > > by disabling the firmware automatic control using
> > > > https://github.com/TomFreudenberg/dell-bios-fan-control and then running
> > > > i8kfan 1 1 and i8kfan 2 2, which brought the fans to 2500 and 5100
> > > > respectively.
> > > > 
> > > > When automatic control is on I can see intermediate speeds as well.
> > > > 
> > > > Is there anything that can be done to help the driver to discover
> > > > the temperature sensors?
> > > > 
> > > > On 11/28/18 12:56 PM, Pali Rohár wrote:
> > > > > On Wednesday 28 November 2018 12:30:31 Michele Sorcinelli wrote:
> > > > > > The output of sensors related to the is:
> > > > > > 
> > > > > > dell_smm-virtual-0
> > > > > > Adapter: Virtual device
> > > > > > fan1:           0 RPM
> > > > > > fan2:           0 RPM
> > > > > 
> > > > > Ok, this means that both fans are turned off.
> > > > > 
> > > > > When you start fans, have both number meaningful/correct value?
> > > > > 
> > > > > > This suggests that temperature sensors are not discovered.
> > > > > 
> > > > > Yes, probably they are unsupported or not discovered. But this is not
> > > > > a problem.
> > > > > 
> > > > > > Speeds reported in the hwmon interface are consistent with the others.
> > > > > > 
> > > > > > 
> > > > > > On 11/27/18 11:06 PM, Michele Sorcinelli wrote:
> > > > > > > Allow the module to be loaded on Dell XPS 9570, without having to use
> > > > > > > the "force=1" option. The module loads without problems, and reports
> > > > > > > correct fan values:
> > > > > > > 
> > > > > > >        $ time cat /proc/i8k
> > > > > > >        1.0 1.5 -1 35 0 0 0 0 -1 -22
> > > > > > >        cat /proc/i8k  0.00s user 0.00s system 7% cpu 0.033 total
> > > > > > > 
> > > > > > > However, the call may freeze the kernel for a very small time due to
> > > > > > > code running in the SSM layer. This is a known issue with
> > > > > > > the driver, and
> > > > > > > can be reproduced with other supported models. Average execution
> > > > > > > time is 33 ms.
> > > > > > > 
> > > > > > > The command line tools from i8kutils can properly set the fan speed,
> > > > > > > although the firmware will override it, unless automatic fan
> > > > > > > control is disabled with the proper SSM call.
> > > > > > > 
> > > > > > > Average fans speed (when firwmare automatic control is off):
> > > > > > > 
> > > > > > > STATE -> RPM
> > > > > > > 0 0 -> 0 0
> > > > > > > 1 1 -> 2500 2500
> > > > > > > 2 2 -> 5100 5100
> > > > > > > 3 3 -> same as 2 2
> > > > > > > ---
> > > > > > >     drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
> > > > > > >     1 file changed, 7 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/hwmon/dell-smm-hwmon.c
> > > > > > > b/drivers/hwmon/dell-smm-hwmon.c
> > > > > > > index 9d3ef879d..367a8a617 100644
> > > > > > > --- a/drivers/hwmon/dell-smm-hwmon.c
> > > > > > > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > > > > > > @@ -1017,6 +1017,13 @@ static const struct dmi_system_id
> > > > > > > i8k_dmi_table[] __initconst = {
> > > > > > >                 DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9560"),
> > > > > > >             },
> > > > > > >         },
> > > > > > > +    {
> > > > > > > +        .ident = "Dell XPS 15 9570",
> > > > > > > +        .matches = {
> > > > > > > +            DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > > > > > > +            DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9570"),
> > > > > > > +        },
> > > > > > > +    },
> > > > > > >         { }
> > > > > > >     };
> > > > > > > 
> > > > > 
> > 

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

[PATCH] dell-smm-hwmon.c: Add support for additional temperature sensors

[Michele Sorcinelli]

The code has been extended to support up to 10 temperature sensors.

The i8k_get_temp_type() calls inside i8k_init_hwmon() have been replaced
with i8k_get_temp() as in some laptop firmwares the related SMM procedure
is not implemented correctly so i8k_get_temp_type() can't be used to
discover temperature sensors.

Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>
---
 drivers/hwmon/dell-smm-hwmon.c | 113 +++++++++++++++++++++++++++------
 1 file changed, 94 insertions(+), 19 deletions(-)
 mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
old mode 100644
new mode 100755
index 367a8a617..de96c9055
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -82,9 +82,16 @@ static bool disallow_fan_support;
 #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
 #define I8K_HWMON_HAVE_TEMP3	(1 << 2)
 #define I8K_HWMON_HAVE_TEMP4	(1 << 3)
-#define I8K_HWMON_HAVE_FAN1	(1 << 4)
-#define I8K_HWMON_HAVE_FAN2	(1 << 5)
-#define I8K_HWMON_HAVE_FAN3	(1 << 6)
+#define I8K_HWMON_HAVE_TEMP5	(1 << 4)
+#define I8K_HWMON_HAVE_TEMP6	(1 << 5)
+#define I8K_HWMON_HAVE_TEMP7	(1 << 6)
+#define I8K_HWMON_HAVE_TEMP8	(1 << 7)
+#define I8K_HWMON_HAVE_TEMP9	(1 << 8)
+#define I8K_HWMON_HAVE_TEMP10	(1 << 9)
+
+#define I8K_HWMON_HAVE_FAN1	(1 << 10)
+#define I8K_HWMON_HAVE_FAN2	(1 << 11)
+#define I8K_HWMON_HAVE_FAN3	(1 << 12)
 
 MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
 MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
@@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
 static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
 static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
 			  3);
+static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
+static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  4);
+static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
+static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  5);
+static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
+static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  6);
+static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
+static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  7);
+static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
+static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  8);
+static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
+static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  9);
+
 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);
@@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
 	&sensor_dev_attr_temp3_label.dev_attr.attr,	/* 5 */
 	&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_fan3_input.dev_attr.attr,	/* 14 */
-	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 15 */
-	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 16 */
+	&sensor_dev_attr_temp5_input.dev_attr.attr,	/* 8 */
+	&sensor_dev_attr_temp5_label.dev_attr.attr,	/* 9 */
+	&sensor_dev_attr_temp6_input.dev_attr.attr,	/* 10 */
+	&sensor_dev_attr_temp6_label.dev_attr.attr,	/* 11 */
+	&sensor_dev_attr_temp7_input.dev_attr.attr,	/* 12 */
+	&sensor_dev_attr_temp7_label.dev_attr.attr,	/* 13 */
+	&sensor_dev_attr_temp8_input.dev_attr.attr,	/* 14 */
+	&sensor_dev_attr_temp8_label.dev_attr.attr,	/* 15 */
+	&sensor_dev_attr_temp9_input.dev_attr.attr,	/* 16 */
+	&sensor_dev_attr_temp9_label.dev_attr.attr,	/* 17 */
+	&sensor_dev_attr_temp10_input.dev_attr.attr,	/* 18 */
+	&sensor_dev_attr_temp10_label.dev_attr.attr,	/* 19 */
+	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 20 */
+	&sensor_dev_attr_fan1_label.dev_attr.attr,	/* 21 */
+	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 22 */
+	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 23 */
+	&sensor_dev_attr_fan2_label.dev_attr.attr,	/* 24 */
+	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 25 */
+	&sensor_dev_attr_fan3_input.dev_attr.attr,	/* 26 */
+	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 27 */
+	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 28 */
 	NULL
 };
 
@@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
 	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_TEMP5))
+		return 0;
+	if (index >= 10 && index <= 11 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
+		return 0;
+	if (index >= 12 && index <= 13 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
+		return 0;
+	if (index >= 14 && index <= 15 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
+		return 0;
+	if (index >= 16 && index <= 17 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
+		return 0;
+	if (index >= 18 && index <= 19 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
+		return 0;
+
+	if (index >= 20 && index <= 21 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
 		return 0;
-	if (index >= 11 && index <= 13 &&
+	if (index >= 23 && index <= 24 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
 		return 0;
-	if (index >= 14 && index <= 16 &&
+	if (index >= 26 && index <= 27 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
 		return 0;
 
@@ -828,19 +885,37 @@ static int __init i8k_init_hwmon(void)
 	i8k_hwmon_flags = 0;
 
 	/* CPU temperature attributes, if temperature type is OK */
-	err = i8k_get_temp_type(0);
+	err = i8k_get_temp(0);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
 	/* check for additional temperature sensors */
-	err = i8k_get_temp_type(1);
+	err = i8k_get_temp(1);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
-	err = i8k_get_temp_type(2);
+	err = i8k_get_temp(2);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
-	err = i8k_get_temp_type(3);
+	err = i8k_get_temp(3);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
+	err = i8k_get_temp(4);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
+	err = i8k_get_temp(5);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
+	err = i8k_get_temp(6);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
+	err = i8k_get_temp(7);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
+	err = i8k_get_temp(8);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
+	err = i8k_get_temp(9);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;
 
 	/* First fan attributes, if fan status or type is OK */
 	err = i8k_get_fan_status(0);
-- 
2.19.2

Re: [PATCH] dell-smm-hwmon.c: Add support for additional temperature sensors

[Michele Sorcinelli]

I've just realized I've messed up with the pwm indexes in i8k_is_visible().

I'll send an amended patch.

Cheers,
Michele.

On 12/7/18 8:29 PM, Michele Sorcinelli wrote:
> The code has been extended to support up to 10 temperature sensors.
> 
> The i8k_get_temp_type() calls inside i8k_init_hwmon() have been replaced
> with i8k_get_temp() as in some laptop firmwares the related SMM procedure
> is not implemented correctly so i8k_get_temp_type() can't be used to
> discover temperature sensors.
> 
> Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>
> ---
>   drivers/hwmon/dell-smm-hwmon.c | 113 +++++++++++++++++++++++++++------
>   1 file changed, 94 insertions(+), 19 deletions(-)
>   mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> old mode 100644
> new mode 100755
> index 367a8a617..de96c9055
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -82,9 +82,16 @@ static bool disallow_fan_support;
>   #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
>   #define I8K_HWMON_HAVE_TEMP3	(1 << 2)
>   #define I8K_HWMON_HAVE_TEMP4	(1 << 3)
> -#define I8K_HWMON_HAVE_FAN1	(1 << 4)
> -#define I8K_HWMON_HAVE_FAN2	(1 << 5)
> -#define I8K_HWMON_HAVE_FAN3	(1 << 6)
> +#define I8K_HWMON_HAVE_TEMP5	(1 << 4)
> +#define I8K_HWMON_HAVE_TEMP6	(1 << 5)
> +#define I8K_HWMON_HAVE_TEMP7	(1 << 6)
> +#define I8K_HWMON_HAVE_TEMP8	(1 << 7)
> +#define I8K_HWMON_HAVE_TEMP9	(1 << 8)
> +#define I8K_HWMON_HAVE_TEMP10	(1 << 9)
> +
> +#define I8K_HWMON_HAVE_FAN1	(1 << 10)
> +#define I8K_HWMON_HAVE_FAN2	(1 << 11)
> +#define I8K_HWMON_HAVE_FAN3	(1 << 12)
>   
>   MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
>   MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
> @@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>   static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
>   static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>   			  3);
> +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  4);
> +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  5);
> +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
> +static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  6);
> +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
> +static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  7);
> +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
> +static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  8);
> +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
> +static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  9);
> +
>   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);
> @@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
>   	&sensor_dev_attr_temp3_label.dev_attr.attr,	/* 5 */
>   	&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_fan3_input.dev_attr.attr,	/* 14 */
> -	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 15 */
> -	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 16 */
> +	&sensor_dev_attr_temp5_input.dev_attr.attr,	/* 8 */
> +	&sensor_dev_attr_temp5_label.dev_attr.attr,	/* 9 */
> +	&sensor_dev_attr_temp6_input.dev_attr.attr,	/* 10 */
> +	&sensor_dev_attr_temp6_label.dev_attr.attr,	/* 11 */
> +	&sensor_dev_attr_temp7_input.dev_attr.attr,	/* 12 */
> +	&sensor_dev_attr_temp7_label.dev_attr.attr,	/* 13 */
> +	&sensor_dev_attr_temp8_input.dev_attr.attr,	/* 14 */
> +	&sensor_dev_attr_temp8_label.dev_attr.attr,	/* 15 */
> +	&sensor_dev_attr_temp9_input.dev_attr.attr,	/* 16 */
> +	&sensor_dev_attr_temp9_label.dev_attr.attr,	/* 17 */
> +	&sensor_dev_attr_temp10_input.dev_attr.attr,	/* 18 */
> +	&sensor_dev_attr_temp10_label.dev_attr.attr,	/* 19 */
> +	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 20 */
> +	&sensor_dev_attr_fan1_label.dev_attr.attr,	/* 21 */
> +	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 22 */
> +	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 23 */
> +	&sensor_dev_attr_fan2_label.dev_attr.attr,	/* 24 */
> +	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 25 */
> +	&sensor_dev_attr_fan3_input.dev_attr.attr,	/* 26 */
> +	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 27 */
> +	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 28 */
>   	NULL
>   };
>   
> @@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
>   	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_TEMP5))
> +		return 0;
> +	if (index >= 10 && index <= 11 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
> +		return 0;
> +	if (index >= 12 && index <= 13 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
> +		return 0;
> +	if (index >= 14 && index <= 15 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
> +		return 0;
> +	if (index >= 16 && index <= 17 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
> +		return 0;
> +	if (index >= 18 && index <= 19 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
> +		return 0;
> +
> +	if (index >= 20 && index <= 21 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
>   		return 0;
> -	if (index >= 11 && index <= 13 &&
> +	if (index >= 23 && index <= 24 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
>   		return 0;
> -	if (index >= 14 && index <= 16 &&
> +	if (index >= 26 && index <= 27 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
>   		return 0;
>   
> @@ -828,19 +885,37 @@ static int __init i8k_init_hwmon(void)
>   	i8k_hwmon_flags = 0;
>   
>   	/* CPU temperature attributes, if temperature type is OK */
> -	err = i8k_get_temp_type(0);
> +	err = i8k_get_temp(0);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
>   	/* check for additional temperature sensors */
> -	err = i8k_get_temp_type(1);
> +	err = i8k_get_temp(1);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
> -	err = i8k_get_temp_type(2);
> +	err = i8k_get_temp(2);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
> -	err = i8k_get_temp_type(3);
> +	err = i8k_get_temp(3);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
> +	err = i8k_get_temp(4);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
> +	err = i8k_get_temp(5);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
> +	err = i8k_get_temp(6);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
> +	err = i8k_get_temp(7);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
> +	err = i8k_get_temp(8);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
> +	err = i8k_get_temp(9);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;
>   
>   	/* First fan attributes, if fan status or type is OK */
>   	err = i8k_get_fan_status(0);
> 

[PATCH] dell-smm-hwmon.c: Additional temperature sensors

[Michele Sorcinelli]

The code has been extended to support up to 10 temperature sensors.

The i8k_get_temp_type() calls inside i8k_init_hwmon() have been replaced
with i8k_get_temp() as in some laptop firmwares the related SMM procedure
is not implemented correctly so i8k_get_temp_type() can't be used to
discover temperature sensors.

Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>
---
 drivers/hwmon/dell-smm-hwmon.c | 113 +++++++++++++++++++++++++++------
 1 file changed, 94 insertions(+), 19 deletions(-)
 mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
old mode 100644
new mode 100755
index 367a8a617..b58f756ea
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -82,9 +82,16 @@ static bool disallow_fan_support;
 #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
 #define I8K_HWMON_HAVE_TEMP3	(1 << 2)
 #define I8K_HWMON_HAVE_TEMP4	(1 << 3)
-#define I8K_HWMON_HAVE_FAN1	(1 << 4)
-#define I8K_HWMON_HAVE_FAN2	(1 << 5)
-#define I8K_HWMON_HAVE_FAN3	(1 << 6)
+#define I8K_HWMON_HAVE_TEMP5	(1 << 4)
+#define I8K_HWMON_HAVE_TEMP6	(1 << 5)
+#define I8K_HWMON_HAVE_TEMP7	(1 << 6)
+#define I8K_HWMON_HAVE_TEMP8	(1 << 7)
+#define I8K_HWMON_HAVE_TEMP9	(1 << 8)
+#define I8K_HWMON_HAVE_TEMP10	(1 << 9)
+
+#define I8K_HWMON_HAVE_FAN1	(1 << 10)
+#define I8K_HWMON_HAVE_FAN2	(1 << 11)
+#define I8K_HWMON_HAVE_FAN3	(1 << 12)
 
 MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
 MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
@@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
 static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
 static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
 			  3);
+static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
+static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  4);
+static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
+static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  5);
+static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
+static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  6);
+static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
+static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  7);
+static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
+static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  8);
+static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
+static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  9);
+
 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);
@@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
 	&sensor_dev_attr_temp3_label.dev_attr.attr,	/* 5 */
 	&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_fan3_input.dev_attr.attr,	/* 14 */
-	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 15 */
-	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 16 */
+	&sensor_dev_attr_temp5_input.dev_attr.attr,	/* 8 */
+	&sensor_dev_attr_temp5_label.dev_attr.attr,	/* 9 */
+	&sensor_dev_attr_temp6_input.dev_attr.attr,	/* 10 */
+	&sensor_dev_attr_temp6_label.dev_attr.attr,	/* 11 */
+	&sensor_dev_attr_temp7_input.dev_attr.attr,	/* 12 */
+	&sensor_dev_attr_temp7_label.dev_attr.attr,	/* 13 */
+	&sensor_dev_attr_temp8_input.dev_attr.attr,	/* 14 */
+	&sensor_dev_attr_temp8_label.dev_attr.attr,	/* 15 */
+	&sensor_dev_attr_temp9_input.dev_attr.attr,	/* 16 */
+	&sensor_dev_attr_temp9_label.dev_attr.attr,	/* 17 */
+	&sensor_dev_attr_temp10_input.dev_attr.attr,	/* 18 */
+	&sensor_dev_attr_temp10_label.dev_attr.attr,	/* 19 */
+	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 20 */
+	&sensor_dev_attr_fan1_label.dev_attr.attr,	/* 21 */
+	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 22 */
+	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 23 */
+	&sensor_dev_attr_fan2_label.dev_attr.attr,	/* 24 */
+	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 25 */
+	&sensor_dev_attr_fan3_input.dev_attr.attr,	/* 26 */
+	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 27 */
+	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 28 */
 	NULL
 };
 
@@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
 	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_TEMP5))
+		return 0;
+	if (index >= 10 && index <= 11 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
+		return 0;
+	if (index >= 12 && index <= 13 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
+		return 0;
+	if (index >= 14 && index <= 15 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
+		return 0;
+	if (index >= 16 && index <= 17 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
+		return 0;
+	if (index >= 18 && index <= 19 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
+		return 0;
+
+	if (index >= 20 && index <= 22 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
 		return 0;
-	if (index >= 11 && index <= 13 &&
+	if (index >= 23 && index <= 25 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
 		return 0;
-	if (index >= 14 && index <= 16 &&
+	if (index >= 26 && index <= 28 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
 		return 0;
 
@@ -828,19 +885,37 @@ static int __init i8k_init_hwmon(void)
 	i8k_hwmon_flags = 0;
 
 	/* CPU temperature attributes, if temperature type is OK */
-	err = i8k_get_temp_type(0);
+	err = i8k_get_temp(0);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
 	/* check for additional temperature sensors */
-	err = i8k_get_temp_type(1);
+	err = i8k_get_temp(1);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
-	err = i8k_get_temp_type(2);
+	err = i8k_get_temp(2);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
-	err = i8k_get_temp_type(3);
+	err = i8k_get_temp(3);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
+	err = i8k_get_temp(4);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
+	err = i8k_get_temp(5);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
+	err = i8k_get_temp(6);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
+	err = i8k_get_temp(7);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
+	err = i8k_get_temp(8);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
+	err = i8k_get_temp(9);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;
 
 	/* First fan attributes, if fan status or type is OK */
 	err = i8k_get_fan_status(0);
-- 
2.19.2

Re: [PATCH] dell-smm-hwmon.c: Additional temperature sensors

[Guenter Roeck]

On 12/7/18 4:56 PM, Michele Sorcinelli wrote:
> The code has been extended to support up to 10 temperature sensors.
> 
> The i8k_get_temp_type() calls inside i8k_init_hwmon() have been replaced
> with i8k_get_temp() as in some laptop firmwares the related SMM procedure
> is not implemented correctly so i8k_get_temp_type() can't be used to
> discover temperature sensors.
> 
> Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>

Please _always_ version your patches, and add a changelog, at least if you
would like to see it applied. I have now two patches which look almost the same,
meaning I would have to pull both versions, compare, and hope to apply the
correct one.

Anyway, I would like to get some feedback if this can cause regressions
on systems which don't support that many sensors and maybe report something
completely different if one tries to read the high-numbered sensors.
I seem to recall that there was a reason for checking the type and not just
trying to read sensor values.

Guenter

> ---
>   drivers/hwmon/dell-smm-hwmon.c | 113 +++++++++++++++++++++++++++------
>   1 file changed, 94 insertions(+), 19 deletions(-)
>   mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> old mode 100644
> new mode 100755
> index 367a8a617..b58f756ea
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -82,9 +82,16 @@ static bool disallow_fan_support;
>   #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
>   #define I8K_HWMON_HAVE_TEMP3	(1 << 2)
>   #define I8K_HWMON_HAVE_TEMP4	(1 << 3)
> -#define I8K_HWMON_HAVE_FAN1	(1 << 4)
> -#define I8K_HWMON_HAVE_FAN2	(1 << 5)
> -#define I8K_HWMON_HAVE_FAN3	(1 << 6)
> +#define I8K_HWMON_HAVE_TEMP5	(1 << 4)
> +#define I8K_HWMON_HAVE_TEMP6	(1 << 5)
> +#define I8K_HWMON_HAVE_TEMP7	(1 << 6)
> +#define I8K_HWMON_HAVE_TEMP8	(1 << 7)
> +#define I8K_HWMON_HAVE_TEMP9	(1 << 8)
> +#define I8K_HWMON_HAVE_TEMP10	(1 << 9)
> +
> +#define I8K_HWMON_HAVE_FAN1	(1 << 10)
> +#define I8K_HWMON_HAVE_FAN2	(1 << 11)
> +#define I8K_HWMON_HAVE_FAN3	(1 << 12)
>   
>   MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
>   MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
> @@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>   static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
>   static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>   			  3);
> +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  4);
> +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  5);
> +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
> +static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  6);
> +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
> +static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  7);
> +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
> +static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  8);
> +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
> +static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  9);
> +
>   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);
> @@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
>   	&sensor_dev_attr_temp3_label.dev_attr.attr,	/* 5 */
>   	&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_fan3_input.dev_attr.attr,	/* 14 */
> -	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 15 */
> -	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 16 */
> +	&sensor_dev_attr_temp5_input.dev_attr.attr,	/* 8 */
> +	&sensor_dev_attr_temp5_label.dev_attr.attr,	/* 9 */
> +	&sensor_dev_attr_temp6_input.dev_attr.attr,	/* 10 */
> +	&sensor_dev_attr_temp6_label.dev_attr.attr,	/* 11 */
> +	&sensor_dev_attr_temp7_input.dev_attr.attr,	/* 12 */
> +	&sensor_dev_attr_temp7_label.dev_attr.attr,	/* 13 */
> +	&sensor_dev_attr_temp8_input.dev_attr.attr,	/* 14 */
> +	&sensor_dev_attr_temp8_label.dev_attr.attr,	/* 15 */
> +	&sensor_dev_attr_temp9_input.dev_attr.attr,	/* 16 */
> +	&sensor_dev_attr_temp9_label.dev_attr.attr,	/* 17 */
> +	&sensor_dev_attr_temp10_input.dev_attr.attr,	/* 18 */
> +	&sensor_dev_attr_temp10_label.dev_attr.attr,	/* 19 */
> +	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 20 */
> +	&sensor_dev_attr_fan1_label.dev_attr.attr,	/* 21 */
> +	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 22 */
> +	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 23 */
> +	&sensor_dev_attr_fan2_label.dev_attr.attr,	/* 24 */
> +	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 25 */
> +	&sensor_dev_attr_fan3_input.dev_attr.attr,	/* 26 */
> +	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 27 */
> +	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 28 */
>   	NULL
>   };
>   
> @@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
>   	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_TEMP5))
> +		return 0;
> +	if (index >= 10 && index <= 11 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
> +		return 0;
> +	if (index >= 12 && index <= 13 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
> +		return 0;
> +	if (index >= 14 && index <= 15 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
> +		return 0;
> +	if (index >= 16 && index <= 17 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
> +		return 0;
> +	if (index >= 18 && index <= 19 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
> +		return 0;
> +
> +	if (index >= 20 && index <= 22 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
>   		return 0;
> -	if (index >= 11 && index <= 13 &&
> +	if (index >= 23 && index <= 25 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
>   		return 0;
> -	if (index >= 14 && index <= 16 &&
> +	if (index >= 26 && index <= 28 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
>   		return 0;
>   
> @@ -828,19 +885,37 @@ static int __init i8k_init_hwmon(void)
>   	i8k_hwmon_flags = 0;
>   
>   	/* CPU temperature attributes, if temperature type is OK */
> -	err = i8k_get_temp_type(0);
> +	err = i8k_get_temp(0);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
>   	/* check for additional temperature sensors */
> -	err = i8k_get_temp_type(1);
> +	err = i8k_get_temp(1);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
> -	err = i8k_get_temp_type(2);
> +	err = i8k_get_temp(2);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
> -	err = i8k_get_temp_type(3);
> +	err = i8k_get_temp(3);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
> +	err = i8k_get_temp(4);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
> +	err = i8k_get_temp(5);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
> +	err = i8k_get_temp(6);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
> +	err = i8k_get_temp(7);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
> +	err = i8k_get_temp(8);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
> +	err = i8k_get_temp(9);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;
>   
>   	/* First fan attributes, if fan status or type is OK */
>   	err = i8k_get_fan_status(0);
> 

Re: [PATCH] dell-smm-hwmon.c: Additional temperature sensors

[Michele Sorcinelli]

> Please _always_ version your patches, and add a changelog, at least if you
> would like to see it applied. I have now two patches which look almost the same,
> meaning I would have to pull both versions, compare, and hope to apply the
> correct one.

The only change is in i8k_is_visible():

     // first version
     +	if (index >= 20 && index <= 22 &&
                 !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
                     return 0;
     -	if (index >= 11 && index <= 13 &&
     +	if (index >= 23 && index <= 25 &&
                 !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
                     return 0;
     -	if (index >= 14 && index <= 16 &&
     +	if (index >= 26 && index <= 28 &&
                 !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
                     return 0;

     // second version
     +	if (index >= 20 && index <= 22 &&
                 !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
                     return 0;
     -	if (index >= 11 && index <= 13 &&
     +	if (index >= 23 && index <= 25 &&
                 !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
                     return 0;
     -	if (index >= 14 && index <= 16 &&
     +	if (index >= 26 && index <= 28 &&
                 !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
                     return 0;

Now the indexes in the if conditions have been changed to properly match the
entries in the i8k_attrs array:

     &sensor_dev_attr_fan1_input.dev_attr.attr,	/* 20 */
     &sensor_dev_attr_fan1_label.dev_attr.attr,	/* 21 */
     &sensor_dev_attr_pwm1.dev_attr.attr,	/* 22 */
     &sensor_dev_attr_fan2_input.dev_attr.attr,	/* 23 */
     &sensor_dev_attr_fan2_label.dev_attr.attr,	/* 24 */
     &sensor_dev_attr_pwm2.dev_attr.attr,	/* 25 */
     &sensor_dev_attr_fan3_input.dev_attr.attr,	/* 26 */
     &sensor_dev_attr_fan3_label.dev_attr.attr,	/* 27 */
     &sensor_dev_attr_pwm3.dev_attr.attr,	/* 28 */

> Anyway, I would like to get some feedback if this can cause regressions
> on systems which don't support that many sensors and maybe report something
> completely different if one tries to read the high-numbered sensors.
> I seem to recall that there was a reason for checking the type and not just
> trying to read sensor values.

AFAIK, i8k_get_temp() should return error if the sensor is not available, thus
the sensor won't be initialized in the hwmon interface. Unfortunately, I don't
have other machines to test it.


On 12/8/18 4:24 AM, Guenter Roeck wrote:
> On 12/7/18 4:56 PM, Michele Sorcinelli wrote:
>> The code has been extended to support up to 10 temperature sensors.
>>
>> The i8k_get_temp_type() calls inside i8k_init_hwmon() have been replaced
>> with i8k_get_temp() as in some laptop firmwares the related SMM procedure
>> is not implemented correctly so i8k_get_temp_type() can't be used to
>> discover temperature sensors.
>>
>> Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>
> 
> Please _always_ version your patches, and add a changelog, at least if you
> would like to see it applied. I have now two patches which look almost the same,
> meaning I would have to pull both versions, compare, and hope to apply the
> correct one.
> 
> Anyway, I would like to get some feedback if this can cause regressions
> on systems which don't support that many sensors and maybe report something
> completely different if one tries to read the high-numbered sensors.
> I seem to recall that there was a reason for checking the type and not just
> trying to read sensor values.
> 
> Guenter
> 
>> ---
>>   drivers/hwmon/dell-smm-hwmon.c | 113 +++++++++++++++++++++++++++------
>>   1 file changed, 94 insertions(+), 19 deletions(-)
>>   mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c
>>
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> old mode 100644
>> new mode 100755
>> index 367a8a617..b58f756ea
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -82,9 +82,16 @@ static bool disallow_fan_support;
>>   #define I8K_HWMON_HAVE_TEMP2    (1 << 1)
>>   #define I8K_HWMON_HAVE_TEMP3    (1 << 2)
>>   #define I8K_HWMON_HAVE_TEMP4    (1 << 3)
>> -#define I8K_HWMON_HAVE_FAN1    (1 << 4)
>> -#define I8K_HWMON_HAVE_FAN2    (1 << 5)
>> -#define I8K_HWMON_HAVE_FAN3    (1 << 6)
>> +#define I8K_HWMON_HAVE_TEMP5    (1 << 4)
>> +#define I8K_HWMON_HAVE_TEMP6    (1 << 5)
>> +#define I8K_HWMON_HAVE_TEMP7    (1 << 6)
>> +#define I8K_HWMON_HAVE_TEMP8    (1 << 7)
>> +#define I8K_HWMON_HAVE_TEMP9    (1 << 8)
>> +#define I8K_HWMON_HAVE_TEMP10    (1 << 9)
>> +
>> +#define I8K_HWMON_HAVE_FAN1    (1 << 10)
>> +#define I8K_HWMON_HAVE_FAN2    (1 << 11)
>> +#define I8K_HWMON_HAVE_FAN3    (1 << 12)
>>   MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
>>   MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
>> @@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>   static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
>>   static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>                 3);
>> +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
>> +static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>> +              4);
>> +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
>> +static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>> +              5);
>> +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
>> +static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>> +              6);
>> +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
>> +static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>> +              7);
>> +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
>> +static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>> +              8);
>> +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
>> +static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>> +              9);
>> +
>>   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);
>> @@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
>>       &sensor_dev_attr_temp3_label.dev_attr.attr,    /* 5 */
>>       &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_fan3_input.dev_attr.attr,    /* 14 */
>> -    &sensor_dev_attr_fan3_label.dev_attr.attr,    /* 15 */
>> -    &sensor_dev_attr_pwm3.dev_attr.attr,        /* 16 */
>> +    &sensor_dev_attr_temp5_input.dev_attr.attr,    /* 8 */
>> +    &sensor_dev_attr_temp5_label.dev_attr.attr,    /* 9 */
>> +    &sensor_dev_attr_temp6_input.dev_attr.attr,    /* 10 */
>> +    &sensor_dev_attr_temp6_label.dev_attr.attr,    /* 11 */
>> +    &sensor_dev_attr_temp7_input.dev_attr.attr,    /* 12 */
>> +    &sensor_dev_attr_temp7_label.dev_attr.attr,    /* 13 */
>> +    &sensor_dev_attr_temp8_input.dev_attr.attr,    /* 14 */
>> +    &sensor_dev_attr_temp8_label.dev_attr.attr,    /* 15 */
>> +    &sensor_dev_attr_temp9_input.dev_attr.attr,    /* 16 */
>> +    &sensor_dev_attr_temp9_label.dev_attr.attr,    /* 17 */
>> +    &sensor_dev_attr_temp10_input.dev_attr.attr,    /* 18 */
>> +    &sensor_dev_attr_temp10_label.dev_attr.attr,    /* 19 */
>> +    &sensor_dev_attr_fan1_input.dev_attr.attr,    /* 20 */
>> +    &sensor_dev_attr_fan1_label.dev_attr.attr,    /* 21 */
>> +    &sensor_dev_attr_pwm1.dev_attr.attr,        /* 22 */
>> +    &sensor_dev_attr_fan2_input.dev_attr.attr,    /* 23 */
>> +    &sensor_dev_attr_fan2_label.dev_attr.attr,    /* 24 */
>> +    &sensor_dev_attr_pwm2.dev_attr.attr,        /* 25 */
>> +    &sensor_dev_attr_fan3_input.dev_attr.attr,    /* 26 */
>> +    &sensor_dev_attr_fan3_label.dev_attr.attr,    /* 27 */
>> +    &sensor_dev_attr_pwm3.dev_attr.attr,        /* 28 */
>>       NULL
>>   };
>> @@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
>>       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_TEMP5))
>> +        return 0;
>> +    if (index >= 10 && index <= 11 &&
>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
>> +        return 0;
>> +    if (index >= 12 && index <= 13 &&
>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
>> +        return 0;
>> +    if (index >= 14 && index <= 15 &&
>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
>> +        return 0;
>> +    if (index >= 16 && index <= 17 &&
>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
>> +        return 0;
>> +    if (index >= 18 && index <= 19 &&
>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
>> +        return 0;
>> +
>> +    if (index >= 20 && index <= 22 &&
>>           !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
>>           return 0;
>> -    if (index >= 11 && index <= 13 &&
>> +    if (index >= 23 && index <= 25 &&
>>           !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
>>           return 0;
>> -    if (index >= 14 && index <= 16 &&
>> +    if (index >= 26 && index <= 28 &&
>>           !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
>>           return 0;
>> @@ -828,19 +885,37 @@ static int __init i8k_init_hwmon(void)
>>       i8k_hwmon_flags = 0;
>>       /* CPU temperature attributes, if temperature type is OK */
>> -    err = i8k_get_temp_type(0);
>> +    err = i8k_get_temp(0);
>>       if (err >= 0)
>>           i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
>>       /* check for additional temperature sensors */
>> -    err = i8k_get_temp_type(1);
>> +    err = i8k_get_temp(1);
>>       if (err >= 0)
>>           i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
>> -    err = i8k_get_temp_type(2);
>> +    err = i8k_get_temp(2);
>>       if (err >= 0)
>>           i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
>> -    err = i8k_get_temp_type(3);
>> +    err = i8k_get_temp(3);
>>       if (err >= 0)
>>           i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
>> +    err = i8k_get_temp(4);
>> +    if (err >= 0)
>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
>> +    err = i8k_get_temp(5);
>> +    if (err >= 0)
>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
>> +    err = i8k_get_temp(6);
>> +    if (err >= 0)
>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
>> +    err = i8k_get_temp(7);
>> +    if (err >= 0)
>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
>> +    err = i8k_get_temp(8);
>> +    if (err >= 0)
>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
>> +    err = i8k_get_temp(9);
>> +    if (err >= 0)
>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;
>>       /* First fan attributes, if fan status or type is OK */
>>       err = i8k_get_fan_status(0);
>>
> 

Re: [PATCH] dell-smm-hwmon.c: Additional temperature sensors

[Michele Sorcinelli]

I'm sorry, the last diff I sent was wrong, this is the correct one.

Original version (wrong indexes):

+	if (index >= 20 && index <= 21 &&
  	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
  		return 0;
-	if (index >= 11 && index <= 13 &&
+	if (index >= 23 && index <= 24 &&
  	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
  		return 0;
-	if (index >= 14 && index <= 16 &&
+	if (index >= 26 && index <= 27 &&
  	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
  		return 0;

Amended version (correct indexes):

     +    if (index >= 20 && index <= 22 &&
                 !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
                     return 0;
     -    if (index >= 11 && index <= 13 &&
     +    if (index >= 23 && index <= 25 &&
                 !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
                     return 0;
     -    if (index >= 14 && index <= 16 &&
     +    if (index >= 26 && index <= 28 &&
                 !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
                     return 0;

Again sorry for this, I appreciate that is very confusing.

On 12/8/18 3:34 PM, Michele Sorcinelli wrote:
>> Please _always_ version your patches, and add a changelog, at least if you
>> would like to see it applied. I have now two patches which look almost the same,
>> meaning I would have to pull both versions, compare, and hope to apply the
>> correct one.
> 
> The only change is in i8k_is_visible():
> 
>      // first version
>      +    if (index >= 20 && index <= 22 &&
>                  !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
>                      return 0;
>      -    if (index >= 11 && index <= 13 &&
>      +    if (index >= 23 && index <= 25 &&
>                  !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
>                      return 0;
>      -    if (index >= 14 && index <= 16 &&
>      +    if (index >= 26 && index <= 28 &&
>                  !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
>                      return 0;
> 
>      // second version
>      +    if (index >= 20 && index <= 22 &&
>                  !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
>                      return 0;
>      -    if (index >= 11 && index <= 13 &&
>      +    if (index >= 23 && index <= 25 &&
>                  !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
>                      return 0;
>      -    if (index >= 14 && index <= 16 &&
>      +    if (index >= 26 && index <= 28 &&
>                  !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
>                      return 0;
> 
> Now the indexes in the if conditions have been changed to properly match the
> entries in the i8k_attrs array:
> 
>      &sensor_dev_attr_fan1_input.dev_attr.attr,    /* 20 */
>      &sensor_dev_attr_fan1_label.dev_attr.attr,    /* 21 */
>      &sensor_dev_attr_pwm1.dev_attr.attr,    /* 22 */
>      &sensor_dev_attr_fan2_input.dev_attr.attr,    /* 23 */
>      &sensor_dev_attr_fan2_label.dev_attr.attr,    /* 24 */
>      &sensor_dev_attr_pwm2.dev_attr.attr,    /* 25 */
>      &sensor_dev_attr_fan3_input.dev_attr.attr,    /* 26 */
>      &sensor_dev_attr_fan3_label.dev_attr.attr,    /* 27 */
>      &sensor_dev_attr_pwm3.dev_attr.attr,    /* 28 */
> 
>> Anyway, I would like to get some feedback if this can cause regressions
>> on systems which don't support that many sensors and maybe report something
>> completely different if one tries to read the high-numbered sensors.
>> I seem to recall that there was a reason for checking the type and not just
>> trying to read sensor values.
> 
> AFAIK, i8k_get_temp() should return error if the sensor is not available, thus
> the sensor won't be initialized in the hwmon interface. Unfortunately, I don't
> have other machines to test it.
> 
> 
> On 12/8/18 4:24 AM, Guenter Roeck wrote:
>> On 12/7/18 4:56 PM, Michele Sorcinelli wrote:
>>> The code has been extended to support up to 10 temperature sensors.
>>>
>>> The i8k_get_temp_type() calls inside i8k_init_hwmon() have been replaced
>>> with i8k_get_temp() as in some laptop firmwares the related SMM procedure
>>> is not implemented correctly so i8k_get_temp_type() can't be used to
>>> discover temperature sensors.
>>>
>>> Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>
>>
>> Please _always_ version your patches, and add a changelog, at least if you
>> would like to see it applied. I have now two patches which look almost the same,
>> meaning I would have to pull both versions, compare, and hope to apply the
>> correct one.
>>
>> Anyway, I would like to get some feedback if this can cause regressions
>> on systems which don't support that many sensors and maybe report something
>> completely different if one tries to read the high-numbered sensors.
>> I seem to recall that there was a reason for checking the type and not just
>> trying to read sensor values.
>>
>> Guenter
>>
>>> ---
>>>   drivers/hwmon/dell-smm-hwmon.c | 113 +++++++++++++++++++++++++++------
>>>   1 file changed, 94 insertions(+), 19 deletions(-)
>>>   mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c
>>>
>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>>> old mode 100644
>>> new mode 100755
>>> index 367a8a617..b58f756ea
>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>> @@ -82,9 +82,16 @@ static bool disallow_fan_support;
>>>   #define I8K_HWMON_HAVE_TEMP2    (1 << 1)
>>>   #define I8K_HWMON_HAVE_TEMP3    (1 << 2)
>>>   #define I8K_HWMON_HAVE_TEMP4    (1 << 3)
>>> -#define I8K_HWMON_HAVE_FAN1    (1 << 4)
>>> -#define I8K_HWMON_HAVE_FAN2    (1 << 5)
>>> -#define I8K_HWMON_HAVE_FAN3    (1 << 6)
>>> +#define I8K_HWMON_HAVE_TEMP5    (1 << 4)
>>> +#define I8K_HWMON_HAVE_TEMP6    (1 << 5)
>>> +#define I8K_HWMON_HAVE_TEMP7    (1 << 6)
>>> +#define I8K_HWMON_HAVE_TEMP8    (1 << 7)
>>> +#define I8K_HWMON_HAVE_TEMP9    (1 << 8)
>>> +#define I8K_HWMON_HAVE_TEMP10    (1 << 9)
>>> +
>>> +#define I8K_HWMON_HAVE_FAN1    (1 << 10)
>>> +#define I8K_HWMON_HAVE_FAN2    (1 << 11)
>>> +#define I8K_HWMON_HAVE_FAN3    (1 << 12)
>>>   MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
>>>   MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
>>> @@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>>   static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
>>>   static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>>                 3);
>>> +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
>>> +static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>> +              4);
>>> +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
>>> +static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>> +              5);
>>> +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
>>> +static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>> +              6);
>>> +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
>>> +static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>> +              7);
>>> +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
>>> +static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>> +              8);
>>> +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
>>> +static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>> +              9);
>>> +
>>>   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);
>>> @@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
>>>       &sensor_dev_attr_temp3_label.dev_attr.attr,    /* 5 */
>>>       &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_fan3_input.dev_attr.attr,    /* 14 */
>>> -    &sensor_dev_attr_fan3_label.dev_attr.attr,    /* 15 */
>>> -    &sensor_dev_attr_pwm3.dev_attr.attr,        /* 16 */
>>> +    &sensor_dev_attr_temp5_input.dev_attr.attr,    /* 8 */
>>> +    &sensor_dev_attr_temp5_label.dev_attr.attr,    /* 9 */
>>> +    &sensor_dev_attr_temp6_input.dev_attr.attr,    /* 10 */
>>> +    &sensor_dev_attr_temp6_label.dev_attr.attr,    /* 11 */
>>> +    &sensor_dev_attr_temp7_input.dev_attr.attr,    /* 12 */
>>> +    &sensor_dev_attr_temp7_label.dev_attr.attr,    /* 13 */
>>> +    &sensor_dev_attr_temp8_input.dev_attr.attr,    /* 14 */
>>> +    &sensor_dev_attr_temp8_label.dev_attr.attr,    /* 15 */
>>> +    &sensor_dev_attr_temp9_input.dev_attr.attr,    /* 16 */
>>> +    &sensor_dev_attr_temp9_label.dev_attr.attr,    /* 17 */
>>> +    &sensor_dev_attr_temp10_input.dev_attr.attr,    /* 18 */
>>> +    &sensor_dev_attr_temp10_label.dev_attr.attr,    /* 19 */
>>> +    &sensor_dev_attr_fan1_input.dev_attr.attr,    /* 20 */
>>> +    &sensor_dev_attr_fan1_label.dev_attr.attr,    /* 21 */
>>> +    &sensor_dev_attr_pwm1.dev_attr.attr,        /* 22 */
>>> +    &sensor_dev_attr_fan2_input.dev_attr.attr,    /* 23 */
>>> +    &sensor_dev_attr_fan2_label.dev_attr.attr,    /* 24 */
>>> +    &sensor_dev_attr_pwm2.dev_attr.attr,        /* 25 */
>>> +    &sensor_dev_attr_fan3_input.dev_attr.attr,    /* 26 */
>>> +    &sensor_dev_attr_fan3_label.dev_attr.attr,    /* 27 */
>>> +    &sensor_dev_attr_pwm3.dev_attr.attr,        /* 28 */
>>>       NULL
>>>   };
>>> @@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
>>>       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_TEMP5))
>>> +        return 0;
>>> +    if (index >= 10 && index <= 11 &&
>>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
>>> +        return 0;
>>> +    if (index >= 12 && index <= 13 &&
>>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
>>> +        return 0;
>>> +    if (index >= 14 && index <= 15 &&
>>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
>>> +        return 0;
>>> +    if (index >= 16 && index <= 17 &&
>>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
>>> +        return 0;
>>> +    if (index >= 18 && index <= 19 &&
>>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
>>> +        return 0;
>>> +
>>> +    if (index >= 20 && index <= 22 &&
>>>           !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
>>>           return 0;
>>> -    if (index >= 11 && index <= 13 &&
>>> +    if (index >= 23 && index <= 25 &&
>>>           !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
>>>           return 0;
>>> -    if (index >= 14 && index <= 16 &&
>>> +    if (index >= 26 && index <= 28 &&
>>>           !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
>>>           return 0;
>>> @@ -828,19 +885,37 @@ static int __init i8k_init_hwmon(void)
>>>       i8k_hwmon_flags = 0;
>>>       /* CPU temperature attributes, if temperature type is OK */
>>> -    err = i8k_get_temp_type(0);
>>> +    err = i8k_get_temp(0);
>>>       if (err >= 0)
>>>           i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
>>>       /* check for additional temperature sensors */
>>> -    err = i8k_get_temp_type(1);
>>> +    err = i8k_get_temp(1);
>>>       if (err >= 0)
>>>           i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
>>> -    err = i8k_get_temp_type(2);
>>> +    err = i8k_get_temp(2);
>>>       if (err >= 0)
>>>           i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
>>> -    err = i8k_get_temp_type(3);
>>> +    err = i8k_get_temp(3);
>>>       if (err >= 0)
>>>           i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
>>> +    err = i8k_get_temp(4);
>>> +    if (err >= 0)
>>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
>>> +    err = i8k_get_temp(5);
>>> +    if (err >= 0)
>>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
>>> +    err = i8k_get_temp(6);
>>> +    if (err >= 0)
>>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
>>> +    err = i8k_get_temp(7);
>>> +    if (err >= 0)
>>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
>>> +    err = i8k_get_temp(8);
>>> +    if (err >= 0)
>>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
>>> +    err = i8k_get_temp(9);
>>> +    if (err >= 0)
>>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;
>>>       /* First fan attributes, if fan status or type is OK */
>>>       err = i8k_get_fan_status(0);
>>>
>>

Re: [PATCH] dell-smm-hwmon.c: Additional temperature sensors

[Guenter Roeck]

On 12/8/18 7:34 AM, Michele Sorcinelli wrote:
>> Please _always_ version your patches, and add a changelog, at least if you
>> would like to see it applied. I have now two patches which look almost the same,
>> meaning I would have to pull both versions, compare, and hope to apply the
>> correct one.
> 
> The only change is in i8k_is_visible():

Please consider reading Documentation/process/submitting-patches.rst,
chapter 2). For simplicity, I am copying the relevant paragraph here.

"
When you submit or resubmit a patch or patch series, include the
complete patch description and justification for it.  Don't just
say that this is version N of the patch (series).  Don't expect the
subsystem maintainer to refer back to earlier patch versions or referenced
URLs to find the patch description and put that into the patch.
I.e., the patch (series) and its description should be self-contained.
This benefits both the maintainers and reviewers.  Some reviewers
probably didn't even receive earlier versions of the patch.
"

Thanks,
Guenter

Re: [PATCH] dell-smm-hwmon.c: Additional temperature sensors

[Pali Rohár]

On Friday 07 December 2018 20:24:49 Guenter Roeck wrote:
> Anyway, I would like to get some feedback if this can cause regressions
> on systems which don't support that many sensors and maybe report something
> completely different if one tries to read the high-numbered sensors.
> I seem to recall that there was a reason for checking the type and not just
> trying to read sensor values.

There can be also different problem for sensors which are turned off.
E.g. on notebooks with switchable graphic cards which have included
temperature sensors. When graphic card is turned off, then SMM returns
error when asking for temperature value (for obvious reason). But
temperature type still returns correct value "this is GPU sensors".

So we cannot replace temp_type check by temp_value check. It introduce
race condition between "starting GPU" and initializing dell-smm hwmon.
Now linux kernel has support for dynamic turning ON/OFF switchable GPU,
so we need to care about these race conditions too.

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

Re: [PATCH] dell-smm-hwmon.c: Additional temperature sensors

[Michele Sorcinelli]

That's a valid point, I honestly didn't think about it.

I opened a thread in the Dell community forum about the broken SSM procedures
but I didn't get any reply. I wonder what's the best way to inform the firmware
developers.

https://www.dell.com/community/XPS/Broken-SMM-procedures-in-XPS-9570-firmware/m-p/6229521

On 12/10/18 10:58 AM, Pali Rohár wrote:
> On Friday 07 December 2018 20:24:49 Guenter Roeck wrote:
>> Anyway, I would like to get some feedback if this can cause regressions
>> on systems which don't support that many sensors and maybe report something
>> completely different if one tries to read the high-numbered sensors.
>> I seem to recall that there was a reason for checking the type and not just
>> trying to read sensor values.
> 
> There can be also different problem for sensors which are turned off.
> E.g. on notebooks with switchable graphic cards which have included
> temperature sensors. When graphic card is turned off, then SMM returns
> error when asking for temperature value (for obvious reason). But
> temperature type still returns correct value "this is GPU sensors".
> 
> So we cannot replace temp_type check by temp_value check. It introduce
> race condition between "starting GPU" and initializing dell-smm hwmon.
> Now linux kernel has support for dynamic turning ON/OFF switchable GPU,
> so we need to care about these race conditions too.
> 

Re: [PATCH] dell-smm-hwmon.c: Additional temperature sensors

[Pali Rohár]

On Monday 10 December 2018 11:53:58 Michele Sorcinelli wrote:
> I wonder what's the best way to inform the firmware developers.

Mario from @Dell wrote how to report firware bugs in discussion:
https://github.com/dell/libsmbios/issues/48#issuecomment-391328501

You should have bought your machine in USA and then open ticket at:
http://www.dell.com/support/incidents-online/us/en/04

For my question how non-USA people should report bugs I have not
received answer yet. But probably similar steps, contacting Dell
support.

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

Re: [PATCH] dell-smm-hwmon.c: Additional temperature sensors

[Michele Sorcinelli]

As far as I know Dell won't help with fixing the SMM layer, they probably
changed something starting with firmware version 1.3.0 and they don't
wanna release information about it.

https://www.hwinfo.com/forum/Thread-Dell-XPS-15-9570-temperatures-not-named-anymore

I wonder if something can be done to force the discovery of the sensors in the driver,
maybe adding a module option to use i8k_get_temp() as probe method as a workaround,
or maybe just forcing that method for this specific model?

Let me know your thoughts.

Thanks,
Michele.

On 12/10/18 10:58 AM, Pali Rohár wrote:
> On Friday 07 December 2018 20:24:49 Guenter Roeck wrote:
>> Anyway, I would like to get some feedback if this can cause regressions
>> on systems which don't support that many sensors and maybe report something
>> completely different if one tries to read the high-numbered sensors.
>> I seem to recall that there was a reason for checking the type and not just
>> trying to read sensor values.
> 
> There can be also different problem for sensors which are turned off.
> E.g. on notebooks with switchable graphic cards which have included
> temperature sensors. When graphic card is turned off, then SMM returns
> error when asking for temperature value (for obvious reason). But
> temperature type still returns correct value "this is GPU sensors".
> 
> So we cannot replace temp_type check by temp_value check. It introduce
> race condition between "starting GPU" and initializing dell-smm hwmon.
> Now linux kernel has support for dynamic turning ON/OFF switchable GPU,
> so we need to care about these race conditions too.
> 

Re: [PATCH] dell-smm-hwmon.c: Additional temperature sensors

[Pali Rohár]

Do you have definite response from Dell support that they are not going
to fix it? Then it is pity :-(

On Thursday 07 February 2019 12:16:06 Michele Sorcinelli wrote:
> As far as I know Dell won't help with fixing the SMM layer, they probably
> changed something starting with firmware version 1.3.0 and they don't
> wanna release information about it.
> 
> https://www.hwinfo.com/forum/Thread-Dell-XPS-15-9570-temperatures-not-named-anymore
> 
> I wonder if something can be done to force the discovery of the sensors in the driver,
> maybe adding a module option to use i8k_get_temp() as probe method as a workaround,
> or maybe just forcing that method for this specific model?
> 
> Let me know your thoughts.
> 
> Thanks,
> Michele.
> 
> On 12/10/18 10:58 AM, Pali Rohár wrote:
> > On Friday 07 December 2018 20:24:49 Guenter Roeck wrote:
> > > Anyway, I would like to get some feedback if this can cause regressions
> > > on systems which don't support that many sensors and maybe report something
> > > completely different if one tries to read the high-numbered sensors.
> > > I seem to recall that there was a reason for checking the type and not just
> > > trying to read sensor values.
> > 
> > There can be also different problem for sensors which are turned off.
> > E.g. on notebooks with switchable graphic cards which have included
> > temperature sensors. When graphic card is turned off, then SMM returns
> > error when asking for temperature value (for obvious reason). But
> > temperature type still returns correct value "this is GPU sensors".
> > 
> > So we cannot replace temp_type check by temp_value check. It introduce
> > race condition between "starting GPU" and initializing dell-smm hwmon.
> > Now linux kernel has support for dynamic turning ON/OFF switchable GPU,
> > so we need to care about these race conditions too.
> > 

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

Re: [PATCH] dell-smm-hwmon.c: Additional temperature sensors

[Michele Sorcinelli]

I think they'll never fix it or release information about the change.
What if we modify the driver to fallback to i8k_get_temp() if i8k_get_temp_type()
fails as a probe method? Or we could add a module option to force that behavior.

Cheers,
Michele.

On 2/7/19 12:40 PM, Pali Rohár wrote:
> Do you have definite response from Dell support that they are not going
> to fix it? Then it is pity :-(
> 
> On Thursday 07 February 2019 12:16:06 Michele Sorcinelli wrote:
>> As far as I know Dell won't help with fixing the SMM layer, they probably
>> changed something starting with firmware version 1.3.0 and they don't
>> wanna release information about it.
>>
>> https://www.hwinfo.com/forum/Thread-Dell-XPS-15-9570-temperatures-not-named-anymore
>>
>> I wonder if something can be done to force the discovery of the sensors in the driver,
>> maybe adding a module option to use i8k_get_temp() as probe method as a workaround,
>> or maybe just forcing that method for this specific model?
>>
>> Let me know your thoughts.
>>
>> Thanks,
>> Michele.
>>
>> On 12/10/18 10:58 AM, Pali Rohár wrote:
>>> On Friday 07 December 2018 20:24:49 Guenter Roeck wrote:
>>>> Anyway, I would like to get some feedback if this can cause regressions
>>>> on systems which don't support that many sensors and maybe report something
>>>> completely different if one tries to read the high-numbered sensors.
>>>> I seem to recall that there was a reason for checking the type and not just
>>>> trying to read sensor values.
>>>
>>> There can be also different problem for sensors which are turned off.
>>> E.g. on notebooks with switchable graphic cards which have included
>>> temperature sensors. When graphic card is turned off, then SMM returns
>>> error when asking for temperature value (for obvious reason). But
>>> temperature type still returns correct value "this is GPU sensors".
>>>
>>> So we cannot replace temp_type check by temp_value check. It introduce
>>> race condition between "starting GPU" and initializing dell-smm hwmon.
>>> Now linux kernel has support for dynamic turning ON/OFF switchable GPU,
>>> so we need to care about these race conditions too.
>>>
> 

Re: [PATCH] dell-smm-hwmon.c: Additional temperature sensors

[Michele Sorcinelli]

Looks like they've broken the SMM method for XPS 9560 too in a recent firmware upgrade.
Have you thought about the possibility to fall back to i8k_get_temp() when the label
method fails, or when the user decides to explicitly enable it as an alternative with
a module option?

Looks like Dell decided to change the API so future firmwares will stop supporting
that method anyway, so I think action must be taken in the driver to address the problem.

Kind regards,
Michele.

On 2/7/19 12:40 PM, Pali Rohár wrote:
> Do you have definite response from Dell support that they are not going
> to fix it? Then it is pity :-(
> 
> On Thursday 07 February 2019 12:16:06 Michele Sorcinelli wrote:
>> As far as I know Dell won't help with fixing the SMM layer, they probably
>> changed something starting with firmware version 1.3.0 and they don't
>> wanna release information about it.
>>
>> https://www.hwinfo.com/forum/Thread-Dell-XPS-15-9570-temperatures-not-named-anymore
>>
>> I wonder if something can be done to force the discovery of the sensors in the driver,
>> maybe adding a module option to use i8k_get_temp() as probe method as a workaround,
>> or maybe just forcing that method for this specific model?
>>
>> Let me know your thoughts.
>>
>> Thanks,
>> Michele.

[PATCH v2] dell-smm-hwmon.c: Additional temperature sensors

[Michele Sorcinelli]

The code has been extended to support up to 10 temperature sensors.

The i8k_get_temp_type() calls inside i8k_init_hwmon() have been replaced
with i8k_get_temp() as in some laptop firmwares the related SMM procedure
is not implemented correctly so i8k_get_temp_type() can't be used to
discover temperature sensors.

Changes from v1:
  - fix wrong indexes in i8k_is_visible() that were causing unavailable
    pwm files to be wrongly exposed by the hwmon interface

Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>
---
 drivers/hwmon/dell-smm-hwmon.c | 113 +++++++++++++++++++++++++++------
 1 file changed, 94 insertions(+), 19 deletions(-)
 mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
old mode 100644
new mode 100755
index 367a8a617..b58f756ea
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -82,9 +82,16 @@ static bool disallow_fan_support;
 #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
 #define I8K_HWMON_HAVE_TEMP3	(1 << 2)
 #define I8K_HWMON_HAVE_TEMP4	(1 << 3)
-#define I8K_HWMON_HAVE_FAN1	(1 << 4)
-#define I8K_HWMON_HAVE_FAN2	(1 << 5)
-#define I8K_HWMON_HAVE_FAN3	(1 << 6)
+#define I8K_HWMON_HAVE_TEMP5	(1 << 4)
+#define I8K_HWMON_HAVE_TEMP6	(1 << 5)
+#define I8K_HWMON_HAVE_TEMP7	(1 << 6)
+#define I8K_HWMON_HAVE_TEMP8	(1 << 7)
+#define I8K_HWMON_HAVE_TEMP9	(1 << 8)
+#define I8K_HWMON_HAVE_TEMP10	(1 << 9)
+
+#define I8K_HWMON_HAVE_FAN1	(1 << 10)
+#define I8K_HWMON_HAVE_FAN2	(1 << 11)
+#define I8K_HWMON_HAVE_FAN3	(1 << 12)

 MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
 MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
@@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
 static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
 static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
 			  3);
+static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
+static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  4);
+static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
+static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  5);
+static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
+static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  6);
+static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
+static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  7);
+static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
+static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  8);
+static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
+static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  9);
+
 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);
@@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
 	&sensor_dev_attr_temp3_label.dev_attr.attr,	/* 5 */
 	&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_fan3_input.dev_attr.attr,	/* 14 */
-	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 15 */
-	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 16 */
+	&sensor_dev_attr_temp5_input.dev_attr.attr,	/* 8 */
+	&sensor_dev_attr_temp5_label.dev_attr.attr,	/* 9 */
+	&sensor_dev_attr_temp6_input.dev_attr.attr,	/* 10 */
+	&sensor_dev_attr_temp6_label.dev_attr.attr,	/* 11 */
+	&sensor_dev_attr_temp7_input.dev_attr.attr,	/* 12 */
+	&sensor_dev_attr_temp7_label.dev_attr.attr,	/* 13 */
+	&sensor_dev_attr_temp8_input.dev_attr.attr,	/* 14 */
+	&sensor_dev_attr_temp8_label.dev_attr.attr,	/* 15 */
+	&sensor_dev_attr_temp9_input.dev_attr.attr,	/* 16 */
+	&sensor_dev_attr_temp9_label.dev_attr.attr,	/* 17 */
+	&sensor_dev_attr_temp10_input.dev_attr.attr,	/* 18 */
+	&sensor_dev_attr_temp10_label.dev_attr.attr,	/* 19 */
+	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 20 */
+	&sensor_dev_attr_fan1_label.dev_attr.attr,	/* 21 */
+	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 22 */
+	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 23 */
+	&sensor_dev_attr_fan2_label.dev_attr.attr,	/* 24 */
+	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 25 */
+	&sensor_dev_attr_fan3_input.dev_attr.attr,	/* 26 */
+	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 27 */
+	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 28 */
 	NULL
 };

@@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
 	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_TEMP5))
+		return 0;
+	if (index >= 10 && index <= 11 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
+		return 0;
+	if (index >= 12 && index <= 13 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
+		return 0;
+	if (index >= 14 && index <= 15 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
+		return 0;
+	if (index >= 16 && index <= 17 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
+		return 0;
+	if (index >= 18 && index <= 19 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
+		return 0;
+
+	if (index >= 20 && index <= 22 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
 		return 0;
-	if (index >= 11 && index <= 13 &&
+	if (index >= 23 && index <= 25 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
 		return 0;
-	if (index >= 14 && index <= 16 &&
+	if (index >= 26 && index <= 28 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
 		return 0;

@@ -828,19 +885,37 @@ static int __init i8k_init_hwmon(void)
 	i8k_hwmon_flags = 0;

 	/* CPU temperature attributes, if temperature type is OK */
-	err = i8k_get_temp_type(0);
+	err = i8k_get_temp(0);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
 	/* check for additional temperature sensors */
-	err = i8k_get_temp_type(1);
+	err = i8k_get_temp(1);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
-	err = i8k_get_temp_type(2);
+	err = i8k_get_temp(2);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
-	err = i8k_get_temp_type(3);
+	err = i8k_get_temp(3);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
+	err = i8k_get_temp(4);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
+	err = i8k_get_temp(5);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
+	err = i8k_get_temp(6);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
+	err = i8k_get_temp(7);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
+	err = i8k_get_temp(8);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
+	err = i8k_get_temp(9);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;

 	/* First fan attributes, if fan status or type is OK */
 	err = i8k_get_fan_status(0);
--
2.19.2

[PATCH v3] dell-smm-hwmon.c: Additional temperature sensors

[Michele Sorcinelli]

The code has been extended to support up to 10 temperature sensors.

Changes from previous patch versions:

- fix wrong index ranges in i8k_is_visible() if conditions
- restore i8k_get_temp_type() as probe method for temp sensors because
  i8k_get_temp() is not reliable (for example it won't work when
  the sensors is turned off)

Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>
---
 drivers/hwmon/dell-smm-hwmon.c | 105 ++++++++++++++++++++++++++++-----
 1 file changed, 90 insertions(+), 15 deletions(-)
 mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
old mode 100644
new mode 100755
index 367a8a617..73a30e3e4
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -82,9 +82,16 @@ static bool disallow_fan_support;
 #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
 #define I8K_HWMON_HAVE_TEMP3	(1 << 2)
 #define I8K_HWMON_HAVE_TEMP4	(1 << 3)
-#define I8K_HWMON_HAVE_FAN1	(1 << 4)
-#define I8K_HWMON_HAVE_FAN2	(1 << 5)
-#define I8K_HWMON_HAVE_FAN3	(1 << 6)
+#define I8K_HWMON_HAVE_TEMP5	(1 << 4)
+#define I8K_HWMON_HAVE_TEMP6	(1 << 5)
+#define I8K_HWMON_HAVE_TEMP7	(1 << 6)
+#define I8K_HWMON_HAVE_TEMP8	(1 << 7)
+#define I8K_HWMON_HAVE_TEMP9	(1 << 8)
+#define I8K_HWMON_HAVE_TEMP10	(1 << 9)
+
+#define I8K_HWMON_HAVE_FAN1	(1 << 10)
+#define I8K_HWMON_HAVE_FAN2	(1 << 11)
+#define I8K_HWMON_HAVE_FAN3	(1 << 12)

 MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
 MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
@@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
 static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
 static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
 			  3);
+static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
+static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  4);
+static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
+static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  5);
+static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
+static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  6);
+static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
+static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  7);
+static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
+static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  8);
+static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
+static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  9);
+
 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);
@@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
 	&sensor_dev_attr_temp3_label.dev_attr.attr,	/* 5 */
 	&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_fan3_input.dev_attr.attr,	/* 14 */
-	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 15 */
-	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 16 */
+	&sensor_dev_attr_temp5_input.dev_attr.attr,	/* 8 */
+	&sensor_dev_attr_temp5_label.dev_attr.attr,	/* 9 */
+	&sensor_dev_attr_temp6_input.dev_attr.attr,	/* 10 */
+	&sensor_dev_attr_temp6_label.dev_attr.attr,	/* 11 */
+	&sensor_dev_attr_temp7_input.dev_attr.attr,	/* 12 */
+	&sensor_dev_attr_temp7_label.dev_attr.attr,	/* 13 */
+	&sensor_dev_attr_temp8_input.dev_attr.attr,	/* 14 */
+	&sensor_dev_attr_temp8_label.dev_attr.attr,	/* 15 */
+	&sensor_dev_attr_temp9_input.dev_attr.attr,	/* 16 */
+	&sensor_dev_attr_temp9_label.dev_attr.attr,	/* 17 */
+	&sensor_dev_attr_temp10_input.dev_attr.attr,	/* 18 */
+	&sensor_dev_attr_temp10_label.dev_attr.attr,	/* 19 */
+	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 20 */
+	&sensor_dev_attr_fan1_label.dev_attr.attr,	/* 21 */
+	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 22 */
+	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 23 */
+	&sensor_dev_attr_fan2_label.dev_attr.attr,	/* 24 */
+	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 25 */
+	&sensor_dev_attr_fan3_input.dev_attr.attr,	/* 26 */
+	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 27 */
+	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 28 */
 	NULL
 };

@@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
 	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_TEMP5))
+		return 0;
+	if (index >= 10 && index <= 11 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
+		return 0;
+	if (index >= 12 && index <= 13 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
+		return 0;
+	if (index >= 14 && index <= 15 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
+		return 0;
+	if (index >= 16 && index <= 17 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
+		return 0;
+	if (index >= 18 && index <= 19 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
+		return 0;
+
+	if (index >= 20 && index <= 22 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
 		return 0;
-	if (index >= 11 && index <= 13 &&
+	if (index >= 23 && index <= 25 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
 		return 0;
-	if (index >= 14 && index <= 16 &&
+	if (index >= 26 && index <= 28 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
 		return 0;

@@ -841,6 +898,24 @@ static int __init i8k_init_hwmon(void)
 	err = i8k_get_temp_type(3);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
+	err = i8k_get_temp_type(4);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
+	err = i8k_get_temp_type(5);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
+	err = i8k_get_temp_type(6);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
+	err = i8k_get_temp_type(7);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
+	err = i8k_get_temp_type(8);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
+	err = i8k_get_temp_type(9);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;

 	/* First fan attributes, if fan status or type is OK */
 	err = i8k_get_fan_status(0);
--
2.20.0

Re: [PATCH v3] dell-smm-hwmon.c: Additional temperature sensors

[Guenter Roeck]

On Fri, Dec 14, 2018 at 11:57:59PM +0000, Michele Sorcinelli wrote:
> The code has been extended to support up to 10 temperature sensors.
> 
> Changes from previous patch versions:
> 
> - fix wrong index ranges in i8k_is_visible() if conditions
> - restore i8k_get_temp_type() as probe method for temp sensors because
>   i8k_get_temp() is not reliable (for example it won't work when
>   the sensors is turned off)
> 
> Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>

Pali, this is waiting for your feedbacck. Any comments ?

Thanks,
Guenter

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 105 ++++++++++++++++++++++++++++-----
>  1 file changed, 90 insertions(+), 15 deletions(-)
>  mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c
> 
> --
> 2.20.0
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> old mode 100644
> new mode 100755
> index 367a8a617..73a30e3e4
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -82,9 +82,16 @@ static bool disallow_fan_support;
>  #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
>  #define I8K_HWMON_HAVE_TEMP3	(1 << 2)
>  #define I8K_HWMON_HAVE_TEMP4	(1 << 3)
> -#define I8K_HWMON_HAVE_FAN1	(1 << 4)
> -#define I8K_HWMON_HAVE_FAN2	(1 << 5)
> -#define I8K_HWMON_HAVE_FAN3	(1 << 6)
> +#define I8K_HWMON_HAVE_TEMP5	(1 << 4)
> +#define I8K_HWMON_HAVE_TEMP6	(1 << 5)
> +#define I8K_HWMON_HAVE_TEMP7	(1 << 6)
> +#define I8K_HWMON_HAVE_TEMP8	(1 << 7)
> +#define I8K_HWMON_HAVE_TEMP9	(1 << 8)
> +#define I8K_HWMON_HAVE_TEMP10	(1 << 9)
> +
> +#define I8K_HWMON_HAVE_FAN1	(1 << 10)
> +#define I8K_HWMON_HAVE_FAN2	(1 << 11)
> +#define I8K_HWMON_HAVE_FAN3	(1 << 12)
> 
>  MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
>  MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
> @@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>  static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
>  static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>  			  3);
> +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  4);
> +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  5);
> +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
> +static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  6);
> +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
> +static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  7);
> +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
> +static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  8);
> +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
> +static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  9);
> +
>  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);
> @@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
>  	&sensor_dev_attr_temp3_label.dev_attr.attr,	/* 5 */
>  	&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_fan3_input.dev_attr.attr,	/* 14 */
> -	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 15 */
> -	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 16 */
> +	&sensor_dev_attr_temp5_input.dev_attr.attr,	/* 8 */
> +	&sensor_dev_attr_temp5_label.dev_attr.attr,	/* 9 */
> +	&sensor_dev_attr_temp6_input.dev_attr.attr,	/* 10 */
> +	&sensor_dev_attr_temp6_label.dev_attr.attr,	/* 11 */
> +	&sensor_dev_attr_temp7_input.dev_attr.attr,	/* 12 */
> +	&sensor_dev_attr_temp7_label.dev_attr.attr,	/* 13 */
> +	&sensor_dev_attr_temp8_input.dev_attr.attr,	/* 14 */
> +	&sensor_dev_attr_temp8_label.dev_attr.attr,	/* 15 */
> +	&sensor_dev_attr_temp9_input.dev_attr.attr,	/* 16 */
> +	&sensor_dev_attr_temp9_label.dev_attr.attr,	/* 17 */
> +	&sensor_dev_attr_temp10_input.dev_attr.attr,	/* 18 */
> +	&sensor_dev_attr_temp10_label.dev_attr.attr,	/* 19 */
> +	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 20 */
> +	&sensor_dev_attr_fan1_label.dev_attr.attr,	/* 21 */
> +	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 22 */
> +	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 23 */
> +	&sensor_dev_attr_fan2_label.dev_attr.attr,	/* 24 */
> +	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 25 */
> +	&sensor_dev_attr_fan3_input.dev_attr.attr,	/* 26 */
> +	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 27 */
> +	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 28 */
>  	NULL
>  };
> 
> @@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
>  	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_TEMP5))
> +		return 0;
> +	if (index >= 10 && index <= 11 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
> +		return 0;
> +	if (index >= 12 && index <= 13 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
> +		return 0;
> +	if (index >= 14 && index <= 15 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
> +		return 0;
> +	if (index >= 16 && index <= 17 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
> +		return 0;
> +	if (index >= 18 && index <= 19 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
> +		return 0;
> +
> +	if (index >= 20 && index <= 22 &&
>  	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
>  		return 0;
> -	if (index >= 11 && index <= 13 &&
> +	if (index >= 23 && index <= 25 &&
>  	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
>  		return 0;
> -	if (index >= 14 && index <= 16 &&
> +	if (index >= 26 && index <= 28 &&
>  	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
>  		return 0;
> 
> @@ -841,6 +898,24 @@ static int __init i8k_init_hwmon(void)
>  	err = i8k_get_temp_type(3);
>  	if (err >= 0)
>  		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
> +	err = i8k_get_temp_type(4);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
> +	err = i8k_get_temp_type(5);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
> +	err = i8k_get_temp_type(6);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
> +	err = i8k_get_temp_type(7);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
> +	err = i8k_get_temp_type(8);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
> +	err = i8k_get_temp_type(9);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;
> 
>  	/* First fan attributes, if fan status or type is OK */
>  	err = i8k_get_fan_status(0);

Re: [PATCH v3] dell-smm-hwmon.c: Additional temperature sensors

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 8145 bytes --]

On Friday 21 December 2018 15:40:32 Guenter Roeck wrote:
> On Fri, Dec 14, 2018 at 11:57:59PM +0000, Michele Sorcinelli wrote:
> > The code has been extended to support up to 10 temperature sensors.
> > 
> > Changes from previous patch versions:
> > 
> > - fix wrong index ranges in i8k_is_visible() if conditions
> > - restore i8k_get_temp_type() as probe method for temp sensors because
> >   i8k_get_temp() is not reliable (for example it won't work when
> >   the sensors is turned off)
> > 
> > Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>
> 
> Pali, this is waiting for your feedbacck. Any comments ?
> 
> Thanks,
> Guenter

Hi! Sorry for delay.

> > ---
> >  drivers/hwmon/dell-smm-hwmon.c | 105 ++++++++++++++++++++++++++++-----
> >  1 file changed, 90 insertions(+), 15 deletions(-)
> >  mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c

This change is wrong, source code files should not be executable.

Otherwise looks good. After removing executable bit, you can add my
Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

> > 
> > --
> > 2.20.0
> > 
> > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > old mode 100644
> > new mode 100755
> > index 367a8a617..73a30e3e4
> > --- a/drivers/hwmon/dell-smm-hwmon.c
> > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > @@ -82,9 +82,16 @@ static bool disallow_fan_support;
> >  #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
> >  #define I8K_HWMON_HAVE_TEMP3	(1 << 2)
> >  #define I8K_HWMON_HAVE_TEMP4	(1 << 3)
> > -#define I8K_HWMON_HAVE_FAN1	(1 << 4)
> > -#define I8K_HWMON_HAVE_FAN2	(1 << 5)
> > -#define I8K_HWMON_HAVE_FAN3	(1 << 6)
> > +#define I8K_HWMON_HAVE_TEMP5	(1 << 4)
> > +#define I8K_HWMON_HAVE_TEMP6	(1 << 5)
> > +#define I8K_HWMON_HAVE_TEMP7	(1 << 6)
> > +#define I8K_HWMON_HAVE_TEMP8	(1 << 7)
> > +#define I8K_HWMON_HAVE_TEMP9	(1 << 8)
> > +#define I8K_HWMON_HAVE_TEMP10	(1 << 9)
> > +
> > +#define I8K_HWMON_HAVE_FAN1	(1 << 10)
> > +#define I8K_HWMON_HAVE_FAN2	(1 << 11)
> > +#define I8K_HWMON_HAVE_FAN3	(1 << 12)
> > 
> >  MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
> >  MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
> > @@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> >  static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
> >  static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> >  			  3);
> > +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
> > +static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> > +			  4);
> > +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
> > +static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> > +			  5);
> > +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
> > +static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> > +			  6);
> > +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
> > +static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> > +			  7);
> > +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
> > +static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> > +			  8);
> > +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
> > +static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> > +			  9);
> > +
> >  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);
> > @@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
> >  	&sensor_dev_attr_temp3_label.dev_attr.attr,	/* 5 */
> >  	&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_fan3_input.dev_attr.attr,	/* 14 */
> > -	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 15 */
> > -	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 16 */
> > +	&sensor_dev_attr_temp5_input.dev_attr.attr,	/* 8 */
> > +	&sensor_dev_attr_temp5_label.dev_attr.attr,	/* 9 */
> > +	&sensor_dev_attr_temp6_input.dev_attr.attr,	/* 10 */
> > +	&sensor_dev_attr_temp6_label.dev_attr.attr,	/* 11 */
> > +	&sensor_dev_attr_temp7_input.dev_attr.attr,	/* 12 */
> > +	&sensor_dev_attr_temp7_label.dev_attr.attr,	/* 13 */
> > +	&sensor_dev_attr_temp8_input.dev_attr.attr,	/* 14 */
> > +	&sensor_dev_attr_temp8_label.dev_attr.attr,	/* 15 */
> > +	&sensor_dev_attr_temp9_input.dev_attr.attr,	/* 16 */
> > +	&sensor_dev_attr_temp9_label.dev_attr.attr,	/* 17 */
> > +	&sensor_dev_attr_temp10_input.dev_attr.attr,	/* 18 */
> > +	&sensor_dev_attr_temp10_label.dev_attr.attr,	/* 19 */
> > +	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 20 */
> > +	&sensor_dev_attr_fan1_label.dev_attr.attr,	/* 21 */
> > +	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 22 */
> > +	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 23 */
> > +	&sensor_dev_attr_fan2_label.dev_attr.attr,	/* 24 */
> > +	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 25 */
> > +	&sensor_dev_attr_fan3_input.dev_attr.attr,	/* 26 */
> > +	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 27 */
> > +	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 28 */
> >  	NULL
> >  };
> > 
> > @@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
> >  	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_TEMP5))
> > +		return 0;
> > +	if (index >= 10 && index <= 11 &&
> > +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
> > +		return 0;
> > +	if (index >= 12 && index <= 13 &&
> > +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
> > +		return 0;
> > +	if (index >= 14 && index <= 15 &&
> > +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
> > +		return 0;
> > +	if (index >= 16 && index <= 17 &&
> > +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
> > +		return 0;
> > +	if (index >= 18 && index <= 19 &&
> > +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
> > +		return 0;
> > +
> > +	if (index >= 20 && index <= 22 &&
> >  	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
> >  		return 0;
> > -	if (index >= 11 && index <= 13 &&
> > +	if (index >= 23 && index <= 25 &&
> >  	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
> >  		return 0;
> > -	if (index >= 14 && index <= 16 &&
> > +	if (index >= 26 && index <= 28 &&
> >  	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
> >  		return 0;
> > 
> > @@ -841,6 +898,24 @@ static int __init i8k_init_hwmon(void)
> >  	err = i8k_get_temp_type(3);
> >  	if (err >= 0)
> >  		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
> > +	err = i8k_get_temp_type(4);
> > +	if (err >= 0)
> > +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
> > +	err = i8k_get_temp_type(5);
> > +	if (err >= 0)
> > +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
> > +	err = i8k_get_temp_type(6);
> > +	if (err >= 0)
> > +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
> > +	err = i8k_get_temp_type(7);
> > +	if (err >= 0)
> > +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
> > +	err = i8k_get_temp_type(8);
> > +	if (err >= 0)
> > +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
> > +	err = i8k_get_temp_type(9);
> > +	if (err >= 0)
> > +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;
> > 
> >  	/* First fan attributes, if fan status or type is OK */
> >  	err = i8k_get_fan_status(0);

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[PATCH v3.1] dell-smm-hwmon.c: Additional temperature sensors

[Michele Sorcinelli]

The code has been extended to support up to 10 temperature sensors.

Changes from previous patch versions:

- fix wrong index ranges in i8k_is_visible() if conditions
- restore i8k_get_temp_type() as probe method for temp sensors because
  i8k_get_temp() is not reliable (for example it won't work when
  sensors are turned off)
- remove wrong executable bit in dell-smm-hwmon.c

Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>
Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/hwmon/dell-smm-hwmon.c | 105 ++++++++++++++++++++++++++++-----
 1 file changed, 90 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 367a8a617..73a30e3e4 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -82,9 +82,16 @@ static bool disallow_fan_support;
 #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
 #define I8K_HWMON_HAVE_TEMP3	(1 << 2)
 #define I8K_HWMON_HAVE_TEMP4	(1 << 3)
-#define I8K_HWMON_HAVE_FAN1	(1 << 4)
-#define I8K_HWMON_HAVE_FAN2	(1 << 5)
-#define I8K_HWMON_HAVE_FAN3	(1 << 6)
+#define I8K_HWMON_HAVE_TEMP5	(1 << 4)
+#define I8K_HWMON_HAVE_TEMP6	(1 << 5)
+#define I8K_HWMON_HAVE_TEMP7	(1 << 6)
+#define I8K_HWMON_HAVE_TEMP8	(1 << 7)
+#define I8K_HWMON_HAVE_TEMP9	(1 << 8)
+#define I8K_HWMON_HAVE_TEMP10	(1 << 9)
+
+#define I8K_HWMON_HAVE_FAN1	(1 << 10)
+#define I8K_HWMON_HAVE_FAN2	(1 << 11)
+#define I8K_HWMON_HAVE_FAN3	(1 << 12)

 MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
 MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
@@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
 static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
 static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
 			  3);
+static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
+static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  4);
+static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
+static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  5);
+static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
+static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  6);
+static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
+static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  7);
+static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
+static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  8);
+static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
+static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
+			  9);
+
 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);
@@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
 	&sensor_dev_attr_temp3_label.dev_attr.attr,	/* 5 */
 	&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_fan3_input.dev_attr.attr,	/* 14 */
-	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 15 */
-	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 16 */
+	&sensor_dev_attr_temp5_input.dev_attr.attr,	/* 8 */
+	&sensor_dev_attr_temp5_label.dev_attr.attr,	/* 9 */
+	&sensor_dev_attr_temp6_input.dev_attr.attr,	/* 10 */
+	&sensor_dev_attr_temp6_label.dev_attr.attr,	/* 11 */
+	&sensor_dev_attr_temp7_input.dev_attr.attr,	/* 12 */
+	&sensor_dev_attr_temp7_label.dev_attr.attr,	/* 13 */
+	&sensor_dev_attr_temp8_input.dev_attr.attr,	/* 14 */
+	&sensor_dev_attr_temp8_label.dev_attr.attr,	/* 15 */
+	&sensor_dev_attr_temp9_input.dev_attr.attr,	/* 16 */
+	&sensor_dev_attr_temp9_label.dev_attr.attr,	/* 17 */
+	&sensor_dev_attr_temp10_input.dev_attr.attr,	/* 18 */
+	&sensor_dev_attr_temp10_label.dev_attr.attr,	/* 19 */
+	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 20 */
+	&sensor_dev_attr_fan1_label.dev_attr.attr,	/* 21 */
+	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 22 */
+	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 23 */
+	&sensor_dev_attr_fan2_label.dev_attr.attr,	/* 24 */
+	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 25 */
+	&sensor_dev_attr_fan3_input.dev_attr.attr,	/* 26 */
+	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 27 */
+	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 28 */
 	NULL
 };

@@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
 	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_TEMP5))
+		return 0;
+	if (index >= 10 && index <= 11 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
+		return 0;
+	if (index >= 12 && index <= 13 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
+		return 0;
+	if (index >= 14 && index <= 15 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
+		return 0;
+	if (index >= 16 && index <= 17 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
+		return 0;
+	if (index >= 18 && index <= 19 &&
+	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
+		return 0;
+
+	if (index >= 20 && index <= 22 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
 		return 0;
-	if (index >= 11 && index <= 13 &&
+	if (index >= 23 && index <= 25 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
 		return 0;
-	if (index >= 14 && index <= 16 &&
+	if (index >= 26 && index <= 28 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
 		return 0;

@@ -841,6 +898,24 @@ static int __init i8k_init_hwmon(void)
 	err = i8k_get_temp_type(3);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
+	err = i8k_get_temp_type(4);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
+	err = i8k_get_temp_type(5);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
+	err = i8k_get_temp_type(6);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
+	err = i8k_get_temp_type(7);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
+	err = i8k_get_temp_type(8);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
+	err = i8k_get_temp_type(9);
+	if (err >= 0)
+		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;

 	/* First fan attributes, if fan status or type is OK */
 	err = i8k_get_fan_status(0);
--
2.20.1

Re: [PATCH v3.1] dell-smm-hwmon.c: Additional temperature sensors

[Michele Sorcinelli]

I've just realized that is unusual to add a minor version for patches.

I thought that it could make sense to highlight that this is a minor
change, in fact I've just removed the executable bit that was wrongly
added in the previous patches.

Please let me know if it's a problem and want me to change the version
number to v4.

On 12/22/18 2:18 PM, Michele Sorcinelli wrote:
> The code has been extended to support up to 10 temperature sensors.
> 
> Changes from previous patch versions:
> 
> - fix wrong index ranges in i8k_is_visible() if conditions
> - restore i8k_get_temp_type() as probe method for temp sensors because
>    i8k_get_temp() is not reliable (for example it won't work when
>    sensors are turned off)
> - remove wrong executable bit in dell-smm-hwmon.c
> 
> Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>
> Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>   drivers/hwmon/dell-smm-hwmon.c | 105 ++++++++++++++++++++++++++++-----
>   1 file changed, 90 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 367a8a617..73a30e3e4 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -82,9 +82,16 @@ static bool disallow_fan_support;
>   #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
>   #define I8K_HWMON_HAVE_TEMP3	(1 << 2)
>   #define I8K_HWMON_HAVE_TEMP4	(1 << 3)
> -#define I8K_HWMON_HAVE_FAN1	(1 << 4)
> -#define I8K_HWMON_HAVE_FAN2	(1 << 5)
> -#define I8K_HWMON_HAVE_FAN3	(1 << 6)
> +#define I8K_HWMON_HAVE_TEMP5	(1 << 4)
> +#define I8K_HWMON_HAVE_TEMP6	(1 << 5)
> +#define I8K_HWMON_HAVE_TEMP7	(1 << 6)
> +#define I8K_HWMON_HAVE_TEMP8	(1 << 7)
> +#define I8K_HWMON_HAVE_TEMP9	(1 << 8)
> +#define I8K_HWMON_HAVE_TEMP10	(1 << 9)
> +
> +#define I8K_HWMON_HAVE_FAN1	(1 << 10)
> +#define I8K_HWMON_HAVE_FAN2	(1 << 11)
> +#define I8K_HWMON_HAVE_FAN3	(1 << 12)
> 
>   MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
>   MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
> @@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>   static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
>   static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>   			  3);
> +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  4);
> +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  5);
> +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
> +static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  6);
> +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
> +static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  7);
> +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
> +static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  8);
> +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
> +static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  9);
> +
>   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);
> @@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
>   	&sensor_dev_attr_temp3_label.dev_attr.attr,	/* 5 */
>   	&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_fan3_input.dev_attr.attr,	/* 14 */
> -	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 15 */
> -	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 16 */
> +	&sensor_dev_attr_temp5_input.dev_attr.attr,	/* 8 */
> +	&sensor_dev_attr_temp5_label.dev_attr.attr,	/* 9 */
> +	&sensor_dev_attr_temp6_input.dev_attr.attr,	/* 10 */
> +	&sensor_dev_attr_temp6_label.dev_attr.attr,	/* 11 */
> +	&sensor_dev_attr_temp7_input.dev_attr.attr,	/* 12 */
> +	&sensor_dev_attr_temp7_label.dev_attr.attr,	/* 13 */
> +	&sensor_dev_attr_temp8_input.dev_attr.attr,	/* 14 */
> +	&sensor_dev_attr_temp8_label.dev_attr.attr,	/* 15 */
> +	&sensor_dev_attr_temp9_input.dev_attr.attr,	/* 16 */
> +	&sensor_dev_attr_temp9_label.dev_attr.attr,	/* 17 */
> +	&sensor_dev_attr_temp10_input.dev_attr.attr,	/* 18 */
> +	&sensor_dev_attr_temp10_label.dev_attr.attr,	/* 19 */
> +	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 20 */
> +	&sensor_dev_attr_fan1_label.dev_attr.attr,	/* 21 */
> +	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 22 */
> +	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 23 */
> +	&sensor_dev_attr_fan2_label.dev_attr.attr,	/* 24 */
> +	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 25 */
> +	&sensor_dev_attr_fan3_input.dev_attr.attr,	/* 26 */
> +	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 27 */
> +	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 28 */
>   	NULL
>   };
> 
> @@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
>   	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_TEMP5))
> +		return 0;
> +	if (index >= 10 && index <= 11 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
> +		return 0;
> +	if (index >= 12 && index <= 13 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
> +		return 0;
> +	if (index >= 14 && index <= 15 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
> +		return 0;
> +	if (index >= 16 && index <= 17 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
> +		return 0;
> +	if (index >= 18 && index <= 19 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
> +		return 0;
> +
> +	if (index >= 20 && index <= 22 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
>   		return 0;
> -	if (index >= 11 && index <= 13 &&
> +	if (index >= 23 && index <= 25 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
>   		return 0;
> -	if (index >= 14 && index <= 16 &&
> +	if (index >= 26 && index <= 28 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
>   		return 0;
> 
> @@ -841,6 +898,24 @@ static int __init i8k_init_hwmon(void)
>   	err = i8k_get_temp_type(3);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
> +	err = i8k_get_temp_type(4);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
> +	err = i8k_get_temp_type(5);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
> +	err = i8k_get_temp_type(6);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
> +	err = i8k_get_temp_type(7);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
> +	err = i8k_get_temp_type(8);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
> +	err = i8k_get_temp_type(9);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;
> 
>   	/* First fan attributes, if fan status or type is OK */
>   	err = i8k_get_fan_status(0);
> --
> 2.20.1
> 

Re: [PATCH v3.1] dell-smm-hwmon.c: Additional temperature sensors

[Guenter Roeck]

On 12/23/18 2:59 PM, Michele Sorcinelli wrote:
> I've just realized that is unusual to add a minor version for patches.
> 

Please don't top-post.

It is also unusual to send a new version of a patch as response to
a previous one. Not that it mattered here, but this can easily result
in a lost patch. It is also desirable to version the changelog itself,
ie describe which change was made in each version of the patch.

> I thought that it could make sense to highlight that this is a minor
> change, in fact I've just removed the executable bit that was wrongly
> added in the previous patches.
> 
> Please let me know if it's a problem and want me to change the version
> number to v4.
> 

The problem is not as much the patch version number, but other changes
made in the driver. Specifically, the driver was converted to use the new
SENSOR_DEVICE_ATTR_{RW,RO} macros.

Please rebase your patch to linux-next (or wait until the next -rc1 is
released and rebase on top of -rc1) and resend. When doing so, please use
the new macros. This should also address its checkpatch issues.

Thanks,
Guenter

> On 12/22/18 2:18 PM, Michele Sorcinelli wrote:
>> The code has been extended to support up to 10 temperature sensors.
>>
>> Changes from previous patch versions:
>>
>> - fix wrong index ranges in i8k_is_visible() if conditions
>> - restore i8k_get_temp_type() as probe method for temp sensors because
>>    i8k_get_temp() is not reliable (for example it won't work when
>>    sensors are turned off)
>> - remove wrong executable bit in dell-smm-hwmon.c
>>
>> Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>
>> Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
>> ---
>>   drivers/hwmon/dell-smm-hwmon.c | 105 ++++++++++++++++++++++++++++-----
>>   1 file changed, 90 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index 367a8a617..73a30e3e4 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -82,9 +82,16 @@ static bool disallow_fan_support;
>>   #define I8K_HWMON_HAVE_TEMP2    (1 << 1)
>>   #define I8K_HWMON_HAVE_TEMP3    (1 << 2)
>>   #define I8K_HWMON_HAVE_TEMP4    (1 << 3)
>> -#define I8K_HWMON_HAVE_FAN1    (1 << 4)
>> -#define I8K_HWMON_HAVE_FAN2    (1 << 5)
>> -#define I8K_HWMON_HAVE_FAN3    (1 << 6)
>> +#define I8K_HWMON_HAVE_TEMP5    (1 << 4)
>> +#define I8K_HWMON_HAVE_TEMP6    (1 << 5)
>> +#define I8K_HWMON_HAVE_TEMP7    (1 << 6)
>> +#define I8K_HWMON_HAVE_TEMP8    (1 << 7)
>> +#define I8K_HWMON_HAVE_TEMP9    (1 << 8)
>> +#define I8K_HWMON_HAVE_TEMP10    (1 << 9)
>> +
>> +#define I8K_HWMON_HAVE_FAN1    (1 << 10)
>> +#define I8K_HWMON_HAVE_FAN2    (1 << 11)
>> +#define I8K_HWMON_HAVE_FAN3    (1 << 12)
>>
>>   MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
>>   MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
>> @@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>   static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
>>   static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>                 3);
>> +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
>> +static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>> +              4);
>> +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
>> +static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>> +              5);
>> +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
>> +static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>> +              6);
>> +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
>> +static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>> +              7);
>> +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
>> +static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>> +              8);
>> +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
>> +static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>> +              9);
>> +
>>   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);
>> @@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
>>       &sensor_dev_attr_temp3_label.dev_attr.attr,    /* 5 */
>>       &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_fan3_input.dev_attr.attr,    /* 14 */
>> -    &sensor_dev_attr_fan3_label.dev_attr.attr,    /* 15 */
>> -    &sensor_dev_attr_pwm3.dev_attr.attr,        /* 16 */
>> +    &sensor_dev_attr_temp5_input.dev_attr.attr,    /* 8 */
>> +    &sensor_dev_attr_temp5_label.dev_attr.attr,    /* 9 */
>> +    &sensor_dev_attr_temp6_input.dev_attr.attr,    /* 10 */
>> +    &sensor_dev_attr_temp6_label.dev_attr.attr,    /* 11 */
>> +    &sensor_dev_attr_temp7_input.dev_attr.attr,    /* 12 */
>> +    &sensor_dev_attr_temp7_label.dev_attr.attr,    /* 13 */
>> +    &sensor_dev_attr_temp8_input.dev_attr.attr,    /* 14 */
>> +    &sensor_dev_attr_temp8_label.dev_attr.attr,    /* 15 */
>> +    &sensor_dev_attr_temp9_input.dev_attr.attr,    /* 16 */
>> +    &sensor_dev_attr_temp9_label.dev_attr.attr,    /* 17 */
>> +    &sensor_dev_attr_temp10_input.dev_attr.attr,    /* 18 */
>> +    &sensor_dev_attr_temp10_label.dev_attr.attr,    /* 19 */
>> +    &sensor_dev_attr_fan1_input.dev_attr.attr,    /* 20 */
>> +    &sensor_dev_attr_fan1_label.dev_attr.attr,    /* 21 */
>> +    &sensor_dev_attr_pwm1.dev_attr.attr,        /* 22 */
>> +    &sensor_dev_attr_fan2_input.dev_attr.attr,    /* 23 */
>> +    &sensor_dev_attr_fan2_label.dev_attr.attr,    /* 24 */
>> +    &sensor_dev_attr_pwm2.dev_attr.attr,        /* 25 */
>> +    &sensor_dev_attr_fan3_input.dev_attr.attr,    /* 26 */
>> +    &sensor_dev_attr_fan3_label.dev_attr.attr,    /* 27 */
>> +    &sensor_dev_attr_pwm3.dev_attr.attr,        /* 28 */
>>       NULL
>>   };
>>
>> @@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
>>       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_TEMP5))
>> +        return 0;
>> +    if (index >= 10 && index <= 11 &&
>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
>> +        return 0;
>> +    if (index >= 12 && index <= 13 &&
>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
>> +        return 0;
>> +    if (index >= 14 && index <= 15 &&
>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
>> +        return 0;
>> +    if (index >= 16 && index <= 17 &&
>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
>> +        return 0;
>> +    if (index >= 18 && index <= 19 &&
>> +        !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
>> +        return 0;
>> +
>> +    if (index >= 20 && index <= 22 &&
>>           !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
>>           return 0;
>> -    if (index >= 11 && index <= 13 &&
>> +    if (index >= 23 && index <= 25 &&
>>           !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
>>           return 0;
>> -    if (index >= 14 && index <= 16 &&
>> +    if (index >= 26 && index <= 28 &&
>>           !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
>>           return 0;
>>
>> @@ -841,6 +898,24 @@ static int __init i8k_init_hwmon(void)
>>       err = i8k_get_temp_type(3);
>>       if (err >= 0)
>>           i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
>> +    err = i8k_get_temp_type(4);
>> +    if (err >= 0)
>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
>> +    err = i8k_get_temp_type(5);
>> +    if (err >= 0)
>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
>> +    err = i8k_get_temp_type(6);
>> +    if (err >= 0)
>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
>> +    err = i8k_get_temp_type(7);
>> +    if (err >= 0)
>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
>> +    err = i8k_get_temp_type(8);
>> +    if (err >= 0)
>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
>> +    err = i8k_get_temp_type(9);
>> +    if (err >= 0)
>> +        i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;
>>
>>       /* First fan attributes, if fan status or type is OK */
>>       err = i8k_get_fan_status(0);
>> -- 
>> 2.20.1
>>
> 

[PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[Michele Sorcinelli]

Reviewed by: Pali Rohár <pali.rohar@gmail.com>

Allow the module to be loaded on Dell XPS 9570, without having to use
the "force=1" option. The module loads without problems, and reports
correct values:

    $ time cat /proc/i8k
    1.0 1.5 -1 35 0 0 0 0 -1 -22
    cat /proc/i8k  0.00s user 0.00s system 7% cpu 0.033 total

However, the call may freeze the kernel for a very small time due to
code running in the SSM layer. This is a known issue with the driver, and
can be reproduced with other supported models. Average execution
time is 33 ms.

The command line tools from i8kutils can properly set the fan speed,
although the firmware will override it, unless automatic fan
control is disabled with the proper SSM call.

Average fans speed (when firwmare automatic control is off):

STATE -> RPM
0 0 -> 0 0
1 1 -> 2500 2500
2 2 -> 5100 5100
3 3 -> same as 2 2

The SSM procedures I8K_SMM_GET_FAN_TYPE and I8K_SMM_GET_TEMP_TYPE are
failing to return valid values to the module, that can't label the fans
and initialize the temperature sensors in the hwmon interface.

This is a problem in the firmware code, and may be eventually addressed
by a new firmware version.

    $ sensors dell_smm-virtual-0
    dell_smm-virtual-0
    Adapter: Virtual device
    fan1:        2531 RPM
    fan2:        2496 RPM

However, the procedures I8K_SMM_GET_FAN and I8K_SMM_GET_TEMP are working
properly, as they return correct values. This means that the module
could be modified to initialize the sensors without labels and report
correct temperatures.
---
 drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 9d3ef879d..367a8a617 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1017,6 +1017,13 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9560"),
 		},
 	},
+	{
+		.ident = "Dell XPS 15 9570",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9570"),
+		},
+	},
 	{ }
 };
 
-- 
2.19.2

Re: [PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[Guenter Roeck]

On Fri, Nov 30, 2018 at 03:44:08PM +0000, Michele Sorcinelli wrote:
> Reviewed by: Pali Rohár <pali.rohar@gmail.com>
> 

This is not where to put a Reviewed-by: tag, and the patch is
not signed and thus can not be applied.

> Allow the module to be loaded on Dell XPS 9570, without having to use
> the "force=1" option. The module loads without problems, and reports
> correct values:
> 
>     $ time cat /proc/i8k
>     1.0 1.5 -1 35 0 0 0 0 -1 -22
>     cat /proc/i8k  0.00s user 0.00s system 7% cpu 0.033 total
> 
> However, the call may freeze the kernel for a very small time due to
> code running in the SSM layer. This is a known issue with the driver, and
> can be reproduced with other supported models. Average execution
> time is 33 ms.

Is it ? My understanding is that it is an issue with the SMM layer.
If you have actual evidence that the problem lies within the driver,
not with SMM, please share that information.

> 
> The command line tools from i8kutils can properly set the fan speed,
> although the firmware will override it, unless automatic fan
> control is disabled with the proper SSM call.
> 
> Average fans speed (when firwmare automatic control is off):
> 
> STATE -> RPM
> 0 0 -> 0 0
> 1 1 -> 2500 2500
> 2 2 -> 5100 5100
> 3 3 -> same as 2 2
> 
> The SSM procedures I8K_SMM_GET_FAN_TYPE and I8K_SMM_GET_TEMP_TYPE are
> failing to return valid values to the module, that can't label the fans
> and initialize the temperature sensors in the hwmon interface.
> 
> This is a problem in the firmware code, and may be eventually addressed
> by a new firmware version.
> 
>     $ sensors dell_smm-virtual-0
>     dell_smm-virtual-0
>     Adapter: Virtual device
>     fan1:        2531 RPM
>     fan2:        2496 RPM
> 
> However, the procedures I8K_SMM_GET_FAN and I8K_SMM_GET_TEMP are working
> properly, as they return correct values. This means that the module
> could be modified to initialize the sensors without labels and report
> correct temperatures.

This is a different problem, not part of this patch, and the information
does not belong here.

Guenter

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 9d3ef879d..367a8a617 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -1017,6 +1017,13 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9560"),
>  		},
>  	},
> +	{
> +		.ident = "Dell XPS 15 9570",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9570"),
> +		},
> +	},
>  	{ }
>  };
>  
> -- 
> 2.19.2
> 

Re: [PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[Michele Sorcinelli]

Hi Guenter, thanks for your feedback.

On 11/30/18 3:49 PM, Guenter Roeck wrote:

> This is not where to put a Reviewed-by: tag, and the patch is
> not signed and thus can not be applied.

You're right. I'll make sure the patch is properly tagged and signed.

> If you have actual evidence that the problem lies within the driver,
> not with SMM, please share that information.

As you said the problem belongs to the SMM layer, the driver can't do anything
about it. I'll remove that part as it's unnecessary and confusing.

> This is a different problem, not part of this patch, and the information
> does not belong here.

I'll remove that part as well.

Cheers,
Michele.

On 11/30/18 3:49 PM, Guenter Roeck wrote:
> On Fri, Nov 30, 2018 at 03:44:08PM +0000, Michele Sorcinelli wrote:
>> Reviewed by: Pali Rohár <pali.rohar@gmail.com>
>>
> 
> This is not where to put a Reviewed-by: tag, and the patch is
> not signed and thus can not be applied.
> 
>> Allow the module to be loaded on Dell XPS 9570, without having to use
>> the "force=1" option. The module loads without problems, and reports
>> correct values:
>>
>>      $ time cat /proc/i8k
>>      1.0 1.5 -1 35 0 0 0 0 -1 -22
>>      cat /proc/i8k  0.00s user 0.00s system 7% cpu 0.033 total
>>
>> However, the call may freeze the kernel for a very small time due to
>> code running in the SSM layer. This is a known issue with the driver, and
>> can be reproduced with other supported models. Average execution
>> time is 33 ms.
> 
> Is it ? My understanding is that it is an issue with the SMM layer.
> If you have actual evidence that the problem lies within the driver,
> not with SMM, please share that information.
> 
>>
>> The command line tools from i8kutils can properly set the fan speed,
>> although the firmware will override it, unless automatic fan
>> control is disabled with the proper SSM call.
>>
>> Average fans speed (when firwmare automatic control is off):
>>
>> STATE -> RPM
>> 0 0 -> 0 0
>> 1 1 -> 2500 2500
>> 2 2 -> 5100 5100
>> 3 3 -> same as 2 2
>>
>> The SSM procedures I8K_SMM_GET_FAN_TYPE and I8K_SMM_GET_TEMP_TYPE are
>> failing to return valid values to the module, that can't label the fans
>> and initialize the temperature sensors in the hwmon interface.
>>
>> This is a problem in the firmware code, and may be eventually addressed
>> by a new firmware version.
>>
>>      $ sensors dell_smm-virtual-0
>>      dell_smm-virtual-0
>>      Adapter: Virtual device
>>      fan1:        2531 RPM
>>      fan2:        2496 RPM
>>
>> However, the procedures I8K_SMM_GET_FAN and I8K_SMM_GET_TEMP are working
>> properly, as they return correct values. This means that the module
>> could be modified to initialize the sensors without labels and report
>> correct temperatures.
> 
> This is a different problem, not part of this patch, and the information
> does not belong here.
> 
> Guenter
> 
>> ---
>>   drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index 9d3ef879d..367a8a617 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -1017,6 +1017,13 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {
>>   			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9560"),
>>   		},
>>   	},
>> +	{
>> +		.ident = "Dell XPS 15 9570",
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9570"),
>> +		},
>> +	},
>>   	{ }
>>   };
>>   
>> -- 
>> 2.19.2
>>

[PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[Michele Sorcinelli]

Allow the module to be loaded on Dell XPS 9570, without having to
provide the "force=1" option.

Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>
Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 9d3ef879d..367a8a617 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1017,6 +1017,13 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9560"),
 		},
 	},
+	{
+		.ident = "Dell XPS 15 9570",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9570"),
+		},
+	},
 	{ }
 };
 
-- 
2.19.2

Re: [PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list

[Guenter Roeck]

On Fri, Nov 30, 2018 at 06:42:56PM +0000, Michele Sorcinelli wrote:
> Allow the module to be loaded on Dell XPS 9570, without having to
> provide the "force=1" option.
> 
> Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>
> Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 9d3ef879d..367a8a617 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -1017,6 +1017,13 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9560"),
>  		},
>  	},
> +	{
> +		.ident = "Dell XPS 15 9570",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9570"),
> +		},
> +	},
>  	{ }
>  };
>