Re: [PATCH v5 03/13] drm/xe/cper: Initialize Intel-specific section header

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe/cper: cper_init_intel_hdr() never sets unknown sig_id to U32_MAX
--

commit 1eb07867e667177c6634aadbe7bf46699728e40f
Author: Badal Nilawar <badal.nilawar@intel.com>

drm/xe/cper: Initialize Intel-specific section header

This commit defines struct xe_cper_intel_hdr for the Intel GPU error
section of a CPER record and adds helpers in drivers/gpu/drm/xe/xe_cper.c
to initialize it from a RAS error counter response.

> diff --git a/drivers/gpu/drm/xe/xe_cper.c b/drivers/gpu/drm/xe/xe_cper.c
> index a9044e708caca..3d7657517223a 100644
> --- a/drivers/gpu/drm/xe/xe_cper.c
> +++ b/drivers/gpu/drm/xe/xe_cper.c

[ ... ]

> +static bool is_valid_sigid(u32 sigid)
> +{
> +	return sigid != U32_MAX;
> +}
> +
>  static bool is_hw_sigid(enum xe_sigid sigid)
>  {
>  	return in_range(sigid, INTEL_SIGID_GPU_XE_HARDWARE_START, INTEL_SIGID_BATCH);
>  }
>  
> +static void cper_init_intel_hdr(struct xe_device *xe,
> +				const struct xe_ras_error_class *counter,
> +				u64 first_timestamp, u32 sig_id, u32 error_count,
> +				struct xe_cper_intel_hdr *ihdr)
> +{
> +	ihdr->counter = *counter;
> +	ihdr->validation_bits |= XE_CPER_VALID_LOCATION;
> +
> +	if (first_timestamp) {
> +		ihdr->first_timestamp = first_timestamp;
> +		ihdr->validation_bits |= XE_CPER_VALID_FIRST_TIMESTAMP;
> +	}
> +
> +	if (is_valid_sigid(sig_id)) {
> +		ihdr->sig_id = sig_id;
> +		ihdr->validation_bits |= XE_CPER_VALID_SIG_ID;
> +	}

[Severity: Medium]
In drivers/gpu/drm/xe/xe_cper_types.h, struct xe_cper_intel_hdr documents
sig_id as:

	/** @sig_id: Aggregated error class SIG ID; set to U32_MAX if unknown */
	u32 sig_id;

However, cper_emit_counter() zero-initializes struct xe_cper_intel_hdr ihdr
before calling cper_init_intel_hdr(). When sig_id is U32_MAX,
is_valid_sigid(sig_id) returns false and skips the assignment, leaving
ihdr->sig_id as 0 instead of U32_MAX.

Conversely, if INTEL_SIGID_INVALID (0) is passed, is_valid_sigid(sig_id)
returns true, leaving ihdr->sig_id as 0 while also setting
XE_CPER_VALID_SIG_ID in ihdr->validation_bits.

Should cper_init_intel_hdr() explicitly set ihdr->sig_id to U32_MAX when
sig_id is unknown, and should is_valid_sigid() also reject
INTEL_SIGID_INVALID?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023043422.681260-15-badal.nilawar@intel.com?part=3