Re: [PATCH v12 07/16] 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] drm/xe/hwmon: dynamic hw queries at probe hide sysfs attributes on timeout
--

commit 4140f9b7c587fc4925ef3079bf0c972416615373
Author: Karthik Poosa <karthik.poosa@intel.com>

drm/xe/hwmon: Consolidate temperature sensor availability checks

This patch introduces a generalized is_temp_available() function to validate
PKG, VRAM, MCTRL, and PCIE temperature sensors. It uses this helper within
xe_hwmon_temp_is_visible() to prevent exposing sysfs attributes for sensors
that are not present during hwmon registration.

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 4463f32c8d8cb..b2f18a32487cc 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -1011,19 +1011,39 @@ static void xe_hwmon_get_voltage(struct xe_hwmon *hwmon, int channel, long *valu
>  	*value = DIV_ROUND_CLOSEST(REG_FIELD_GET(VOLTAGE_MASK, reg_val) * 2500, SF_VOLTAGE);
>  }
>  
> -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]
If xe_mmio_read32() returns an uninitialized value (like 0) during early boot,
does returning false here permanently prevent the creation of the sysfs
attribute for this sensor?

In mmio_temp_valid(), 0 is treated as an invalid temperature, which might only
be a temporary state during device probe before the hardware is fully
initialized. Since this is called from the is_visible callback, returning
false will hide the attribute permanently.

> +
> +		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]
Do these dynamic PCODE mailbox reads at probe time risk permanently hiding
the MCTRL and PCIE sysfs attributes if there is a transient error?

If get_mc_temp() or get_pcie_temp() encounter a mailbox timeout or other
transient failure when called from xe_hwmon_temp_is_visible(), it evaluates
to false, preventing the hwmon core from creating the sysfs files.

Prior to this patch, MCTRL and PCIE merely checked hwmon->temp.count
statically without issuing a dynamic read.

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

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