Re: [PATCH v6 3/6] drm/xe/xe_ras: Add support to clear error counter

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

On Thu, May 14, 2026 at 10:52:09AM +0530, Riana Tauro wrote:
> Add structures and helper function to clear error counter value.

...

> +/* RAS response status codes */
> +enum xe_ras_response_status {
> +	XE_RAS_STATUS_SUCCESS = 0,
> +	XE_RAS_STATUS_INVALID_PARAM,
> +	XE_RAS_STATUS_OP_NOT_SUPPORTED,
> +	XE_RAS_STATUS_TIMEOUT,
> +	XE_RAS_STATUS_HARDWARE_FAILURE,
> +	XE_RAS_STATUS_INSUFFICIENT_RESOURCES,
> +	XE_RAS_STATUS_UNKNOWN_ERROR

Nit: 'UNKNOWN' is sufficient.

> +};
> +
>  static const char *const xe_ras_severities[] = {
>  	[XE_RAS_SEV_NOT_SUPPORTED]		= "Not Supported",
>  	[XE_RAS_SEV_CORRECTABLE]		= "Correctable Error",
> @@ -53,6 +64,16 @@ static const char *const xe_ras_components[] = {
>  };
>  static_assert(ARRAY_SIZE(xe_ras_components) == XE_RAS_COMP_MAX);
>  
> +static const int ras_status_to_errno_map[] = {

Nit: Redundant 'map', just use 'xe' prefix.

> +	[XE_RAS_STATUS_SUCCESS]			= 0,
> +	[XE_RAS_STATUS_INVALID_PARAM]		= -EINVAL,
> +	[XE_RAS_STATUS_OP_NOT_SUPPORTED]	= -EOPNOTSUPP,
> +	[XE_RAS_STATUS_TIMEOUT]			= -ETIMEDOUT,
> +	[XE_RAS_STATUS_HARDWARE_FAILURE]	= -EIO,
> +	[XE_RAS_STATUS_INSUFFICIENT_RESOURCES]	= -ENOSPC,
> +	[XE_RAS_STATUS_UNKNOWN_ERROR]		= -EIO

-EPROTO?

> +};

I think switch() would be good enough but if you want to use array, let's
also have static_assert() against XE_RAS_STATUS_MAX.

...

> +static int ras_status_to_errno(enum xe_ras_response_status status)
> +{
> +	if (status > XE_RAS_STATUS_UNKNOWN_ERROR)

With _MAX in place, we can make this consistent with other similar helpers.

> +		status = XE_RAS_STATUS_UNKNOWN_ERROR;
> +
> +	return ras_status_to_errno_map[status];
> +}

...

> +	xe_dbg(xe, "[RAS]: %s %s counter cleared\n",

Let's try to make this consistent across series.

How about "<component> <severity> <operation>:<value>"?

> +	       comp_to_str(response.counter.common.component),
> +	       sev_to_str(response.counter.common.severity));

Same comment as last patch.

...

> +/**
> + * struct xe_ras_clear_counter_request - Request for clearing an error counter
> + */
> +struct xe_ras_clear_counter_request {
> +	/** @counter: Counter class to be cleared */
> +	struct xe_ras_error_class counter;
> +	/** @reserved: Reserved for future use */
> +	u32 reserved;
> +} __packed;
> +
> +/**
> + * struct xe_ras_clear_counter_response - Response after clearing an error counter
> + */
> +struct xe_ras_clear_counter_response {
> +	/** @counter: Counter class that was cleared */
> +	struct xe_ras_error_class counter;
> +	/** @prev_value: Counter value before clearing */
> +	u32 prev_value;

Nit: Postfix is a bit more suitable for variants, i.e. value_prev.

> +	/** @clear_timestamp: Timestamp when the counter was cleared */
> +	u64 clear_timestamp;

It's already 'clear_counter', so perhaps just 'timestamp'?

Raag

> +	/** @status: Status of the clear operation */
> +	u32 status;
> +	/** @reserved: Reserved for future use */
> +	u32 reserved[3];
> +} __packed;