Re: [PATCH v3 01/12] drm/xe/cper: Hardware error CPER reporting from xe_log

["Nilawar, Badal" <badal.nilawar@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6816 bytes --]


On 07-09-2026 18:08, Michal Wajdeczko wrote:
>
> On 9/6/2026 7:26 PM, Badal Nilawar wrote:
>> Introduce xe_emit_hardware_error_cper() as public entry point
>> for CPER reporting.
>>
>> Wire xe_log to route hardware SIGIDs through the new helper.
>> No functional change is intended yet, as the CPER emission logic
>> is added in follow-up patches.
>>
>> Signed-off-by: Badal Nilawar<badal.nilawar@intel.com>
>> ---
>>   drivers/gpu/drm/xe/Makefile  |  2 ++
>>   drivers/gpu/drm/xe/xe_cper.c | 37 ++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_cper.h | 25 ++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_log.c  | 17 +++++++++++------
>>   4 files changed, 75 insertions(+), 6 deletions(-)
>>   create mode 100644 drivers/gpu/drm/xe/xe_cper.c
>>   create mode 100644 drivers/gpu/drm/xe/xe_cper.h
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index 67b8b5477639..06b064add77d 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -166,6 +166,8 @@ xe-$(CONFIG_HWMON) += xe_hwmon.o
>>   xe-$(CONFIG_PERF_EVENTS) += xe_pmu.o
>>   xe-$(CONFIG_CONFIGFS_FS) += xe_configfs.o
>>   
>> +xe-$(CONFIG_UEFI_CPER_X86) += xe_cper.o
> shouldn't we use just CONFIG_UEFI_CPER ?
Will check.
>
>> +
>>   # graphics virtualization (SR-IOV) support
>>   xe-y += \
>>   	xe_gt_sriov_vf.o \
>> diff --git a/drivers/gpu/drm/xe/xe_cper.c b/drivers/gpu/drm/xe/xe_cper.c
>> new file mode 100644
>> index 000000000000..e8017e3ee3a0
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_cper.c
>> @@ -0,0 +1,37 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2026 Intel Corporation
>> + */
>> +
>> +#include <linux/pci.h>
>> +
>> +#include <drm/drm_print.h>
> do we need this?
will add in the patch where it is needed.
>
>> +
>> +#include "xe_cper.h"
>> +#include "xe_device.h"
>> +#include "xe_ras_types.h"
>> +
>> +/**
>> + * xe_emit_hardware_error_cper() - Emit a hardware error CPER record
>> + * @pdev: PCI device associated with the Xe device
>> + * @cper_sev: CPER severity
>> + * @sigid: Error signature identifier
>> + * @error_class: Hardware error classification details
>> + * @response: Response of get counter
>> + *
>> + * Emit a CPER record for a hardware error
>> + */
>> +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 (!xe)
>> +		return;
>> +
>> +	if ((int)sigid >= INTEL_SIGID_GPU_XE_HARDWARE_START)
>> +		return;
>> +
>> +	/* TODO */
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_cper.h b/drivers/gpu/drm/xe/xe_cper.h
>> new file mode 100644
>> index 000000000000..c4be7f25a369
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_cper.h
>> @@ -0,0 +1,25 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2026 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_CPER_H_
>> +#define _XE_CPER_H_
>> +
>> +#include "abi/xe_sigid_abi.h"
> maybe just:
>
> 	enum xe_sigid sigid;
Ok.
>
>> +
>> +struct pci_dev;
>> +struct xe_ras_error_class;
>> +struct xe_ras_get_counter_response;
>> +
>> +#if IS_REACHABLE(CONFIG_UEFI_CPER_X86)
>> +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);
> since you're introducing xe_cper component, all its public functions
> shall use xe_cper prefix:
>
> 	xe_cper_emit_hardware_error(

Sure.  Kept _cper in the end to indicate cper record of hardware error 
is being emitted.

>
> and since all HW errors are expected to come from the xe, no need for pdev:
>
> 	xe_cper_emit_hardware_error(struct xe_device *xe,
Ok.
>
> also it's better to let the caller pass whatever data was given in xe_log macros:
>
> 	xe_cper_emit_hardware_error(struct xe_device *xe,
> 				int cper_sev, enum xe_sigid sigid,
> 				const void *data, size_t len)
>
> and do any data validation/conversion inside xe_cper code
>
> and since xe_ras_get_counter_response is now optional, just define another function (when needed) that takes already validated data:
>
> 	xe_cper_emit_hardware_error_details(struct xe_device *xe,
> 				int cper_sev, enum xe_sigid sigid,
> 				const struct xe_ras_error_class *counter,
> 				const struct xe_ras_get_counter_response *response);

Will think about this. Don't want to keep to many layers. May be a one 
more wrapper function in xe_cper.c, which will do validation and decide 
which path hw or fw cper to follow.
IMO inside log_emit_cper itself validation should be done.

>
>> +#else
>> +static inline 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) {}
>> +#endif
>> +#endif /* _XE_CPER_H_ */
>> diff --git a/drivers/gpu/drm/xe/xe_log.c b/drivers/gpu/drm/xe/xe_log.c
>> index 5549ef6966fd..2957adec41aa 100644
>> --- a/drivers/gpu/drm/xe/xe_log.c
>> +++ b/drivers/gpu/drm/xe/xe_log.c
>> @@ -8,17 +8,27 @@
>>   
>>   #include "abi/xe_log_abi.h"
>>   
>> +#include "xe_cper.h"
>>   #include "xe_device.h"
>>   #include "xe_log.h"
>>   #include "xe_printk.h"
>>   
>> +static bool is_hw_sigid(enum xe_sigid sigid)
>> +{
>> +	return (int)sigid >= INTEL_SIGID_GPU_XE_HARDWARE_START;
>> +}
>> +
>>   static void log_emit_cper(struct pci_dev *pdev, int cper_sev, enum xe_sigid sigid,
>>   			  u32 component, u32 location, const void *data, size_t len,
>>   			  struct va_format *vaf)
>>   {
>>   	KUNIT_STATIC_STUB_REDIRECT(log_emit_cper, pdev, cper_sev, sigid,
>>   				   component, location, data, len, vaf);
>> -	/* TODO */
>> +
>> +	if (is_hw_sigid(sigid) && !IS_ERR(data))
>> +		xe_emit_hardware_error_cper(pdev, cper_sev, sigid,
>> +					    (struct xe_ras_error_class *)data, NULL);
> you shouldn't blindly convert data to xe_ras_error_class
> you shall at least check if len == sizeof(xe_ras_error_class)
> and IMO it would be better to move that checks to xe_cper code
> (as maybe we can still emit some CPER records for ERR_PTR data?

Without valid error class no point in emitting hardware CPER. May be as 
software CPER this can be considered later.

Thanks,
Badal

>
>
>> +	/* TODO software CPER */
>>   }
>>   
>>   static const char *log_unknown_component_prefix(u32 component)
>> @@ -100,11 +110,6 @@ static const char *log_location_prefix(struct pci_dev *pdev, u32 location, char
>>   	return buf;
>>   }
>>   
>> -static bool is_hw_sigid(enum xe_sigid sigid)
>> -{
>> -	return (int)sigid >= INTEL_SIGID_GPU_XE_HARDWARE_START;
>> -}
>> -
>>   static bool is_sev_error(int cper_sev)
>>   {
>>   	return cper_sev != CPER_SEV_INFORMATIONAL;

[-- Attachment #2: Type: text/html, Size: 9137 bytes --]