Re: [PATCH v13 14/16] drm/xe/vsec: Update PMT internal access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe/vsec: missing wait_for_fw() and invalid caching in xe_vsec_get_guid()
--

commit ff1f060a60a0a4bf5c3f7818992f8347fb84bca2
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Update PMT internal access

This commit updates read_residency_counter(), xe_hwmon_energy_get(), and
xe_get_pcode_version() to retrieve the device-specific PMT telemetry GUID via
xe_vsec_get_guid() instead of reading BMG_PUNIT_TELEMETRY_GUID directly.

> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index 8c901e54f3847..fa9f45d565cb9 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c
> @@ -365,11 +365,17 @@ ALLOW_ERROR_INJECTION(xe_pcode_probe_early, ERRNO); /* See xe_pci_probe */
>  int xe_get_pcode_version(struct xe_device *xe, struct xe_pcode_version *version)
>  {
>  	int ret = 0;
> +	u32 guid;
>  
>  	guard(xe_pm_runtime)(xe);
>  
> -	ret = xe_pmt_telem_read(xe->drm.dev,
> -				xe_mmio_read32(xe_root_tile_mmio(xe), BMG_PUNIT_TELEMETRY_GUID),
> +	ret = xe_vsec_get_guid(xe, &guid);
> +	if (ret) {
> +		xe_warn(xe, "PMT device is not available: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = xe_pmt_telem_read(xe->drm.dev, guid,
>  				(u64 *)version, PUNIT_VERSION_OFFSET, sizeof(*version));

[Severity: Medium]
On XE_CRESCENTISLAND, can calling xe_vsec_get_guid() before xe_pmt_telem_read()
access the CRI PMT discovery window before OOBMSM firmware is ready and
corrupt punit_guid_cache?

In xe_debugfs_register(), pcode_info is registered on both XE_BATTLEMAGE and
XE_CRESCENTISLAND, and reading it invokes:

  pcode_info() -> xe_get_pcode_version() -> xe_vsec_get_guid()

Unlike xe_pmt_telem_read(), xe_pmt_read_reg(), and xe_pmt_write_reg(),
xe_vsec_get_guid() in drivers/gpu/drm/xe/xe_vsec.c does not call wait_for_fw()
before programming the SoC remapper and reading CRI_PUNIT_TELEMETRY_GUID:

drivers/gpu/drm/xe/xe_vsec.c:xe_vsec_get_guid() {
	...
	case XE_CRESCENTISLAND:
		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
		*guid = xe_mmio_read32(mmio, CRI_PUNIT_TELEMETRY_GUID);
		break;
	...
	/* the GUID is static, read once and cache for all requests */
	if (*guid)
		xe->pmt.punit_guid_cache = *guid;
}

If pcode_info (or read_residency_counter()) is read while late-bound OOBMSM
firmware is not yet ready (for example during initial probe while
cri_late_bind_probe_work is polling, or after runtime resume from D3cold),
reading CRI_PUNIT_TELEMETRY_GUID before wait_for_fw() has two failure modes:

1. If the premature read returns 0, xe_vsec_get_guid() still returns 0 with
   *guid == 0, and xe_pmt_telem_read() then fails immediately in
   xe_guid_decode() with -ENODEV before reaching its own wait_for_fw() check:

drivers/gpu/drm/xe/xe_vsec.c:xe_pmt_telem_read() {
	...
	ret = xe_guid_decode(guid, &mem_region, &offset);
	if (ret)
		goto dev_exit;
	...
	if (!wait_for_fw(xe)) {
		ret = -ENODATA;
		goto runtime_exit;
	}
}

2. If the premature read returns a non-zero uninitialized or all-ones value
   (such as 0xffffffff), xe_vsec_get_guid() caches it in
   xe->pmt.punit_guid_cache permanently, causing all subsequent calls to
   xe_vsec_get_guid(), xe_get_pcode_version(), and read_residency_counter() to
   fail even after OOBMSM firmware becomes ready.

Should xe_vsec_get_guid() call wait_for_fw(xe) before reading the GUID (and
validate the GUID before caching it)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102221314.119028-18-michael.j.ruhl@intel.com?part=14