Re: [PATCH v2] hwmon: (coretemp) Return -ENODATA for invalid temperature readings

[Guenter Roeck]

On Tue, Oct 06, 2026 at 06:04:56AM -0700, Ricardo Neri wrote:
> IA32_THERM_STATUS and IA32_PACKAGE_THERM_STATUS have a valid bit, bit 31.
> The Intel SDM documents it only for the former, but the package register
> implements it too: x86_pkg_temp_thermal has required it since commit
> f1a18a105660 ("Thermal: CPU Package temperature thermal").
> 
> This driver checked the bit for both registers as well, until
> commit bf6ea084ebb5 ("hwmon: (coretemp) Do not return -EAGAIN for low
> temperatures") made it ignore the bit because some CPUs clear it while
> the temperature is too low to be measured. Since then, temp%d_input
> reports such a reading as a real temperature. Userspace cannot tell it
> apart from a genuinely low one.
> 
> Fail the read with -ENODATA while the CPU reports the reading as invalid.
> On a CPU that clears the bit, reading temp%d_input now fails where it
> used to return a low value. sensors(1) shows N/A in its place.
> 
> Suggested-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> Link: https://lore.kernel.org/all/46f9f319-de71-412f-a424-6cb801478456@xxxxxxxxxxxx/
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@xxxxxxxxxxxxxxx>

Applied.

Thanks,
Guenter

> ---
> Changes in v2:
> - Patches 1 and 2 of v1 have been applied. This is a rework of patch 3.
> - Drop temp%d_fault and fail temp%d_input with -ENODATA instead. (Guenter)
> - Link to v1: https://lore.kernel.org/r/20260924-coretemp-temp-fault-v1-0-1884f0ff97d5@xxxxxxxxxxxxxxx
> ---
>  Documentation/hwmon/coretemp.rst |  3 +++
>  drivers/hwmon/coretemp.c         | 21 +++++++++++++++------
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> 
> ---
> base-commit: 4781ca52761e666cf18b591e6bb0478396c90320
> change-id: 20261003-coretemp-enodata-e424540e3b4c
> 
> Best regards,
> 
> diff --git a/Documentation/hwmon/coretemp.rst b/Documentation/hwmon/coretemp.rst
> index 349301683381..09f885a847e3 100644
> --- a/Documentation/hwmon/coretemp.rst
> +++ b/Documentation/hwmon/coretemp.rst
> @@ -50,6 +50,9 @@ All sysfs entries are named with their core_id (represented here by 'X').
>  
>  ================= ========================================================
>  tempX_input	  Core temperature (in millidegrees Celsius).
> +		  Reading fails with -ENODATA while the CPU reports the
> +		  temperature as invalid. Some CPUs do this when the
> +		  temperature is too low to be measured.
>  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..a9edbb4924a7 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -60,6 +60,8 @@ enum coretemp_attr_index {
>  
>  /*
>   * Per-Core Temperature Data
> + * @temp_valid: Whether the CPU reported @temp as valid. @temp is not
> + *		reported when this is false.
>   * @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 +74,7 @@ enum coretemp_attr_index {
>   */
>  struct temp_data {
>  	int temp;
> +	bool temp_valid;
>  	int tjmax;
>  	unsigned long last_updated;
>  	unsigned int cpu;
> @@ -398,7 +401,8 @@ static ssize_t show_temp(struct device *dev,
>  {
>  	struct msr val;
>  	struct temp_data *tdata = container_of(devattr, struct temp_data, sd_attrs[ATTR_TEMP]);
> -	int tjmax;
> +	int tjmax, temp;
> +	bool valid;
>  
>  	mutex_lock(&tdata->update_lock);
>  
> @@ -407,17 +411,22 @@ static ssize_t show_temp(struct device *dev,
>  		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.
> +		 * Bit 31 indicates whether the reading is valid. Some CPUs
> +		 * clear it while the temperature is too low to be measured.
> +		 * Such a reading is not a temperature. Do not report it.
>  		 */
> +		tdata->temp_valid = (val.l >> 31) & 1;
>  		tdata->temp = tjmax - ((val.l >> 16) & 0xff) * 1000;
>  		tdata->last_updated = jiffies;
>  	}
>  
> +	valid = tdata->temp_valid;
> +	temp = tdata->temp;
>  	mutex_unlock(&tdata->update_lock);
> -	return sprintf(buf, "%d\n", tdata->temp);
> +
> +	if (!valid)
> +		return -ENODATA;
> +	return sprintf(buf, "%d\n", temp);
>  }
>  
>  static int create_core_attrs(struct temp_data *tdata, struct device *dev)