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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

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 ?

> +
>  # 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?

> +
> +#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;

> +
> +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(

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, 

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);

> +#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?


> +	/* 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;