k8temp warn about errata

Following patch adds warning about wrong CPU temperature readouts on all fam f rev f revision of CPUs.

Used switch statement, more code changes follows.

Signed-off-by Rudolf Marek <[email protected]>

It should "fix" http://bugzilla.kernel.org/show_bug.cgi?idˆ66

Index: linux-2.6.27-rc7/drivers/hwmon/k8temp.c
=================================> --- linux-2.6.27-rc7.orig/drivers/hwmon/k8temp.c 2008-09-28 11:01:45.855284456 +0200
+++ linux-2.6.27-rc7/drivers/hwmon/k8temp.c 2008-09-28 11:13:42.396117790 +0200

drivers/hwmon/k8temp.c +2 -0

@@ -34,6 +34,7 @@
 
 #define TEMP_FROM_REG(val)	(((((val) >> 16) & 0xff) - 49) * 1000)
 #define REG_TEMP	0xe4
+#define REG_CPUID	0xfc
 #define SEL_PLACE	0x40
 #define SEL_CORE	0x04
 

@@ -47,6 +48,7 @@
 	/* registers values */
 	u8 sensorsp;		/* sensor presence bits - SEL_CORE & SEL_PLACE */
 	u32 temp[2][2];		/* core, place */
+	u8 fam;
 };
 
 static struct k8temp_data *k8temp_update_device(struct device *dev)

Hi Rudolf,

Quoted 8 lines · Rudolf Marek

Following patch adds warning about wrong CPU temperature readouts on all fam f rev f revision of CPUs.

Used switch statement, more code changes follows.

Signed-off-by Rudolf Marek <[email protected]>

It should "fix" http://bugzilla.kernel.org/show_bug.cgi?idˆ66

Thanks for working on this. Review:

Quoted 20 lines

Index: linux-2.6.27-rc7/drivers/hwmon/k8temp.c
=================================>> --- linux-2.6.27-rc7.orig/drivers/hwmon/k8temp.c 2008-09-28 11:01:45.855284456 +0200
+++ linux-2.6.27-rc7/drivers/hwmon/k8temp.c 2008-09-28 11:13:42.396117790 +0200

drivers/hwmon/k8temp.c +2 -0

@@ -34,6 +34,7 @@
 
 #define TEMP_FROM_REG(val)   (((((val) >> 16) & 0xff) - 49) * 1000)
 #define REG_TEMP     0xe4
+#define REG_CPUID    0xfc
 #define SEL_PLACE    0x40
 #define SEL_CORE     0x04
 

@@ -47,6 +48,7 @@
      /* registers values */
      u8 sensorsp;            /* sensor presence bits - SEL_CORE & SEL_PLACE */
      u32 temp[2][2];         /* core, place */
+     u8 fam;
 };
 
 static struct k8temp_data *k8temp_update_device(struct device *dev)

drivers/hwmon/k8temp.c line 149

Quoted 7 lines

drivers/hwmon/k8temp.c +1 -0

@@ -141,6 +143,7 @@
      int err;
      u8 scfg;
      u32 temp;
+
      struct k8temp_data *data;
      u32 cpuid = cpuid_eax(1);
 

Unrelated whitespace change, please revert.

drivers/hwmon/k8temp.c line 161

Quoted 5 lines

drivers/hwmon/k8temp.c +1 -0

@@ -155,6 +158,18 @@
              goto exit;
      }
 
+     /* get real PCI based cpuid, prior revF of fam 0Fh, this reg is 0 */

That's tricky. It took me some time to figure out why the warning wouldn't be displayed on all family 0Fh CPUs ;)

drivers/hwmon/k8temp.c line 165

Quoted 4 lines

drivers/hwmon/k8temp.c +4 -0

+     pci_read_config_dword(pdev, REG_CPUID, &cpuid);
+
+     data->fam = (cpuid & 0x00000f00) >> 8;
+     data->fam += (cpuid & 0x00f00000) >> 20;

The extended family code is an 8-bit value, so this should be:

+ data->fam += (cpuid & 0x0ff00000) >> 20;

I don't know if the result is guaranteed to fit in a u8.

drivers/hwmon/k8temp.c line 168

Quoted 3 lines

drivers/hwmon/k8temp.c +3 -0

+
+     switch (data->fam) {
+             case 0xf:

Coding style: case is normally aligned on switch (see Chapter 1:
Indentation of Documentation/CodingStyle.)

drivers/hwmon/k8temp.c line 175

Quoted 7 lines

drivers/hwmon/k8temp.c +4 -0

+                     dev_warn(&pdev->dev, "Temperature readouts might be wrong"
+                                             " - check errata #141\n");
+     }
+
      pci_read_config_byte(pdev, REG_TEMP, &scfg);
      scfg &= ~(SEL_PLACE | SEL_CORE);                /* Select sensor 0, core0 */
      pci_write_config_byte(pdev, REG_TEMP, scfg);

This is a good start, but I don't think this is enough. I don't expect desktop users to search the kernel logs for that kind of information. The situation is made worse by the fact that the k8temp driver is one of the few hwmon drivers which auto-loads, so many users see the reported temperature, not just those who ran sensors-detect. So I'd rather implement a more "active" mechanism. I have several ideas in mind and would welcome comments on them.

If all revision F and later CPUs are affected by the errata and the temperature reported is never correct, we should simply blacklist these CPUs. I guess this isn't the case, otherwise you'd have proposed that we do that long ago.

If most but not all of these CPUs are affected, then it would make sense to disable them by default but give the user a chance to still enable them (using a module parameter.)

If there are more revision F and later CPUs with working thermal diodes, and working ones can be told from non-working ones based on the exact revision, we could implement blacklisting and/or whitelisting base on the revision.

Does anyone have an idea about the ratio of working/broken revision F and later CPU models?

Does the status (broken or not) of the thermal diode depend on the exact CPU revision, or is it purely random?

Alternatively, or additionally, we could use a heuristic to detect broken diodes. I don't much like this in general, as monitoring should normally not assume anything on what it is monitoring, but in this specific case I think it makes some sense because the thermal sensor in question is not a generic chip and brokenness is so widely spread.

When looking at reports for broken K8 diodes, for example at:
http://bugzilla.kernel.org/show_bug.cgi?idˆ66
it is immediately visible to a human that:

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:  +17 C
Core0 Temp:   +3 C
Core1 Temp:  +21 C
Core1 Temp:   +5 C

are NOT reasonable temperatures for a CPU.

Other obviously wrong outputs (gathered on the lm-sensors mailing list):

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:   +4°C
Core0 Temp:   -1°C
Core1 Temp:   -2°C
Core1 Temp:  -20°C

k8temp-pci-00c3

Adapter: PCI adapter
temp1:        -9°C
temp2:       -20°C
temp3:       -18°C
temp4:       -14°C

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:    -6°C
Core0 Temp:   -11°C
Core1 Temp:    +5°C
Core1 Temp:   -13°C

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:   -1°C
Core0 Temp:   +1°C
Core1 Temp:   +6°C
Core1 Temp:   +6°C

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:   +9°C
Core0 Temp:   +0°C
Core1 Temp:   +3°C
Core1 Temp:  +11°C

The following outputs, OTOH, look pretty good (also gathered on the lm-sensors mailing list):

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:  +42.0°C
Core0 Temp:  +39.0°C
Core1 Temp:  +36.0°C
Core1 Temp:  +39.0°C

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:  +43.0°C
Core1 Temp:  +37.0°C

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:  +41.0°C
Core1 Temp:  +46.0°C

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:  +38.0°C
Core1 Temp:  +25.0°C

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:  +31.0°C
Core1 Temp:  +32.0°C

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:  +32°C
Core1 Temp:  +35°C

k8temp-pci-00c3

Adapter: PCI adapter
Core1 Temp:   +43°C
Core2 Temp:   +46°C

There were also a few reports which I'm not sure if there are good or bad:

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:  +26 C
Core1 Temp:  +24 C

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:  +17°C
Core1 Temp:  +25°C

So I am under the impression that it should not be too difficult to come up with a heuristic to spot obviously broken sensors:
* Apparently all affected CPUs have 4 thermal sensors (I don't know if this is because all revision F and later CPUs have 4 thermal sensors?)
* Average of the 4 sensors is below 15 degrees C.
* Minimum of the 4 sensors is below 5 degrees C

So we could build a heuristic based on the number of sensors and either the average or the minimum temperature and reject by default all revision F and later CPUs which do not pass the test. We would of course provide a way (module parameter) for the user to change the temperature limit or maybe disable the heuristic entirely.

Of course this isn't a perfect solution, as there are still CPUs in the gray zone for which no heuristic can say whether they are good or bad. Also, there is a small risk that a CPU which is at the limit will be rejected sometimes but not always, so hwmon device numbering may change from one boot to the next. However, I think that a good heuristic would solve way more problems than it would cause.

Comments anyone?

Thanks,
--
Jean Delvare

If all revision F and later CPUs are affected by the errata and the temperature reported is never correct, we should simply blacklist these CPUs. I guess this isn't the case, otherwise you'd have proposed that we do that long ago.

If most but not all of these CPUs are affected, then it would make sense to disable them by default but give the user a chance to still enable them (using a module parameter.)

If there are more revision F and later CPUs with working thermal diodes, and working ones can be told from non-working ones based on the exact revision, we could implement blacklisting and/or whitelisting base on the revision.

Does anyone have an idea about the ratio of working/broken revision F and later CPU models?

Does the status (broken or not) of the thermal diode depend on the exact CPU revision, or is it purely random?

Alternatively, or additionally, we could use a heuristic to detect broken diodes. I don't much like this in general, as monitoring should normally not assume anything on what it is monitoring, but in this specific case I think it makes some sense because the thermal sensor in question is not a generic chip and brokenness is so widely spread.

Sound like a good idea to me

For those values:

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:  +26 C
Core1 Temp:  +24 C

k8temp-pci-00c3

Adapter: PCI adapter
Core0 Temp:  +17°C
Core1 Temp:  +25°C

I think they are correct. In winter, I place my computer outside (-10C/-15C). The idle core temp can be pretty low.

So it's important
to give the user a way to force the sensor activation or adjust the temperatures level for the heuristic.

So in this statement:
* Average of the 4 sensors is below 15 degrees C.
* Minimum of the 4 sensors is below 5 degrees C

The user need to be able to update 15 and 5.

Also, since in all examples the average temperature is lower than 15 degrees C (-4,75; -15,25; -6,25; 3; 5,75) I will recommand to put it closer to 10 degrees C.

Thanks for the review. I'm attaching fixed version.

Following patch adds warning about wrong CPU temperature readouts on all fam f rev f revision of CPUs.

Used switch statement, more code changes follows.

Signed-off-by Rudolf Marek <[email protected]>

Quoted 4 lines

If all revision F and later CPUs are affected by the errata and the temperature reported is never correct, we should simply blacklist these CPUs. I guess this isn't the case, otherwise you'd have proposed that we do that long ago.

Yes.

Quoted 3 lines

If most but not all of these CPUs are affected, then it would make sense to disable them by default but give the user a chance to still enable them (using a module parameter.)

I tried hard to get this info from AMD. All versions are affected, but some may be fixed/not tested.

Quoted 4 lines

If there are more revision F and later CPUs with working thermal diodes, and working ones can be told from non-working ones based on the exact revision, we could implement blacklisting and/or whitelisting base on the revision.

The errata is for all revs.

Quoted 2 lines

Does anyone have an idea about the ratio of working/broken revision F and later CPU models?

Dont sorry.

Quoted 2 lines

Does the status (broken or not) of the thermal diode depend on the exact CPU revision, or is it purely random?

Perhaps random.

Quoted 5 lines

Alternatively, or additionally, we could use a heuristic to detect broken diodes. I don't much like this in general, as monitoring should normally not assume anything on what it is monitoring, but in this specific case I think it makes some sense because the thermal sensor in question is not a generic chip and brokenness is so widely spread.

I agree. This could work. However I would like to first get the fam 10h and fam 11h support in, because otherwise I would need to rewrite the patches again and changing lot of at the same time is no good.

So please can you accept the patch as it is now and have a look on second one too? I fixed the case indent and we need to write some docs too, but first check code.

Andreas can you test please the second patch too? I'm especially curious if you see the high limit when running sensors command.

Once second patch is in, I would like to rewrite the driver with the help of heuristic and put there more generic logic for "place/core" selectors and temp1_ etc sysfs files creation logic.

Hi Rudolf,

Quoted 8 lines · Rudolf Marek

Thanks for the review. I'm attaching fixed version.

Following patch adds warning about wrong CPU temperature readouts on all fam f rev f revision of CPUs.

Used switch statement, more code changes follows.

Signed-off-by Rudolf Marek <[email protected]>

I've applied it, thanks. Note that I added a "break" at the end of the case 0xf, otherwise it's an invitation to get things wrong in the next patch...

Quoted 12 lines

Quoted 4 lines

If all revision F and later CPUs are affected by the errata and the temperature reported is never correct, we should simply blacklist these CPUs. I guess this isn't the case, otherwise you'd have proposed that we do that long ago.

Yes.

Quoted 3 lines

If most but not all of these CPUs are affected, then it would make sense to disable them by default but give the user a chance to still enable them (using a module parameter.)

I tried hard to get this info from AMD. All versions are affected, but some may be fixed/not tested.

Quoted 5 lines

Quoted 4 lines

If there are more revision F and later CPUs with working thermal diodes, and working ones can be told from non-working ones based on the exact revision, we could implement blacklisting and/or whitelisting base on the revision.

The errata is for all revs.

For what it's worth, Jordan Crouse seems to think that blacklisting on a per-revision basis may still work.

Quoted 2 lines · Jean Delvare

For what it's worth, Jordan Crouse seems to think that blacklisting on a per-revision basis may still work.

I think it can. A much larger sample would probably need to be taken to be completely sure - but I hope that we'll find that the problem is deterministic enough for a blacklist. I think we would agree that a blacklist would be the more user friendly solution.

Hi Rudolf,

Quoted 5 lines · Rudolf Marek

Thanks for the review. I'm attaching fixed version.

Following patch adds warning about wrong CPU temperature readouts on all fam f rev f revision of CPUs.

Quoted 3 lines

Used switch statement, more code changes follows.

Signed-off-by Rudolf Marek <[email protected]>

I've applied it, thanks. Note that I added a "break" at the end of the case 0xf, otherwise it's an invitation to get things wrong in the next patch...

your git tree is at git://jdelvare.pck.nerim.net/jdelvare-2.6, correct? At least that's what you have used in your pull requests.

Did you apply Rudolf's patch to this git tree?

I just want to know against which tree I need to create patches when adapting k8temp.

Quoted 8 lines · Jordan Crouse

Quoted 2 lines · Jean Delvare

For what it's worth, Jordan Crouse seems to think that blacklisting on a per-revision basis may still work.

I think it can. A much larger sample would probably need to be taken to be completely sure - but I hope that we'll find that the problem is deterministic enough for a blacklist. I think we would agree that a blacklist would be the more user friendly solution.

OK, but then we should probably extend Rudolf's patch to ask users potentially affected by the errata to report to us. The report should include the CPUID information (e.g. contents of /proc/cpuinfo), the output of sensors (or the raw temperature values from sysfs), and whether or not the user thinks the temperature values are correct.

Quoted 3 lines

If most but not all of these CPUs are affected, then it would make sense to disable them by default but give the user a chance to still enable them (using a module parameter.)

I tried hard to get this info from AMD. All versions are affected, but some may be fixed/not tested.

Quoted 4 lines

If there are more revision F and later CPUs with working thermal diodes, and working ones can be told from non-working ones based on the exact revision, we could implement blacklisting and/or whitelisting base on the revision.

The errata is for all revs.

Quoted 2 lines

Does anyone have an idea about the ratio of working/broken revision F and later CPU models?

Dont sorry.

Andreas can you test please the second patch too? I'm especially curious if you see the high limit when running sensors command.

Once second patch is in, I would like to rewrite the driver with the help of heuristic and put there more generic logic for "place/core" selectors and temp1_ etc sysfs files creation logic.

Quoted 10 lines · Jean Delvare

Quoted 8 lines · Jordan Crouse

Quoted 2 lines · Jean Delvare

For what it's worth, Jordan Crouse seems to think that blacklisting on a per-revision basis may still work.

I think it can. A much larger sample would probably need to be taken to be completely sure - but I hope that we'll find that the problem is deterministic enough for a blacklist. I think we would agree that a blacklist would be the more user friendly solution.

Quoted 5 lines

OK, but then we should probably extend Rudolf's patch to ask users potentially affected by the errata to report to us. The report should include the CPUID information (e.g. contents of /proc/cpuinfo), the output of sensors (or the raw temperature values from sysfs), and whether or not the user thinks the temperature values are correct.

How do you find out whether your system is affected by this erratum? The erratum is: due to inaccuracy of reported temperature (doesn't meet a certain accuracy threshold) triggering of HTC/STC feature is inaccurate.

I am just curious how you'd like to determine the accuracy of the thermal sensor ... As an example, if the sensor reports 70 degC when the true temperature is 65 degC -- is it worth to blacklist it?

Jordan, do you know more details about the deviation of the reported temparature sensor values from the real ones?

I'd prefer not to blacklist but to keep the warning about potential inaccurate temperature values as introduced by Rudolf. I use k8temp on my private machines -- Athlon X2 and a Turion X2 (both are revF CPUs and thus affected by erratum 141). I admit, this is more or less a gimmick but I would miss it (if blacklisted).

Quoted 3 lines · Andreas Herrmann

I am just curious how you'd like to determine the accuracy of the thermal sensor ... As an example, if the sensor reports 70 degC when the true temperature is 65 degC -- is it worth to blacklist it?

If we are 100% certain that this is the case then yes, I would blacklist it.

The whole point of hardware monitoring is to report accurate values.

Additional software can be used to take actions based on temperature values, for example fan speed regulation or CPU frequency changes. If you can't trust the temperature readings then these operations become dangerous.

That being said, I guess that the example above is essentially theoretical? Most cases we've seen so far were not off by 5 degrees. They were plain wrong, with reported temperatures being in the -20 to +15 degrees C.

Quoted 8 lines

Jordan, do you know more details about the deviation of the reported temparature sensor values from the real ones?

I'd prefer not to blacklist but to keep the warning about potential inaccurate temperature values as introduced by Rudolf. I use k8temp on my private machines -- Athlon X2 and a Turion X2 (both are revF CPUs and thus affected by erratum 141). I admit, this is more or less a gimmick but I would miss it (if blacklisted).

Whatever we end up with, we will add a module parameter to let the user force the driver binding, exactly for advanced users like you.

My point with the blacklist is that we should not report knowingly incorrect values to the user _by default_, especially given that the k8temp driver loads automatically. I think it is better to not report anything by default rather than potentially wrong values. But I also agree that we must provide a way to bypass the tests, if nothing else, because our blacklist or heuristic may be incorrect.

Quoted 6 lines · Jean Delvare

The whole point of hardware monitoring is to report accurate values.

Additional software can be used to take actions based on temperature values, for example fan speed regulation or CPU frequency changes. If you can't trust the temperature readings then these operations become dangerous.

Then we need to blacklist it. The suggested workaround for this erratum is to use "temperature measurements from an analog thermal diode" for thermal control ...
If the system should be designed not to rely on the thermal sensor (due to accuracy issue) the OS shouldn't use it either.

Quoted 4 lines

That being said, I guess that the example above is essentially theoretical? Most cases we've seen so far were not off by 5 degrees. They were plain wrong, with reported temperatures being in the -20 to +15 degrees C.

Was this observed with k8temp or other sensor chips?

Hi Rudolf,

drivers/hwmon/k8temp.c line 162

Quoted 9 lines · Rudolf Marek

Index: linux-2.6.27-rc7/drivers/hwmon/k8temp.c
=================================> > --- linux-2.6.27-rc7.orig/drivers/hwmon/k8temp.c 2008-09-28 11:01:45.855284456 +0200
+++ linux-2.6.27-rc7/drivers/hwmon/k8temp.c 2008-09-28 11:13:42.396117790 +0200

drivers/hwmon/k8temp.c +2 -0

@@ -155,6 +158,18 @@
 		goto exit;
 	}
 
+	/* get real PCI based cpuid, prior revF of fam 0Fh, this reg is 0 */
+	pci_read_config_dword(pdev, REG_CPUID, &cpuid);

I am just curious whether you have tested this on a CPU revision prior revF. Because "BIOS and Kernel Developer's Guide for AMD Athlon 64 and AMD Opteron Processors" suggests that this register exists for those older CPUs:

"CPUID Fn[8000_0001,0000_0001]_EAX Family, Model, Feature Identifiers

This register provides identical information to Function 3, Offset FCh."

(I don't have access to such a CPU model at the moment and thus can't double-check this now.)

Quoted 8 lines · Jean Delvare

Quoted 7 lines · Jordan Crouse

Quoted 2 lines · Jean Delvare

For what it's worth, Jordan Crouse seems to think that blacklisting on a per-revision basis may still work.

I think it can. A much larger sample would probably need to be taken to be completely sure - but I hope that we'll find that the problem is deterministic enough for a blacklist. I think we would agree that a blacklist would be the more user friendly solution.

Quoted 5 lines

OK, but then we should probably extend Rudolf's patch to ask users potentially affected by the errata to report to us. The report should include the CPUID information (e.g. contents of /proc/cpuinfo), the output of sensors (or the raw temperature values from sysfs), and whether or not the user thinks the temperature values are correct.

How do you find out whether your system is affected by this erratum? The erratum is: due to inaccuracy of reported temperature (doesn't meet a certain accuracy threshold) triggering of HTC/STC feature is inaccurate.

I am just curious how you'd like to determine the accuracy of the thermal sensor ... As an example, if the sensor reports 70 degC when the true temperature is 65 degC -- is it worth to blacklist it?

Jordan, do you know more details about the deviation of the reported temparature sensor values from the real ones?

I'd prefer not to blacklist but to keep the warning about potential inaccurate temperature values as introduced by Rudolf. I use k8temp on my private machines -- Athlon X2 and a Turion X2 (both are revF CPUs and thus affected by erratum 141). I admit, this is more or less a gimmick but I would miss it (if blacklisted).

Nothing scientific. The AMD errata team might know more about it. I can identify the obviously broken sensors - my athlon X2 system for example tells me the cores are 7C and 3C respectfully, but I don't know if you could tell the difference between a well working sensor and a marginally working sensor, especially with differing work conditions. The best you could do is to figure out how cold the core could possibly run, and then omit anything under that.

You might do a better job if you could compare the core temperature against the system monitor - they should only differ by a few degrees (I think there is some math about how much the external and internal diodes should differ). That said, thats not the sort of math you could do in the kernel driver, you would need the user land to find the other sensor and do the calculations.