Re: [PATCH v9 07/14] drm/xe/hwmon: Consolidate temperature sensor availability checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/xe/hwmon: live hardware read during probe permanently hides sysfs attributes on transient failure
- [Medium] drm/xe/hwmon: endianness bug when extracting thermal limits via union
--

commit bbb594b78c17c03bdb11a59f0ce797f39d14c3e9
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() function that validates multiple sensor types using new
helpers. It uses this new function during hwmon registration to ensure
attributes for absent sensors are not exposed.

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index accccadbe3e67..62e341ef37e9b 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c

[ ... ]

> @@ -1008,19 +1008,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)) {

[Severity: Medium]
Since this function is called during hwmon registration to determine attribute
visibility, does this live hardware read risk permanently hiding sysfs
attributes if the hardware is transiently busy or uninitialized?

If the sensor returns 0 (which mmio_temp_valid treats as invalid) during
driver probe, the attribute will be hidden for the entire boot cycle.

> +			drm_dbg(&hwmon->xe->drm,
> +				"channel %d temperature unavailable, val 0x%x\n", channel, reg_val);
> +			return false;
> +		}
> +
> +		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);

[Severity: Medium]
Similar to the MMIO read above, could this PCODE mailbox read cause
attributes to be permanently hidden?

If the PCODE mailbox transiently times out during driver probe,
get_mc_temp() will return an error, causing the MCTRL attribute to be
permanently hidden for the duration of the boot cycle.

> +	case CHANNEL_PCIE:
> +		return hwmon->temp.count && !get_pcie_temp(hwmon, NULL);
> +	default:
> +		return false;
> +	}
>  }
>  
>  static umode_t
> @@ -1030,14 +1050,13 @@ xe_hwmon_temp_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>  	case hwmon_temp_emergency:
>  		switch (channel) {
>  		case CHANNEL_PKG:
> -			return hwmon->temp.limit[TEMP_LIMIT_PKG_SHUTDOWN] ? 0444 : 0;
> -		case CHANNEL_VRAM:
> -			return hwmon->temp.limit[TEMP_LIMIT_MEM_SHUTDOWN] ? 0444 : 0;
>  		case CHANNEL_MCTRL:
>  		case CHANNEL_PCIE:
> -			return hwmon->temp.count ? 0444 : 0;
> +			return (is_temp_available(hwmon, channel) &&
> +				hwmon->temp.limit[TEMP_LIMIT_PKG_SHUTDOWN]) ? 0444 : 0;

[Severity: Medium]
Does extracting thermal limits this way introduce an endianness bug?

While looking at how the limits are accessed here, the xe_hwmon_thermal_info
struct aliases u8 arrays with u32 arrays:

        union {
                u8 limit[TEMP_LIMIT_MAX];
                u32 data[DIV_ROUND_UP(TEMP_LIMIT_MAX, sizeof(u32))];
        };

If the driver reads 32-bit values from PCODE into the u32 arrays, extracting
individual 8-bit limits by indexing into the u8 arrays assumes a little-endian
memory layout. On big-endian architectures, index 0 will map to bits 24:31
instead of bits 0:7, potentially causing the driver to extract incorrect
thermal limits.

> +		case CHANNEL_VRAM:
>  		case CHANNEL_VRAM_N...CHANNEL_VRAM_N_MAX:
> -			return (is_vram_ch_available(hwmon, channel) &&
> +			return (is_temp_available(hwmon, channel) &&
>  				hwmon->temp.limit[TEMP_LIMIT_MEM_SHUTDOWN]) ? 0444 : 0;

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