Re: [PATCH v2 4/5] drm/xe/hwmon: Decode CRI temperature registers as IEEE-754

["Poosa, Karthik" <karthik.poosa@intel.com>]

On 02-09-2026 23:38, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Missing bounds check for negative shift values leads to integer overflow on 32-bit systems.
> - [Medium] Missing REG_TEMP register definitions for XE_CRESCENTISLAND causes temperature attributes to be hidden and new decoding logic to be dead code.
> --
>
> commit 0e96a51c64ab3a240ac604df829dbc789366c5c7
> Author: Karthik Poosa <karthik.poosa@intel.com>
> Subject: drm/xe/hwmon: Decode CRI temperature registers as IEEE-754
>
> This commit adds support for decoding Crescent Island (CRI) hardware
> temperature registers, which encode package and VRAM temperatures as
> IEEE-754 floating-point values. It introduces an integer-based decoder
> to properly convert and expose these values through the hwmon subsystem.
>
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
>> index 3a675d7b6758b..c2b8d4c79783a 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> [ ... ]
>> +static int xe_hwmon_temperature_decode(u32 reg_val, long *temperature)
>> +{
>> +	u32 exponent = (reg_val >> 23) & 0xff;
>> +	u32 mantissa = reg_val & 0x7fffff;
>> +	s64 value;
>> +	int shift;
> [ ... ]
>> +	value = (s64)(mantissa | BIT(23)) * MILLIDEGREE_PER_DEGREE;
>> +	shift = exponent - 150;
>> +	if (shift >= 0) {
>> +		if (shift >= BITS_PER_LONG || value > (LONG_MAX >> shift))
>> +			return -ERANGE;
>> +		value <<= shift;
>> +	} else if (shift > -64) {
>> +		value >>= -shift;
> [Severity: Medium]
> Does this negative shift path need bounds checking for 32-bit systems?
>
> Since long is 32 bits on those systems, the computed value before the
> shift might require up to 34 bits. If the exponent is 148 or 149
> (resulting in a shift of -2 or -1), the right-shifted value can still
> take 32 or 33 bits, exceeding LONG_MAX.
>
> This would cause the value to silently overflow when assigned to
> long *temperature, resulting in a wrapped negative temperature instead of
> returning an error.

Below check shall be added for this in next revision


+       /* Ensure the magnitude fits in long, which is 32-bit on some 
arches. */
+       if (value > LONG_MAX)
+               return -ERANGE;

>> +	} else {
>> +		value = 0;
>> +	}
>> +
>> +	if (reg_val & BIT(31))
>> +		value = -value;
>> +
>> +	*temperature = value;
>> +	return 0;
>> +}
> [ ... ]
>> @@ -1135,26 +1187,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_VRAM_N...CHANNEL_VRAM_N_MAX:
>>   			reg_val = xe_mmio_read32(mmio, xe_hwmon_get_reg(hwmon, REG_TEMP, channel));
>> +			if (hwmon->xe->info.platform >= XE_CRESCENTISLAND) {
>> +				return xe_hwmon_temperature_decode((u32)reg_val, val);
> [Severity: Medium]
> Will this new decoding logic for XE_CRESCENTISLAND ever be reached?
>
> When the hwmon subsystem initializes and calls xe_hwmon_temp_is_visible(),
> it checks is_temp_available(), which in turn calls
> xe_hwmon_get_reg(..., REG_TEMP, channel).
>
> Currently, xe_hwmon_get_reg() only defines REG_TEMP for BATTLEMAGE and DG2,
> meaning it falls through and returns an invalid XE_REG(0) for Crescent
> Island. Because the register is invalid, is_temp_available() will return
> false, making the sysfs temperature attributes invisible and leaving this
> decoding logic as dead code.
>
> Should xe_hwmon_get_reg() be updated to include Crescent Island?

This looks like false warning,
Similar comment see in patch 2 as well : https://patchwork.freedesktop.org/patch/750270/?series=172689&rev=2

Cresent Island check is already present, please check.

https://gitlab.freedesktop.org/drm/tip/-/blob/drm-tip/drivers/gpu/drm/xe/xe_hwmon.c?ref_type=heads#L269

xe_hwmon_reg hwmon_reg,
                        int channel)
{
      struct xe_device *xe = hwmon->xe;

      switch (hwmon_reg) {
      case REG_TEMP:
          if (xe->info.platform == XE_BATTLEMAGE || xe->info.platform == XE_CRESCENTISLAND) {
              if (channel == CHANNEL_PKG)
                  return BMG_PACKAGE_TEMPERATURE;
              else if (channel == CHANNEL_VRAM)
                  return BMG_VRAM_TEMPERATURE;
              else if (in_range(channel, CHANNEL_VRAM_N, MAX_VRAM_CHANNELS))

>
>> +			} 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.
>> +				 */
> [ ... ]
>