Re: [PATCH v3 08/12] drm/xe/xe_ras: Report device memory errors using SIGID

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Double-bit ECC memory errors are incorrectly reported with CPER_SEV_INFORMATIONAL severity despite being severe enough to trigger a device reset.

Pre-existing issues:
- [High] Hardware CPER records are silently discarded due to an inverted SIGID check.
--

commit 0369a03d8670385fb852c68b7ab519615f2c4e05
Author: Badal Nilawar <badal.nilawar@intel.com>

drm/xe/xe_ras: Report device memory errors using SIGID

This commit updates device memory error reporting to use the
xe_log_comp_info helper. It maps memory errors like poison, data parity,
and double-bit ECC to this new telemetry interface.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 288dbc0942f58..3a7f80370827e 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c

[ ... ]

> @@ -387,14 +387,17 @@ static u8 handle_device_memory_errors(struct xe_device *xe, struct xe_ras_error_
>  	 */
>  	switch (info->category) {
>  	case XE_RAS_MEMORY_POISON:
> -		xe_info(xe, "[RAS]: Poison error detected\n");
> +		xe_log_comp_info(xe, DEVICE_MEMORY, &arr->counter, sizeof(arr->counter),
> +				 "Poison error detected\n");
>  		break;
>  	case XE_RAS_MEMORY_DATA_PARITY:
> -		xe_info(xe, "[RAS]: Data parity error detected\n");
> +		xe_log_comp_info(xe, DEVICE_MEMORY, &arr->counter, sizeof(arr->counter),
> +				 "Data parity error detected\n");
>  		break;
>  	case XE_RAS_MEMORY_DB_ECC:
> -		xe_info(xe, "[RAS]: Double-bit ECC error detected at sw address 0x%llx\n",
> -			info->sw_address);
> +		xe_log_comp_info(xe, DEVICE_MEMORY, &arr->counter, sizeof(arr->counter),
> +				 "Double-bit ECC error detected at sw address 0x%llx\n",
> +				 info->sw_address);

[Severity: Medium]
Will this incorrectly report double-bit ECC memory errors with
CPER_SEV_INFORMATIONAL severity despite being severe enough to trigger a
device reset?

The xe_log_comp_info macro hardcodes the CPER severity to
CPER_SEV_INFORMATIONAL. After logging, the switch statement falls through
to the default case and returns XE_RAS_RECOVERY_ACTION_RESET. 

Could this break automated fleet management that acts on CPER severities
because a severe uncorrectable double-bit ECC error is logged as
informational while the driver actually resets the GPU?

>  		/* TODO: Add page offlining for Double-bit ECC error */
>  		fallthrough;
>  	default:
>  		return XE_RAS_RECOVERY_ACTION_RESET;
>  	}

[ ... ]


This isn't a bug introduced by this patch, but looking at the broader
telemetry handling in drivers/gpu/drm/xe/xe_cper.c:

> void xe_emit_hardware_error_cper(struct pci_dev *pdev, int cper_sev, enum xe_sigid sigid,
> 				 struct xe_ras_error_class *counter,
> 				 struct xe_ras_get_counter_response *response)
> {
> 	struct xe_device *xe = pdev_to_xe_device(pdev);
>       ...
> 
> 	if ((int)sigid >= INTEL_SIGID_GPU_XE_HARDWARE_START)
> 		return;
>       ...

[Severity: High]
Are hardware CPER records being silently discarded here due to an inverted
SIGID check?

When xe_emit_hardware_error_cper() is called to emit a record for a hardware
error, the sigid parameter is greater than or equal to
INTEL_SIGID_GPU_XE_HARDWARE_START, which is the base value for all hardware
SIGIDs. 

This condition evaluates to true, causing the function to immediately return
before any CPER record is emitted, rendering the logging API calls
completely ineffective for hardware faults.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906172604.2215987-14-badal.nilawar@intel.com?part=8