[PATCH] i8k: Add support for temperature sensor labels

[PATCH] i8k: Add support for temperature sensor labels

[Pali Rohár]

This patch adds labels for temperature sensors if SMM function with EAX register
0x11a3 reports it. These informations was taken from DOS binary NBSVC.MDM.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/char/i8k.c |  110 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 88 insertions(+), 22 deletions(-)

diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
index e34a019..77af46b 100644
--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -42,6 +42,7 @@
 #define I8K_SMM_GET_FAN		0x00a3
 #define I8K_SMM_GET_SPEED	0x02a3
 #define I8K_SMM_GET_TEMP	0x10a3
+#define I8K_SMM_GET_TEMP_TYPE	0x11a3
 #define I8K_SMM_GET_DELL_SIG1	0xfea3
 #define I8K_SMM_GET_DELL_SIG2	0xffa3
 
@@ -288,6 +289,14 @@ static int i8k_set_fan(int fan, int speed)
 	return i8k_smm(&regs) ? : i8k_get_fan_status(fan);
 }
 
+static int i8k_get_temp_type(int sensor)
+{
+	struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP_TYPE, };
+
+	regs.ebx = sensor & 0xff;
+	return i8k_smm(&regs) ? : regs.eax & 0xff;
+}
+
 /*
  * Read the cpu temperature.
  */
@@ -493,6 +502,29 @@ static int i8k_open_fs(struct inode *inode, struct file *file)
  * Hwmon interface
  */
 
+static ssize_t i8k_hwmon_show_temp_label(struct device *dev,
+				   struct device_attribute *devattr,
+				   char *buf)
+{
+	static const char * const labels[] = {
+		"CPU",
+		"GPU",
+		"SODIMM",
+		"Other",
+		"Ambient",
+		"Other",
+	};
+	int index = to_sensor_dev_attr(devattr)->index;
+	int type;
+
+	type = i8k_get_temp_type(index);
+	if (type < 0)
+		return type;
+	if (type >= ARRAY_SIZE(labels))
+		type = ARRAY_SIZE(labels) - 1;
+	return sprintf(buf, "%s\n", labels[type]);
+}
+
 static ssize_t i8k_hwmon_show_temp(struct device *dev,
 				   struct device_attribute *devattr,
 				   char *buf)
@@ -555,9 +587,13 @@ static ssize_t i8k_hwmon_set_pwm(struct device *dev,
 }
 
 static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, 0);
 static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 1);
+static SENSOR_DEVICE_ATTR(temp2_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, 1);
 static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 2);
+static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, 2);
 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(fan1_input, S_IRUGO, i8k_hwmon_show_fan, NULL,
 			  I8K_FAN_LEFT);
 static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, i8k_hwmon_show_pwm,
@@ -569,31 +605,35 @@ static SENSOR_DEVICE_ATTR(pwm2, S_IRUGO | S_IWUSR, i8k_hwmon_show_pwm,
 
 static struct attribute *i8k_attrs[] = {
 	&sensor_dev_attr_temp1_input.dev_attr.attr,	/* 0 */
-	&sensor_dev_attr_temp2_input.dev_attr.attr,	/* 1 */
-	&sensor_dev_attr_temp3_input.dev_attr.attr,	/* 2 */
-	&sensor_dev_attr_temp4_input.dev_attr.attr,	/* 3 */
-	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 4 */
-	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 5 */
-	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 6 */
-	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 7 */
+	&sensor_dev_attr_temp1_label.dev_attr.attr,	/* 1 */
+	&sensor_dev_attr_temp2_input.dev_attr.attr,	/* 2 */
+	&sensor_dev_attr_temp2_label.dev_attr.attr,	/* 3 */
+	&sensor_dev_attr_temp3_input.dev_attr.attr,	/* 4 */
+	&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_pwm1.dev_attr.attr,		/* 9 */
+	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 10 */
+	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 11 */
 	NULL
 };
 
 static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
 			      int index)
 {
-	if (index == 0 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
+	if (index >= 0 && index <= 1 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
 		return 0;
-	if (index == 1 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP2))
+	if (index >= 2 && index <= 3 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP2))
 		return 0;
-	if (index == 2 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP3))
+	if (index >= 4 && index <= 5 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP3))
 		return 0;
-	if (index == 3 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP4))
+	if (index >= 6 && index <= 7 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP4))
 		return 0;
-	if (index >= 4 && index <= 5 &&
+	if (index >= 8 && index <= 9 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
 		return 0;
-	if (index >= 6 && index <= 7 &&
+	if (index >= 10 && index <= 11 &&
 	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
 		return 0;
 
@@ -606,6 +646,37 @@ static const struct attribute_group i8k_group = {
 };
 __ATTRIBUTE_GROUPS(i8k);
 
+static bool __init i8k_check_temp(int sensor)
+{
+	int err;
+
+	/*
+	 * Check if temperature sensor type is valid.
+	 *
+	 * If it is valid then sensor should work. But some sensors are not
+	 * available at any time. E.g GPU sensor on Optimus/PowerExpress/Enduro
+	 * card does not work (or return bogus value) when card is turned off.
+	 * So this function should not fail in this case.
+	 */
+	err = i8k_get_temp_type(sensor);
+	if (err >= 0)
+		return true;
+
+	/*
+	 * Check if temperatrue reading does not fail.
+	 *
+	 * Sometimes detection of temperature sensor type does not work but
+	 * reading temperature working fine. Sometimes temperature value is too
+	 * high and i8k_get_temp() returns -ERANGE. But there is no reason to
+	 * ignore these temperature sensors.
+	 */
+	err = i8k_get_temp(sensor);
+	if (err >= 0 || err == -ERANGE)
+		return true;
+
+	return false;
+}
+
 static int __init i8k_init_hwmon(void)
 {
 	int err;
@@ -613,18 +684,13 @@ static int __init i8k_init_hwmon(void)
 	i8k_hwmon_flags = 0;
 
 	/* CPU temperature attributes, if temperature reading is OK */
-	err = i8k_get_temp(0);
-	if (err >= 0 || err == -ERANGE)
+	if (i8k_check_temp(0))
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
-	/* check for additional temperature sensors */
-	err = i8k_get_temp(1);
-	if (err >= 0 || err == -ERANGE)
+	if (i8k_check_temp(1))
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
-	err = i8k_get_temp(2);
-	if (err >= 0 || err == -ERANGE)
+	if (i8k_check_temp(2))
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
-	err = i8k_get_temp(3);
-	if (err >= 0 || err == -ERANGE)
+	if (i8k_check_temp(3))
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
 
 	/* Left fan attributes, if left fan is present */
-- 
1.7.9.5

Re: [PATCH] i8k: Add support for temperature sensor labels

[Pali Rohár]

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

On Saturday 29 November 2014 17:04:07 Pali Rohár wrote:
> This patch adds labels for temperature sensors if SMM function
> with EAX register 0x11a3 reports it. These informations was
> taken from DOS binary NBSVC.MDM.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/char/i8k.c |  110
> +++++++++++++++++++++++++++++++++++++++++----------- 1 file
> changed, 88 insertions(+), 22 deletions(-)

I tested patch on Latitude E6440 and i8k CPU & GPU temps match 
intel coretemp & amd radeion temps.

But I would like if somebody with other Dell laptop can test if 
temperature labels are correct...

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

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

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On 11/29/2014 08:09 AM, Pali Rohár wrote:
> On Saturday 29 November 2014 17:04:07 Pali Rohár wrote:
>> This patch adds labels for temperature sensors if SMM function
>> with EAX register 0x11a3 reports it. These informations was
>> taken from DOS binary NBSVC.MDM.
>>
>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>> ---
>>   drivers/char/i8k.c |  110
>> +++++++++++++++++++++++++++++++++++++++++----------- 1 file
>> changed, 88 insertions(+), 22 deletions(-)
>
> I tested patch on Latitude E6440 and i8k CPU & GPU temps match
> intel coretemp & amd radeion temps.
>
> But I would like if somebody with other Dell laptop can test if
> temperature labels are correct...
>

I have a couple; I'll try to give it a test this weekend.

Guenter

Re: [PATCH] i8k: Add support for temperature sensor labels

[Pali Rohár]

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

On Saturday 29 November 2014 17:24:56 Guenter Roeck wrote:
> On 11/29/2014 08:09 AM, Pali Rohár wrote:
> > On Saturday 29 November 2014 17:04:07 Pali Rohár wrote:
> >> This patch adds labels for temperature sensors if SMM
> >> function with EAX register 0x11a3 reports it. These
> >> informations was taken from DOS binary NBSVC.MDM.
> >> 
> >> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> >> ---
> >> 
> >>   drivers/char/i8k.c |  110
> >> 
> >> +++++++++++++++++++++++++++++++++++++++++----------- 1 file
> >> changed, 88 insertions(+), 22 deletions(-)
> > 
> > I tested patch on Latitude E6440 and i8k CPU & GPU temps
> > match intel coretemp & amd radeion temps.
> > 
> > But I would like if somebody with other Dell laptop can test
> > if temperature labels are correct...
> 
> I have a couple; I'll try to give it a test this weekend.
> 
> Guenter

Ok, thanks! Can you include also this patch when you will do testing?

https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-testing&id=723493ca59c8d81fed3e7f261165fee493a29ffa

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

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

Re: [PATCH] i8k: Add support for temperature sensor labels

[Steven Honeyman]

On 29 November 2014 at 16:09, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Saturday 29 November 2014 17:04:07 Pali Rohár wrote:
>> This patch adds labels for temperature sensors if SMM function
>> with EAX register 0x11a3 reports it. These informations was
>> taken from DOS binary NBSVC.MDM.
>>
>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>> ---
>>  drivers/char/i8k.c |  110
>> +++++++++++++++++++++++++++++++++++++++++----------- 1 file
>> changed, 88 insertions(+), 22 deletions(-)
>
> I tested patch on Latitude E6440 and i8k CPU & GPU temps match
> intel coretemp & amd radeion temps.
>
> But I would like if somebody with other Dell laptop can test if
> temperature labels are correct...

Tested and working on E6540:
(I manually masked the serial/service tag)

-----Before-----

*[steven@e6540 linux]$ cat /proc/i8k
1.0 A12 6xxxxxx 44 -22 1 -22 3223 -1 -22
*[steven@e6540 linux]$ sensors
i8k-virtual-0
Adapter: Virtual device
fan2:        3223 RPM
temp1:        +46.0°C
temp2:        +44.0°C
temp3:        +38.0°C

coretemp-isa-0000
Adapter: ISA adapter
Physical id 0:  +49.0°C  (high = +84.0°C, crit = +100.0°C)
Core 0:         +45.0°C  (high = +84.0°C, crit = +100.0°C)
Core 1:         +49.0°C  (high = +84.0°C, crit = +100.0°C)

-----After-----

*[steven@e6540 ~]$ cat /proc/i8k
1.0 A12 6xxxxxx 47 -22 1 -22 3176 -1 -22
*[steven@e6540 ~]$ sensors
i8k-virtual-0
Adapter: Virtual device
fan2:        3176 RPM
CPU:          +47.0°C
Ambient:      +45.0°C
SODIMM:       +38.0°C

coretemp-isa-0000
Adapter: ISA adapter
Physical id 0:  +47.0°C  (high = +84.0°C, crit = +100.0°C)
Core 0:         +45.0°C  (high = +84.0°C, crit = +100.0°C)
Core 1:         +46.0°C  (high = +84.0°C, crit = +100.0°C)

Re: [PATCH] i8k: Add support for temperature sensor labels

[Gabriele Mazzotta]

On Saturday 29 November 2014 17:09:35 Pali Rohár wrote:
> On Saturday 29 November 2014 17:04:07 Pali Rohár wrote:
> > This patch adds labels for temperature sensors if SMM function
> > with EAX register 0x11a3 reports it. These informations was
> > taken from DOS binary NBSVC.MDM.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> > 
> >  drivers/char/i8k.c |  110
> > 
> > +++++++++++++++++++++++++++++++++++++++++----------- 1 file
> > changed, 88 insertions(+), 22 deletions(-)
> 
> I tested patch on Latitude E6440 and i8k CPU & GPU temps match
> intel coretemp & amd radeion temps.
> 
> But I would like if somebody with other Dell laptop can test if
> temperature labels are correct...

I tested it on my XPS13 9333, here what sensors outputs:

acpitz-virtual-0
Adapter: Virtual device
temp1:        +27.8°C  (crit = +105.0°C)
temp2:        +29.8°C  (crit = +105.0°C)

coretemp-isa-0000
Adapter: ISA adapter
Physical id 0:  +62.0°C  (high = +100.0°C, crit = +100.0°C)
Core 0:         +62.0°C  (high = +100.0°C, crit = +100.0°C)
Core 1:         +61.0°C  (high = +100.0°C, crit = +100.0°C)

i8k-virtual-0
Adapter: Virtual device
fan2:           0 RPM
CPU:          +62.0°C  
Ambient:      +49.0°C  
SODIMM:       +46.0°C  
temp4:            N/A

CPU seems to be correct, but I can't say anything on Ambient and SODIMM.
temp4 is constantly equal to SODIMM without this patch, so I'd say N/A
is correct.


Gabriele

Re: [PATCH] i8k: Add support for temperature sensor labels

[Pali Rohár]

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

On Saturday 29 November 2014 18:07:19 Gabriele Mazzotta wrote:
> On Saturday 29 November 2014 17:09:35 Pali Rohár wrote:
> > On Saturday 29 November 2014 17:04:07 Pali Rohár wrote:
> > > This patch adds labels for temperature sensors if SMM
> > > function with EAX register 0x11a3 reports it. These
> > > informations was taken from DOS binary NBSVC.MDM.
> > > 
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > ---
> > > 
> > >  drivers/char/i8k.c |  110
> > > 
> > > +++++++++++++++++++++++++++++++++++++++++----------- 1
> > > file changed, 88 insertions(+), 22 deletions(-)
> > 
> > I tested patch on Latitude E6440 and i8k CPU & GPU temps
> > match intel coretemp & amd radeion temps.
> > 
> > But I would like if somebody with other Dell laptop can test
> > if temperature labels are correct...
> 
> I tested it on my XPS13 9333, here what sensors outputs:
> 
> acpitz-virtual-0
> Adapter: Virtual device
> temp1:        +27.8°C  (crit = +105.0°C)
> temp2:        +29.8°C  (crit = +105.0°C)
> 
> coretemp-isa-0000
> Adapter: ISA adapter
> Physical id 0:  +62.0°C  (high = +100.0°C, crit = +100.0°C)
> Core 0:         +62.0°C  (high = +100.0°C, crit = +100.0°C)
> Core 1:         +61.0°C  (high = +100.0°C, crit = +100.0°C)
> 
> i8k-virtual-0
> Adapter: Virtual device
> fan2:           0 RPM
> CPU:          +62.0°C
> Ambient:      +49.0°C
> SODIMM:       +46.0°C
> temp4:            N/A
> 
> CPU seems to be correct, but I can't say anything on Ambient
> and SODIMM. temp4 is constantly equal to SODIMM without this
> patch, so I'd say N/A is correct.
> 
> 
> Gabriele

It is unknown for me how to directly read Ambient and SODIMM
temperatures (without Dell SMM functions). So we can only trust
Dell SMM that it reporting correct values and type is really
Ambient and SODIMM.

And about temp4:

Label is not set when SMM function fails. Original DOS NBSVC.MDM
just ignore all sensors for which SMM type function fails.

This patch should not disable any sensor, so if you previously
had some value (<= 128°C) and now not, then there is some bug.

Can you test this patch?
https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-testing&id=723493ca59c8d81fed3e7f261165fee493a29ffa

It is possible that same value is caused by incorrect use of
prev[] array which should be fixed by above patch.

Can you test i8k with and without above patch?

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

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

Re: [PATCH] i8k: Add support for temperature sensor labels

[Gabriele Mazzotta]

On Saturday 29 November 2014 18:18:18 Pali Rohár wrote:
> On Saturday 29 November 2014 18:07:19 Gabriele Mazzotta wrote:
> > On Saturday 29 November 2014 17:09:35 Pali Rohár wrote:
> > > On Saturday 29 November 2014 17:04:07 Pali Rohár wrote:
> > > > This patch adds labels for temperature sensors if SMM
> > > > function with EAX register 0x11a3 reports it. These
> > > > informations was taken from DOS binary NBSVC.MDM.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/char/i8k.c |  110
> > > > 
> > > > +++++++++++++++++++++++++++++++++++++++++----------- 1
> > > > file changed, 88 insertions(+), 22 deletions(-)
> > > 
> > > I tested patch on Latitude E6440 and i8k CPU & GPU temps
> > > match intel coretemp & amd radeion temps.
> > > 
> > > But I would like if somebody with other Dell laptop can test
> > > if temperature labels are correct...
> > 
> > I tested it on my XPS13 9333, here what sensors outputs:
> > 
> > acpitz-virtual-0
> > Adapter: Virtual device
> > temp1:        +27.8°C  (crit = +105.0°C)
> > temp2:        +29.8°C  (crit = +105.0°C)
> > 
> > coretemp-isa-0000
> > Adapter: ISA adapter
> > Physical id 0:  +62.0°C  (high = +100.0°C, crit = +100.0°C)
> > Core 0:         +62.0°C  (high = +100.0°C, crit = +100.0°C)
> > Core 1:         +61.0°C  (high = +100.0°C, crit = +100.0°C)
> > 
> > i8k-virtual-0
> > Adapter: Virtual device
> > fan2:           0 RPM
> > CPU:          +62.0°C
> > Ambient:      +49.0°C
> > SODIMM:       +46.0°C
> > temp4:            N/A
> > 
> > CPU seems to be correct, but I can't say anything on Ambient
> > and SODIMM. temp4 is constantly equal to SODIMM without this
> > patch, so I'd say N/A is correct.
> > 
> > 
> > Gabriele
> 
> It is unknown for me how to directly read Ambient and SODIMM
> temperatures (without Dell SMM functions). So we can only trust
> Dell SMM that it reporting correct values and type is really
> Ambient and SODIMM.
> 
> And about temp4:
> 
> Label is not set when SMM function fails. Original DOS NBSVC.MDM
> just ignore all sensors for which SMM type function fails.
> 
> This patch should not disable any sensor, so if you previously
> had some value (<= 128°C) and now not, then there is some bug.
> 
> Can you test this patch?
> https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/comm
> it/?h=char-misc-testing&id=723493ca59c8d81fed3e7f261165fee493a29ffa
> 
> It is possible that same value is caused by incorrect use of
> prev[] array which should be fixed by above patch.
> 
> Can you test i8k with and without above patch?

I did some more tests. What I think is happening is that temp4_label
returns -22, so I sensors prints N/A without actually reading temp4_input.

I'm doing some tests to understand what's going on with temp4_input.
It reports the value of the last temp*_input I read. If I read it right
after I loaded i8k, I get an error (-22).

The same doesn't happen for temp3_label, which constantly returns -22.

I already had https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-testing&id=723493ca59c8d81fed3e7f261165fee493a29ffa
applied. Without it, things seem not to change much.
I either get an error (-22) or some incorrect values (for now always
1000) when I read temp4_input right after i8k was loaded.
Once I read some other temp*_input, I always get the last value read.

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On 11/29/2014 10:27 AM, Gabriele Mazzotta wrote:
> On Saturday 29 November 2014 18:18:18 Pali Rohár wrote:
>> On Saturday 29 November 2014 18:07:19 Gabriele Mazzotta wrote:
>>> On Saturday 29 November 2014 17:09:35 Pali Rohár wrote:
>>>> On Saturday 29 November 2014 17:04:07 Pali Rohár wrote:
>>>>> This patch adds labels for temperature sensors if SMM
>>>>> function with EAX register 0x11a3 reports it. These
>>>>> informations was taken from DOS binary NBSVC.MDM.
>>>>>
>>>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>>>>> ---
>>>>>
>>>>>   drivers/char/i8k.c |  110
>>>>>
>>>>> +++++++++++++++++++++++++++++++++++++++++----------- 1
>>>>> file changed, 88 insertions(+), 22 deletions(-)
>>>>
>>>> I tested patch on Latitude E6440 and i8k CPU & GPU temps
>>>> match intel coretemp & amd radeion temps.
>>>>
>>>> But I would like if somebody with other Dell laptop can test
>>>> if temperature labels are correct...
>>>
>>> I tested it on my XPS13 9333, here what sensors outputs:
>>>
>>> acpitz-virtual-0
>>> Adapter: Virtual device
>>> temp1:        +27.8°C  (crit = +105.0°C)
>>> temp2:        +29.8°C  (crit = +105.0°C)
>>>
>>> coretemp-isa-0000
>>> Adapter: ISA adapter
>>> Physical id 0:  +62.0°C  (high = +100.0°C, crit = +100.0°C)
>>> Core 0:         +62.0°C  (high = +100.0°C, crit = +100.0°C)
>>> Core 1:         +61.0°C  (high = +100.0°C, crit = +100.0°C)
>>>
>>> i8k-virtual-0
>>> Adapter: Virtual device
>>> fan2:           0 RPM
>>> CPU:          +62.0°C
>>> Ambient:      +49.0°C
>>> SODIMM:       +46.0°C
>>> temp4:            N/A
>>>
>>> CPU seems to be correct, but I can't say anything on Ambient
>>> and SODIMM. temp4 is constantly equal to SODIMM without this
>>> patch, so I'd say N/A is correct.
>>>
>>>
>>> Gabriele
>>
>> It is unknown for me how to directly read Ambient and SODIMM
>> temperatures (without Dell SMM functions). So we can only trust
>> Dell SMM that it reporting correct values and type is really
>> Ambient and SODIMM.
>>
>> And about temp4:
>>
>> Label is not set when SMM function fails. Original DOS NBSVC.MDM
>> just ignore all sensors for which SMM type function fails.
>>
>> This patch should not disable any sensor, so if you previously
>> had some value (<= 128°C) and now not, then there is some bug.
>>
>> Can you test this patch?
>> https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/comm
>> it/?h=char-misc-testing&id=723493ca59c8d81fed3e7f261165fee493a29ffa
>>
>> It is possible that same value is caused by incorrect use of
>> prev[] array which should be fixed by above patch.
>>
>> Can you test i8k with and without above patch?
>
> I did some more tests. What I think is happening is that temp4_label
> returns -22, so I sensors prints N/A without actually reading temp4_input.
>
> I'm doing some tests to understand what's going on with temp4_input.
> It reports the value of the last temp*_input I read. If I read it right
> after I loaded i8k, I get an error (-22).
>
> The same doesn't happen for temp3_label, which constantly returns -22.
>
> I already had https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-testing&id=723493ca59c8d81fed3e7f261165fee493a29ffa
> applied. Without it, things seem not to change much.
> I either get an error (-22) or some incorrect values (for now always
> 1000) when I read temp4_input right after i8k was loaded.
> Once I read some other temp*_input, I always get the last value read.
>
I am seeing exactly the same behavior on an XPS13.

Guenter

Re: [PATCH] i8k: Add support for temperature sensor labels

[Pali Rohár]

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

On Saturday 29 November 2014 19:58:28 Guenter Roeck wrote:
> On 11/29/2014 10:27 AM, Gabriele Mazzotta wrote:
> > On Saturday 29 November 2014 18:18:18 Pali Rohár wrote:
> >> On Saturday 29 November 2014 18:07:19 Gabriele Mazzotta 
wrote:
> >>> On Saturday 29 November 2014 17:09:35 Pali Rohár wrote:
> >>>> On Saturday 29 November 2014 17:04:07 Pali Rohár wrote:
> >>>>> This patch adds labels for temperature sensors if SMM
> >>>>> function with EAX register 0x11a3 reports it. These
> >>>>> informations was taken from DOS binary NBSVC.MDM.
> >>>>> 
> >>>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> >>>>> ---
> >>>>> 
> >>>>>   drivers/char/i8k.c |  110
> >>>>> 
> >>>>> +++++++++++++++++++++++++++++++++++++++++----------- 1
> >>>>> file changed, 88 insertions(+), 22 deletions(-)
> >>>> 
> >>>> I tested patch on Latitude E6440 and i8k CPU & GPU temps
> >>>> match intel coretemp & amd radeion temps.
> >>>> 
> >>>> But I would like if somebody with other Dell laptop can
> >>>> test if temperature labels are correct...
> >>> 
> >>> I tested it on my XPS13 9333, here what sensors outputs:
> >>> 
> >>> acpitz-virtual-0
> >>> Adapter: Virtual device
> >>> temp1:        +27.8°C  (crit = +105.0°C)
> >>> temp2:        +29.8°C  (crit = +105.0°C)
> >>> 
> >>> coretemp-isa-0000
> >>> Adapter: ISA adapter
> >>> Physical id 0:  +62.0°C  (high = +100.0°C, crit =
> >>> +100.0°C) Core 0:         +62.0°C  (high = +100.0°C, crit
> >>> = +100.0°C) Core 1:         +61.0°C  (high = +100.0°C,
> >>> crit = +100.0°C)
> >>> 
> >>> i8k-virtual-0
> >>> Adapter: Virtual device
> >>> fan2:           0 RPM
> >>> CPU:          +62.0°C
> >>> Ambient:      +49.0°C
> >>> SODIMM:       +46.0°C
> >>> temp4:            N/A
> >>> 
> >>> CPU seems to be correct, but I can't say anything on
> >>> Ambient and SODIMM. temp4 is constantly equal to SODIMM
> >>> without this patch, so I'd say N/A is correct.
> >>> 
> >>> 
> >>> Gabriele
> >> 
> >> It is unknown for me how to directly read Ambient and
> >> SODIMM temperatures (without Dell SMM functions). So we
> >> can only trust Dell SMM that it reporting correct values
> >> and type is really Ambient and SODIMM.
> >> 
> >> And about temp4:
> >> 
> >> Label is not set when SMM function fails. Original DOS
> >> NBSVC.MDM just ignore all sensors for which SMM type
> >> function fails.
> >> 
> >> This patch should not disable any sensor, so if you
> >> previously had some value (<= 128°C) and now not, then
> >> there is some bug.
> >> 
> >> Can you test this patch?
> >> https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-mi
> >> sc.git/comm
> >> it/?h=char-misc-testing&id=723493ca59c8d81fed3e7f261165fee
> >> 493a29ffa
> >> 
> >> It is possible that same value is caused by incorrect use
> >> of prev[] array which should be fixed by above patch.
> >> 
> >> Can you test i8k with and without above patch?
> > 
> > I did some more tests. What I think is happening is that
> > temp4_label returns -22, so I sensors prints N/A without
> > actually reading temp4_input.
> > 
> > I'm doing some tests to understand what's going on with
> > temp4_input. It reports the value of the last temp*_input I
> > read. If I read it right after I loaded i8k, I get an error
> > (-22).
> > 
> > The same doesn't happen for temp3_label, which constantly
> > returns -22.
> > 
> > I already had
> > https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-mi
> > sc.git/commit/?h=char-misc-testing&id=723493ca59c8d81fed3e7f
> > 261165fee493a29ffa applied. Without it, things seem not to
> > change much. I either get an error (-22) or some incorrect
> > values (for now always 1000) when I read temp4_input right
> > after i8k was loaded. Once I read some other temp*_input, I
> > always get the last value read.
> 
> I am seeing exactly the same behavior on an XPS13.
> 
> Guenter

Original Dell DOS executable ignores all temperature sensors if 
type SMM function fails (if I decoded and understand that DOS 
assembler code correctly). So maybe we should do same...

But because our i8k.c code ignores sensor only if it returns 
invalid temperature, there could be possible regression that on 
same machines type SMM function is not implemented or not 
working...

What do you think?

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

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

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On 11/29/2014 11:07 AM, Pali Rohár wrote:
> On Saturday 29 November 2014 19:58:28 Guenter Roeck wrote:
>> On 11/29/2014 10:27 AM, Gabriele Mazzotta wrote:
>>> On Saturday 29 November 2014 18:18:18 Pali Rohár wrote:
>>>> On Saturday 29 November 2014 18:07:19 Gabriele Mazzotta
> wrote:
>>>>> On Saturday 29 November 2014 17:09:35 Pali Rohár wrote:
>>>>>> On Saturday 29 November 2014 17:04:07 Pali Rohár wrote:
>>>>>>> This patch adds labels for temperature sensors if SMM
>>>>>>> function with EAX register 0x11a3 reports it. These
>>>>>>> informations was taken from DOS binary NBSVC.MDM.
>>>>>>>
>>>>>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>>    drivers/char/i8k.c |  110
>>>>>>>
>>>>>>> +++++++++++++++++++++++++++++++++++++++++----------- 1
>>>>>>> file changed, 88 insertions(+), 22 deletions(-)
>>>>>>
>>>>>> I tested patch on Latitude E6440 and i8k CPU & GPU temps
>>>>>> match intel coretemp & amd radeion temps.
>>>>>>
>>>>>> But I would like if somebody with other Dell laptop can
>>>>>> test if temperature labels are correct...
>>>>>
>>>>> I tested it on my XPS13 9333, here what sensors outputs:
>>>>>
>>>>> acpitz-virtual-0
>>>>> Adapter: Virtual device
>>>>> temp1:        +27.8°C  (crit = +105.0°C)
>>>>> temp2:        +29.8°C  (crit = +105.0°C)
>>>>>
>>>>> coretemp-isa-0000
>>>>> Adapter: ISA adapter
>>>>> Physical id 0:  +62.0°C  (high = +100.0°C, crit =
>>>>> +100.0°C) Core 0:         +62.0°C  (high = +100.0°C, crit
>>>>> = +100.0°C) Core 1:         +61.0°C  (high = +100.0°C,
>>>>> crit = +100.0°C)
>>>>>
>>>>> i8k-virtual-0
>>>>> Adapter: Virtual device
>>>>> fan2:           0 RPM
>>>>> CPU:          +62.0°C
>>>>> Ambient:      +49.0°C
>>>>> SODIMM:       +46.0°C
>>>>> temp4:            N/A
>>>>>
>>>>> CPU seems to be correct, but I can't say anything on
>>>>> Ambient and SODIMM. temp4 is constantly equal to SODIMM
>>>>> without this patch, so I'd say N/A is correct.
>>>>>
>>>>>
>>>>> Gabriele
>>>>
>>>> It is unknown for me how to directly read Ambient and
>>>> SODIMM temperatures (without Dell SMM functions). So we
>>>> can only trust Dell SMM that it reporting correct values
>>>> and type is really Ambient and SODIMM.
>>>>
>>>> And about temp4:
>>>>
>>>> Label is not set when SMM function fails. Original DOS
>>>> NBSVC.MDM just ignore all sensors for which SMM type
>>>> function fails.
>>>>
>>>> This patch should not disable any sensor, so if you
>>>> previously had some value (<= 128°C) and now not, then
>>>> there is some bug.
>>>>
>>>> Can you test this patch?
>>>> https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-mi
>>>> sc.git/comm
>>>> it/?h=char-misc-testing&id=723493ca59c8d81fed3e7f261165fee
>>>> 493a29ffa
>>>>
>>>> It is possible that same value is caused by incorrect use
>>>> of prev[] array which should be fixed by above patch.
>>>>
>>>> Can you test i8k with and without above patch?
>>>
>>> I did some more tests. What I think is happening is that
>>> temp4_label returns -22, so I sensors prints N/A without
>>> actually reading temp4_input.
>>>
>>> I'm doing some tests to understand what's going on with
>>> temp4_input. It reports the value of the last temp*_input I
>>> read. If I read it right after I loaded i8k, I get an error
>>> (-22).
>>>
>>> The same doesn't happen for temp3_label, which constantly
>>> returns -22.
>>>
>>> I already had
>>> https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-mi
>>> sc.git/commit/?h=char-misc-testing&id=723493ca59c8d81fed3e7f
>>> 261165fee493a29ffa applied. Without it, things seem not to
>>> change much. I either get an error (-22) or some incorrect
>>> values (for now always 1000) when I read temp4_input right
>>> after i8k was loaded. Once I read some other temp*_input, I
>>> always get the last value read.
>>
>> I am seeing exactly the same behavior on an XPS13.
>>
>> Guenter
>
> Original Dell DOS executable ignores all temperature sensors if
> type SMM function fails (if I decoded and understand that DOS
> assembler code correctly). So maybe we should do same...
>
> But because our i8k.c code ignores sensor only if it returns
> invalid temperature, there could be possible regression that on
> same machines type SMM function is not implemented or not
> working...
>
> What do you think?
>
Guess we should be able to do what the DOS executable does.
Let me test on a couple of older systems and let's make
a decision based on the results.

Guenter

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On 11/29/2014 11:07 AM, Pali Rohár wrote:
> On Saturday 29 November 2014 19:58:28 Guenter Roeck wrote:
>> On 11/29/2014 10:27 AM, Gabriele Mazzotta wrote:
>>> On Saturday 29 November 2014 18:18:18 Pali Rohár wrote:
>>>> On Saturday 29 November 2014 18:07:19 Gabriele Mazzotta
> wrote:
>>>>> On Saturday 29 November 2014 17:09:35 Pali Rohár wrote:
>>>>>> On Saturday 29 November 2014 17:04:07 Pali Rohár wrote:
>>>>>>> This patch adds labels for temperature sensors if SMM
>>>>>>> function with EAX register 0x11a3 reports it. These
>>>>>>> informations was taken from DOS binary NBSVC.MDM.
>>>>>>>
>>>>>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>>    drivers/char/i8k.c |  110
>>>>>>>
>>>>>>> +++++++++++++++++++++++++++++++++++++++++----------- 1
>>>>>>> file changed, 88 insertions(+), 22 deletions(-)
>>>>>>
>>>>>> I tested patch on Latitude E6440 and i8k CPU & GPU temps
>>>>>> match intel coretemp & amd radeion temps.
>>>>>>
>>>>>> But I would like if somebody with other Dell laptop can
>>>>>> test if temperature labels are correct...
>>>>>
>>>>> I tested it on my XPS13 9333, here what sensors outputs:
>>>>>
>>>>> acpitz-virtual-0
>>>>> Adapter: Virtual device
>>>>> temp1:        +27.8°C  (crit = +105.0°C)
>>>>> temp2:        +29.8°C  (crit = +105.0°C)
>>>>>
>>>>> coretemp-isa-0000
>>>>> Adapter: ISA adapter
>>>>> Physical id 0:  +62.0°C  (high = +100.0°C, crit =
>>>>> +100.0°C) Core 0:         +62.0°C  (high = +100.0°C, crit
>>>>> = +100.0°C) Core 1:         +61.0°C  (high = +100.0°C,
>>>>> crit = +100.0°C)
>>>>>
>>>>> i8k-virtual-0
>>>>> Adapter: Virtual device
>>>>> fan2:           0 RPM
>>>>> CPU:          +62.0°C
>>>>> Ambient:      +49.0°C
>>>>> SODIMM:       +46.0°C
>>>>> temp4:            N/A
>>>>>
>>>>> CPU seems to be correct, but I can't say anything on
>>>>> Ambient and SODIMM. temp4 is constantly equal to SODIMM
>>>>> without this patch, so I'd say N/A is correct.
>>>>>
>>>>>
>>>>> Gabriele
>>>>
>>>> It is unknown for me how to directly read Ambient and
>>>> SODIMM temperatures (without Dell SMM functions). So we
>>>> can only trust Dell SMM that it reporting correct values
>>>> and type is really Ambient and SODIMM.
>>>>
>>>> And about temp4:
>>>>
>>>> Label is not set when SMM function fails. Original DOS
>>>> NBSVC.MDM just ignore all sensors for which SMM type
>>>> function fails.
>>>>
>>>> This patch should not disable any sensor, so if you
>>>> previously had some value (<= 128°C) and now not, then
>>>> there is some bug.
>>>>
>>>> Can you test this patch?
>>>> https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-mi
>>>> sc.git/comm
>>>> it/?h=char-misc-testing&id=723493ca59c8d81fed3e7f261165fee
>>>> 493a29ffa
>>>>
>>>> It is possible that same value is caused by incorrect use
>>>> of prev[] array which should be fixed by above patch.
>>>>
>>>> Can you test i8k with and without above patch?
>>>
>>> I did some more tests. What I think is happening is that
>>> temp4_label returns -22, so I sensors prints N/A without
>>> actually reading temp4_input.
>>>
>>> I'm doing some tests to understand what's going on with
>>> temp4_input. It reports the value of the last temp*_input I
>>> read. If I read it right after I loaded i8k, I get an error
>>> (-22).
>>>
>>> The same doesn't happen for temp3_label, which constantly
>>> returns -22.
>>>
>>> I already had
>>> https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-mi
>>> sc.git/commit/?h=char-misc-testing&id=723493ca59c8d81fed3e7f
>>> 261165fee493a29ffa applied. Without it, things seem not to
>>> change much. I either get an error (-22) or some incorrect
>>> values (for now always 1000) when I read temp4_input right
>>> after i8k was loaded. Once I read some other temp*_input, I
>>> always get the last value read.
>>
>> I am seeing exactly the same behavior on an XPS13.
>>
>> Guenter
>
> Original Dell DOS executable ignores all temperature sensors if
> type SMM function fails (if I decoded and understand that DOS
> assembler code correctly). So maybe we should do same...
>
> But because our i8k.c code ignores sensor only if it returns
> invalid temperature, there could be possible regression that on
> same machines type SMM function is not implemented or not
> working...
>
> What do you think?
>

Tested with XPS13, Studio 1555 (with GPU), and XPS M140.
Reading the type works with all of them. The Studio 1555
reports the GPU temperature in temp4. The M140 is quite old
(about 10 years), so I guess we can be reasonably sure that
all laptops currently in use support reporting the type.

Do you know what is returned for the type if the GPU is turned
off on a system with GPU ? I think that is the only open
question.

Thanks,
Guenter

Re: [PATCH] i8k: Add support for temperature sensor labels

[Pali Rohár]

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

On Sunday 30 November 2014 01:07:15 Guenter Roeck wrote:
> On 11/29/2014 11:07 AM, Pali Rohár wrote:
> > On Saturday 29 November 2014 19:58:28 Guenter Roeck wrote:
> >> On 11/29/2014 10:27 AM, Gabriele Mazzotta wrote:
> >>> On Saturday 29 November 2014 18:18:18 Pali Rohár wrote:
> >>>> On Saturday 29 November 2014 18:07:19 Gabriele Mazzotta
> > 
> > wrote:
> >>>>> On Saturday 29 November 2014 17:09:35 Pali Rohár wrote:
> >>>>>> On Saturday 29 November 2014 17:04:07 Pali Rohár wrote:
> >>>>>>> This patch adds labels for temperature sensors if SMM
> >>>>>>> function with EAX register 0x11a3 reports it. These
> >>>>>>> informations was taken from DOS binary NBSVC.MDM.
> >>>>>>> 
> >>>>>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> >>>>>>> ---
> >>>>>>> 
> >>>>>>>    drivers/char/i8k.c |  110
> >>>>>>> 
> >>>>>>> +++++++++++++++++++++++++++++++++++++++++----------- 1
> >>>>>>> file changed, 88 insertions(+), 22 deletions(-)
> >>>>>> 
> >>>>>> I tested patch on Latitude E6440 and i8k CPU & GPU
> >>>>>> temps match intel coretemp & amd radeion temps.
> >>>>>> 
> >>>>>> But I would like if somebody with other Dell laptop can
> >>>>>> test if temperature labels are correct...
> >>>>> 
> >>>>> I tested it on my XPS13 9333, here what sensors outputs:
> >>>>> 
> >>>>> acpitz-virtual-0
> >>>>> Adapter: Virtual device
> >>>>> temp1:        +27.8°C  (crit = +105.0°C)
> >>>>> temp2:        +29.8°C  (crit = +105.0°C)
> >>>>> 
> >>>>> coretemp-isa-0000
> >>>>> Adapter: ISA adapter
> >>>>> Physical id 0:  +62.0°C  (high = +100.0°C, crit =
> >>>>> +100.0°C) Core 0:         +62.0°C  (high = +100.0°C,
> >>>>> crit = +100.0°C) Core 1:         +61.0°C  (high =
> >>>>> +100.0°C, crit = +100.0°C)
> >>>>> 
> >>>>> i8k-virtual-0
> >>>>> Adapter: Virtual device
> >>>>> fan2:           0 RPM
> >>>>> CPU:          +62.0°C
> >>>>> Ambient:      +49.0°C
> >>>>> SODIMM:       +46.0°C
> >>>>> temp4:            N/A
> >>>>> 
> >>>>> CPU seems to be correct, but I can't say anything on
> >>>>> Ambient and SODIMM. temp4 is constantly equal to SODIMM
> >>>>> without this patch, so I'd say N/A is correct.
> >>>>> 
> >>>>> 
> >>>>> Gabriele
> >>>> 
> >>>> It is unknown for me how to directly read Ambient and
> >>>> SODIMM temperatures (without Dell SMM functions). So we
> >>>> can only trust Dell SMM that it reporting correct values
> >>>> and type is really Ambient and SODIMM.
> >>>> 
> >>>> And about temp4:
> >>>> 
> >>>> Label is not set when SMM function fails. Original DOS
> >>>> NBSVC.MDM just ignore all sensors for which SMM type
> >>>> function fails.
> >>>> 
> >>>> This patch should not disable any sensor, so if you
> >>>> previously had some value (<= 128°C) and now not, then
> >>>> there is some bug.
> >>>> 
> >>>> Can you test this patch?
> >>>> https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-> >>>> mi sc.git/comm
> >>>> it/?h=char-misc-testing&id=723493ca59c8d81fed3e7f261165fe
> >>>> e 493a29ffa
> >>>> 
> >>>> It is possible that same value is caused by incorrect use
> >>>> of prev[] array which should be fixed by above patch.
> >>>> 
> >>>> Can you test i8k with and without above patch?
> >>> 
> >>> I did some more tests. What I think is happening is that
> >>> temp4_label returns -22, so I sensors prints N/A without
> >>> actually reading temp4_input.
> >>> 
> >>> I'm doing some tests to understand what's going on with
> >>> temp4_input. It reports the value of the last temp*_input
> >>> I read. If I read it right after I loaded i8k, I get an
> >>> error (-22).
> >>> 
> >>> The same doesn't happen for temp3_label, which constantly
> >>> returns -22.
> >>> 
> >>> I already had
> >>> https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-m
> >>> i
> >>> sc.git/commit/?h=char-misc-testing&id=723493ca59c8d81fed3
> >>> e7f 261165fee493a29ffa applied. Without it, things seem
> >>> not to change much. I either get an error (-22) or some
> >>> incorrect values (for now always 1000) when I read
> >>> temp4_input right after i8k was loaded. Once I read some
> >>> other temp*_input, I always get the last value read.
> >> 
> >> I am seeing exactly the same behavior on an XPS13.
> >> 
> >> Guenter
> > 
> > Original Dell DOS executable ignores all temperature sensors
> > if type SMM function fails (if I decoded and understand
> > that DOS assembler code correctly). So maybe we should do
> > same...
> > 
> > But because our i8k.c code ignores sensor only if it returns
> > invalid temperature, there could be possible regression that
> > on same machines type SMM function is not implemented or
> > not working...
> > 
> > What do you think?
> 
> Tested with XPS13, Studio 1555 (with GPU), and XPS M140.
> Reading the type works with all of them. The Studio 1555
> reports the GPU temperature in temp4. The M140 is quite old
> (about 10 years), so I guess we can be reasonably sure that
> all laptops currently in use support reporting the type.
> 

Good. Then I will split this patch into two parts. One which adds 
labels and one which change init code to register only those 
sensors which have valid type.

> Do you know what is returned for the type if the GPU is turned
> off on a system with GPU ? I think that is the only open
> question.
> 

Yes, on my E6440 in both cases when GPU is turned off and on is 
returned same type (GPU). So this does not help us.

> Thanks,
> Guenter

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

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

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On 11/30/2014 01:53 AM, Pali Rohár wrote:
[ ... ]
>>>
>>> Original Dell DOS executable ignores all temperature sensors
>>> if type SMM function fails (if I decoded and understand
>>> that DOS assembler code correctly). So maybe we should do
>>> same...
>>>
>>> But because our i8k.c code ignores sensor only if it returns
>>> invalid temperature, there could be possible regression that
>>> on same machines type SMM function is not implemented or
>>> not working...
>>>
>>> What do you think?
>>
>> Tested with XPS13, Studio 1555 (with GPU), and XPS M140.
>> Reading the type works with all of them. The Studio 1555
>> reports the GPU temperature in temp4. The M140 is quite old
>> (about 10 years), so I guess we can be reasonably sure that
>> all laptops currently in use support reporting the type.
>>
>
> Good. Then I will split this patch into two parts. One which adds
> labels and one which change init code to register only those
> sensors which have valid type.
>
Ok.

>> Do you know what is returned for the type if the GPU is turned
>> off on a system with GPU ? I think that is the only open
>> question.
>>
>
> Yes, on my E6440 in both cases when GPU is turned off and on is
> returned same type (GPU). So this does not help us.
>
Unless I misunderstand you, it does help us; it simplifies
sensor detection since we don't have to handle the special case
that the GPU is turned off anymore.

Thanks,
Guenter

Re: [PATCH] i8k: Add support for temperature sensor labels

[Pali Rohár]

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

On Sunday 30 November 2014 17:00:18 Guenter Roeck wrote:
> On 11/30/2014 01:53 AM, Pali Rohár wrote:
> [ ... ]
> 
> >>> Original Dell DOS executable ignores all temperature
> >>> sensors if type SMM function fails (if I decoded and
> >>> understand that DOS assembler code correctly). So maybe
> >>> we should do same...
> >>> 
> >>> But because our i8k.c code ignores sensor only if it
> >>> returns invalid temperature, there could be possible
> >>> regression that on same machines type SMM function is not
> >>> implemented or not working...
> >>> 
> >>> What do you think?
> >> 
> >> Tested with XPS13, Studio 1555 (with GPU), and XPS M140.
> >> Reading the type works with all of them. The Studio 1555
> >> reports the GPU temperature in temp4. The M140 is quite old
> >> (about 10 years), so I guess we can be reasonably sure that
> >> all laptops currently in use support reporting the type.
> > 
> > Good. Then I will split this patch into two parts. One which
> > adds labels and one which change init code to register only
> > those sensors which have valid type.
> 
> Ok.
> 
> >> Do you know what is returned for the type if the GPU is
> >> turned off on a system with GPU ? I think that is the only
> >> open question.
> > 
> > Yes, on my E6440 in both cases when GPU is turned off and on
> > is returned same type (GPU). So this does not help us.
> 
> Unless I misunderstand you, it does help us; it simplifies
> sensor detection since we don't have to handle the special
> case that the GPU is turned off anymore.
> 
> Thanks,
> Guenter

Yes, sensor type is returned always correctly, so this is good.

I mean that it cannot be used for detecting if GPU is turned on 
or off.

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

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

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On 11/30/2014 09:44 AM, Pali Rohár wrote:
> On Sunday 30 November 2014 17:00:18 Guenter Roeck wrote:
>> On 11/30/2014 01:53 AM, Pali Rohár wrote:
>> [ ... ]
>>
>>>>> Original Dell DOS executable ignores all temperature
>>>>> sensors if type SMM function fails (if I decoded and
>>>>> understand that DOS assembler code correctly). So maybe
>>>>> we should do same...
>>>>>
>>>>> But because our i8k.c code ignores sensor only if it
>>>>> returns invalid temperature, there could be possible
>>>>> regression that on same machines type SMM function is not
>>>>> implemented or not working...
>>>>>
>>>>> What do you think?
>>>>
>>>> Tested with XPS13, Studio 1555 (with GPU), and XPS M140.
>>>> Reading the type works with all of them. The Studio 1555
>>>> reports the GPU temperature in temp4. The M140 is quite old
>>>> (about 10 years), so I guess we can be reasonably sure that
>>>> all laptops currently in use support reporting the type.
>>>
>>> Good. Then I will split this patch into two parts. One which
>>> adds labels and one which change init code to register only
>>> those sensors which have valid type.
>>
>> Ok.
>>
>>>> Do you know what is returned for the type if the GPU is
>>>> turned off on a system with GPU ? I think that is the only
>>>> open question.
>>>
>>> Yes, on my E6440 in both cases when GPU is turned off and on
>>> is returned same type (GPU). So this does not help us.
>>
>> Unless I misunderstand you, it does help us; it simplifies
>> sensor detection since we don't have to handle the special
>> case that the GPU is turned off anymore.
>>
>> Thanks,
>> Guenter
>
> Yes, sensor type is returned always correctly, so this is good.
>
> I mean that it cannot be used for detecting if GPU is turned on
> or off.
>

Yes, but that is a good thing, because it helps us figuring out
if the GPU sensor should be enabled or not during probe,
so we no longer need to try reading the temperature, and we
no longer have to handle the case of "GPU supported, but turned
off" during probe.

When reading the temperature, returning -ENODATA if the GPU
is turned off is the best we can do.

Thanks,
Guenter

Re: [PATCH] i8k: Add support for temperature sensor labels

[Pali Rohár]

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

On Sunday 30 November 2014 18:54:21 Guenter Roeck wrote:
> On 11/30/2014 09:44 AM, Pali Rohár wrote:
> > On Sunday 30 November 2014 17:00:18 Guenter Roeck wrote:
> >> On 11/30/2014 01:53 AM, Pali Rohár wrote:
> >> [ ... ]
> >> 
> >>>>> Original Dell DOS executable ignores all temperature
> >>>>> sensors if type SMM function fails (if I decoded and
> >>>>> understand that DOS assembler code correctly). So maybe
> >>>>> we should do same...
> >>>>> 
> >>>>> But because our i8k.c code ignores sensor only if it
> >>>>> returns invalid temperature, there could be possible
> >>>>> regression that on same machines type SMM function is
> >>>>> not implemented or not working...
> >>>>> 
> >>>>> What do you think?
> >>>> 
> >>>> Tested with XPS13, Studio 1555 (with GPU), and XPS M140.
> >>>> Reading the type works with all of them. The Studio 1555
> >>>> reports the GPU temperature in temp4. The M140 is quite
> >>>> old (about 10 years), so I guess we can be reasonably
> >>>> sure that all laptops currently in use support reporting
> >>>> the type.
> >>> 
> >>> Good. Then I will split this patch into two parts. One
> >>> which adds labels and one which change init code to
> >>> register only those sensors which have valid type.
> >> 
> >> Ok.
> >> 
> >>>> Do you know what is returned for the type if the GPU is
> >>>> turned off on a system with GPU ? I think that is the
> >>>> only open question.
> >>> 
> >>> Yes, on my E6440 in both cases when GPU is turned off and
> >>> on is returned same type (GPU). So this does not help us.
> >> 
> >> Unless I misunderstand you, it does help us; it simplifies
> >> sensor detection since we don't have to handle the special
> >> case that the GPU is turned off anymore.
> >> 
> >> Thanks,
> >> Guenter
> > 
> > Yes, sensor type is returned always correctly, so this is
> > good.
> > 
> > I mean that it cannot be used for detecting if GPU is turned
> > on or off.
> 
> Yes, but that is a good thing, because it helps us figuring
> out if the GPU sensor should be enabled or not during probe,
> so we no longer need to try reading the temperature, and we
> no longer have to handle the case of "GPU supported, but
> turned off" during probe.
> 
> When reading the temperature, returning -ENODATA if the GPU
> is turned off is the best we can do.
> 
> Thanks,
> Guenter

Yes, you are right. I will create new patches.

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

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

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On 11/30/2014 10:00 AM, Pali Rohár wrote:
[ ...]

>
> Yes, you are right. I will create new patches.
>
Hi Pali,

When doing so, please watch out for line lengths.
Some of your previous patches created lines with more than
80 characters per line.

Thanks,
Guenter

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On 11/29/2014 11:07 AM, Pali Rohár wrote:
[ ... ]
>
> Original Dell DOS executable ignores all temperature sensors if
> type SMM function fails (if I decoded and understand that DOS
> assembler code correctly). So maybe we should do same...
>
Pali,

Makes me wonder - does the assembler code tell you what to do
if the reported temperature is invalid, and does it distinguish
between error codes ? So far we have
	0x99 - presumably a spurious error
	0xc1 - GPU temperature sensor, GPU turned off

It would be nice if we could find a better solution for error handling.

Thanks,
Guenter

Re: [PATCH] i8k: Add support for temperature sensor labels

[Pali Rohár]

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

On Sunday 30 November 2014 02:25:09 Guenter Roeck wrote:
> On 11/29/2014 11:07 AM, Pali Rohár wrote:
> [ ... ]
> 
> > Original Dell DOS executable ignores all temperature sensors
> > if type SMM function fails (if I decoded and understand
> > that DOS assembler code correctly). So maybe we should do
> > same...
> 
> Pali,
> 
> Makes me wonder - does the assembler code tell you what to do
> if the reported temperature is invalid, and does it
> distinguish between error codes ?

I do not see anything like that. But there are lot of indirect 
calls (offset to pointer to function is stored in some global at 
init zero data), so it is hard to understand what that DOS binary 
is doing. I'm happy that I decoded loop which trying to call that 
type function and if it does not fail then it call read 
temperature function. And in that section I do not see any error 
handling of invalid values (but it could be somewhere else).

Anyway DOS binary is quite old (7 years maybe?). It is not even 
available for my last E6440 model. Now all new Dell laptops have 
EFI system and ePSA application (new version of diagnostic tool 
which reports info about fan, temperature, ...). That tool looks 
like is burned directly into machine (I can start it with empty 
HDD from Setup screen) or into BIOS image.

And what is interesting about this ePSA:

* it show more temperature sensors (battery temperature)
* it show correct RPM of fan and *can* control fan speed

I think that DOS binary has no idea about Optimus or PowerExpress 
cards so for that error handling we need to understand what is 
doing new EFI ePSA application...

And because function for turning card on/off is controlled via 
ACPI I bet that DOS or EFI application does not touch it, so 
assume that card is always on and does not need any error 
handling.

Another info about DOS binary: After SMM code for reading fan RPM 
is finished, then function divide returned RPM value by some 
number stored in local data. So now I think that magic fan 
multiplier is not constant, but runtime value. I will try to look 
at it, if we can fix this problem in linux i8k.c.

> So far we have
> 	0x99 - presumably a spurious error
> 	0xc1 - GPU temperature sensor, GPU turned off
> 
> It would be nice if we could find a better solution for error
> handling.
> 

Yes, but now we can only guess... My idea is that Dell SMM 
handler does not check GPU presence at runtime and just try to 
read info from PCI bus. And because turned off card is not there 
just random (or non random) garbage is returned...

> Thanks,
> Guenter

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

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

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On 11/30/2014 02:11 AM, Pali Rohár wrote:
> On Sunday 30 November 2014 02:25:09 Guenter Roeck wrote:
>> On 11/29/2014 11:07 AM, Pali Rohár wrote:
>> [ ... ]
>>
>>> Original Dell DOS executable ignores all temperature sensors
>>> if type SMM function fails (if I decoded and understand
>>> that DOS assembler code correctly). So maybe we should do
>>> same...
>>
>> Pali,
>>
>> Makes me wonder - does the assembler code tell you what to do
>> if the reported temperature is invalid, and does it
>> distinguish between error codes ?
>
> I do not see anything like that. But there are lot of indirect
> calls (offset to pointer to function is stored in some global at
> init zero data), so it is hard to understand what that DOS binary
> is doing. I'm happy that I decoded loop which trying to call that
> type function and if it does not fail then it call read
> temperature function. And in that section I do not see any error
> handling of invalid values (but it could be somewhere else).
>
> Anyway DOS binary is quite old (7 years maybe?). It is not even
> available for my last E6440 model. Now all new Dell laptops have
> EFI system and ePSA application (new version of diagnostic tool
> which reports info about fan, temperature, ...). That tool looks
> like is burned directly into machine (I can start it with empty
> HDD from Setup screen) or into BIOS image.
>
> And what is interesting about this ePSA:
>
> * it show more temperature sensors (battery temperature)
> * it show correct RPM of fan and *can* control fan speed
>
> I think that DOS binary has no idea about Optimus or PowerExpress
> cards so for that error handling we need to understand what is
> doing new EFI ePSA application...
>
> And because function for turning card on/off is controlled via
> ACPI I bet that DOS or EFI application does not touch it, so
> assume that card is always on and does not need any error
> handling.
>
> Another info about DOS binary: After SMM code for reading fan RPM
> is finished, then function divide returned RPM value by some
> number stored in local data. So now I think that magic fan
> multiplier is not constant, but runtime value. I will try to look
> at it, if we can fix this problem in linux i8k.c.
>
It might be system specific. After all, it is known that old laptops
need a different multiplier.

>> So far we have
>> 	0x99 - presumably a spurious error
>> 	0xc1 - GPU temperature sensor, GPU turned off
>>
>> It would be nice if we could find a better solution for error
>> handling.
>>
>
> Yes, but now we can only guess... My idea is that Dell SMM
> handler does not check GPU presence at runtime and just try to
> read info from PCI bus. And because turned off card is not there
> just random (or non random) garbage is returned...
>
Well, it was worth a try. It might be that, or SMM does handle it,
but that the DOS application is too old to understand it.

Thanks,
Guenter

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On 11/29/2014 08:04 AM, Pali Rohár wrote:
> This patch adds labels for temperature sensors if SMM function with EAX register
> 0x11a3 reports it. These informations was taken from DOS binary NBSVC.MDM.
>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>   drivers/char/i8k.c |  110 +++++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 88 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> index e34a019..77af46b 100644
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -42,6 +42,7 @@
>   #define I8K_SMM_GET_FAN		0x00a3
>   #define I8K_SMM_GET_SPEED	0x02a3
>   #define I8K_SMM_GET_TEMP	0x10a3
> +#define I8K_SMM_GET_TEMP_TYPE	0x11a3
>   #define I8K_SMM_GET_DELL_SIG1	0xfea3
>   #define I8K_SMM_GET_DELL_SIG2	0xffa3
>
> @@ -288,6 +289,14 @@ static int i8k_set_fan(int fan, int speed)
>   	return i8k_smm(&regs) ? : i8k_get_fan_status(fan);
>   }
>
> +static int i8k_get_temp_type(int sensor)
> +{
> +	struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP_TYPE, };
> +
> +	regs.ebx = sensor & 0xff;
> +	return i8k_smm(&regs) ? : regs.eax & 0xff;
> +}
> +
>   /*
>    * Read the cpu temperature.
>    */
> @@ -493,6 +502,29 @@ static int i8k_open_fs(struct inode *inode, struct file *file)
>    * Hwmon interface
>    */
>
> +static ssize_t i8k_hwmon_show_temp_label(struct device *dev,
> +				   struct device_attribute *devattr,
> +				   char *buf)
> +{
> +	static const char * const labels[] = {
> +		"CPU",
> +		"GPU",
> +		"SODIMM",
> +		"Other",
> +		"Ambient",
> +		"Other",
> +	};
> +	int index = to_sensor_dev_attr(devattr)->index;
> +	int type;
> +
> +	type = i8k_get_temp_type(index);
> +	if (type < 0)
> +		return type;
> +	if (type >= ARRAY_SIZE(labels))
> +		type = ARRAY_SIZE(labels) - 1;
> +	return sprintf(buf, "%s\n", labels[type]);
> +}
> +
>   static ssize_t i8k_hwmon_show_temp(struct device *dev,
>   				   struct device_attribute *devattr,
>   				   char *buf)
> @@ -555,9 +587,13 @@ static ssize_t i8k_hwmon_set_pwm(struct device *dev,
>   }
>
>   static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, 0);
>   static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 1);
> +static SENSOR_DEVICE_ATTR(temp2_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, 1);
>   static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 2);
> +static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, 2);
>   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(fan1_input, S_IRUGO, i8k_hwmon_show_fan, NULL,
>   			  I8K_FAN_LEFT);
>   static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, i8k_hwmon_show_pwm,
> @@ -569,31 +605,35 @@ static SENSOR_DEVICE_ATTR(pwm2, S_IRUGO | S_IWUSR, i8k_hwmon_show_pwm,
>
>   static struct attribute *i8k_attrs[] = {
>   	&sensor_dev_attr_temp1_input.dev_attr.attr,	/* 0 */
> -	&sensor_dev_attr_temp2_input.dev_attr.attr,	/* 1 */
> -	&sensor_dev_attr_temp3_input.dev_attr.attr,	/* 2 */
> -	&sensor_dev_attr_temp4_input.dev_attr.attr,	/* 3 */
> -	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 4 */
> -	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 5 */
> -	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 6 */
> -	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 7 */
> +	&sensor_dev_attr_temp1_label.dev_attr.attr,	/* 1 */
> +	&sensor_dev_attr_temp2_input.dev_attr.attr,	/* 2 */
> +	&sensor_dev_attr_temp2_label.dev_attr.attr,	/* 3 */
> +	&sensor_dev_attr_temp3_input.dev_attr.attr,	/* 4 */
> +	&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_pwm1.dev_attr.attr,		/* 9 */
> +	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 10 */
> +	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 11 */
>   	NULL
>   };
>
>   static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
>   			      int index)
>   {
> -	if (index == 0 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
> +	if (index >= 0 && index <= 1 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
>   		return 0;
> -	if (index == 1 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP2))
> +	if (index >= 2 && index <= 3 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP2))
>   		return 0;
> -	if (index == 2 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP3))
> +	if (index >= 4 && index <= 5 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP3))
>   		return 0;
> -	if (index == 3 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP4))
> +	if (index >= 6 && index <= 7 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP4))
>   		return 0;
> -	if (index >= 4 && index <= 5 &&
> +	if (index >= 8 && index <= 9 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
>   		return 0;
> -	if (index >= 6 && index <= 7 &&
> +	if (index >= 10 && index <= 11 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
>   		return 0;
>
> @@ -606,6 +646,37 @@ static const struct attribute_group i8k_group = {
>   };
>   __ATTRIBUTE_GROUPS(i8k);
>
> +static bool __init i8k_check_temp(int sensor)
> +{
> +	int err;
> +
> +	/*
> +	 * Check if temperature sensor type is valid.
> +	 *
> +	 * If it is valid then sensor should work. But some sensors are not
> +	 * available at any time. E.g GPU sensor on Optimus/PowerExpress/Enduro
> +	 * card does not work (or return bogus value) when card is turned off.
> +	 * So this function should not fail in this case.
> +	 */
> +	err = i8k_get_temp_type(sensor);
> +	if (err >= 0)
> +		return true;
> +
Are you sure this function is provided for all systems ?
I am a bit concerned that we may wrongly disable sensors this way,
especially on older systems.

It might be safer to create another set of flags and enable the labels
only if the sensor is known to exist and if reading its type (label) works.

> +	/*
> +	 * Check if temperatrue reading does not fail.
> +	 *
temperature

> +	 * Sometimes detection of temperature sensor type does not work but
> +	 * reading temperature working fine. Sometimes temperature value is too
> +	 * high and i8k_get_temp() returns -ERANGE. But there is no reason to
> +	 * ignore these temperature sensors.
> +	 */
> +	err = i8k_get_temp(sensor);
> +	if (err >= 0 || err == -ERANGE)
> +		return true;
> +

Please simplify to
	return err >= 0 || err == -ERANGE;

> +	return false;
> +}
> +
>   static int __init i8k_init_hwmon(void)
>   {
>   	int err;
> @@ -613,18 +684,13 @@ static int __init i8k_init_hwmon(void)
>   	i8k_hwmon_flags = 0;
>
>   	/* CPU temperature attributes, if temperature reading is OK */
> -	err = i8k_get_temp(0);
> -	if (err >= 0 || err == -ERANGE)
> +	if (i8k_check_temp(0))

The introduction of i8k_check_temp() should be a separate patch.

Thanks,
Guenter

>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
> -	/* check for additional temperature sensors */
> -	err = i8k_get_temp(1);
> -	if (err >= 0 || err == -ERANGE)
> +	if (i8k_check_temp(1))
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
> -	err = i8k_get_temp(2);
> -	if (err >= 0 || err == -ERANGE)
> +	if (i8k_check_temp(2))
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
> -	err = i8k_get_temp(3);
> -	if (err >= 0 || err == -ERANGE)
> +	if (i8k_check_temp(3))
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
>
>   	/* Left fan attributes, if left fan is present */
>

Re: [PATCH] i8k: Add support for temperature sensor labels

[Pali Rohár]

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

On Saturday 29 November 2014 17:24:08 Guenter Roeck wrote:
> On 11/29/2014 08:04 AM, Pali Rohár wrote:
> > +static bool __init i8k_check_temp(int sensor)
> > +{
> > +	int err;
> > +
> > +	/*
> > +	 * Check if temperature sensor type is valid.
> > +	 *
> > +	 * If it is valid then sensor should work. But some
> > sensors are not +	 * available at any time. E.g GPU sensor
> > on Optimus/PowerExpress/Enduro +	 * card does not work (or
> > return bogus value) when card is turned off. +	 * So this
> > function should not fail in this case. +	 */
> > +	err = i8k_get_temp_type(sensor);
> > +	if (err >= 0)
> > +		return true;
> > +
> 
> Are you sure this function is provided for all systems ?
> I am a bit concerned that we may wrongly disable sensors this
> way, especially on older systems.
> 

I do not know if that function is provided on all systems. But 
this code does not disable sensors. If function fail, then we 
fallback to temperature read down. Return true means that we 
enable sensor.

> It might be safer to create another set of flags and enable
> the labels only if the sensor is known to exist and if
> reading its type (label) works.
> 
> > +	/*
> > +	 * Check if temperatrue reading does not fail.
> > +	 *
> 
> temperature
> 
> > +	 * Sometimes detection of temperature sensor type does not
> > work but +	 * reading temperature working fine. Sometimes
> > temperature value is too +	 * high and i8k_get_temp()
> > returns -ERANGE. But there is no reason to +	 * ignore
> > these temperature sensors.
> > +	 */
> > +	err = i8k_get_temp(sensor);
> > +	if (err >= 0 || err == -ERANGE)
> > +		return true;
> > +
> 
> Please simplify to
> 	return err >= 0 || err == -ERANGE;
> 

I'm using style:

check_function_1();
if (not_failed)
	return true;

check_function_2();
if (not_failed)
	return true;

return false; // disable sensor

So I think it is better to have same style for both checks...

> > +	return false;
> > +}
> > +
> > 
> >   static int __init i8k_init_hwmon(void)
> >   {
> >   
> >   	int err;
> > 
> > @@ -613,18 +684,13 @@ static int __init i8k_init_hwmon(void)
> > 
> >   	i8k_hwmon_flags = 0;
> >   	
> >   	/* CPU temperature attributes, if temperature reading is
> >   	OK */
> > 
> > -	err = i8k_get_temp(0);
> > -	if (err >= 0 || err == -ERANGE)
> > +	if (i8k_check_temp(0))
> 
> The introduction of i8k_check_temp() should be a separate
> patch.
> 
> Thanks,
> Guenter
> 

Ok.

> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
> > 
> > -	/* check for additional temperature sensors */
> > -	err = i8k_get_temp(1);
> > -	if (err >= 0 || err == -ERANGE)
> > +	if (i8k_check_temp(1))
> > 
> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
> > 
> > -	err = i8k_get_temp(2);
> > -	if (err >= 0 || err == -ERANGE)
> > +	if (i8k_check_temp(2))
> > 
> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
> > 
> > -	err = i8k_get_temp(3);
> > -	if (err >= 0 || err == -ERANGE)
> > +	if (i8k_check_temp(3))
> > 
> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
> >   	
> >   	/* Left fan attributes, if left fan is present */

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

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

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On 11/29/2014 08:30 AM, Pali Rohár wrote:
> On Saturday 29 November 2014 17:24:08 Guenter Roeck wrote:
>> On 11/29/2014 08:04 AM, Pali Rohár wrote:
>>> +static bool __init i8k_check_temp(int sensor)
>>> +{
>>> +	int err;
>>> +
>>> +	/*
>>> +	 * Check if temperature sensor type is valid.
>>> +	 *
>>> +	 * If it is valid then sensor should work. But some
>>> sensors are not +	 * available at any time. E.g GPU sensor
>>> on Optimus/PowerExpress/Enduro +	 * card does not work (or
>>> return bogus value) when card is turned off. +	 * So this
>>> function should not fail in this case. +	 */
>>> +	err = i8k_get_temp_type(sensor);
>>> +	if (err >= 0)
>>> +		return true;
>>> +
>>
>> Are you sure this function is provided for all systems ?
>> I am a bit concerned that we may wrongly disable sensors this
>> way, especially on older systems.
>>
>
> I do not know if that function is provided on all systems. But
> this code does not disable sensors. If function fail, then we
> fallback to temperature read down. Return true means that we
> enable sensor.
>

You are right. Guess I didn't have enough coffee this morning.

Thanks,
Guenter

Re: [PATCH] i8k: Add support for temperature sensor labels

[Greg Kroah-Hartman]

On Sat, Nov 29, 2014 at 05:04:07PM +0100, Pali Rohár wrote:
> This patch adds labels for temperature sensors if SMM function with EAX register
> 0x11a3 reports it. These informations was taken from DOS binary NBSVC.MDM.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/char/i8k.c |  110 +++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 88 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> index e34a019..77af46b 100644
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -42,6 +42,7 @@
>  #define I8K_SMM_GET_FAN		0x00a3
>  #define I8K_SMM_GET_SPEED	0x02a3
>  #define I8K_SMM_GET_TEMP	0x10a3
> +#define I8K_SMM_GET_TEMP_TYPE	0x11a3
>  #define I8K_SMM_GET_DELL_SIG1	0xfea3
>  #define I8K_SMM_GET_DELL_SIG2	0xffa3
>  
> @@ -288,6 +289,14 @@ static int i8k_set_fan(int fan, int speed)
>  	return i8k_smm(&regs) ? : i8k_get_fan_status(fan);
>  }
>  
> +static int i8k_get_temp_type(int sensor)
> +{
> +	struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP_TYPE, };
> +
> +	regs.ebx = sensor & 0xff;
> +	return i8k_smm(&regs) ? : regs.eax & 0xff;
> +}
> +
>  /*
>   * Read the cpu temperature.
>   */
> @@ -493,6 +502,29 @@ static int i8k_open_fs(struct inode *inode, struct file *file)
>   * Hwmon interface
>   */
>  
> +static ssize_t i8k_hwmon_show_temp_label(struct device *dev,
> +				   struct device_attribute *devattr,
> +				   char *buf)
> +{
> +	static const char * const labels[] = {
> +		"CPU",
> +		"GPU",
> +		"SODIMM",
> +		"Other",
> +		"Ambient",
> +		"Other",
> +	};
> +	int index = to_sensor_dev_attr(devattr)->index;
> +	int type;
> +
> +	type = i8k_get_temp_type(index);
> +	if (type < 0)
> +		return type;
> +	if (type >= ARRAY_SIZE(labels))
> +		type = ARRAY_SIZE(labels) - 1;
> +	return sprintf(buf, "%s\n", labels[type]);
> +}

No Documentation/ABI/ entry for your new sysfs file?

Re: [PATCH] i8k: Add support for temperature sensor labels

[Pali Rohár]

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

On Saturday 29 November 2014 18:43:15 Greg Kroah-Hartman wrote:
> On Sat, Nov 29, 2014 at 05:04:07PM +0100, Pali Rohár wrote:
> > This patch adds labels for temperature sensors if SMM
> > function with EAX register 0x11a3 reports it. These
> > informations was taken from DOS binary NBSVC.MDM.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> > 
> >  drivers/char/i8k.c |  110
> >  +++++++++++++++++++++++++++++++++++++++++----------- 1
> >  file changed, 88 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> > index e34a019..77af46b 100644
> > --- a/drivers/char/i8k.c
> > +++ b/drivers/char/i8k.c
> > @@ -42,6 +42,7 @@
> > 
> >  #define I8K_SMM_GET_FAN		0x00a3
> >  #define I8K_SMM_GET_SPEED	0x02a3
> >  #define I8K_SMM_GET_TEMP	0x10a3
> > 
> > +#define I8K_SMM_GET_TEMP_TYPE	0x11a3
> > 
> >  #define I8K_SMM_GET_DELL_SIG1	0xfea3
> >  #define I8K_SMM_GET_DELL_SIG2	0xffa3
> > 
> > @@ -288,6 +289,14 @@ static int i8k_set_fan(int fan, int
> > speed)
> > 
> >  	return i8k_smm(&regs) ? : i8k_get_fan_status(fan);
> >  
> >  }
> > 
> > +static int i8k_get_temp_type(int sensor)
> > +{
> > +	struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP_TYPE, };
> > +
> > +	regs.ebx = sensor & 0xff;
> > +	return i8k_smm(&regs) ? : regs.eax & 0xff;
> > +}
> > +
> > 
> >  /*
> >  
> >   * Read the cpu temperature.
> >   */
> > 
> > @@ -493,6 +502,29 @@ static int i8k_open_fs(struct inode
> > *inode, struct file *file)
> > 
> >   * Hwmon interface
> >   */
> > 
> > +static ssize_t i8k_hwmon_show_temp_label(struct device
> > *dev, +				   struct device_attribute *devattr,
> > +				   char *buf)
> > +{
> > +	static const char * const labels[] = {
> > +		"CPU",
> > +		"GPU",
> > +		"SODIMM",
> > +		"Other",
> > +		"Ambient",
> > +		"Other",
> > +	};
> > +	int index = to_sensor_dev_attr(devattr)->index;
> > +	int type;
> > +
> > +	type = i8k_get_temp_type(index);
> > +	if (type < 0)
> > +		return type;
> > +	if (type >= ARRAY_SIZE(labels))
> > +		type = ARRAY_SIZE(labels) - 1;
> > +	return sprintf(buf, "%s\n", labels[type]);
> > +}
> 
> No Documentation/ABI/ entry for your new sysfs file?

It is standard hwmon sysfs entry which is used by other hwmon 
drivers... It is not i8k.ko driver specific.

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

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

Re: [PATCH] i8k: Add support for temperature sensor labels

[Greg Kroah-Hartman]

On Sat, Nov 29, 2014 at 06:49:43PM +0100, Pali Rohár wrote:
> On Saturday 29 November 2014 18:43:15 Greg Kroah-Hartman wrote:
> > On Sat, Nov 29, 2014 at 05:04:07PM +0100, Pali Rohár wrote:
> > > This patch adds labels for temperature sensors if SMM
> > > function with EAX register 0x11a3 reports it. These
> > > informations was taken from DOS binary NBSVC.MDM.
> > > 
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > ---
> > > 
> > >  drivers/char/i8k.c |  110
> > >  +++++++++++++++++++++++++++++++++++++++++----------- 1
> > >  file changed, 88 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> > > index e34a019..77af46b 100644
> > > --- a/drivers/char/i8k.c
> > > +++ b/drivers/char/i8k.c
> > > @@ -42,6 +42,7 @@
> > > 
> > >  #define I8K_SMM_GET_FAN		0x00a3
> > >  #define I8K_SMM_GET_SPEED	0x02a3
> > >  #define I8K_SMM_GET_TEMP	0x10a3
> > > 
> > > +#define I8K_SMM_GET_TEMP_TYPE	0x11a3
> > > 
> > >  #define I8K_SMM_GET_DELL_SIG1	0xfea3
> > >  #define I8K_SMM_GET_DELL_SIG2	0xffa3
> > > 
> > > @@ -288,6 +289,14 @@ static int i8k_set_fan(int fan, int
> > > speed)
> > > 
> > >  	return i8k_smm(&regs) ? : i8k_get_fan_status(fan);
> > >  
> > >  }
> > > 
> > > +static int i8k_get_temp_type(int sensor)
> > > +{
> > > +	struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP_TYPE, };
> > > +
> > > +	regs.ebx = sensor & 0xff;
> > > +	return i8k_smm(&regs) ? : regs.eax & 0xff;
> > > +}
> > > +
> > > 
> > >  /*
> > >  
> > >   * Read the cpu temperature.
> > >   */
> > > 
> > > @@ -493,6 +502,29 @@ static int i8k_open_fs(struct inode
> > > *inode, struct file *file)
> > > 
> > >   * Hwmon interface
> > >   */
> > > 
> > > +static ssize_t i8k_hwmon_show_temp_label(struct device
> > > *dev, +				   struct device_attribute *devattr,
> > > +				   char *buf)
> > > +{
> > > +	static const char * const labels[] = {
> > > +		"CPU",
> > > +		"GPU",
> > > +		"SODIMM",
> > > +		"Other",
> > > +		"Ambient",
> > > +		"Other",
> > > +	};
> > > +	int index = to_sensor_dev_attr(devattr)->index;
> > > +	int type;
> > > +
> > > +	type = i8k_get_temp_type(index);
> > > +	if (type < 0)
> > > +		return type;
> > > +	if (type >= ARRAY_SIZE(labels))
> > > +		type = ARRAY_SIZE(labels) - 1;
> > > +	return sprintf(buf, "%s\n", labels[type]);
> > > +}
> > 
> > No Documentation/ABI/ entry for your new sysfs file?
> 
> It is standard hwmon sysfs entry which is used by other hwmon 
> drivers... It is not i8k.ko driver specific.

Ok, then why is this driver not in a "hwmon-standard" location in the
kernel tree?

greg k-h

Re: [PATCH] i8k: Add support for temperature sensor labels

[Pali Rohár]

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

On Saturday 29 November 2014 18:51:52 Greg Kroah-Hartman wrote:
> On Sat, Nov 29, 2014 at 06:49:43PM +0100, Pali Rohár wrote:
> > On Saturday 29 November 2014 18:43:15 Greg Kroah-Hartman 
wrote:
> > > On Sat, Nov 29, 2014 at 05:04:07PM +0100, Pali Rohár wrote:
> > > > This patch adds labels for temperature sensors if SMM
> > > > function with EAX register 0x11a3 reports it. These
> > > > informations was taken from DOS binary NBSVC.MDM.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/char/i8k.c |  110
> > > >  +++++++++++++++++++++++++++++++++++++++++----------- 1
> > > >  file changed, 88 insertions(+), 22 deletions(-)
> > > > 
> > > > diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> > > > index e34a019..77af46b 100644
> > > > --- a/drivers/char/i8k.c
> > > > +++ b/drivers/char/i8k.c
> > > > @@ -42,6 +42,7 @@
> > > > 
> > > >  #define I8K_SMM_GET_FAN		0x00a3
> > > >  #define I8K_SMM_GET_SPEED	0x02a3
> > > >  #define I8K_SMM_GET_TEMP	0x10a3
> > > > 
> > > > +#define I8K_SMM_GET_TEMP_TYPE	0x11a3
> > > > 
> > > >  #define I8K_SMM_GET_DELL_SIG1	0xfea3
> > > >  #define I8K_SMM_GET_DELL_SIG2	0xffa3
> > > > 
> > > > @@ -288,6 +289,14 @@ static int i8k_set_fan(int fan, int
> > > > speed)
> > > > 
> > > >  	return i8k_smm(&regs) ? : i8k_get_fan_status(fan);
> > > >  
> > > >  }
> > > > 
> > > > +static int i8k_get_temp_type(int sensor)
> > > > +{
> > > > +	struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP_TYPE,
> > > > }; +
> > > > +	regs.ebx = sensor & 0xff;
> > > > +	return i8k_smm(&regs) ? : regs.eax & 0xff;
> > > > +}
> > > > +
> > > > 
> > > >  /*
> > > >  
> > > >   * Read the cpu temperature.
> > > >   */
> > > > 
> > > > @@ -493,6 +502,29 @@ static int i8k_open_fs(struct inode
> > > > *inode, struct file *file)
> > > > 
> > > >   * Hwmon interface
> > > >   */
> > > > 
> > > > +static ssize_t i8k_hwmon_show_temp_label(struct device
> > > > *dev, +				   struct device_attribute *devattr,
> > > > +				   char *buf)
> > > > +{
> > > > +	static const char * const labels[] = {
> > > > +		"CPU",
> > > > +		"GPU",
> > > > +		"SODIMM",
> > > > +		"Other",
> > > > +		"Ambient",
> > > > +		"Other",
> > > > +	};
> > > > +	int index = to_sensor_dev_attr(devattr)->index;
> > > > +	int type;
> > > > +
> > > > +	type = i8k_get_temp_type(index);
> > > > +	if (type < 0)
> > > > +		return type;
> > > > +	if (type >= ARRAY_SIZE(labels))
> > > > +		type = ARRAY_SIZE(labels) - 1;
> > > > +	return sprintf(buf, "%s\n", labels[type]);
> > > > +}
> > > 
> > > No Documentation/ABI/ entry for your new sysfs file?
> > 
> > It is standard hwmon sysfs entry which is used by other
> > hwmon drivers... It is not i8k.ko driver specific.
> 
> Ok, then why is this driver not in a "hwmon-standard" location
> in the kernel tree?
> 
> greg k-h

This is question not for me but for maintainers of hwmon and char 
trees... I just contributed patch for existing driver.

But my idea is that because this driver exported temperature data 
for a long time only via /proc/i8k character file (since 2.4? or 
earlier?) and only 3 years ago was added standard hwmon interface 
to existing code (949a9d70020defd7c241607ab3ed037ea88f551c).

So now there are both interfaces. Standard hwmon and old /proc/.

CCing Jean Delvare (author of hwmon i8k code)

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

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

Re: [PATCH] i8k: Add support for temperature sensor labels

[Jean Delvare]

Hi Pali, hi Greg,

Le Saturday 29 November 2014 à 19:04 +0100, Pali Rohár a écrit :
> On Saturday 29 November 2014 18:51:52 Greg Kroah-Hartman wrote:
> > On Sat, Nov 29, 2014 at 06:49:43PM +0100, Pali Rohár wrote:
> > > On Saturday 29 November 2014 18:43:15 Greg Kroah-Hartman wrote:
> > > > No Documentation/ABI/ entry for your new sysfs file?
> > > 
> > > It is standard hwmon sysfs entry which is used by other
> > > hwmon drivers... It is not i8k.ko driver specific.
> > 
> > Ok, then why is this driver not in a "hwmon-standard" location
> > in the kernel tree?
> > 
> > greg k-h
> 
> This is question not for me but for maintainers of hwmon and char 
> trees... I just contributed patch for existing driver.
> 
> But my idea is that because this driver exported temperature data 
> for a long time only via /proc/i8k character file (since 2.4? or 
> earlier?) and only 3 years ago was added standard hwmon interface 
> to existing code (949a9d70020defd7c241607ab3ed037ea88f551c).
> 
> So now there are both interfaces. Standard hwmon and old /proc/.
> 
> CCing Jean Delvare (author of hwmon i8k code)

Yes, that's for historical reasons. Also the i8k driver does more than
just hardware monitoring, it handles function keys and power status too.
And it is heavily platform-specific. So while I agree that drivers/char
isn't necessarily the best place for this driver, I'd rather move it to
drivers/platform/x86 together with other laptop vendor-specific drivers,
than to drivers/hwmon.

Ultimately /proc/i8k should be killed altogether and every function
should use the appropriate standard interface. But I don't have any Dell
laptop around so I won't go into that myself.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On 12/02/2014 05:23 AM, Jean Delvare wrote:
> Hi Pali, hi Greg,
>
> Le Saturday 29 November 2014 à 19:04 +0100, Pali Rohár a écrit :
>> On Saturday 29 November 2014 18:51:52 Greg Kroah-Hartman wrote:
>>> On Sat, Nov 29, 2014 at 06:49:43PM +0100, Pali Rohár wrote:
>>>> On Saturday 29 November 2014 18:43:15 Greg Kroah-Hartman wrote:
>>>>> No Documentation/ABI/ entry for your new sysfs file?
>>>>
>>>> It is standard hwmon sysfs entry which is used by other
>>>> hwmon drivers... It is not i8k.ko driver specific.
>>>
>>> Ok, then why is this driver not in a "hwmon-standard" location
>>> in the kernel tree?
>>>
>>> greg k-h
>>
>> This is question not for me but for maintainers of hwmon and char
>> trees... I just contributed patch for existing driver.
>>
>> But my idea is that because this driver exported temperature data
>> for a long time only via /proc/i8k character file (since 2.4? or
>> earlier?) and only 3 years ago was added standard hwmon interface
>> to existing code (949a9d70020defd7c241607ab3ed037ea88f551c).
>>
>> So now there are both interfaces. Standard hwmon and old /proc/.
>>
>> CCing Jean Delvare (author of hwmon i8k code)
>
> Yes, that's for historical reasons. Also the i8k driver does more than
> just hardware monitoring, it handles function keys and power status too.
> And it is heavily platform-specific. So while I agree that drivers/char
> isn't necessarily the best place for this driver, I'd rather move it to
> drivers/platform/x86 together with other laptop vendor-specific drivers,
> than to drivers/hwmon.
>
> Ultimately /proc/i8k should be killed altogether and every function
> should use the appropriate standard interface. But I don't have any Dell
> laptop around so I won't go into that myself.
>
The only function not covered by hwmon, as far as I can see, is reading
the "Fn key" status, whatever that is, and reporting the power status
(AC or battery).

The first seems to be covered by the already existing dell drivers in
platform/x86. The latter must be covered as well; Linux does
display the correct power status on all my Dell laptops.

Changing the ABI is not supposed to happen, so I am not sure if we can
just drop the /proc interface.

Guenter

Re: [PATCH] i8k: Add support for temperature sensor labels

[Jean Delvare]

Hi Guenter,

On Tue, 02 Dec 2014 06:26:29 -0800, Guenter Roeck wrote:
> On 12/02/2014 05:23 AM, Jean Delvare wrote:
> > Ultimately /proc/i8k should be killed altogether and every function
> > should use the appropriate standard interface. But I don't have any Dell
> > laptop around so I won't go into that myself.
>
> The only function not covered by hwmon, as far as I can see, is reading
> the "Fn key" status, whatever that is, and reporting the power status
> (AC or battery).
> 
> The first seems to be covered by the already existing dell drivers in
> platform/x86. The latter must be covered as well; Linux does
> display the correct power status on all my Dell laptops.

How old are they? The i8k driver is ancestral, so I could imagine that
i8k is the only way to report power status on very old Dell laptop
models. Or maybe not, I just don't know.

> Changing the ABI is not supposed to happen, so I am not sure if we can
> just drop the /proc interface.

Dropping deprecated interfaces after a transition period is not so
uncommon. AFAIK the proc interface of i8k is only used by the i8kutils
user-space package, which FWIW is no longer part of openSUSE. The
dell-laptop and dell-wmi drivers are almost 6 years old, that looks
like an already very comfortable transition period to me.

That being said, that's only my opinion and I'm not going to fight for
this. If the hwmon interface is moved to dell-laptop and i8k is killed,
I'm happy. If the i8k driver is moved to drivers/hwmon and stripped
down, I'm happy. But if none of this happens, I don't really care, to
be honest.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i8k: Add support for temperature sensor labels

[Pali Rohár]

On Wed Dec   3 10:09:28 2014 Jean Delvare <jdelvare@suse.de> wrote:
> Hi Guenter,
> 
> On Tue, 02 Dec 2014 06:26:29 -0800, Guenter Roeck wrote:
> > On 12/02/2014 05:23 AM, Jean Delvare wrote:
> > > Ultimately /proc/i8k should be killed altogether and every function
> > > should use the appropriate standard interface. But I don't have any
> > > Dell laptop around so I won't go into that myself.
> > 
> > The only function not covered by hwmon, as far as I can see, is reading
> > the "Fn key" status, whatever that is, and reporting the power status
> > (AC or battery).
> > 
> > The first seems to be covered by the already existing dell drivers in
> > platform/x86. The latter must be covered as well; Linux does
> > display the correct power status on all my Dell laptops.
> 
> How old are they? The i8k driver is ancestral, so I could imagine that
> i8k is the only way to report power status on very old Dell laptop
> models. Or maybe not, I just don't know.
> 

It was also for laptops around year 2000. So maybe yes, those laptops probably do not have working acpi power status...

> > Changing the ABI is not supposed to happen, so I am not sure if we can
> > just drop the /proc interface.
> 
> Dropping deprecated interfaces after a transition period is not so
> uncommon. AFAIK the proc interface of i8k is only used by the i8kutils
> user-space package, which FWIW is no longer part of openSUSE.

There are some forks of this package which some users use. And openSUSE is not only one linux distribution. E.g. In Ubuntu and Debian i8kutils package is still available.

> The dell-laptop and dell-wmi drivers are almost 6 years old, that looks
> like an already very comfortable transition period to me.
> 

dell-laptop is using another Dell SMM interface (this one documented), but it does not support temperature and fan control status. It is not acpi driver.

dell-wmi is using acpi/wmi for receiving events (nothing more).

I would like to see i8k driver not integrated into dell-laptop becuase it use totally different interface...

Anyway, what does i8k mean? That dell interface is somehow called DELLDIAG, so I do not know what i8k means...

> That being said, that's only my opinion and I'm not going to fight for
> this. If the hwmon interface is moved to dell-laptop and i8k is killed,
> I'm happy. If the i8k driver is moved to drivers/hwmon and stripped
> down, I'm happy. But if none of this happens, I don't really care, to
> be honest.
> 

For new laptops Fn key status is not supported by i8k (al least on my E6440) and power status is reported by acpi.

So what would be probably better is to split driver into two: One which provides only /proc/i8k interface (unchanged) and other one which provides hwmon interface.

Users of new laptops do not need /proc/i8k anymore (only if they use i8kutils), so would be able to compile only hwmon...

Anyway, do we need to touch /proc/i8k code? It supports only one (first) temperature sensor, one fan control... And i think it is already separated from hwmon code (which doing some detection of more sensors, fans, ...), so what about moving hwmon interface into separate module? We just need some common code for executing SMM function.

> -- 
> Jean Delvare
> SUSE L3 Support

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

Re: [PATCH] i8k: Add support for temperature sensor labels

[Jean Delvare]

On Wed, 03 Dec 2014 10:25:26 +0100, Pali Rohár wrote:
> Anyway, what does i8k mean? That dell interface is somehow called DELLDIAG, so I do not know what i8k means...

I think i8k stands for "Inspiron 8000", most likely the first model
supported by that driver:
http://www.dell.com/support/home/us/en/19/product-support/product/inspiron-8000/manuals

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On Wed, Dec 03, 2014 at 10:09:28AM +0100, Jean Delvare wrote:
> Hi Guenter,
> 
> On Tue, 02 Dec 2014 06:26:29 -0800, Guenter Roeck wrote:
> > On 12/02/2014 05:23 AM, Jean Delvare wrote:
> > > Ultimately /proc/i8k should be killed altogether and every function
> > > should use the appropriate standard interface. But I don't have any Dell
> > > laptop around so I won't go into that myself.
> >
> > The only function not covered by hwmon, as far as I can see, is reading
> > the "Fn key" status, whatever that is, and reporting the power status
> > (AC or battery).
> > 
> > The first seems to be covered by the already existing dell drivers in
> > platform/x86. The latter must be covered as well; Linux does
> > display the correct power status on all my Dell laptops.
> 
> How old are they? The i8k driver is ancestral, so I could imagine that
> i8k is the only way to report power status on very old Dell laptop
> models. Or maybe not, I just don't know.
> 
> > Changing the ABI is not supposed to happen, so I am not sure if we can
> > just drop the /proc interface.
> 
> Dropping deprecated interfaces after a transition period is not so
> uncommon. AFAIK the proc interface of i8k is only used by the i8kutils
> user-space package, which FWIW is no longer part of openSUSE. The

It is still in Ubuntu 14.04.

> dell-laptop and dell-wmi drivers are almost 6 years old, that looks
> like an already very comfortable transition period to me.
> 
Hi Jean,

Fine with me. My oldest laptop is about 10 years old.

> That being said, that's only my opinion and I'm not going to fight for
> this. If the hwmon interface is moved to dell-laptop and i8k is killed,
> I'm happy. If the i8k driver is moved to drivers/hwmon and stripped
> down, I'm happy. But if none of this happens, I don't really care, to
> be honest.
> 
I am all for getting rid of it, question is only how to do it,
and if it really can be done.

A quick glance into Lauchpad shows that i8kutils is still maintained
and used, including the sometimes dirty tricks it provides. 

Guenter

Re: [PATCH] i8k: Add support for temperature sensor labels

[Jean Delvare]

On Wed, 3 Dec 2014 11:14:46 -0800, Guenter Roeck wrote:
> A quick glance into Lauchpad shows that i8kutils is still maintained
> and used, including the sometimes dirty tricks it provides. 

Don't draw conclusions too fast though... People may just not have
realized that they no longer need it. Old tricks are hard to forget.

The only way to make sure is to find current users of i8kutils and see
what they (think they would) lose if i8kutils goes away.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On 11/29/2014 09:51 AM, Greg Kroah-Hartman wrote:
> On Sat, Nov 29, 2014 at 06:49:43PM +0100, Pali Rohár wrote:
>> On Saturday 29 November 2014 18:43:15 Greg Kroah-Hartman wrote:
>>> On Sat, Nov 29, 2014 at 05:04:07PM +0100, Pali Rohár wrote:
>>>> This patch adds labels for temperature sensors if SMM
>>>> function with EAX register 0x11a3 reports it. These
>>>> informations was taken from DOS binary NBSVC.MDM.
>>>>
>>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>>>> ---
>>>>
>>>>   drivers/char/i8k.c |  110
>>>>   +++++++++++++++++++++++++++++++++++++++++----------- 1
>>>>   file changed, 88 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
>>>> index e34a019..77af46b 100644
>>>> --- a/drivers/char/i8k.c
>>>> +++ b/drivers/char/i8k.c
>>>> @@ -42,6 +42,7 @@
>>>>
>>>>   #define I8K_SMM_GET_FAN		0x00a3
>>>>   #define I8K_SMM_GET_SPEED	0x02a3
>>>>   #define I8K_SMM_GET_TEMP	0x10a3
>>>>
>>>> +#define I8K_SMM_GET_TEMP_TYPE	0x11a3
>>>>
>>>>   #define I8K_SMM_GET_DELL_SIG1	0xfea3
>>>>   #define I8K_SMM_GET_DELL_SIG2	0xffa3
>>>>
>>>> @@ -288,6 +289,14 @@ static int i8k_set_fan(int fan, int
>>>> speed)
>>>>
>>>>   	return i8k_smm(&regs) ? : i8k_get_fan_status(fan);
>>>>
>>>>   }
>>>>
>>>> +static int i8k_get_temp_type(int sensor)
>>>> +{
>>>> +	struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP_TYPE, };
>>>> +
>>>> +	regs.ebx = sensor & 0xff;
>>>> +	return i8k_smm(&regs) ? : regs.eax & 0xff;
>>>> +}
>>>> +
>>>>
>>>>   /*
>>>>
>>>>    * Read the cpu temperature.
>>>>    */
>>>>
>>>> @@ -493,6 +502,29 @@ static int i8k_open_fs(struct inode
>>>> *inode, struct file *file)
>>>>
>>>>    * Hwmon interface
>>>>    */
>>>>
>>>> +static ssize_t i8k_hwmon_show_temp_label(struct device
>>>> *dev, +				   struct device_attribute *devattr,
>>>> +				   char *buf)
>>>> +{
>>>> +	static const char * const labels[] = {
>>>> +		"CPU",
>>>> +		"GPU",
>>>> +		"SODIMM",
>>>> +		"Other",
>>>> +		"Ambient",
>>>> +		"Other",
>>>> +	};
>>>> +	int index = to_sensor_dev_attr(devattr)->index;
>>>> +	int type;
>>>> +
>>>> +	type = i8k_get_temp_type(index);
>>>> +	if (type < 0)
>>>> +		return type;
>>>> +	if (type >= ARRAY_SIZE(labels))
>>>> +		type = ARRAY_SIZE(labels) - 1;
>>>> +	return sprintf(buf, "%s\n", labels[type]);
>>>> +}
>>>
>>> No Documentation/ABI/ entry for your new sysfs file?
>>
>> It is standard hwmon sysfs entry which is used by other hwmon
>> drivers... It is not i8k.ko driver specific.
>
> Ok, then why is this driver not in a "hwmon-standard" location in the
> kernel tree?
>
Another historic reason. hwmon support was added to the driver
only in 2011. Initially it only had support for the proc interface.
The driver exists at least since 2005, when Linux started to use git.

Maybe we should move it ?

Guenter

Re: [PATCH] i8k: Add support for temperature sensor labels

[Guenter Roeck]

On 11/29/2014 09:43 AM, Greg Kroah-Hartman wrote:
> On Sat, Nov 29, 2014 at 05:04:07PM +0100, Pali Rohár wrote:
>> This patch adds labels for temperature sensors if SMM function with EAX register
>> 0x11a3 reports it. These informations was taken from DOS binary NBSVC.MDM.
>>
>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>> ---
>>   drivers/char/i8k.c |  110 +++++++++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 88 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
>> index e34a019..77af46b 100644
>> --- a/drivers/char/i8k.c
>> +++ b/drivers/char/i8k.c
>> @@ -42,6 +42,7 @@
>>   #define I8K_SMM_GET_FAN		0x00a3
>>   #define I8K_SMM_GET_SPEED	0x02a3
>>   #define I8K_SMM_GET_TEMP	0x10a3
>> +#define I8K_SMM_GET_TEMP_TYPE	0x11a3
>>   #define I8K_SMM_GET_DELL_SIG1	0xfea3
>>   #define I8K_SMM_GET_DELL_SIG2	0xffa3
>>
>> @@ -288,6 +289,14 @@ static int i8k_set_fan(int fan, int speed)
>>   	return i8k_smm(&regs) ? : i8k_get_fan_status(fan);
>>   }
>>
>> +static int i8k_get_temp_type(int sensor)
>> +{
>> +	struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP_TYPE, };
>> +
>> +	regs.ebx = sensor & 0xff;
>> +	return i8k_smm(&regs) ? : regs.eax & 0xff;
>> +}
>> +
>>   /*
>>    * Read the cpu temperature.
>>    */
>> @@ -493,6 +502,29 @@ static int i8k_open_fs(struct inode *inode, struct file *file)
>>    * Hwmon interface
>>    */
>>
>> +static ssize_t i8k_hwmon_show_temp_label(struct device *dev,
>> +				   struct device_attribute *devattr,
>> +				   char *buf)
>> +{
>> +	static const char * const labels[] = {
>> +		"CPU",
>> +		"GPU",
>> +		"SODIMM",
>> +		"Other",
>> +		"Ambient",
>> +		"Other",
>> +	};Documentation/hwmon/
>> +	int index = to_sensor_dev_attr(devattr)->index;
>> +	int type;
>> +
>> +	type = i8k_get_temp_type(index);
>> +	if (type < 0)
>> +		return type;
>> +	if (type >= ARRAY_SIZE(labels))
>> +		type = ARRAY_SIZE(labels) - 1;
>> +	return sprintf(buf, "%s\n", labels[type]);
>> +}
>
> No Documentation/ABI/ entry for your new sysfs file?
>
That would be Documentation/hwmon/sysfs-interface,
presumably for historic reasons (its existence precedes
the existence of Documentation/ABI).

Question might be if we should move that file at some point
to Documentation/ABI/<some-name>, though that would be a
major effort.

Guenter