Hi Jean,
On Mon, Jun 06, 2011 at 09:57:53AM -0400, Jean Delvare wrote:
> Hi Guenter, Fenghua,
>
> On Wed, 1 Jun 2011 11:08:43 -0700, Guenter Roeck wrote:
> > Further relax temperature range checks after reading the IA32_TEMPERATURE_TARGET
> > register. If the register returns a value other than 0 in bits 16..32, assume
> > that the returned value is correct.
> >
> > This change applies to both packet and core temperature limits.
>
> Sorry for the later reply. Looks good to me, tested OK on my 3 systems.
>
> See comment below though.
>
> > Cc: Carsten Emde <C.Emde@xxxxxxxxx>
> > Cc: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> > Cc: Jean Delvare <khali@xxxxxxxxxxxx>
> > Signed-off-by: Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>
> > ---
> > drivers/hwmon/coretemp.c | 4 ++--
> > 1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> > index 1680977..85e9379 100644
> > --- a/drivers/hwmon/coretemp.c
> > +++ b/drivers/hwmon/coretemp.c
> > @@ -296,7 +296,7 @@ static int get_tjmax(struct cpuinfo_x86 *c, u32 id, struct device *dev)
> > * If the TjMax is not plausible, an assumption
> > * will be used
> > */
> > - if (val >= 70 && val <= 125) {
> > + if (val) {
> > dev_info(dev, "TjMax is %d C.\n", val);
> > return val * 1000;
> > }
> > @@ -326,7 +326,7 @@ static int get_pkg_tjmax(unsigned int cpu, struct device *dev)
> > err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET, &eax, &edx);
> > if (!err) {
> > val = (eax >> 16) & 0xff;
> > - if (val > 80 && val < 120)
> > + if (val)
> > return val * 1000;
> > }
> > dev_warn(dev, "Unable to read Pkg-TjMax from CPU:%u\n", cpu);
>
> While we're here, I don't quite get the rationale for having separate
> functions get_tjmax() and get_pkg_tjmax(). They do pretty much the
> same, don't they? Except that get_pkg_tjmax defaults to 100°C when
> get_tjmax calls adjust_tjmax(). This seems wrong, even though I guess
> it makes no difference in practice as adjust_tjmax() should only be
> needed for older CPU models without the package temperature sensor.
>
That was my assumption as well.
> Going one step further, if MSR_IA32_TEMPERATURE_TARGET is the right MSR
> for the package TjMax, then this pretty much means that this MSR (or at
> least bits 23:16 in it) is not per-core but per-package (it will return
> the same value on every core.) Then why are we calling get_tjmax (and
> then adjust_tjmax) on every core if the result will always be the same?
> This seems conceptually wrong and expensive.
>
I have not really thought about it myself, but that seems to be likely.
> So I would suggest that we move tjmax to struct pdev_entry, and only
> fetch it once from MSR_IA32_TEMPERATURE_TARGET. This would also solve a
Makes sense to me, though I'd like to get input from Fenghua if your analysis
is correct.
> minor annoyance I'm seeing in my kernel logs with the latest version of
> the driver:
>
> coretemp coretemp.0: TjMax is 97 C.
> coretemp coretemp.0: TjMax is 97 C.
> coretemp coretemp.0: TjMax is 97 C.
> coretemp coretemp.0: TjMax is 97 C.
>
> Imagine the result on a system with more CPUs and more core per CPUs...
> Not sure if the message itself is very valuable anyway, as tjmax will
> be visible in the output of "sensors" or any other monitoring
> application, but if we keep it, it should only be displayed once per
> physical CPU.
>
We should probably make the message a debug message or remove it entirely.
After all, we don't display this kind of stuff for other drivers either. And,
yes, I do get it 16 times on one of the systems I tested it with ;).
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
