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

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

On Thu, Apr 23, 2026 at 04:46:27PM +0530, Tauro, Riana wrote:
> 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

I thought it'd help with the wrapping but ofcourse it's a personal
preference.

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

Yes, but the converted error code is not u32.

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

Fair, but then I'd expect them to be present for all commands, or perhaps
I failed to read the specs correctly, did I? ;)

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

Yes, it's for "final" members which often have _MAX suffix and aren't
expected to change once in place.

Raag

> > > +	XE_SYSCTRL_CMD_CLEAR_COUNTER = 0x04
> > Also please align with tabs for readability.
> 
> Sure
> 
> Thanks
> Riana
> 
> > 
> > Raag
> > 
> > >   };
> > >   enum xe_sysctrl_group {
> > > -- 
> > > 2.47.1
> > >