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

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

On 13-10-2026 07:54, sashiko-bot@kernel.org wrote:
> 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?
An all-ones read means the PCIe link has failed, which is a device-level
condition handled outside hwmon. Once the device is unavailable,
xe_mmio_read32() returns 0, which is already treated as invalid.

mmio_temp_valid() is only used at probe, not in xe_hwmon_temp_read(). In the
next version I'll can add the check to the MMIO read path and return 
-ENXIO, to
match the mailbox sensors.
>> +}
>> +
>> +/* 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)