[lm-sensors] coretemp - take3

[lm-sensors] coretemp - take3

[Rudolf Marek]

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

[lm-sensors] coretemp - take3

[Nicolas Boichat]

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  */
 

[lm-sensors] coretemp - take3

[Nicolas Boichat]

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

[lm-sensors] coretemp - take3

[Rudolf Marek]

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

[lm-sensors] coretemp - take3

[Alexey Dobriyan]

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.

[lm-sensors] coretemp - take3

[Rudolf Marek]

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

Re: [lm-sensors] coretemp - take3

[Justin Piszcz]

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

Re: coretemp - take3

[Justin Piszcz]

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?

Re: [lm-sensors] coretemp - take3

[Rudolf Marek]

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

Re: coretemp - take3

[Rudolf Marek]

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