Re: [PATCH v13 13/16] drm/xe/vsec: Add PMT GUID internal access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/xe/vsec: missing wait_for_fw() and zero-GUID check in xe_vsec_get_guid()
- [Low] drm/xe/vsec: missing kernel-doc for xe_vsec_get_guid()
--

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

drm/xe/vsec: Add PMT GUID internal access

This commit renames PUNIT_TELEMETRY_GUID to BMG_PUNIT_TELEMETRY_GUID, adds
CRI_PUNIT_TELEMETRY_GUID for Crescent Island, and introduces
xe_vsec_get_guid() to read and cache the PUNIT telemetry GUID based on the
platform.

> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index cc8e0e6bed9a8..7430185ad41a9 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -490,6 +490,8 @@ struct xe_device {
>  		struct delayed_work work;
>  		/** @pmt.retry_count: late-bind probe retry */
>  		u32 retry_count;
> +		/** @pmt.punit_guid_cache: cache of the PUINT GUID */
> +		u32 punit_guid_cache;

[Severity: Low]
This isn't a bug, but there is a minor typo in the kernel-doc comment here
(PUINT instead of PUNIT).

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 5384fef5e5ac5..1ed904ec14965 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -579,6 +579,51 @@ static void vsec_disable_late_bind_work(void *arg)
>  		xe_pm_runtime_put(xe);
>  }
>  
> +int xe_vsec_get_guid(struct xe_device *xe, u32 *guid)
> +{

[Severity: Low]
This isn't a bug, but since Documentation/gpu/xe/xe_vsec.rst pulls
kernel-doc from drivers/gpu/drm/xe/xe_vsec.c and the other exported VSEC
entry points (xe_pmt_telem_read() and xe_vsec_init()) are documented,
should xe_vsec_get_guid() also have a kernel-doc comment describing its
parameters, runtime PM and xe->pmt.lock locking expectations, and return
values?

> +	struct xe_mmio *mmio = xe_root_tile_mmio(xe);
> +	int ret = 0;
> +
> +	xe_assert(xe, !IS_SRIOV_VF(xe));
> +
> +	if (!xe_pm_runtime_get_if_active(xe))
> +		return -ENODATA;
> +
> +	mutex_lock(&xe->pmt.lock);
> +
> +	if (xe->pmt.punit_guid_cache) {
> +		*guid = xe->pmt.punit_guid_cache;
> +		goto unlock;
> +	}
> +
> +	switch (xe->info.platform) {
> +	case XE_BATTLEMAGE:
> +		*guid = xe_mmio_read32(mmio, BMG_PUNIT_TELEMETRY_GUID);
> +		break;
> +
> +	case XE_CRESCENTISLAND:
> +		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +		*guid = xe_mmio_read32(mmio, CRI_PUNIT_TELEMETRY_GUID);
> +		break;
> +
> +	default:
> +		*guid = 0;
> +		drm_err(&xe->drm, "Unsupported platform: %u\n", xe->info.platform);
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	/* the GUID is static, read once and cache for all requests */
> +	if (*guid)
> +		xe->pmt.punit_guid_cache = *guid;
> +
> +unlock:
> +	mutex_unlock(&xe->pmt.lock);
> +	xe_pm_runtime_put(xe);
> +
> +	return ret;
> +}

[Severity: Medium]
On XE_CRESCENTISLAND, can xe_vsec_get_guid() be called before the late-bound
OOBMSM firmware has finished initializing (for example, via debugfs or hwmon
after xe_vsec_init() defers discovery to cri_late_bind_probe_work(), or right
after runtime/system resume)?

The other PMT access helpers in drivers/gpu/drm/xe/xe_vsec.c (such as
xe_pmt_telem_read() and xe_pmt_read_reg()) check wait_for_fw(xe) before
taking xe->pmt.lock, programming CRI_IDX_TELEM_DISCOVERY, and reading the
discovery window:

drivers/gpu/drm/xe/xe_vsec.c:xe_pmt_read_reg() {
	...
	xe_pm_runtime_get(xe);
	if (!wait_for_fw(xe)) {
		ret = -ENODATA;
		goto runtime_exit;
	}

	scoped_guard(mutex, &xe->pmt.lock) {
		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);

		*reg = readl(disc_addr);
	}
	...
}

Without wait_for_fw(xe) in xe_vsec_get_guid(), if reading
CRI_PUNIT_TELEMETRY_GUID before firmware is ready returns a non-zero bogus
value (such as 0xffffffff), won't xe->pmt.punit_guid_cache permanently cache
that invalid value for the lifetime of the device?

Conversely, if xe_mmio_read32() returns 0 (either before firmware is ready
or when !mmio_available(mmio)), xe_vsec_get_guid() skips caching *guid, but
ret remains 0. Should xe_vsec_get_guid() wait for firmware readiness via
wait_for_fw(xe) and return an error code when *guid is 0 (or invalid) rather
than returning 0 with *guid == 0?

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