Re: [PATCH v2 2/5] 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 2 potential issue(s) to consider:
- [Medium] VRAM temperature sensors will not be exposed on Crescent Island (CRI) platforms because `xe_hwmon_get_reg()` lacks `REG_TEMP` support for `XE_CRESCENTISLAND`.
- [Medium] On Battlemage (BMG) platforms, all VRAM temperature sensors will be silently hidden if the thermal PCODE mailbox initialization fails.
--

commit 426a936975a4587d5003554dc21e461abf340940
Author: Karthik Poosa <karthik.poosa@intel.com>

drm/xe/hwmon: Use VRAM temperature sensor count from thermal config on CRI
    
Read the number of VRAM temperature sensor channels from the second byte
of READ_THERMAL_CONFIG on CRI platforms. Use the reported count to avoid
exposing hwmon attributes for unavailable VRAM temperature sensors, while
retaining the maximum supported channel count on non-CRI platforms.

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 768f6d68b2916..51a6664eec485 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
[ ... ]
> @@ -271,7 +275,7 @@ static struct xe_reg xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg
>  				return BMG_PACKAGE_TEMPERATURE;
>  			else if (channel == CHANNEL_VRAM)
>  				return BMG_VRAM_TEMPERATURE;
> -			else if (in_range(channel, CHANNEL_VRAM_N, MAX_VRAM_CHANNELS))
> +			else if (in_range(channel, CHANNEL_VRAM_N, hwmon->temp.vram_count))
>  				return BMG_VRAM_TEMPERATURE_N(channel - CHANNEL_VRAM_N);
>  		} else if (xe->info.platform == XE_DG2) {
>  			if (channel == CHANNEL_PKG)

[Severity: Medium]
Will VRAM temperature sensors be exposed on XE_CRESCENTISLAND platforms?

When checking sysfs attribute visibility, is_temp_available() queries
xe_hwmon_get_reg() for REG_TEMP. Since xe_hwmon_get_reg() only has branches
for XE_BATTLEMAGE and XE_DG2, it returns an invalid register (XE_REG(0)) for
XE_CRESCENTISLAND platforms.

This causes is_temp_available() to unconditionally return false. Does this
code need a branch for XE_CRESCENTISLAND to return the correct VRAM
temperature registers?

[ ... ]
> @@ -786,6 +854,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 > XE_MAX_VRAM_CHANNELS) {
> +			drm_warn(&hwmon->xe->drm, "VRAM channel count %d exceeds max %d, clamping\n",
> +				 hwmon->temp.vram_count, XE_MAX_VRAM_CHANNELS);
> +			hwmon->temp.vram_count = XE_MAX_VRAM_CHANNELS;
> +		}
> +	} else {
> +		hwmon->temp.vram_count = BMG_MAX_VRAM_CHANNELS;
> +	}
> +
>  	return ret;
>  }

[Severity: Medium]
Will this silently hide all VRAM temperature sensors on Battlemage (BMG)
platforms if the thermal PCODE mailbox initialization fails?

If xe_hwmon_pcode_read_thermal_info() encounters a failure (such as an error
reading READ_THERMAL_LIMITS or READ_THERMAL_CONFIG earlier in the function),
it returns early. This skips the fallback assignment of hwmon->temp.vram_count
at the end of the function.

Because hwmon->temp.vram_count is 0-initialized, the in_range() check will
subsequently evaluate to false in xe_hwmon_get_reg(), returning an invalid
register and hiding the sensors. Previously, this path fell back to a
hardcoded MAX_VRAM_CHANNELS value, preserving visibility.

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