[lm-sensors] coretemp - take3
Rudolf Marek
r.marek at assembler.cz
Tue Mar 20 22:51:48 CET 2007
Hello Alexey, Nicolas
> Frankly I don't understand when one should use rdmsr_safe() instead of
> rdmsr(). p4-clockmod driver worked fine with plain rdmsr/wrmsr.
> For general education, why this driver uses _safe functions?
It uses not well documented register 0xEE which might not even exist, and I do
not want to oops the kernel. I'm in touch with Intel, but it goes slowly.
> What's wrong with http://ssh.cz/~ruik/patches/add-msr-io-safe.patch
> is:
> a) naming: _on_cpu is suffix and thus should be last
> FOO(...) => FOO_on_cpu(unsigned int cpu, ...)
Ok I will name it rdmsr_safe_on_cpu
>
> b) static qualifier should be first.
Aha good catch!
> c) coding style in wrmsr_on_cpu/rdmsr_on_cpu/...
>
> static int foo(...)
> {
> ...
> }
Yep sorry I will fix it.
>
> d) +#include <linux/errno.h>
>
> what for? those dummy inlines don't include -E*
Well I tried to compile the kernel with allnoconfig and then just enable SMP and
coretemp, and the kernel screamed for the header files in that rdmsr_safe (so
no EIO and the other was not defined) There was a problem even before the
coretemp actually started to compile. Maybe it should be in different patch...
>> I will answer the rest in the evening.
Small glitch in coretemp_init:
printk(KERN_NOTICE DRVNAME ": This driver uses undocumented features"
"of Core CPU. Temperature might be wrong!\n");
Ok will fix.
Also, in __wrmsr_on_cpu_safe:
rv->err = wrmsr_safe(rv->msr_no, &rv->l, &rv->h);
should be:
rv->err = wrmsr_safe(rv->msr_no, rv->l, rv->h);
Will fix too. Thanks for the feedback. It is real pity that I do not have more
time for this those days :/
I will spin the new patches to this thread, with the fixes, so we can start
accepting/review phase for individual patches.
Thanks,
Rudolf
More information about the lm-sensors
mailing list