[PATCH 3/3] hwmon: (coretemp) Add temp%d_fault sysfs attribute

[Ricardo Neri <ricardo.neri-calderon@linux.intel.com>]

The temp%d_input sysfs attribute reports the CPU core temperature
regardless of the valid bit (bit 31) in IA32_[PACKAGE]_THERM_STATUS.
Commit bf6ea084ebb5 ("hwmon: (coretemp) Do not return -EAGAIN for low
temperatures") established this behavior after observing that some Intel
CPUs do not set the valid bit while the temperature is too low to be
measured. The value reported is of more use than an error. Changing this is
not an option: the interface is long-established and userspace relies on
it never failing.

Userspace, however, cannot distinguish a genuinely low temperature from
one the CPU could not measure. Until that commit it could: reading
temp%d_input returned -EAGAIN. That signal was removed and nothing
replaced it. Report the inverse of the valid bit through the standard hwmon
temp%d_fault attribute. Its documented meaning is that the measurement
for that channel should not be trusted. Tools built on libsensors consume
the standard attribute.

temp%d_input continues to ignore the valid bit. A new update_temp() helper
refreshes the cached temperature and its fault state together, so both
attributes are served from the same reading of the status register.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 Documentation/hwmon/coretemp.rst |  4 +++
 drivers/hwmon/coretemp.c         | 68 +++++++++++++++++++++++++++-------------
 2 files changed, 51 insertions(+), 21 deletions(-)

diff --git a/Documentation/hwmon/coretemp.rst b/Documentation/hwmon/coretemp.rst
index 349301683381..e3e3205b5155 100644
--- a/Documentation/hwmon/coretemp.rst
+++ b/Documentation/hwmon/coretemp.rst
@@ -50,6 +50,10 @@ All sysfs entries are named with their core_id (represented here by 'X').
 
 ================= ========================================================
 tempX_input	  Core temperature (in millidegrees Celsius).
+tempX_fault	  Set when the CPU reports the reading in tempX_input as
+		  invalid. Some CPUs do this while the temperature is too
+		  low to be measured. tempX_input still reports a value in
+		  this state; it is low or zero.
 tempX_max	  Maximum recommended operating temperature (Tcontrol).
 		  All cooling devices should be turned on.
 tempX_crit	  Maximum junction temperature (in millidegrees Celsius).
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 0ab6bbff5637..e0fb7c0ef4d0 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -45,6 +45,7 @@ enum coretemp_attr_index {
 	ATTR_LABEL,
 	ATTR_CRIT_ALARM,
 	ATTR_TEMP,
+	ATTR_TEMP_FAULT,
 	ATTR_TJMAX,
 	ATTR_TTARGET,
 	MAX_CORE_ATTRS = ATTR_TJMAX + 1,	/* Maximum no of basic attrs */
@@ -60,6 +61,9 @@ enum coretemp_attr_index {
 
 /*
  * Per-Core Temperature Data
+ * @temp_fault: The temperature in @temp is not to be trusted. Mirrors the
+ *		meaning of the tempX_fault attribute, and is therefore the
+ *		inverse of the valid bit the CPU reports in @status_reg.
  * @tjmax: The static tjmax value when tjmax cannot be retrieved from
  *		IA32_TEMPERATURE_TARGET MSR.
  * @last_updated: The time when the current temperature value was updated
@@ -72,6 +76,7 @@ enum coretemp_attr_index {
  */
 struct temp_data {
 	int temp;
+	bool temp_fault;
 	int tjmax;
 	unsigned long last_updated;
 	unsigned int cpu;
@@ -393,41 +398,62 @@ static ssize_t show_ttarget(struct device *dev,
 	return sprintf(buf, "%d\n", ttarget);
 }
 
-static ssize_t show_temp(struct device *dev,
-			 struct device_attribute *devattr, char *buf)
+/* Requires tdata->update_lock to be held. */
+static void update_temp(struct temp_data *tdata, struct device *dev)
 {
 	struct msr val;
-	struct temp_data *tdata = container_of(devattr, struct temp_data, sd_attrs[ATTR_TEMP]);
 	int tjmax;
 
-	mutex_lock(&tdata->update_lock);
-
 	/* Check whether the time interval has elapsed */
-	if (time_after(jiffies, tdata->last_updated + HZ)) {
-		tjmax = get_tjmax(tdata, dev);
-		rdmsrq_on_cpu(tdata->cpu, tdata->status_reg, &val.q);
-		/*
-		 * Ignore the valid bit. In all observed cases the register
-		 * value is either low or zero if the valid bit is 0.
-		 * Return it instead of reporting an error which doesn't
-		 * really help at all.
-		 */
-		tdata->temp = tjmax - ((val.l >> 16) & 0xff) * 1000;
-		tdata->last_updated = jiffies;
-	}
+	if (!time_after(jiffies, tdata->last_updated + HZ))
+		return;
+
+	tjmax = get_tjmax(tdata, dev);
+
+	rdmsrq_on_cpu(tdata->cpu, tdata->status_reg, &val.q);
+
+	/*
+	 * Cache the temperature even when the CPU reports it as invalid. In
+	 * all observed cases the register value is either low or zero if the
+	 * valid bit is 0, which is of more use than reporting an error.
+	 * tempX_fault tells userspace when the temperature is unreliable.
+	 */
+	tdata->temp_fault = !((val.l >> 31) & 1);
+	tdata->temp = tjmax - ((val.l >> 16) & 0xff) * 1000;
+	tdata->last_updated = jiffies;
+}
+
+static ssize_t show_temp(struct device *dev,
+			 struct device_attribute *devattr, char *buf)
+{
+	struct temp_data *tdata = container_of(devattr, struct temp_data, sd_attrs[ATTR_TEMP]);
+
+	guard(mutex)(&tdata->update_lock);
+	update_temp(tdata, dev);
 
-	mutex_unlock(&tdata->update_lock);
 	return sprintf(buf, "%d\n", tdata->temp);
 }
 
+static ssize_t show_temp_fault(struct device *dev,
+			       struct device_attribute *devattr, char *buf)
+{
+	struct temp_data *tdata = container_of(devattr, struct temp_data,
+					       sd_attrs[ATTR_TEMP_FAULT]);
+
+	guard(mutex)(&tdata->update_lock);
+	update_temp(tdata, dev);
+
+	return sprintf(buf, "%d\n", tdata->temp_fault);
+}
+
 static int create_core_attrs(struct temp_data *tdata, struct device *dev)
 {
 	static ssize_t (*const rd_ptr[TOTAL_ATTRS]) (struct device *dev,
 			struct device_attribute *devattr, char *buf) = {
-			show_label, show_crit_alarm, show_temp, show_tjmax,
-			show_ttarget };
+			show_label, show_crit_alarm, show_temp, show_temp_fault,
+			show_tjmax, show_ttarget };
 	static const char *const suffixes[TOTAL_ATTRS] = {
-		"label", "crit_alarm", "input", "crit", "max"
+		"label", "crit_alarm", "input", "fault", "crit", "max"
 	};
 	int i;
 

-- 
2.43.0