Re: [PATCH v3 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 3 potential issue(s) to consider:

New issues:
- [Low] Kernel-doc syntax violation for struct member documentation.

Pre-existing issues:
- [High] xe_hwmon_pcode_rmw_power_limit ignores xe_pcode_read failures, leading to hardware state corruption.
- [High] Incorrect bounds check for CRI platforms causes silent field overflow when encoding power limits.
--

commit b3104079752794b6ca510cf54096b19d2c7bfc90
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_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index f88bacf63c83c..74037a9c32f7e 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -178,6 +178,10 @@ struct xe_device {
>  		u8 has_drm_ras:1;
>  		/** @info.has_fan_control: Device supports fan control */
>  		u8 has_fan_control:1;
> +		/*
> +		 * @info.has_fixed_vram_channels: Device has fixed VRAM temperature channel count.
> +		 */
> +		u8 has_fixed_vram_channels:1;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Low]
Is this block comment meant to be a kernel-doc comment?

Using a standard C block comment (/*) instead of a kernel-doc comment (/**)
for the newly added has_fixed_vram_channels member will cause kernel-doc
tooling to emit a "not described" warning when generating documentation.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does xe_hwmon_pcode_rmw_power_limit()
safely handle PCODE read failures?

If xe_pcode_read() fails, it logs the error but proceeds to perform a
read-modify-write on val0 and val1, which remain initialized to 0:

drivers/gpu/drm/xe/xe_hwmon.c:xe_hwmon_pcode_rmw_power_limit() {
	...
	ret = xe_pcode_read(root_tile, PCODE_MBOX(...), &val0, &val1);
	if (ret)
		drm_dbg(&hwmon->xe->drm, "read failed ch %d val0 0x%08x, val1 0x%08x, ret %d\n",
			channel, val0, val1, ret);

	if (attr == PL1_HWMON_ATTR)
		val0 = (val0 & ~clr) | set;
	else if (attr == PL2_HWMON_ATTR)
		val1 = (val1 & ~clr) | set;
	...
	ret = xe_pcode_write64_timeout(root_tile, PCODE_MBOX(...), val0, val1, PL_WRITE_MBX_TIMEOUT_MS);
	...
}

Writing these 0 values back via xe_pcode_write64_timeout() inadvertently zeroes
out the non-targeted power limit and its time windows. Can this disable critical
sustained power limits and cause severe performance throttling?


[Severity: High]
This is a pre-existing issue, but does xe_hwmon_power_max_interval_store()
properly validate the time window for CRI platforms?

The sysfs store function bounds-checks the input time window against max_win
(calculated using legacy encoding up to ~256 seconds) and then shifts the
value before placing it into the 7-bit PWR_LIM_TIME field:

drivers/gpu/drm/xe/xe_hwmon.c:xe_hwmon_power_max_interval_store() {
	...
	if (val > max_win)
		return -EINVAL;

	if (hwmon->xe->info.platform >= XE_CRESCENTISLAND) {
		/* ... */
		rxy = REG_FIELD_PREP(PWR_LIM_TIME, (val << 2));
	}
	...
}

Inputs larger than ~31 ms will silently overflow the 7-bit PWR_LIM_TIME field.
Will this write a truncated, drastically reduced time window to the hardware,
restricting the GPU's ability to absorb bursty workloads and causing immediate
throttling?

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