Re: [PATCH v5 07/13] drm/xe/hwmon: Consolidate temperature sensor availability checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hwmon: initialization race causes permanent attribute loss on slow sensors
--

commit c02db655e6dabb60e61a8acbe0daf64a353e248d
Author: Karthik Poosa <karthik.poosa@intel.com>
Subject: drm/xe/hwmon: Consolidate temperature sensor availability checks

This commit replaces is_vram_ch_available() with a generalized 
is_temp_available() to validate multiple sensor types. It uses this new 
helper during hwmon registration to prevent exposing sysfs attributes for 
sensors that are not present.

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 7bb7a5f402d1e..d62f95a1912f3 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
[ ... ]
> -static inline bool is_vram_ch_available(struct xe_hwmon *hwmon, int channel)
> +static bool is_temp_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;
> +	struct xe_reg reg;
> +	u32 reg_val;
>  
> -	vram_reg = xe_hwmon_get_reg(hwmon, REG_TEMP, channel);
> -	if (!xe_reg_is_valid(vram_reg) || !xe_mmio_read32(mmio, vram_reg))
> -		return false;
> +	switch (channel) {
> +	case CHANNEL_PKG:
> +	case CHANNEL_VRAM:
> +	case CHANNEL_VRAM_N...CHANNEL_VRAM_N_MAX:
> +		reg = xe_hwmon_get_reg(hwmon, REG_TEMP, channel);
> +		if (!xe_reg_is_valid(reg))
> +			return false;
> +
> +		reg_val = xe_mmio_read32(mmio, reg);
> +		if (!mmio_temp_valid(hwmon, reg_val)) {
> +			drm_dbg(&hwmon->xe->drm,
> +				"channel %d temperature unavailable, val 0x%x\n", channel, reg_val);
> +			return false;
> +		}

[Severity: Medium]
Could this cause functional sensors to be permanently hidden if the firmware
is slow to initialize them?

During hwmon registration, is_temp_available() performs a live MMIO read to
determine attribute visibility. If the firmware hasn't populated a valid
temperature yet at the exact moment of device probe (for instance, returning
0 or U32_MAX), mmio_temp_valid() will return false.

Since this determines sysfs attribute visibility, doesn't this create a
silent failure where perfectly functional PKG sensors disappear intermittently
depending on boot timing?

> +
> +		if (channel >= CHANNEL_VRAM_N)
> +			sprintf(hwmon->temp.vram_label[channel - CHANNEL_VRAM_N],
> +				"vram_ch_%d", channel - CHANNEL_VRAM_N);
>  
> -	/* Create label only for available vram channel */
> -	sprintf(hwmon->temp.vram_label[vram_id], "vram_ch_%d", vram_id);
> -	return true;
> +		return true;
> +	case CHANNEL_MCTRL:
> +		return hwmon->temp.count && !get_mc_temp(hwmon, NULL);
> +	case CHANNEL_PCIE:
> +		return hwmon->temp.count && !get_pcie_temp(hwmon, NULL);

[Severity: Medium]
Similarly, does this live PCODE mailbox polling risk permanently hiding the
Memory Controller and PCIe temperature sensors?

If PCODE is slow to respond during device probe, get_mc_temp() and
get_pcie_temp() might fail or return invalid data. Because this check
replaces static capability checks (such as hwmon->temp.count) with live
sensor data, it seems to introduce a race condition where a temporary
initialization delay results in permanent attribute loss.

> +	default:
> +		return false;
> +	}
>  }

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