[lm-sensors] [PATCH 79/79] hwmon: (it87) Fix vrm value range

[lm-sensors] [PATCH 79/79] hwmon: (it87) Fix vrm value range

[Guenter Roeck]

When updating vrm, the value range was not limited. This could result in more or
less random vrm values if the value provided by the user was larger than 255.
Fix by limiting the range to 0..255 using the SENSORS_LIMIT macro.

Cc: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
 drivers/hwmon/it87.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 0b204e4..bd8775c 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -1324,7 +1324,7 @@ static ssize_t store_vrm_reg(struct device *dev, struct device_attribute *attr,
 	if (kstrtoul(buf, 10, &val) < 0)
 		return -EINVAL;
 
-	data->vrm = val;
+	data->vrm = SENSORS_LIMIT(val, 0, 255);
 
 	return count;
 }
-- 
1.7.3.1


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 79/79] hwmon: (it87) Fix vrm value range

[Jean Delvare]

Hi Guenter,

On Mon, 23 Jan 2012 18:49:58 -0800, Guenter Roeck wrote:
> When updating vrm, the value range was not limited. This could result in more or
> less random vrm values if the value provided by the user was larger than 255.
> Fix by limiting the range to 0..255 using the SENSORS_LIMIT macro.
> 
> Cc: Jean Delvare <khali@linux-fr.org>
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
>  drivers/hwmon/it87.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 0b204e4..bd8775c 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -1324,7 +1324,7 @@ static ssize_t store_vrm_reg(struct device *dev, struct device_attribute *attr,
>  	if (kstrtoul(buf, 10, &val) < 0)
>  		return -EINVAL;
>  
> -	data->vrm = val;
> +	data->vrm = SENSORS_LIMIT(val, 0, 255);
>  
>  	return count;
>  }

To be honest, I don't know what to do with this.

You're replacing a silent failure with another. I'd rather return an
error if the value doesn't fit. as clamping to 255 will lead to a
failure in vid_from_reg() later anyway.

On the other hand, we don't validate the VRM value here anyway, it
could be in the 0-255 range and still lead to a failure in
vid_from_reg().

We could improve this of course, but is it worth it? As documented in
Documentation/hwmon/sysfs-interface, changing the vrm value manually
should no longer be needed. libsensors does even ignore this attribute
completely. And newer hardware tends to no longer use the VID pins at
all. Actually hwmon-vid is in a pretty bad shape at the moment at least
for all Intel CPUs after Conroe (2007, yeah!) for which we set the VRM
to 82 i.e. Pentium II/III. And nobody complained yet as far as I know!

So I'm really not sure if it is worth spending time on. It might make
more sense to turn all these vrm attributes read-only (and ultimately
kill them altogether.) If we really want to let the user force the vrm
value if automatic detection failed, it would probably be better done
as a module parameter to hwmon-vid. It is certainly less flexible, but
at least we don't have to repeat it in every hwmon driver.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 79/79] hwmon: (it87) Fix vrm value range

[Guenter Roeck]

On Tue, Jan 24, 2012 at 03:50:12AM -0500, Jean Delvare wrote:
> Hi Guenter,
> 
> On Mon, 23 Jan 2012 18:49:58 -0800, Guenter Roeck wrote:
> > When updating vrm, the value range was not limited. This could result in more or
> > less random vrm values if the value provided by the user was larger than 255.
> > Fix by limiting the range to 0..255 using the SENSORS_LIMIT macro.
> > 
> > Cc: Jean Delvare <khali@linux-fr.org>
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> >  drivers/hwmon/it87.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> > index 0b204e4..bd8775c 100644
> > --- a/drivers/hwmon/it87.c
> > +++ b/drivers/hwmon/it87.c
> > @@ -1324,7 +1324,7 @@ static ssize_t store_vrm_reg(struct device *dev, struct device_attribute *attr,
> >  	if (kstrtoul(buf, 10, &val) < 0)
> >  		return -EINVAL;
> >  
> > -	data->vrm = val;
> > +	data->vrm = SENSORS_LIMIT(val, 0, 255);
> >  
> >  	return count;
> >  }
> 
> To be honest, I don't know what to do with this.
> 
Same as will all the others, really ;).

> You're replacing a silent failure with another. I'd rather return an
> error if the value doesn't fit. as clamping to 255 will lead to a
> failure in vid_from_reg() later anyway.
> 
I don't mind returning an error instead of using SENSORS_LIMIT(), but doesn't
that defeat the idea behind SENSORS_LIMIT() ?

> On the other hand, we don't validate the VRM value here anyway, it
> could be in the 0-255 range and still lead to a failure in
> vid_from_reg().
> 
... and vid_from_reg() may return 0 even if the value is valid (or at least
it does not have an explicit error return code). So I don't really know
how to validate it. We would have to add a vrm validation call.

> We could improve this of course, but is it worth it? As documented in
> Documentation/hwmon/sysfs-interface, changing the vrm value manually
> should no longer be needed. libsensors does even ignore this attribute
> completely. And newer hardware tends to no longer use the VID pins at
> all. Actually hwmon-vid is in a pretty bad shape at the moment at least
> for all Intel CPUs after Conroe (2007, yeah!) for which we set the VRM
> to 82 i.e. Pentium II/III. And nobody complained yet as far as I know!
> 
> So I'm really not sure if it is worth spending time on. It might make
> more sense to turn all these vrm attributes read-only (and ultimately
> kill them altogether.) If we really want to let the user force the vrm
> value if automatic detection failed, it would probably be better done
> as a module parameter to hwmon-vid. It is certainly less flexible, but
> at least we don't have to repeat it in every hwmon driver.
> 
Your call - let me know.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 79/79] hwmon: (it87) Fix vrm value range

[Guenter Roeck]

On Tue, Jan 24, 2012 at 03:50:12AM -0500, Jean Delvare wrote:
> Hi Guenter,
> 
> On Mon, 23 Jan 2012 18:49:58 -0800, Guenter Roeck wrote:
> > When updating vrm, the value range was not limited. This could result in more or
> > less random vrm values if the value provided by the user was larger than 255.
> > Fix by limiting the range to 0..255 using the SENSORS_LIMIT macro.
> > 
> > Cc: Jean Delvare <khali@linux-fr.org>
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> >  drivers/hwmon/it87.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> > index 0b204e4..bd8775c 100644
> > --- a/drivers/hwmon/it87.c
> > +++ b/drivers/hwmon/it87.c
> > @@ -1324,7 +1324,7 @@ static ssize_t store_vrm_reg(struct device *dev, struct device_attribute *attr,
> >  	if (kstrtoul(buf, 10, &val) < 0)
> >  		return -EINVAL;
> >  
> > -	data->vrm = val;
> > +	data->vrm = SENSORS_LIMIT(val, 0, 255);
> >  
> >  	return count;
> >  }
> 
> To be honest, I don't know what to do with this.
> 
> You're replacing a silent failure with another. I'd rather return an
> error if the value doesn't fit. as clamping to 255 will lead to a
> failure in vid_from_reg() later anyway.
> 
> On the other hand, we don't validate the VRM value here anyway, it
> could be in the 0-255 range and still lead to a failure in
> vid_from_reg().
> 
> We could improve this of course, but is it worth it? As documented in
> Documentation/hwmon/sysfs-interface, changing the vrm value manually
> should no longer be needed. libsensors does even ignore this attribute
> completely. And newer hardware tends to no longer use the VID pins at
> all. Actually hwmon-vid is in a pretty bad shape at the moment at least
> for all Intel CPUs after Conroe (2007, yeah!) for which we set the VRM
> to 82 i.e. Pentium II/III. And nobody complained yet as far as I know!
> 
If it is like me, I always thought it was a chip problem that it displays
a voltage of 0. There is no warning, so my take is that people simply don't
realize that the CPU model is not recognized.

> So I'm really not sure if it is worth spending time on. It might make
> more sense to turn all these vrm attributes read-only (and ultimately
> kill them altogether.) If we really want to let the user force the vrm
> value if automatic detection failed, it would probably be better done
> as a module parameter to hwmon-vid. It is certainly less flexible, but
> at least we don't have to repeat it in every hwmon driver.
> 
After looking into it a bit more, I think we should move the vrm attributes
to hwmon-vid (and fix hwmon-vid).

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 79/79] hwmon: (it87) Fix vrm value range

[Guenter Roeck]

On Tue, 2012-01-24 at 10:11 -0500, Guenter Roeck wrote:
> On Tue, Jan 24, 2012 at 03:50:12AM -0500, Jean Delvare wrote:
> > Hi Guenter,
> > 
> > On Mon, 23 Jan 2012 18:49:58 -0800, Guenter Roeck wrote:
> > > When updating vrm, the value range was not limited. This could result in more or
> > > less random vrm values if the value provided by the user was larger than 255.
> > > Fix by limiting the range to 0..255 using the SENSORS_LIMIT macro.
> > > 
> > > Cc: Jean Delvare <khali@linux-fr.org>
> > > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > > ---
> > >  drivers/hwmon/it87.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> > > index 0b204e4..bd8775c 100644
> > > --- a/drivers/hwmon/it87.c
> > > +++ b/drivers/hwmon/it87.c
> > > @@ -1324,7 +1324,7 @@ static ssize_t store_vrm_reg(struct device *dev, struct device_attribute *attr,
> > >  	if (kstrtoul(buf, 10, &val) < 0)
> > >  		return -EINVAL;
> > >  
> > > -	data->vrm = val;
> > > +	data->vrm = SENSORS_LIMIT(val, 0, 255);
> > >  
> > >  	return count;
> > >  }
> > 
> > To be honest, I don't know what to do with this.
> > 
> > You're replacing a silent failure with another. I'd rather return an
> > error if the value doesn't fit. as clamping to 255 will lead to a
> > failure in vid_from_reg() later anyway.
> > 
> > On the other hand, we don't validate the VRM value here anyway, it
> > could be in the 0-255 range and still lead to a failure in
> > vid_from_reg().
> > 
> > We could improve this of course, but is it worth it? As documented in
> > Documentation/hwmon/sysfs-interface, changing the vrm value manually
> > should no longer be needed. libsensors does even ignore this attribute
> > completely. And newer hardware tends to no longer use the VID pins at
> > all. Actually hwmon-vid is in a pretty bad shape at the moment at least
> > for all Intel CPUs after Conroe (2007, yeah!) for which we set the VRM
> > to 82 i.e. Pentium II/III. And nobody complained yet as far as I know!
> > 
> If it is like me, I always thought it was a chip problem that it displays
> a voltage of 0. There is no warning, so my take is that people simply don't
> realize that the CPU model is not recognized.
> 
> > So I'm really not sure if it is worth spending time on. It might make
> > more sense to turn all these vrm attributes read-only (and ultimately
> > kill them altogether.) If we really want to let the user force the vrm
> > value if automatic detection failed, it would probably be better done
> > as a module parameter to hwmon-vid. It is certainly less flexible, but
> > at least we don't have to repeat it in every hwmon driver.
> > 
> After looking into it a bit more, I think we should move the vrm attributes
> to hwmon-vid (and fix hwmon-vid).

Looked into this a bit. I have a list of CPU models, but how do I find
out which Intel CPU model supports which VRM version ?

Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 79/79] hwmon: (it87) Fix vrm value range

[Jean Delvare]

On Tue, 24 Jan 2012 10:47:00 -0800, Guenter Roeck wrote:
> On Tue, 2012-01-24 at 10:11 -0500, Guenter Roeck wrote:
> > After looking into it a bit more, I think we should move the vrm attributes
> > to hwmon-vid (and fix hwmon-vid).
> 
> Looked into this a bit. I have a list of CPU models, but how do I find
> out which Intel CPU model supports which VRM version ?

You have to check the datasheet of every CPU model (or family of
models) separately. For example, you can get the datasheet for Pentium
III from:
http://www.intel.com/design/pentiumiii/datashts/244452.htm

Then search the document for "VRM", and you'll come to the conclusion
that it uses VRM 8.4 (which is already supported by hwmon-vid.)

I'm not sure what you're trying to fix exactly, BTW. The problem I did
identify is for Intel CPU family 6, models < 0x9. These are currently
covered by a generic entry in vrm_models[]:

	{X86_VENDOR_INTEL, 0x6, ANY, ANY, 82},		/* any P6 */

and this should be changed to explicit entries instead, so that the
generic entry can be removed. But I don't know which of these model
numbers were actually used (nor what model names they correspond to.)
Well, according to the datasheet above, models 7 and 8 are for the Slot
1 version of the Pentium III, but for lower model numbers some research
will be needed.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 79/79] hwmon: (it87) Fix vrm value range

[Guenter Roeck]

Hi Jean,

On Tue, 2012-01-24 at 14:49 -0500, Jean Delvare wrote:
> On Tue, 24 Jan 2012 10:47:00 -0800, Guenter Roeck wrote:
> > On Tue, 2012-01-24 at 10:11 -0500, Guenter Roeck wrote:
> > > After looking into it a bit more, I think we should move the vrm attributes
> > > to hwmon-vid (and fix hwmon-vid).
> > 
> > Looked into this a bit. I have a list of CPU models, but how do I find
> > out which Intel CPU model supports which VRM version ?
> 
> You have to check the datasheet of every CPU model (or family of
> models) separately. For example, you can get the datasheet for Pentium
> III from:
> http://www.intel.com/design/pentiumiii/datashts/244452.htm
> 
> Then search the document for "VRM", and you'll come to the conclusion
> that it uses VRM 8.4 (which is already supported by hwmon-vid.)
> 
> I'm not sure what you're trying to fix exactly, BTW. The problem I did
> identify is for Intel CPU family 6, models < 0x9. These are currently
> covered by a generic entry in vrm_models[]:
> 
> 	{X86_VENDOR_INTEL, 0x6, ANY, ANY, 82},		/* any P6 */
> 
> and this should be changed to explicit entries instead, so that the
> generic entry can be removed. But I don't know which of these model
> numbers were actually used (nor what model names they correspond to.)

Most are defined in Intel Application Note 485.

> Well, according to the datasheet above, models 7 and 8 are for the Slot
> 1 version of the Pentium III, but for lower model numbers some research
> will be needed.
> 
Intel CPU model numbers 0x10 and above are not handled correctly. The
code computes eff_model but does not take the extended model number into
account for model 0x06 CPUs (for example, eff_model is computed as 0xa
on my SandyBridge system, when it should be 0x2a). 

Other family 0x6 models I know of so far are:

0x1	Pentium Pro
0x3	Pentium Pro
0x5	Pentium II, Xeon
0x6	Celeron
0x7	Pentium III, Xeon
0x8	Pentium III, Xeon

0x16	Celeron (65 nm)
0x17	Core 2, Xeon (45nm)
0x1A	Core i7 LGA1366 (45nm)
0x1C	Atom (45nm)
0x1D	Xeon MP (45 nm)
0x1E	Core i5, i7 LGA1366 (45nm)
0x1F	Core i5, i7
0x25	Core i3, i5, i7 LGA1156 (32nm)
0x2A	Core i5, i7 2xxx LGA1155 (32nm)
0x2C	Core i7 LGA1366 (32nm) 6 Core
0x2D	SandyBridge Xeon
0x2E	Xeon 7500 (45 nm)
0x2F	Xeon 7500 (32 nm)

Presumably we would want to know the VRM version for those to be able to
support them correctly.

I hoped there was an easier version to find the VRM version than by
searching through the various datasheets.

Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 79/79] hwmon: (it87) Fix vrm value range

[Jean Delvare]

On Tue, 24 Jan 2012 06:47:41 -0800, Guenter Roeck wrote:
> On Tue, Jan 24, 2012 at 03:50:12AM -0500, Jean Delvare wrote:
> > You're replacing a silent failure with another. I'd rather return an
> > error if the value doesn't fit. as clamping to 255 will lead to a
> > failure in vid_from_reg() later anyway.
>
> I don't mind returning an error instead of using SENSORS_LIMIT(), but doesn't
> that defeat the idea behind SENSORS_LIMIT() ?

Not really. The point of SENSORS_LIMIT() is to clamp the value provided
by the user when the hardware limits aren't known by the user. If the
user wants to set the high voltage limit to 2.2 V and the ADC only
supports 2.04 V max, it doesn't seem fair to yell at the user. Even
more so when the value being set may have been computed by libsensors
from a formula in sensors.conf [1].

For attributes with a well-defined range (e.g. pwm[1-*]) or holding
discrete values (e.g. temp[1-*]_type) clamping makes no sense. VRM
clearly belongs to this second category.

[1] Even this could be discussed. On most chips, setting the high limit
to the max will disable the associated alarm, which may deserve better
reporting to the user than silent clamping. Oh well.

> > On the other hand, we don't validate the VRM value here anyway, it
> > could be in the 0-255 range and still lead to a failure in
> > vid_from_reg().
>
> ... and vid_from_reg() may return 0 even if the value is valid (or at least
> it does not have an explicit error return code). So I don't really know
> how to validate it. We would have to add a vrm validation call.

Correct. vid_from_reg() could simply report an error when vrm value is
unsupported, so it could be used for validation. This is probably
easier to maintain than a separate function.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 79/79] hwmon: (it87) Fix vrm value range

[Jean Delvare]

On Tue, 24 Jan 2012 07:11:27 -0800, Guenter Roeck wrote:
> On Tue, Jan 24, 2012 at 03:50:12AM -0500, Jean Delvare wrote:
> > We could improve this of course, but is it worth it? As documented in
> > Documentation/hwmon/sysfs-interface, changing the vrm value manually
> > should no longer be needed. libsensors does even ignore this attribute
> > completely. And newer hardware tends to no longer use the VID pins at
> > all. Actually hwmon-vid is in a pretty bad shape at the moment at least
> > for all Intel CPUs after Conroe (2007, yeah!) for which we set the VRM
> > to 82 i.e. Pentium II/III. And nobody complained yet as far as I know!
>
> If it is like me, I always thought it was a chip problem that it displays
> a voltage of 0. There is no warning, so my take is that people simply don't
> realize that the CPU model is not recognized.

There are 3 different problems here.

Problem #1 is the bug in hwmon-vid which causes all unknown Intel
family 6 processors to use VRM 8.2, even though this is wrong on any
post-2007 CPU.

Problem #2 is that unknown VRM for a recent CPU will only be notified
to the kernel log, which regular users don't read. Then you indeed get
a cpu0_vid value of 0. That's apparently what you were seeing on your
system. It would certainly be better to _not_ expose the VID value at
all if we don't know how to decode it.

Problem #3 is that most boards no longer have the VID pins routed to
the hardware monitoring chip, because the same information can be
retrieved from MSR and board makers don't want to route 6 to 8 extra
wires from the CPU to the Super-I/O without a good reason. In general
the VID pins can be used for other functions too. But there is not
always an easy way to figure that out, so on most recent systems,
cpu0_vrm will return a bogus value. We can probably fix some of these
cases but not all.

I expect future Super-I/O chips to drop the VID input feature
altogether, which will make the situation better, hopefully.

> > So I'm really not sure if it is worth spending time on. It might make
> > more sense to turn all these vrm attributes read-only (and ultimately
> > kill them altogether.) If we really want to let the user force the vrm
> > value if automatic detection failed, it would probably be better done
> > as a module parameter to hwmon-vid. It is certainly less flexible, but
> > at least we don't have to repeat it in every hwmon driver.
>
> After looking into it a bit more, I think we should move the vrm attributes
> to hwmon-vid (and fix hwmon-vid).

Agreed.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 79/79] hwmon: (it87) Fix vrm value range

[Guenter Roeck]

On Wed, Jan 25, 2012 at 05:17:24AM -0500, Jean Delvare wrote:
> On Tue, 24 Jan 2012 07:11:27 -0800, Guenter Roeck wrote:
> > On Tue, Jan 24, 2012 at 03:50:12AM -0500, Jean Delvare wrote:
> > > We could improve this of course, but is it worth it? As documented in
> > > Documentation/hwmon/sysfs-interface, changing the vrm value manually
> > > should no longer be needed. libsensors does even ignore this attribute
> > > completely. And newer hardware tends to no longer use the VID pins at
> > > all. Actually hwmon-vid is in a pretty bad shape at the moment at least
> > > for all Intel CPUs after Conroe (2007, yeah!) for which we set the VRM
> > > to 82 i.e. Pentium II/III. And nobody complained yet as far as I know!
> >
> > If it is like me, I always thought it was a chip problem that it displays
> > a voltage of 0. There is no warning, so my take is that people simply don't
> > realize that the CPU model is not recognized.
> 
> There are 3 different problems here.
> 
> Problem #1 is the bug in hwmon-vid which causes all unknown Intel
> family 6 processors to use VRM 8.2, even though this is wrong on any
> post-2007 CPU.

Working on fixing that. Looks like all the recent CPUs use VRD 11.0 or 11.1.
There seem to be some supporting VRD 12.0, (the SandyBridge chips using the
larger socket) but I did not find cpuid values for those.

> Problem #2 is that unknown VRM for a recent CPU will only be notified
> to the kernel log, which regular users don't read. Then you indeed get
> a cpu0_vid value of 0. That's apparently what you were seeing on your
> system. It would certainly be better to _not_ expose the VID value at
> all if we don't know how to decode it.
> 
Actually, I have problem #1 on one board, and problem #3 on the other
(the one with SandyBridge). vid reads 0xff on that board. On the other board,
vid erroneously reports a voltage of 2.048V. I never thought about that
until you brought it up.

Thanks,
Guenter

> Problem #3 is that most boards no longer have the VID pins routed to
> the hardware monitoring chip, because the same information can be
> retrieved from MSR and board makers don't want to route 6 to 8 extra
> wires from the CPU to the Super-I/O without a good reason. In general
> the VID pins can be used for other functions too. But there is not
> always an easy way to figure that out, so on most recent systems,
> cpu0_vrm will return a bogus value. We can probably fix some of these
> cases but not all.
> 
> I expect future Super-I/O chips to drop the VID input feature
> altogether, which will make the situation better, hopefully.
> 
> > > So I'm really not sure if it is worth spending time on. It might make
> > > more sense to turn all these vrm attributes read-only (and ultimately
> > > kill them altogether.) If we really want to let the user force the vrm
> > > value if automatic detection failed, it would probably be better done
> > > as a module parameter to hwmon-vid. It is certainly less flexible, but
> > > at least we don't have to repeat it in every hwmon driver.
> >
> > After looking into it a bit more, I think we should move the vrm attributes
> > to hwmon-vid (and fix hwmon-vid).
> 
> Agreed.
> 
> -- 
> Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 79/79] hwmon: (it87) Fix vrm value range

[Jean Delvare]

On Wed, 25 Jan 2012 02:31:39 -0800, Guenter Roeck wrote:
> On Wed, Jan 25, 2012 at 05:17:24AM -0500, Jean Delvare wrote:
> > There are 3 different problems here.
> > 
> > Problem #1 is the bug in hwmon-vid which causes all unknown Intel
> > family 6 processors to use VRM 8.2, even though this is wrong on any
> > post-2007 CPU.
> 
> Working on fixing that. Looks like all the recent CPUs use VRD 11.0 or 11.1.
> There seem to be some supporting VRD 12.0, (the SandyBridge chips using the
> larger socket) but I did not find cpuid values for those.

There may be a problem with CPU models which can support an alternative
VID encoding for compatibility. I seem to recall that the early VRD
11.0 CPUs could also output VRD 10.x, but I could never find out how to
know when the actually did that.

> > Problem #2 is that unknown VRM for a recent CPU will only be notified
> > to the kernel log, which regular users don't read. Then you indeed get
> > a cpu0_vid value of 0. That's apparently what you were seeing on your
> > system. It would certainly be better to _not_ expose the VID value at
> > all if we don't know how to decode it.
>
> Actually, I have problem #1 on one board, and problem #3 on the other
> (the one with SandyBridge). vid reads 0xff on that board. On the other board,
> vid erroneously reports a voltage of 2.048V. I never thought about that
> until you brought it up.

2.050 V actually, this is 0x00 decoded as VRM 8.2. 

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 79/79] hwmon: (it87) Fix vrm value range

[Guenter Roeck]

On Thu, 2012-01-26 at 16:03 -0500, Jean Delvare wrote:
> On Wed, 25 Jan 2012 02:31:39 -0800, Guenter Roeck wrote:
> > On Wed, Jan 25, 2012 at 05:17:24AM -0500, Jean Delvare wrote:
> > > There are 3 different problems here.
> > > 
> > > Problem #1 is the bug in hwmon-vid which causes all unknown Intel
> > > family 6 processors to use VRM 8.2, even though this is wrong on any
> > > post-2007 CPU.
> > 
> > Working on fixing that. Looks like all the recent CPUs use VRD 11.0 or 11.1.
> > There seem to be some supporting VRD 12.0, (the SandyBridge chips using the
> > larger socket) but I did not find cpuid values for those.
> 
> There may be a problem with CPU models which can support an alternative
> VID encoding for compatibility. I seem to recall that the early VRD
> 11.0 CPUs could also output VRD 10.x, but I could never find out how to
> know when the actually did that.
> 
Doesn't sound good. Worse, recent CPUs don't seem to report the current
VID in MSR_IA32_PERF_STATUS anymore.

> > > Problem #2 is that unknown VRM for a recent CPU will only be notified
> > > to the kernel log, which regular users don't read. Then you indeed get
> > > a cpu0_vid value of 0. That's apparently what you were seeing on your
> > > system. It would certainly be better to _not_ expose the VID value at
> > > all if we don't know how to decode it.
> >
> > Actually, I have problem #1 on one board, and problem #3 on the other
> > (the one with SandyBridge). vid reads 0xff on that board. On the other board,
> > vid erroneously reports a voltage of 2.048V. I never thought about that
> > until you brought it up.
> 
> 2.050 V actually, this is 0x00 decoded as VRM 8.2. 
> 
You are right. Obviously 0x00 is just as bad as 0xff. This is with
Intel(R) Core(TM) i3 CPU 540  @ 3.07GHz and NCT6775F.

Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 79/79] hwmon: (it87) Fix vrm value range

[Jean Delvare]

On Thu, 26 Jan 2012 13:53:03 -0800, Guenter Roeck wrote:
> Doesn't sound good. Worse, recent CPUs don't seem to report the current
> VID in MSR_IA32_PERF_STATUS anymore.

I seem to remember that this register was never officially documented
as holding the VID value. Which is why the coretemp driver doesn't
support it. I'm really not sure what Intel is up to with this, it would
have been very easy and useful to properly implement and document this.

> (...)
> You are right. Obviously 0x00 is just as bad as 0xff.

The problem is that the VRM/VRD specs don't treat 0xff differently from
other codes. So while it looks highly suspicious when you see that, we
can't discard this value at driver level because it _could_ be correct,
according to the spec.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors