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

[sashiko-bot@kernel.org]

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.

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

> +	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;

[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;

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