Re: [PATCH v4 02/12] drm/xe/cper: Retrieve the error counter record for CPER reporting

[Raag Jadav <raag.jadav@intel.com>]

On Fri, Oct 09, 2026 at 04:16:05PM +0530, Badal Nilawar wrote:
> Retrieve error counter record required to build a CPER record.
> 
> Assisted-by: Copilot:Claude-Opus-5.5
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
> v4:
>   - Drop redundant xe_ras_counter_is_valid() check (Raag)
>   - Add might_sleep() to assert that the caller is in a sleepable
>     context (sashiko)
>   - Add xe_device_assert_mem_access() to assert the outer runtime PM
>     wake reference requirement (sashiko)
> ---
>  drivers/gpu/drm/xe/xe_cper.c | 14 ++++++++++-
>  drivers/gpu/drm/xe/xe_ras.c  | 45 ++++++++++++++++++++++++------------
>  drivers/gpu/drm/xe/xe_ras.h  |  2 ++
>  3 files changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_cper.c b/drivers/gpu/drm/xe/xe_cper.c
> index c5caa0e8f496..4f0ed5096aac 100644
> --- a/drivers/gpu/drm/xe/xe_cper.c
> +++ b/drivers/gpu/drm/xe/xe_cper.c
> @@ -4,10 +4,13 @@
>   */
>  
>  #include <linux/err.h>
> +#include <linux/kernel.h>
>  #include <linux/minmax.h>
>  
>  #include "xe_cper.h"
>  #include "xe_device.h"
> +#include "xe_printk.h"
> +#include "xe_ras.h"
>  #include "xe_ras_types.h"
>  
>  static bool is_hw_sigid(enum xe_sigid sigid)
> @@ -18,7 +21,16 @@ static bool is_hw_sigid(enum xe_sigid sigid)
>  static void xe_cper_emit_hw_err(struct xe_device *xe, int cper_sev, enum xe_sigid sigid,
>  				const struct xe_ras_error_class *error_class)
>  {
> -	/* TODO */
> +	struct xe_ras_get_counter_response local_resp;

Why 'local'? Do we have any other?

> +	/* Fetching the counter sends a sysctrl command */
> +	might_sleep();
> +	xe_device_assert_mem_access(xe);
> +
> +	if (xe_ras_get_counter_response(xe, error_class, &local_resp)) {

Make the naming consistent with existing code, i.e.
xe_ras_get_counter_response(xe, &counter, &response)

> +		xe_err(xe, "CPER: failed to get counter, skipping record\n");
> +		return;
> +	}
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 7a85735c57d5..5cfc9beff8c1 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
> @@ -103,8 +103,6 @@ static const char * const gpu_health_states[] = {
>  };
>  static_assert(ARRAY_SIZE(gpu_health_states) == XE_RAS_HEALTH_MAX);
>  
> -static int get_counter(struct xe_device *xe, struct xe_ras_error_class *counter, u32 *value);
> -
>  static u8 drm_to_xe_ras_severity(u8 severity)
>  {
>  	switch (severity) {
> @@ -284,21 +282,21 @@ static void ras_usp_aer_init(struct xe_device *xe)
>  static void ras_send_error_event(struct xe_device *xe, u8 severity, u8 component)
>  {
>  	struct xe_ras_error_class counter = {0};
> +	struct xe_ras_get_counter_response response = {0};

Hm, not all users of get_counter() are interested in the entire response.
So why not have it wrapped? Perhaps something like

static get_counter()
{
	response = {0};
	
	ret = xe_ras_get_counter_response(xe, &counter, &response);
	if (ret)
		return ret;

	*value = response.value;
	return 0;
}

Tidy? ;)

>  	u8 drm_severity, drm_component;
> -	u32 value;
>  	int ret;
>  
>  	counter.common.severity = severity;
>  	counter.common.component = component;
>  
> -	ret = get_counter(xe, &counter, &value);
> +	ret = xe_ras_get_counter_response(xe, &counter, &response);
>  	if (ret)
>  		return;
>  
>  	drm_severity = xe_to_drm_ras_severity(severity);
>  	drm_component = xe_to_drm_ras_component(component);
>  
> -	xe_drm_ras_event(xe, drm_component, drm_severity, value);
> +	xe_drm_ras_event(xe, drm_component, drm_severity, response.value);
>  }
>  
>  static u8 handle_core_compute_errors(struct xe_ras_error_array *arr)
> @@ -433,19 +431,30 @@ void xe_ras_counter_threshold_crossed(struct xe_device *xe,
>  	}
>  }
>  
> -static int get_counter(struct xe_device *xe, struct xe_ras_error_class *counter, u32 *value)
> +/**
> + * xe_ras_get_counter_response() - Get error counter record
> + * @xe: Xe device instance
> + * @counter: RAS error class
> + * @out: Counter record retrieved

Nit: ... to be retrieved

> + * Retrieve the counter record of a specific error class from firmware.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +int xe_ras_get_counter_response(struct xe_device *xe, const struct xe_ras_error_class *counter,
> +				struct xe_ras_get_counter_response *out)

'response'?

>  {
> -	struct xe_ras_get_counter_response response = {0};
>  	struct xe_ras_get_counter_request request = {0};
>  	struct xe_sysctrl_mailbox_command command = {0};
>  	struct xe_ras_error_common *common;
>  	size_t rlen;
>  	int ret;
>  
> +	memset(out, 0, sizeof(*out));

Nack. No modifying input data, the caller is responsible for this.

>  	request.counter = *counter;
>  
>  	xe_sysctrl_create_command(&command, XE_SYSCTRL_GROUP_GFSP, XE_SYSCTRL_CMD_GET_COUNTER,
> -				  &request, sizeof(request), &response, sizeof(response));
> +				  &request, sizeof(request), out, sizeof(*out));
>  
>  	ret = xe_sysctrl_send_command(&xe->sc, &command, &rlen);
>  	if (ret) {
> @@ -453,19 +462,18 @@ static int get_counter(struct xe_device *xe, struct xe_ras_error_class *counter,
>  		return ret;
>  	}
>  
> -	if (rlen != sizeof(response)) {
> +	if (rlen != sizeof(*out)) {
>  		xe_err(xe, "sysctrl: unexpected get counter response length %zu (expected %zu)\n",
> -		       rlen, sizeof(response));
> +		       rlen, sizeof(*out));
>  		return -EIO;
>  	}
>  
> -	if (!ras_counter_is_valid(xe, &response.counter))
> +	if (!ras_counter_is_valid(xe, &out->counter))
>  		return -EBADMSG;
>  
> -	common = &response.counter.common;
> -	*value = response.value;
> +	common = &out->counter.common;
>  
> -	xe_dbg(xe, "[RAS]: get counter %u for %s %s\n", *value, comp_to_str(common->component),
> +	xe_dbg(xe, "[RAS]: get counter %u for %s %s\n", out->value, comp_to_str(common->component),
>  	       sev_to_str(common->severity));
>  
>  	return 0;
> @@ -596,12 +604,19 @@ enum xe_ras_recovery_action xe_ras_process_errors(struct xe_device *xe)
>  int xe_ras_get_counter(struct xe_device *xe, u8 severity, u8 component, u32 *value)
>  {
>  	struct xe_ras_error_class counter = {0};
> +	struct xe_ras_get_counter_response response = {0};

With get_counter() intact, this won't be needed.

Raag

> +	int ret;
>  
>  	counter.common.severity = drm_to_xe_ras_severity(severity);
>  	counter.common.component = drm_to_xe_ras_component(component);
>  
>  	guard(xe_pm_runtime)(xe);
> -	return get_counter(xe, &counter, value);
> +	ret = xe_ras_get_counter_response(xe, &counter, &response);
> +	if (ret)
> +		return ret;
> +
> +	*value = response.value;
> +	return 0;
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/xe/xe_ras.h b/drivers/gpu/drm/xe/xe_ras.h
> index 0b8669f28d56..84700ea63394 100644
> --- a/drivers/gpu/drm/xe/xe_ras.h
> +++ b/drivers/gpu/drm/xe/xe_ras.h
> @@ -20,5 +20,7 @@ int xe_ras_get_threshold(struct xe_device *xe, u8 severity, u8 component, u32 *t
>  int xe_ras_set_threshold(struct xe_device *xe, u8 severity, u8 component, u32 threshold);
>  void xe_ras_init(struct xe_device *xe);
>  enum xe_ras_recovery_action xe_ras_process_errors(struct xe_device *xe);
> +int xe_ras_get_counter_response(struct xe_device *xe, const struct xe_ras_error_class *counter,
> +				struct xe_ras_get_counter_response *out);
>  
>  #endif
> -- 
> 2.54.0
>