All of lore.kernel.org
 help / color / mirror / Atom feed
From: r.marek@assembler.cz (Rudolf Marek)
To: lm-sensors@vger.kernel.org
Subject: [lm-sensors] coretemp - take3
Date: Tue, 20 Mar 2007 21:51:48 +0000	[thread overview]
Message-ID: <46005774.8030704@assembler.cz> (raw)
In-Reply-To: <45FDC7A4.9070601@assembler.cz>

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


      parent reply	other threads:[~2007-03-20 21:51 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=46005774.8030704@assembler.cz \
    --to=r.marek@assembler.cz \
    --cc=lm-sensors@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.