Re: [PATCH v2 07/10] drm/xe/vsec: Crescent Island PMT decode

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The Crescent Island (CRI) PMT initialization is inaccessible due to missing platform integration in the initialization path, rendering the new decode logic dead code.

Pre-existing issues:
- [High] Unhandled cap_type and instance combinations in guid_decode functions silently return success with a 0 offset, leading to incorrect base register reads.
--

--- Patch [7]: [PATCH v2 07/10] drm/xe/vsec: Crescent Island PMT decode ---
commit 8ae722df1dc1347897983eec5f487f23bc897a93
Author: Michael J. Ruhl <[email protected]>

drm/xe/vsec: Crescent Island PMT decode

This commit adds support for decoding the PMT data area on Crescent
Island (CRI) devices. It introduces new magic numbers, a new decode
function for CRI, and updates the telemetry read callback to use a
device-specific base offset.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 6345b0b4b26b8..dc42b9492428a 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
[ ... ]
> @@ -138,45 +162,92 @@ enum capability {
>  	WATCHER,
>  };
>  
> -static int xe_guid_decode(u32 guid, int *index, u32 *offset)
> +static int bmg_guid_decode(u32 guid, int *index, u32 *offset)
>  {
>  	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
>  	u32 cap_type  = FIELD_GET(GUID_CAP_TYPE, guid);
[ ... ]
>  	switch (record_id) {
>  	case PUNIT:
> -		*index = 0;
> +		*index = BMG_IDX_TELEM_PUNIT;
>  		if (cap_type == TELEMETRY)
> -			*offset = PUNIT_TELEMETRY_OFFSET;
> +			*offset = BMG_PUNIT_TELEMETRY_OFFSET;
>  		else
> -			*offset = PUNIT_WATCHER_OFFSET;
> +			*offset = BMG_PUNIT_WATCHER_OFFSET;
>  		break;
>  
>  	case OOBMSM_0:
> -		*index = 1;
> +		*index = BMG_IDX_TELEM_OOBMSM;
>  		if (cap_type == WATCHER)
> -			*offset = OOBMSM_0_WATCHER_OFFSET;
> +			*offset = BMG_OOBMSM_0_WATCHER_OFFSET;
>  		break;

[Severity: High]
This is a pre-existing issue, but if cap_type is not WATCHER here, this
case silently falls through, leaves offset as 0, and returns 0 indicating
success.

Will this misdirect reads to the base PMT offset since offset remains 0,
potentially leaking data or causing hardware hangs?

> +static int cri_guid_decode(u32 guid, int *index, u32 *offset)
> +{
[ ... ]
> +	case OOBMSM_0:
> +		*index = CRI_IDX_TELEM_OOBMSM;
> +		switch (instance) {
> +		case 0:
> +			if (cap_type == WATCHER) {
> +				*index = CRI_IDX_WATCHER_OOBMSM;
> +				*offset = CRI_OOBMSM_WATCHER_OFFSET;
> +			}
> +			break;

[Severity: High]
Similarly, in this newly added function, if cap_type is not WATCHER for
instance 0, it silently breaks and returns 0.

Should this code return an error code like -EINVAL instead of falling
through to a success return with an offset of 0?

[ ... ]
> @@ -286,6 +374,7 @@ void xe_vsec_init(struct xe_device *xe)
>  	case XE_VSEC_BMG:
>  		if (!xe->soc_remapper.set_telem_region)
>  			return;
> +		xe->pmt.base_offset = BMG_TELEMETRY_OFFSET;
>  		info->priv_data = &xe_pmt_cb;
>  		break;
>  	default:

[Severity: Medium]
Is the XE_VSEC_CRI case missing from this switch statement?

If get_platform_info() correctly returns XE_VSEC_CRI, there appears to be
no case here to handle it, meaning base_offset and the pmt callbacks will
remain uninitialized.

Additionally, if get_platform_info() still filters out platforms greater
than XE_BATTLEMAGE, does it also need to be updated to allow the CRI
platform to be returned so this initialization path can be reached?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7