Re: [PATCH 2/3] drm/xe/hwmon: Use VRAM temperature sensor count from thermal config on CRI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Regression / Missing Initialization of vram_count disables MMIO sensors
- [Medium] Incomplete Implementation / Unexposed Hardware Monitoring Channels
- [Medium] Dead Code / Unreachable Feature for XE_CRESCENTISLAND (CRI) platforms
--

--- Patch [2]: [PATCH 2/3] drm/xe/hwmon: Use VRAM temperature sensor count from thermal config on CRI ---
commit dad00759fd95979bfa227e2bf51f1aad6c7133a4
Author: Karthik Poosa <karthik.poosa@intel.com>
Subject: drm/xe/hwmon: Use VRAM temperature sensor count from thermal config on CRI

This patch dynamically reads the number of VRAM temperature channels on
Crescent Island platforms. It increases the maximum channel count and uses
the pcode thermal config to determine available sensors.

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index c15312b0858ca..8c8a519711230 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -39,7 +39,8 @@ enum xe_hwmon_reg_operation {
>  	REG_READ64,
>  };
>  
> -#define MAX_VRAM_CHANNELS      (16)
> +/* Maximum number of VRAM channels supported by Xe */
> +#define MAX_VRAM_CHANNELS      (80)

[Severity: Medium]
Will the hwmon subsystem actually expose these additional channels?

Looking at the hwmon_info array in xe_hwmon.c, it is statically defined
with exactly 21 temperature items, ending with a NULL terminator.
Because the hwmon core stops iterating at the terminator, sensors past
vram_ch_16 won't be queried or exported to sysfs, making this increase
to 80 ineffective.

[ ... ]

> @@ -786,6 +790,17 @@ static int xe_hwmon_pcode_read_thermal_info(struct xe_hwmon *hwmon)
>  	drm_dbg(&hwmon->xe->drm, "thermal config count 0x%x\n", config);
>  	hwmon->temp.count = REG_FIELD_GET(TEMP_MASK, config);
>  
> +	if (hwmon->xe->info.platform >= XE_CRESCENTISLAND) {
> +		hwmon->temp.vram_count = REG_FIELD_GET(VRAM_COUNT_MASK, config);

[Severity: Medium]
Are the parsed VRAM channels accessible on Crescent Island platforms?

When is_vram_ch_available() runs, it calls xe_hwmon_get_reg() to get the
temperature register. However, xe_hwmon_get_reg() does not currently have
a branch for XE_CRESCENTISLAND in the REG_TEMP switch case.

Because it falls through and returns XE_REG(0), xe_reg_is_valid() will
fail in is_vram_ch_available(), making these newly parsed sensors
unreachable for CRI platforms.

> +		if (hwmon->temp.vram_count > MAX_VRAM_CHANNELS && hwmon->temp.vram_count) {
> +			drm_warn(&hwmon->xe->drm, "VRAM channel count %d exceeds max %d, clamping\n",
> +				 hwmon->temp.vram_count, MAX_VRAM_CHANNELS);
> +			hwmon->temp.vram_count = MAX_VRAM_CHANNELS;
> +		}
> +	} else {
> +		hwmon->temp.vram_count = 16; /* For older platforms, max is 16 VRAM channels */
> +	}
> +
>  	return ret;
>  }

[ ... ]

> @@ -964,6 +979,9 @@ static inline bool is_vram_ch_available(struct xe_hwmon *hwmon, int channel)
>  	u32 reg_val;
>  	u8 temp;
>  
> +	if (vram_id >= hwmon->temp.vram_count)
> +		return false;

[Severity: High]
Does this new check rely on pcode succeeding for older platforms?

If xe_hwmon_pcode_read_thermal_info() returns early due to a pcode mailbox
error when reading READ_THERMAL_LIMITS or READ_THERMAL_CONFIG, or is skipped
entirely, hwmon->temp.vram_count will remain 0.

Because of this check, an early return would unconditionally disable all
MMIO-based VRAM temperature sensors on platforms like BMG, losing existing
functionality. Previously, these sensors relied on a static maximum and
were completely decoupled from pcode initialization success.

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