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

[Pali RohÃr]

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@xxxxxxxxx>
> >>>>>>> ---
> >>>>>>> 
> >>>>>>>    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@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part.