Re: [PATCH v3 3/5] drm/xe/xe_ras: Add helper to clear error counter

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

On 4/23/2026 4:31 PM, Raag Jadav wrote:
> On Tue, Apr 21, 2026 at 08:20:59PM +0530, Riana Tauro wrote:
>> Add structures and helper function to clear error counter value.
> ...
>
>> +/**
>> + * xe_ras_clear_error_counter() - Clear error counter value
>> + * @xe: xe device instance
>> + * @severity: Error severity level to be cleared
>> + * @error_id: Error component to be cleared
>> + *
>> + * This function clears the value of a specific RAS error counter based on
>> + * the provided severity and component.
>> + *
>> + * Return: 0 on success, negative error code on failure.
>> + */
>> +int xe_ras_clear_error_counter(struct xe_device *xe, enum drm_xe_ras_error_severity severity,
> Same comment as last patch.

Any reason? clear_error_counter is better since it states what type of 
counter to clear

>
>> +			       u32 error_id)
>> +{
>> +	struct xe_ras_clear_counter_response response = {0};
>> +	struct xe_ras_clear_counter_request request = {0};
>> +	struct xe_sysctrl_mailbox_command command = {0};
>> +	struct xe_ras_error_class *error_class = &request.error_class;
>> +	enum xe_sysctrl_gfsp_cmd cmd = XE_SYSCTRL_CMD_CLEAR_COUNTER;
>> +	size_t rlen;
>> +	u32 status;
> Shouldn't this be int? Or why not just reuse ret?

status from firmware structure is u32. Reused the same

>
>> +	int ret;
>> +
>> +	error_class->common.severity = drm_to_xe_ras_severity[severity];
>> +	error_class->common.component = drm_to_xe_ras_component[error_id];
>> +
>> +	prepare_sysctrl_command(&command, cmd, &request, sizeof(request),
>> +				&response, sizeof(response));
>> +
>> +	guard(xe_pm_runtime)(xe);
>> +	ret = xe_sysctrl_send_command(&xe->sc, &command, &rlen);
>> +	if (ret) {
>> +		xe_err(xe, "sysctrl: cmd %d error ret %d\n", cmd, ret);
>> +		return ret;
>> +	}
>> +
>> +	if (rlen != sizeof(response)) {
>> +		xe_err(xe, "sysctrl: cmd %d response size mismatch. Expected %zu, got %zu\n",
>> +		       cmd, sizeof(response), rlen);
>> +		return -EIO;
>> +	}
>> +
>> +	status = xe_ras_status_to_errno[response.status];
>> +	if (status) {
>> +		xe_err(xe, "sysctrl: cmd %d failed %d\n", cmd, status);
>> +		return status;
> Hm, so I'm wondering if this is useful to end user? Note, anything exposed
> to the user will be hard to change once in place.

These are general error codes. Why do you think it will not be useful to 
end user?
It could be an invalid error counter or a timedout operation.

>
>> +	}
>> +
>> +	return 0;
>> +}
> ...
>
>> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
>> @@ -14,9 +14,11 @@
>>    * enum xe_sysctrl_gfsp_cmd  - Sysctrl Command ID's for GFSP group
>>    *
>>    * @XE_SYSCTRL_CMD_GET_COUNTER: Get error counter value
>> + * @XE_SYSCTRL_CMD_CLEAR_COUNTER: Clear error counter value
>>    */
>>   enum xe_sysctrl_gfsp_cmd  {
>> -	XE_SYSCTRL_CMD_GET_COUNTER = 0x03
>> +	XE_SYSCTRL_CMD_GET_COUNTER = 0x03,
> Redundant churn. Please add comma in original patch.

I followed the previous feedback to avoid trailing commas on final 
members :(

>
>> +	XE_SYSCTRL_CMD_CLEAR_COUNTER = 0x04
> Also please align with tabs for readability.

Sure

Thanks
Riana

>
> Raag
>
>>   };
>>   
>>   enum xe_sysctrl_group {
>> -- 
>> 2.47.1
>>