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

[sashiko-bot@kernel.org]

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.

> +	} 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?

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

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