Re: [PATCH v9 03/14] drm/xe/hwmon: Fix memory controller thermal data handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] xe: endianness bug when unpacking thermal sensor values via union overlay
--

commit 99c258fb8e157f67678f926764db2d41f2794285
Author: Karthik Poosa <karthik.poosa@intel.com>

drm/xe/hwmon: Fix memory controller thermal data handling

This commit updates get_mc_temp() to read all required thermal groups and uses
an aligned u32 buffer for PCODE accesses to avoid unaligned memory accesses
and ensure correct MC temperature reporting.

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index c63aa708ab6fc..33d0ca7d86e44 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -143,8 +143,12 @@ struct xe_hwmon_thermal_info {
>  	};
>  	/** @count: no of temperature sensors available for the platform */
>  	u8 count;
> -	/** @value: per-sensor raw mailbox temperature; bit7=sign, bits6:0=magnitude */
> -	u8 value[U8_MAX];
> +	union {
> +		/** @value: per-sensor raw mailbox temperature; bit7=sign, bits6:0=magnitude */
> +		u8 value[U8_MAX + 1];
> +		/** @dword: sensor values as dwords, u32-aligned for pcode reads */
> +		u32 dword[DIV_ROUND_UP(U8_MAX + 1, sizeof(u32))];
> +	};
>  	/** @vram_label: vram label names */
>  	char vram_label[MAX_VRAM_CHANNELS][MAX_LABEL_SIZE];
>  };

[Severity: Low]
Does this struct layout rely on a little-endian memory layout?

Overlaying a byte array over a dword array via a union structurally embeds an
endianness assumption. On big-endian architectures, the bytes of the 32-bit
hardware registers unpacked into the dword array would be reversed when
accessed through the value array.

[ ... ]
> @@ -831,16 +835,16 @@ static int mbx_temp_decode(u8 value)
>  static int get_mc_temp(struct xe_hwmon *hwmon, long *val)
>  {
>  	struct xe_tile *root_tile = xe_device_get_root_tile(hwmon->xe);
> -	u32 *dword = (u32 *)hwmon->temp.value;
>  	s32 average = 0;
>  	int ret, i;
>  
> -	for (i = 0; i < DIV_ROUND_UP(TEMP_LIMIT_MAX, sizeof(u32)); i++) {
> +	for (i = 0; i < DIV_ROUND_UP(hwmon->temp.count, sizeof(u32)); i++) {
>  		ret = xe_pcode_read(root_tile, PCODE_MBOX(PCODE_THERMAL_INFO, READ_THERMAL_DATA, i),
> -				    (dword + i), NULL);
> +				    &hwmon->temp.dword[i], NULL);
>  		if (ret)
>  			return ret;
> -		drm_dbg(&hwmon->xe->drm, "thermal data for group %d val 0x%x\n", i, dword[i]);
> +		drm_dbg(&hwmon->xe->drm, "thermal data for group %d val 0x%x\n", i,
> +			hwmon->temp.dword[i]);
>  	}
>  
>  	for (i = TEMP_INDEX_MCTRL; i < hwmon->temp.count - 1; i++)
>  		average += mbx_temp_decode(hwmon->temp.value[i]);

[Severity: Low]
Does this regression decode the wrong byte from the 32-bit hardware register
on big-endian systems?

Since the value array aliases the dword array in xe_hwmon_thermal_info, this
loop will extract incorrect bytes on big-endian architectures and report the
wrong temperatures.

While Intel GPUs are rarely deployed on big-endian hosts, the xe driver's
Kconfig does not restrict compilation to little-endian or x86 architectures.

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