Re: [PATCH v5 11/13] drm/xe/hwmon: Decode CRI temperature registers as IEEE-754

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hwmon: xe: incorrect IEEE-754 decoding applied to VRAM_N on Crescent Island
--

commit 8039331718fb03f6dfcbe8bbe7a3375383a0c5d2
Author: Karthik Poosa <karthik.poosa@intel.com>

drm/xe/hwmon: Decode CRI temperature registers as IEEE-754

This commit decodes the package and VRAM temperatures on Crescent Island
from an IEEE-754 floating-point format to integer millidegrees.

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index e1cff5700e7f9..c411cd375aa1b 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c

[ ... ]

> @@ -1249,26 +1312,27 @@ xe_hwmon_temp_read(struct xe_hwmon *hwmon, u32 attr, int channel, long *val)
>  	switch (attr) {
>  	case hwmon_temp_input:
>  		switch (channel) {
> -		case CHANNEL_PKG:
> -		case CHANNEL_VRAM:
> -			reg_val = xe_mmio_read32(mmio, xe_hwmon_get_reg(hwmon, REG_TEMP, channel));
> -
> -			/* HW register value is in degrees Celsius, convert to millidegrees. */
> -			*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
> -			return 0;
>  		case CHANNEL_MCTRL:
>  			return get_mc_temp(hwmon, val);
>  		case CHANNEL_PCIE:
>  			return get_pcie_temp(hwmon, val);
> +		case CHANNEL_PKG:
> +		case CHANNEL_VRAM:
>  		case CHANNEL_VRAM_N...CHANNEL_VRAM_N_MAX:
>  			reg_val = xe_mmio_read32(mmio, xe_hwmon_get_reg(hwmon, REG_TEMP, channel));
> -			/*
> -			 * This temperature format is 24 bit [31:8] signed integer and 8 bit
> -			 * [7:0] fraction.
> -			 */
> -			*val = (s32)(REG_FIELD_GET(TEMP_MASK_VRAM_N, reg_val)) *
> -				(REG_FIELD_GET(TEMP_SIGN_MASK, reg_val) ? -1 : 1) *
> -				 MILLIDEGREE_PER_DEGREE;
> +			if (hwmon->xe->info.platform >= XE_CRESCENTISLAND) {
> +				return xe_hwmon_temperature_decode((u32)reg_val, val);

[Severity: Medium]
Will this indiscriminately apply IEEE-754 decoding to all VRAM channels on
Crescent Island, including CHANNEL_VRAM_N?

Looking at xe_hwmon_get_reg(), VRAM_N on XE_CRESCENTISLAND still uses
the legacy BMG_VRAM_TEMPERATURE_N registers:

	case REG_TEMP:
		if (xe->info.platform == XE_CRESCENTISLAND) {
			if (channel == CHANNEL_PKG)
				...
			else if (in_range(channel, CHANNEL_VRAM_N, hwmon->temp.vram_count) &&
				 xe_hwmon_vram_channel_enabled(hwmon, channel - CHANNEL_VRAM_N))
				return BMG_VRAM_TEMPERATURE_N(channel - CHANNEL_VRAM_N);

Since BMG_VRAM_TEMPERATURE_N registers encode temperatures in a 24.8
fixed-point format, parsing them as IEEE-754 floating-point values will
result in incorrectly reporting sensor values when reading tempN_input via
sysfs.

Could this condition be narrowed to exclude CHANNEL_VRAM_N?

> +			} else if (channel >= CHANNEL_VRAM_N) {
> +				/*
> +				 * This temperature format is 24 bit [31:8] signed integer and 8 bit
> +				 * [7:0] fraction for platforms before CRI.
> +				 */
> +				*val = (s32)REG_FIELD_GET(TEMP_MASK_VRAM_N, reg_val) *
> +					(REG_FIELD_GET(TEMP_SIGN_MASK, reg_val) ? -1 : 1) *
> +					MILLIDEGREE_PER_DEGREE;
> +			} else {
> +				*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
> +			}
>  			return 0;
>  		default:
>  			return -EOPNOTSUPP;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924111156.2628915-1-karthik.poosa@intel.com?part=11