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

["Tauro, Riana" <riana.tauro@intel.com>]

On 5/18/2026 11:28 PM, Raag Jadav wrote:
> 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.

Changing it to switch.

>
>> +};
>> +
>>   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?

This is protocol error. Would be misleading to userspace. -EIO is better 
for unknown errors
indicating an hardware error

>
>> +};
> 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>"?

There is no value here from firmware.
I am adding

"xe_dbg(xe, "[RAS]: clear counter for %s %s\n", 
comp_to_str(counter->common.component),
            sev_to_str(counter->common.severity));"

Hope this is okay

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

prefix should also be okay for variants.

>
>> +	/** @clear_timestamp: Timestamp when the counter was cleared */
>> +	u64 clear_timestamp;
> It's already 'clear_counter', so perhaps just 'timestamp'?

sure

Thanks
Riana

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