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:

New issues:
- [High] VRAM temperature sensors are silently lost on platforms where the thermal mailbox read fails or is unsupported.
- [High] The dynamic VRAM temperature sensor count feature for CRI is completely non-functional due to missing static attribute definitions and register mappings.

Pre-existing issues:
- [Medium] The hardware monitoring device is registered directly from the DRM subsystem rather than using an auxiliary device driver.
--

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

This patch reads the number of VRAM temperature sensor channels from the
second byte of READ_THERMAL_CONFIG on CRI platforms. It uses this reported
count to cap the exposed hwmon attributes for VRAM temperature sensors,
while retaining the maximum supported channel count for non-CRI platforms.

> 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

[ ... ]

> @@ -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);
> +		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;
> +		}

[Severity: High]
Can the extra sensors on CRI platforms actually be exposed to sysfs?

The HWMON_CHANNEL_INFO(temp, ...) static attribute array doesn't appear
to be expanded in this patch. Since the static array caps the maximum
channels visible to sysfs, won't the hwmon core stop querying early
regardless of the dynamic vram_count value?

Also, looking at xe_hwmon_get_reg(), there doesn't seem to be a branch 
for XE_CRESCENTISLAND under the REG_TEMP case. Will is_vram_ch_available()
receive an invalid register and fail for all VRAM sensors on CRI?

> +	} else {
> +		hwmon->temp.vram_count = 16; /* For older platforms, max is 16 VRAM channels */
> +	}
> +
>  	return ret;
>  }

[Severity: High]
If xe_pcode_read() fails earlier in xe_hwmon_pcode_read_thermal_info(), the
function returns early. Does this leave hwmon->temp.vram_count uninitialized 
at 0 instead of setting the fallback value of 16?

> @@ -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]
If vram_count was left at 0 due to an early return during the pcode read,
this bounds check will unconditionally return false. 

Does this mean a transient pcode error permanently disables all VRAM
temperature monitoring on older platforms, whereas previously the channels
were still exposed?

[ ... ]

> @@ -1621,6 +1639,13 @@ int xe_hwmon_register(struct xe_device *xe)
>  
>  	xe_hwmon_get_preregistration_info(hwmon);
>  
> +	hwmon->temp.vram_label = devm_kcalloc(dev, hwmon->temp.vram_count,
> +					      MAX_LABEL_SIZE, GFP_KERNEL);
> +	if (!hwmon->temp.vram_label) {
> +		xe->hwmon = NULL;
> +		return -ENOMEM;
> +	}
> +
>  	drm_dbg(&xe->drm, "Register xe hwmon interface\n");
>  
>  	/*  hwmon_dev points to device hwmon<i> */

[Severity: Medium]
This is a pre-existing issue, but should this hardware monitoring device be
registered using an auxiliary device driver?

The hwmon subsystem documentation dictates that if the main functionality of
a chip is not hardware monitoring (such as a DRM controller), its hardware
monitoring functionality should be implemented as an auxiliary device driver
in drivers/hwmon/, rather than being registered directly from the DRM
subsystem.

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