Re: [PATCH v11 04/16] drm/xe/hwmon: Add helpers to validate thermal sensor readings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hwmon: MMIO read errors (U32_MAX) are treated as valid temperatures on legacy platforms
--

commit 30c789d23390d6abf7d18b4ec4f037b60404f1b9
Author: Karthik Poosa <karthik.poosa@intel.com>

drm/xe/hwmon: Add helpers to validate thermal sensor readings

This commit introduces mmio_temp_valid() and mbx_temp_valid() to check
whether a temperature sensor reading is valid. It gates these checks behind
the has_temp_sentinel flag.

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 2d1fa9159c58b..712a21a996362 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c

[ ... ]

> @@ -825,6 +828,23 @@ static int xe_hwmon_pcode_read_thermal_info(struct xe_hwmon *hwmon)
>  	return ret;
>  }
>  
> +static inline bool mmio_temp_valid(const struct xe_hwmon *hwmon, u32 value)
> +{
> +	/*
> +	 * On platforms lacking temperature sentinel support, a value of 0 is treated
> +	 * as invalid. While 0°C is theoretically possible, it is highly unlikely for
> +	 * a discrete GPU in normal environments, making it a reasonable fallback
> +	 * indicator for invalid or uninitialized temperature readings.
> +	 */
> +	return hwmon->xe->info.has_temp_sentinel ? value != U32_MAX : value != 0;

[Severity: Medium]
Does this inadvertently treat PCIe MMIO read errors as valid temperatures on
legacy platforms?

When a PCIe target abort or link failure occurs, xe_mmio_read32() returns
U32_MAX (0xFFFFFFFF). If the platform lacks has_temp_sentinel support, the
condition evaluates value != 0. Because 0xFFFFFFFF is not 0, this returns
true.

Later, in xe_hwmon_temp_read(), this U32_MAX value will be blindly decoded:

*val = (s32)REG_FIELD_GET(TEMP_MASK_VRAM_N, reg_val) *
        (REG_FIELD_GET(TEMP_SIGN_MASK, reg_val) ? -1 : 1) *
        MILLIDEGREE_PER_DEGREE;

This could result in absurd temperature readings like -8,388,607 or 255
instead of correctly propagating an error code when the hardware fails.

Should U32_MAX be universally treated as invalid, regardless of the
has_temp_sentinel flag?

> +}
> +
> +/* Mailbox temperature is sign-magnitude (bit 7 sign, bits 6:0 magnitude). */
> +static inline bool mbx_temp_valid(const struct xe_hwmon *hwmon, u8 value)

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