Module Name:    src
Committed By:   martin
Date:           Sat Jun 22 09:57:21 UTC 2024

Modified Files:
        src/sys/arch/x86/x86 [netbsd-10]: coretemp.c

Log Message:
Pull up following revision(s) (requested by gutteridge in ticket #717):

        sys/arch/x86/x86/coretemp.c: revision 1.40

        sys/arch/x86/x86/coretemp.c: revision 1.41

coretemp.c: fix grammar in a warning message
(I get several of these warnings on boot on a particular machine. Now,
it also seems that the code isn't retrieving the correct value, either;
TBD.)

coretemp.c: don't accept impossibly low TjMax values
r. 1.39 introduced a regression where instead of applying a reasonable
default maximum (as was done prior to that change), incorrect values
were accepted and applied, as failures to retrieve an expected MSR
value weren't accounted for.

Apply different logic for unexpectedly low vs. high maximums, with
distinct warnings for each. Also add another warning about a retrieval
failure right at the outset (which also just uses the default, then).

This change fundamentally doesn't address the fact that
__SHIFTOUT(msr, MSR_TEMP_TARGET_READOUT)
doesn't necessarily return a valid value. It just restores prior
behaviour, which is more reasonable than applying a zero value, which
started happening on some older hardware. (I infer this is most likely
an issue with dated generations of Intel hardware with this feature.)

The challenge is that this evidently isn't all documented properly
anywhere. Various "magic values" in this driver need further
investigation.

While here, also fix output so warnings are cleanly formatted, rather
than the slightly scrambled way they were appearing.

Tested on older Intel hardware I had on hand:
E7500 (now falls back to default 100 rather than 0)
E5540 (successfully retrieves 97, as before)
i5-3340M (successfully retrieves 105, as before)


To generate a diff of this commit:
cvs rdiff -u -r1.38.4.1 -r1.38.4.2 src/sys/arch/x86/x86/coretemp.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/sys/arch/x86/x86/coretemp.c
diff -u src/sys/arch/x86/x86/coretemp.c:1.38.4.1 src/sys/arch/x86/x86/coretemp.c:1.38.4.2
--- src/sys/arch/x86/x86/coretemp.c:1.38.4.1	Sat Jul 29 10:58:02 2023
+++ src/sys/arch/x86/x86/coretemp.c	Sat Jun 22 09:57:21 2024
@@ -1,4 +1,4 @@
-/* $NetBSD: coretemp.c,v 1.38.4.1 2023/07/29 10:58:02 martin Exp $ */
+/* $NetBSD: coretemp.c,v 1.38.4.2 2024/06/22 09:57:21 martin Exp $ */
 
 /*-
  * Copyright (c) 2011 The NetBSD Foundation, Inc.
@@ -61,7 +61,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: coretemp.c,v 1.38.4.1 2023/07/29 10:58:02 martin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: coretemp.c,v 1.38.4.2 2024/06/22 09:57:21 martin Exp $");
 
 #include <sys/param.h>
 #include <sys/device.h>
@@ -110,7 +110,7 @@ static int	coretemp_match(device_t, cfda
 static void	coretemp_attach(device_t, device_t, void *);
 static int	coretemp_detach(device_t, int);
 static int	coretemp_quirks(struct cpu_info *);
-static void	coretemp_tjmax(device_t);
+static int	coretemp_tjmax(device_t);
 static void	coretemp_refresh(struct sysmon_envsys *, envsys_data_t *);
 static void	coretemp_refresh_xcall(void *, void *);
 
@@ -194,9 +194,10 @@ coretemp_attach(device_t parent, device_
 	if (sysmon_envsys_register(sc->sc_sme) != 0)
 		goto fail;
 
-	coretemp_tjmax(self);
-	aprint_verbose(", Tjmax=%d", sc->sc_tjmax);
-	aprint_normal("\n");
+	if (coretemp_tjmax(self) == 0) {
+		aprint_verbose(", Tjmax=%d", sc->sc_tjmax);
+		aprint_normal("\n");
+	}
 	return;
 
 fail:
@@ -258,7 +259,7 @@ coretemp_quirks(struct cpu_info *ci)
 	return 1;
 }
 
-void
+static int
 coretemp_tjmax(device_t self)
 {
 	struct coretemp_softc *sc = device_private(self);
@@ -288,17 +289,19 @@ coretemp_tjmax(device_t self)
 		if ((model < 0x17) && ((msr & __BIT(28)) == 0))
 			goto notee;
 
-		if (rdmsr_safe(MSR_IA32_EXT_CONFIG, &msr) == EFAULT)
-			return;
+		if (rdmsr_safe(MSR_IA32_EXT_CONFIG, &msr) == EFAULT) {
+			aprint_normal("\n");
+			aprint_error_dev(sc->sc_dev,
+			    "Failed to read MSR_IA32_EXT_CONFIG MSR. "
+			    "Using default (%d)\n", sc->sc_tjmax);
+			return 1;
+		}
 
-		if ((msr & __BIT(30)) != 0) {
+		if ((msr & __BIT(30)) != 0)
 			sc->sc_tjmax = 85;
-			return;
-		}
 	} else if (model == 0x17 && stepping == 0x06) {
 		/* The mobile Penryn family. */
 		sc->sc_tjmax = 105;
-		return;
 	} else if (model == 0x1c) {
 		if (stepping == 0x0a) {
 			/* 45nm Atom D400, N400 and D500 series */
@@ -307,21 +310,39 @@ coretemp_tjmax(device_t self)
 			sc->sc_tjmax = 90;
 	} else {
 notee:
-		/* Attempt to get Tj(max) from IA32_TEMPERATURE_TARGET. */
+		/* 
+		 * Attempt to get Tj(max) from IA32_TEMPERATURE_TARGET.
+		 * It is not fully known which CPU models have the MSR.
+		 */
 		if (rdmsr_safe(MSR_TEMPERATURE_TARGET, &msr) == EFAULT) {
+			aprint_normal("\n");
 			aprint_error_dev(sc->sc_dev,
 			    "Failed to read TEMPERATURE_TARGET MSR. "
-			    "Use the default (%d)\n", sc->sc_tjmax);
-			return;
+			    "Using default (%d)\n", sc->sc_tjmax);
+			return 1;
 		}
 
 		tjmax = __SHIFTOUT(msr, MSR_TEMP_TARGET_READOUT);
-		if ((tjmax < TJMAX_LIMIT_LOW) || (tjmax > TJMAX_LIMIT_HIGH))
+		if (tjmax < TJMAX_LIMIT_LOW) {
+			aprint_normal("\n");
 			aprint_error_dev(sc->sc_dev,
-			    "WARNING: Tjmax(%d) might exceeded the limit.\n",
+			    "WARNING: Tjmax(%d) retrieved was below expected range, "
+				"using default (%d).\n", tjmax, sc->sc_tjmax);
+			return 1;
+		}
+
+		if (tjmax > TJMAX_LIMIT_HIGH) {
+			aprint_normal("\n");
+			aprint_error_dev(sc->sc_dev,
+			    "WARNING: Tjmax(%d) might exceed the limit.\n",
 			    tjmax);
+			sc->sc_tjmax = tjmax;
+			return 1;
+		}
 		sc->sc_tjmax = tjmax;
 	}
+
+	return 0;
 }
 
 static void

Reply via email to