Re: [PATCH 1/3] drm/xe/hwmon: Detect unavailable temperature sensors

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

On 25-08-2026 00:29, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Incorrect register bitmask `TEMP_MASK` extracts fractional temperature instead of integer, improperly hiding valid VRAM sensors.
> - [Medium] Inverted short-circuit evaluation causes unconditional hardware PCODE reads and excessive log spam during hwmon registration.
> - [Medium] Threshold limit sysfs attributes are exposed for unavailable sensors due to missing validity checks.
> --
>
> --- Patch [1]: [PATCH 1/3] drm/xe/hwmon: Detect unavailable temperature sensors ---
> commit de3d36ad17dd257811068fa1f47a4b3779a65fb1
> Author: Karthik Poosa <karthik.poosa@intel.com>
>
> drm/xe/hwmon: Detect unavailable temperature sensors
>
> Add is_temp_valid() to validate sensor presence.
> A temperature reading of 0xFF on CRI platforms indicates that the
> corresponding sensor is not present and should be treated as unavailable.
>
> Use this check from xe_hwmon_temp_is_visible() callback so that attributes
> for unavailable sensors are not exposed during hwmon device registration.
>
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
>> index de3f2aeffc3fb..c15312b0858ca 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> [ ... ]
>
>> @@ -804,11 +813,25 @@ static int get_mc_temp(struct xe_hwmon *hwmon, long *val)
>>   		drm_dbg(&hwmon->xe->drm, "thermal data for group %d val 0x%x\n", i, dword[i]);
>>   	}
>>   
>> -	for (i = TEMP_INDEX_MCTRL; i < hwmon->temp.count - 1; i++)
>> -		average += hwmon->temp.value[i];
>> +	for (i = TEMP_INDEX_MCTRL; i < hwmon->temp.count - 1; i++) {
>> +		if (is_temp_valid(hwmon, hwmon->temp.value[i])) {
>> +			average += hwmon->temp.value[i];
>> +			count++;
>> +		} else {
>> +			drm_dbg(&hwmon->xe->drm, "mc temp sensor %d not available, val 0x%x\n",
>> +				i, hwmon->temp.value[i]);
>> +		}
>> +	}
>> +
>> +	if (!count) {
>> +		drm_warn(&hwmon->xe->drm, "no memory temp sensors available!\n");
> [Severity: Medium]
> Will this warning be emitted multiple times during device registration?
>
> Because hwmon subsystem core evaluates the is_visible callback multiple times
> per channel, and the logic in xe_hwmon_temp_is_visible() evaluates
> get_mc_temp() first, this warning may trigger excessively if the sensor is
> intentionally absent.

Yes it does.

We can change these messages to |drm_dbg()| to avoid generating 
excessive warnings in such cases.

>> +		return -ENXIO;
>> +	}
>> +
>> +	average /= count;
>> +	if (val)
>> +		*val = average * MILLIDEGREE_PER_DEGREE;
>>   
>> -	average /= (hwmon->temp.count - TEMP_INDEX_MCTRL - 1);
>> -	*val = average * MILLIDEGREE_PER_DEGREE;
>>   	return 0;
>>   }
> [ ... ]
>
>> @@ -932,11 +961,21 @@ static inline bool is_vram_ch_available(struct xe_hwmon *hwmon, int channel)
>>   	struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
>>   	int vram_id = channel - CHANNEL_VRAM_N;
>>   	struct xe_reg vram_reg;
>> +	u32 reg_val;
>> +	u8 temp;
>>   
>>   	vram_reg = xe_hwmon_get_reg(hwmon, REG_TEMP, channel);
>> -	if (!xe_reg_is_valid(vram_reg) || !xe_mmio_read32(mmio, vram_reg))
>> +	if (!xe_reg_is_valid(vram_reg))
>>   		return false;
>>   
>> +	reg_val = xe_mmio_read32(mmio, vram_reg);
>> +	temp = REG_FIELD_GET(TEMP_MASK, reg_val);
> [Severity: High]
> Does this extract the correct bits for the VRAM channel temperature value?
>
> For CHANNEL_VRAM_N, TEMP_MASK extracts bits 7:0, which may represent the
> fractional part of the temperature rather than the integer temperature.
> The integer temperature is typically in bits 31:8 and extracted with
> TEMP_MASK_VRAM_N.
>
> If a valid sensor has a temperature with a 0 fractional part (or 0xFF on newer
> platforms), could it be falsely detected as missing and hidden?
You are right, from VRAM individual channel temperature, we should 
extract with TEMP_MASK_VRAM_N.
>
>> +	if (!is_temp_valid(hwmon, temp)) {
>> +		drm_dbg(&hwmon->xe->drm, "vram channel %d unavailable, val 0x%x\n", vram_id,
>> +			reg_val);
>> +		return false;
>> +	}
>> +
>>   	/* Create label only for available vram channel */
>>   	sprintf(hwmon->temp.vram_label[vram_id], "vram_ch_%d", vram_id);
>>   	return true;
>> @@ -953,8 +992,9 @@ xe_hwmon_temp_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>>   		case CHANNEL_VRAM:
>>   			return hwmon->temp.limit[TEMP_LIMIT_MEM_SHUTDOWN] ? 0444 : 0;
> [Severity: Medium]
> Will this expose threshold limit attributes for unavailable sensors?
>
> The input attribute for CHANNEL_PKG and CHANNEL_VRAM is now correctly hidden
> if the sensor returns an unavailable sentinel. However, hwmon_temp_emergency
> and hwmon_temp_crit do not seem to check is_temp_valid() for these channels.
> Could this lead to userspace seeing emergency/crit attributes for sensors
> that don't actually exist?
>
>>   		case CHANNEL_MCTRL:
>> +			return !get_mc_temp(hwmon, NULL) && hwmon->temp.count ? 0444 : 0;
Currently, temperature sensor detection is supported only for mailbox 
and per-channel VRAM temperature sensors.
We have requested support from the corresponding team to provide similar 
validity information for package and VRAM temperatures as well.
Once that support becomes available, we can extend the implementation 
accordingly.
> [Severity: Medium]
> Does this execute a synchronous hardware read before checking hwmon->temp.count?
>
> Because this uses !get_mc_temp(hwmon, NULL) && hwmon->temp.count,
> get_mc_temp() is unconditionally executed first due to left-to-right
> evaluation. If temp.count is 0, should the condition short-circuit before
> performing the hardware read by placing the temp.count check first?
>
>>   		case CHANNEL_PCIE:
>> -			return hwmon->temp.count ? 0444 : 0;
>> +			return !get_pcie_temp(hwmon, NULL) && hwmon->temp.count ? 0444 : 0;
Agree, I shall change this in next revision.