Re: [PATCH v13 15/16] drm/xe/vsec: Refactor platform check

[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 GUID validation in xe_vsec_get_guid()
--

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

drm/xe/vsec: Refactor platform check

This commit combines the platform checks in xe_debugfs_register() so that
both debugfs_residencies and pcode_info_debugfs are registered on Battlemage
and Crescent Island PF devices.

> diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
> index b56e4aad40696..5a82f8eab2c66 100644
> --- a/drivers/gpu/drm/xe/xe_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_debugfs.c
> @@ -702,23 +702,22 @@ void xe_debugfs_register(struct xe_device *xe)
>  				 ARRAY_SIZE(debugfs_list),
>  				 root, minor);
>  
> -	if (xe->info.platform == XE_BATTLEMAGE && !IS_SRIOV_VF(xe)) {
> -		drm_debugfs_create_files(debugfs_residencies,
> -					 ARRAY_SIZE(debugfs_residencies),
> -					 root, minor);
> -	}
> -
>  	/*
> -	 * Pcode version read from PMT is currently only supported on CRI and BMG platforms in PF
> -	 * mode, as both platforms support the necessary telemetry read mechanism and have a fixed
> -	 * PUNIT_VERSION_OFFSET.
> +	 * Residencies and Pcode version read from PMT is currently only supported on CRI and BMG
> +	 * platforms in PF mode.  Both platforms support the necessary telemetry read mechanism
> +	 * and have a fixed offsets for the required data.
>  	 * Attempting this access on other platforms must be verified before enabling support.
>  	 */
>  	if (!IS_SRIOV_VF(xe) &&
> -	    (xe->info.platform == XE_CRESCENTISLAND || xe->info.platform == XE_BATTLEMAGE))
> +	    (xe->info.platform == XE_CRESCENTISLAND || xe->info.platform == XE_BATTLEMAGE)) {
> +		drm_debugfs_create_files(debugfs_residencies,
> +					 ARRAY_SIZE(debugfs_residencies),
> +					 root, minor);
> +
>  		drm_debugfs_create_files(pcode_info_debugfs,
>  					 ARRAY_SIZE(pcode_info_debugfs),
>  					 root, minor);
> +	}

[Severity: Medium]
With debugfs_residencies now exposed on XE_CRESCENTISLAND alongside
pcode_info_debugfs, what happens if userspace reads dgfx_pkg_residencies,
dgfx_pcie_link_residencies, or pcode_info while OOBMSM firmware
initialization (cri_late_bind_probe_work) is still pending after
xe_vsec_init()?

In read_residency_counter() (and xe_get_pcode_version()), xe_vsec_get_guid()
is called before xe_pmt_telem_read():

drivers/gpu/drm/xe/xe_debugfs.c:read_residency_counter() {
	ret = xe_vsec_get_guid(xe, &guid);
	if (ret) {
		drm_warn(&xe->drm, "PMT device is not available: %d\n", ret);
		return;
	}

	ret = xe_pmt_telem_read(xe->drm.dev, guid, &residency, offset, sizeof(residency));
}

Unlike xe_pmt_telem_read(), xe_pmt_read_reg(), and xe_pmt_write_reg(), which
all call wait_for_fw(xe) before setting the SoC remapper index and reading
the PMT window, xe_vsec_get_guid() accesses CRI_PUNIT_TELEMETRY_GUID without
waiting for firmware readiness or validating the returned 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 CRI_PUNIT_TELEMETRY_GUID reads as 0 before firmware is ready,
xe_vsec_get_guid() returns 0 with *guid == 0, and xe_pmt_telem_read() then
fails xe_guid_decode() with -ENODEV before reaching wait_for_fw(xe). If the
unready register returns a non-zero bogus value (such as 0xffffffff),
xe_vsec_get_guid() caches it in xe->pmt.punit_guid_cache, permanently
breaking subsequent telemetry reads for the lifetime of the device.

Should xe_vsec_get_guid() call wait_for_fw(xe) and validate the GUID before
caching it in xe->pmt.punit_guid_cache?

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