* [lm-sensors] coretemp - take3
@ 2007-03-18 23:13 Rudolf Marek
2007-03-19 3:58 ` Nicolas Boichat
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Rudolf Marek @ 2007-03-18 23:13 UTC (permalink / raw)
To: lm-sensors
Hello all,
I'm CCing all interested parties, if some isn't please tell!
On URL bellow is current version of coretemp driver.
http://ssh.cz/~ruik/patches/
I do not own this CPU (happy user of overclocked Opteron ;) So it would be great
if someone can give it testing. We need to test:
CONFIG_SMP y/n
CONFIG_CPU_HOTPLUG y/n
and also for x86_64 and i386 builds.
What was fixed? I fixed the issues Jean pointed out, I created separate patch
for ...msr_safe stuff.
http://lists.lm-sensors.org/pipermail/lm-sensors/2007-March/019169.html
http://lists.lm-sensors.org/pipermail/lm-sensors/2007-March/019175.html
All is only compile tested (I tried SMP y/n with hotplug Y on x86_64)!
And considering late hours spent with this I would be careful ;)
Reviews/comments/testing welcome!
Note that you will need at least lm-sensors 2.10.2.
Rudolf
^ permalink raw reply [flat|nested] 10+ messages in thread* [lm-sensors] coretemp - take3 2007-03-18 23:13 [lm-sensors] coretemp - take3 Rudolf Marek @ 2007-03-19 3:58 ` Nicolas Boichat 2007-03-19 4:17 ` Nicolas Boichat ` (3 subsequent siblings) 4 siblings, 0 replies; 10+ messages in thread From: Nicolas Boichat @ 2007-03-19 3:58 UTC (permalink / raw) To: lm-sensors Hello, Rudolf Marek wrote: > Hello all, > > I'm CCing all interested parties, if some isn't please tell! > > On URL bellow is current version of coretemp driver. > > http://ssh.cz/~ruik/patches/ > > I do not own this CPU (happy user of overclocked Opteron ;) So it would be great > if someone can give it testing. We need to test: > > CONFIG_SMP y/n > CONFIG_CPU_HOTPLUG y/n > and also for x86_64 and i386 builds. > > What was fixed? I fixed the issues Jean pointed out, I created separate patch > for ...msr_safe stuff. > In arch/i386/lib/msr-on-cpu.c, I'm not sure if you really need to create 2 versions of the functions (safe and unsafe). Anyway, no one uses these functions in the kernel yet, so I think you can simply convert them to safe versions. 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); > http://lists.lm-sensors.org/pipermail/lm-sensors/2007-March/019169.html > http://lists.lm-sensors.org/pipermail/lm-sensors/2007-March/019175.html > > All is only compile tested (I tried SMP y/n with hotplug Y on x86_64)! > And considering late hours spent with this I would be careful ;) > > Reviews/comments/testing welcome! Just before you published your versions of the patches, I had another version ready and tested on x86 (at least the read function), that doesn't create two version of each function. I attached the patch here. I you prefer your version, I'll test it. I have another patch removing redundant functions in arch/i386/kernel/msr.c, using the new functions in arch/i386/lib/msr-on-cpu.c, I'll publish it here if you prefer my version, otherwise I'll adapt it to your prototypes. Best regards, Nicolas Use safe versions (i.e. return error code) of rdmsr and wrmsr in arch/i386/lib/msr-on-cpu.c and arch/x86_64/lib/msr-on-cpu.c. Signed-off-by: Nicolas Boichat <nicolas at boichat.ch> --- arch/i386/lib/msr-on-cpu.c | 47 ++++++++++++++++++++++++++++---------------- include/asm-i386/msr.h | 13 +++++++----- include/asm-x86_64/msr.h | 14 +++++++------ 3 files changed, 45 insertions(+), 29 deletions(-) diff --git a/arch/i386/lib/msr-on-cpu.c b/arch/i386/lib/msr-on-cpu.c index 1c46bda..be672df 100644 --- a/arch/i386/lib/msr-on-cpu.c +++ b/arch/i386/lib/msr-on-cpu.c @@ -3,55 +3,68 @@ #include <linux/smp.h> #include <asm/msr.h> -struct msr_info { +struct msr_command { + int err; u32 msr_no; u32 l, h; }; -static void __rdmsr_on_cpu(void *info) +static void __rdmsr_on_cpu(void *command) { - struct msr_info *rv = info; + struct msr_command *rv = command; - rdmsr(rv->msr_no, rv->l, rv->h); + rv->err = rdmsr_safe(rv->msr_no, &rv->l, &rv->h); } -void rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h) +int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h) { + int ret; + preempt_disable(); if (smp_processor_id() = cpu) - rdmsr(msr_no, *l, *h); + ret = rdmsr_safe(msr_no, l, h); else { - struct msr_info rv; + struct msr_command rv; rv.msr_no = msr_no; - smp_call_function_single(cpu, __rdmsr_on_cpu, &rv, 0, 1); - *l = rv.l; - *h = rv.h; + ret = smp_call_function_single(cpu, __rdmsr_on_cpu, &rv, 0, 1); + if (!ret) { + *l = rv.l; + *h = rv.h; + ret = rv.err; + } } preempt_enable(); + return ret; } -static void __wrmsr_on_cpu(void *info) +static void __wrmsr_on_cpu(void *command) { - struct msr_info *rv = info; + struct msr_command *rv = command; - wrmsr(rv->msr_no, rv->l, rv->h); + rv->err = wrmsr_safe(rv->msr_no, rv->l, rv->h); } -void wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h) +int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h) { + int ret; + preempt_disable(); if (smp_processor_id() = cpu) - wrmsr(msr_no, l, h); + ret = wrmsr_safe(msr_no, l, h); else { - struct msr_info rv; + struct msr_command rv; rv.msr_no = msr_no; rv.l = l; rv.h = h; - smp_call_function_single(cpu, __wrmsr_on_cpu, &rv, 0, 1); + ret = smp_call_function_single(cpu, __wrmsr_on_cpu, &rv, 0, 1); + if (!ret) { + ret = rv.err; + } } preempt_enable(); + return ret; } EXPORT_SYMBOL(rdmsr_on_cpu); diff --git a/include/asm-i386/msr.h b/include/asm-i386/msr.h index ec3b680..8caec04 100644 --- a/include/asm-i386/msr.h +++ b/include/asm-i386/msr.h @@ -4,6 +4,7 @@ #ifdef CONFIG_PARAVIRT #include <asm/paravirt.h> #else +#include <linux/errno.h> /* * Access to machine-specific registers (available on 586 and better only) @@ -84,16 +85,16 @@ static inline void wrmsrl (unsigned long msr, unsigned long long val) #endif /* !CONFIG_PARAVIRT */ #ifdef CONFIG_SMP -void rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h); -void wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h); +int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h); +int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h); #else /* CONFIG_SMP */ -static inline void rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h) +static inline int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h) { - rdmsr(msr_no, *l, *h); + return rdmsr_safe(msr_no, l, h); } -static inline void wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h) +static inline int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h) { - wrmsr(msr_no, l, h); + return wrmsr_safe(msr_no, l, h); } #endif /* CONFIG_SMP */ diff --git a/include/asm-x86_64/msr.h b/include/asm-x86_64/msr.h index 902f9a5..789d23c 100644 --- a/include/asm-x86_64/msr.h +++ b/include/asm-x86_64/msr.h @@ -2,6 +2,8 @@ #define X86_64_MSR_H 1 #ifndef __ASSEMBLY__ +#include <linux/errno.h> + /* * Access to machine-specific registers (available on 586 and better only) * Note: the rd* operations modify the parameters directly (without using @@ -161,16 +163,16 @@ static inline unsigned int cpuid_edx(unsigned int op) #define MSR_IA32_UCODE_REV 0x8b #ifdef CONFIG_SMP -void rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h); -void wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h); +int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h); +int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h); #else /* CONFIG_SMP */ -static inline void rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h) +static inline int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h) { - rdmsr(msr_no, *l, *h); + return rdmsr_safe(msr_no, l, h); } -static inline void wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h) +static inline int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h) { - wrmsr(msr_no, l, h); + return wrmsr_safe(msr_no, l, h); } #endif /* CONFIG_SMP */ ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [lm-sensors] coretemp - take3 2007-03-18 23:13 [lm-sensors] coretemp - take3 Rudolf Marek 2007-03-19 3:58 ` Nicolas Boichat @ 2007-03-19 4:17 ` Nicolas Boichat 2007-05-02 22:41 ` Justin Piszcz 2007-03-19 6:31 ` [lm-sensors] " Rudolf Marek ` (2 subsequent siblings) 4 siblings, 1 reply; 10+ messages in thread From: Nicolas Boichat @ 2007-03-19 4:17 UTC (permalink / raw) To: lm-sensors Rudolf Marek wrote: > Hello all, > > I'm CCing all interested parties, if some isn't please tell! > > On URL bellow is current version of coretemp driver. > > http://ssh.cz/~ruik/patches/ Small glitch in coretemp_init: printk(KERN_NOTICE DRVNAME ": This driver uses undocumented features" "of Core CPU. Temperature might be wrong!\n"); Prints this in dmesg: [46487.231000] coretemp: This driver uses undocumented featuresof Core CPU. Temperature might be wrong! (Notice the space missing between "features" and "of") Best regards, Nicolas ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [lm-sensors] coretemp - take3 2007-03-19 4:17 ` Nicolas Boichat @ 2007-05-02 22:41 ` Justin Piszcz 0 siblings, 0 replies; 10+ messages in thread From: Justin Piszcz @ 2007-05-02 22:41 UTC (permalink / raw) To: Nicolas Boichat Cc: Rudolf Marek, LM Sensors, bytes, debian-user, linux-kernel On Mon, 19 Mar 2007, Nicolas Boichat wrote: > Rudolf Marek wrote: >> Hello all, >> >> I'm CCing all interested parties, if some isn't please tell! >> >> On URL bellow is current version of coretemp driver. >> >> http://ssh.cz/~ruik/patches/ > > Small glitch in coretemp_init: > > printk(KERN_NOTICE DRVNAME ": This driver uses undocumented features" > > "of Core CPU. Temperature might be wrong!\n"); > > > Prints this in dmesg: > > [46487.231000] coretemp: This driver uses undocumented featuresof Core CPU. Temperature might be wrong! > > > (Notice the space missing between "features" and "of") > > Best regards, > > Nicolas > This did not make it into 2.6.21? Will it get into 2.6.22? _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: coretemp - take3 @ 2007-05-02 22:41 ` Justin Piszcz 0 siblings, 0 replies; 10+ messages in thread From: Justin Piszcz @ 2007-05-02 22:41 UTC (permalink / raw) To: Nicolas Boichat Cc: Rudolf Marek, LM Sensors, bytes, debian-user, linux-kernel On Mon, 19 Mar 2007, Nicolas Boichat wrote: > Rudolf Marek wrote: >> Hello all, >> >> I'm CCing all interested parties, if some isn't please tell! >> >> On URL bellow is current version of coretemp driver. >> >> http://ssh.cz/~ruik/patches/ > > Small glitch in coretemp_init: > > printk(KERN_NOTICE DRVNAME ": This driver uses undocumented features" > > "of Core CPU. Temperature might be wrong!\n"); > > > Prints this in dmesg: > > [46487.231000] coretemp: This driver uses undocumented featuresof Core CPU. Temperature might be wrong! > > > (Notice the space missing between "features" and "of") > > Best regards, > > Nicolas > This did not make it into 2.6.21? Will it get into 2.6.22? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [lm-sensors] coretemp - take3 2007-05-02 22:41 ` Justin Piszcz @ 2007-05-03 6:40 ` Rudolf Marek -1 siblings, 0 replies; 10+ messages in thread From: Rudolf Marek @ 2007-05-03 6:40 UTC (permalink / raw) To: Justin Piszcz Cc: Nicolas Boichat, LM Sensors, bytes, debian-user, linux-kernel Hi all, > This did not make it into 2.6.21? Will it get into 2.6.22? Well this does not depend on me. But this is already in mm: + x86-msr-add-support-for-safe-variants.patch added to -mm tree (this night came some mail, that it has been added - same came few days back) Rudolf _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: coretemp - take3 @ 2007-05-03 6:40 ` Rudolf Marek 0 siblings, 0 replies; 10+ messages in thread From: Rudolf Marek @ 2007-05-03 6:40 UTC (permalink / raw) To: Justin Piszcz Cc: Nicolas Boichat, LM Sensors, bytes, debian-user, linux-kernel Hi all, > This did not make it into 2.6.21? Will it get into 2.6.22? Well this does not depend on me. But this is already in mm: + x86-msr-add-support-for-safe-variants.patch added to -mm tree (this night came some mail, that it has been added - same came few days back) Rudolf ^ permalink raw reply [flat|nested] 10+ messages in thread
* [lm-sensors] coretemp - take3 2007-03-18 23:13 [lm-sensors] coretemp - take3 Rudolf Marek 2007-03-19 3:58 ` Nicolas Boichat 2007-03-19 4:17 ` Nicolas Boichat @ 2007-03-19 6:31 ` Rudolf Marek 2007-03-19 14:17 ` Alexey Dobriyan 2007-03-20 21:51 ` Rudolf Marek 4 siblings, 0 replies; 10+ messages in thread From: Rudolf Marek @ 2007-03-19 6:31 UTC (permalink / raw) To: lm-sensors Alexey, Maybe OpenVZ needs both safe and unsafe variants in msr-on-cpu? >> On URL bellow is current version of coretemp driver. >> >> http://ssh.cz/~ruik/patches/ >> >> What was fixed? I fixed the issues Jean pointed out, I created separate patch >> for ...msr_safe stuff. >> > In arch/i386/lib/msr-on-cpu.c, I'm not sure if you really need to create > 2 versions of the functions (safe and unsafe). Anyway, no one uses these > functions in the kernel yet, so I think you can simply convert them to > safe versions. -snip- I will answer the rest in the evening. Rudolf ^ permalink raw reply [flat|nested] 10+ messages in thread
* [lm-sensors] coretemp - take3 2007-03-18 23:13 [lm-sensors] coretemp - take3 Rudolf Marek ` (2 preceding siblings ...) 2007-03-19 6:31 ` [lm-sensors] " Rudolf Marek @ 2007-03-19 14:17 ` Alexey Dobriyan 2007-03-20 21:51 ` Rudolf Marek 4 siblings, 0 replies; 10+ messages in thread From: Alexey Dobriyan @ 2007-03-19 14:17 UTC (permalink / raw) To: lm-sensors On Mon, Mar 19, 2007 at 07:31:11AM +0100, Rudolf Marek wrote: > Maybe OpenVZ needs both safe and unsafe variants in msr-on-cpu? 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? 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, ...) b) static qualifier should be first. c) coding style in wrmsr_on_cpu/rdmsr_on_cpu/... static int foo(...) { ... } d) +#include <linux/errno.h> what for? those dummy inlines don't include -E* > >> On URL bellow is current version of coretemp driver. > >> > >> http://ssh.cz/~ruik/patches/ > >> > >> What was fixed? I fixed the issues Jean pointed out, I created separate patch > >> for ...msr_safe stuff. > >> > > In arch/i386/lib/msr-on-cpu.c, I'm not sure if you really need to create > > 2 versions of the functions (safe and unsafe). Anyway, no one uses these > > functions in the kernel yet, so I think you can simply convert them to > > safe versions. > > > -snip- > > I will answer the rest in the evening. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [lm-sensors] coretemp - take3 2007-03-18 23:13 [lm-sensors] coretemp - take3 Rudolf Marek ` (3 preceding siblings ...) 2007-03-19 14:17 ` Alexey Dobriyan @ 2007-03-20 21:51 ` Rudolf Marek 4 siblings, 0 replies; 10+ messages in thread From: Rudolf Marek @ 2007-03-20 21:51 UTC (permalink / raw) To: lm-sensors 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2007-05-03 6:41 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-03-18 23:13 [lm-sensors] coretemp - take3 Rudolf Marek 2007-03-19 3:58 ` Nicolas Boichat 2007-03-19 4:17 ` Nicolas Boichat 2007-05-02 22:41 ` Justin Piszcz 2007-05-02 22:41 ` Justin Piszcz 2007-05-03 6:40 ` [lm-sensors] " Rudolf Marek 2007-05-03 6:40 ` Rudolf Marek 2007-03-19 6:31 ` [lm-sensors] " Rudolf Marek 2007-03-19 14:17 ` Alexey Dobriyan 2007-03-20 21:51 ` Rudolf Marek
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.