Re: [PATCH v1 2/2] drm/xe/xe_ras: Add RAS support for GPU health indicator

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Apr 16, 2026 at 01:54:18PM +0200, Andi Shyti wrote:
> Hi Soham,
> 
> On Thu, Apr 16, 2026 at 03:06:10PM +0530, Soham Purkait wrote:
> > GPU health indicator exposes a single sysfs interface, gpu_health,
> > at the device level, allowing administrators and management tools to
> > query the GPU health status. The interface permits both read and write
> > operations on PF and native functions, while on VFs it is exposed as
> > read-only.
> 
> Can you describe better the interfaces? The input and output
> values?
> 
> > v1:
> >  - gpu_health is read-write on PFs and native functions. It is read-only
> >    on VFs. VF write attempts are rejected.
> 
> Are you adding a changelog for V1?
> 
> > Signed-off-by: Soham Purkait <soham.purkait@intel.com>
> 
> ...
> 
> > +static const char * const gpu_health_states[] = { "ok", "warning", "critical" };
> > +static const char * const gpu_health_fmt[] = {
> > +	"[%s] %s %s\n",
> > +	"%s [%s] %s\n",
> > +	"%s %s [%s]\n",
> > +};
> 
> Please, don't use complex sentences in sysfs outputs. Use a
> single string/character/value

I like this one better. So we don't need to have an uAPI entry to define
the meaning of 0, 1, 2.

Regarding the sysfs rules, as long as it is one entry per sysfs we should
be compliant with the rule. So, we should be good here.

This style is consistent with the style used in /sys/power/ entries for instance:

$ cat /sys/power/mem_sleep
[s2idle] deep

$ cat /sys/power/state
freeze mem disk

on everything else I agree with Andi.

> 
> ...
> 
> > +static ssize_t gpu_health_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct xe_device *xe = kdev_to_xe_device(dev);
> > +	struct xe_sysctrl_mailbox_command command = {0};
> > +	struct xe_ras_health_get_response response = {0};
> > +	struct xe_ras_health_get_input request = {0};
> > +	u8 health;
> > +	int ret;
> > +	size_t rlen = 0;
> > +
> > +	prepare_sysctrl_command(&command, XE_SYSCTRL_CMD_GET_HEALTH, &request,
> > +				sizeof(request), &response, sizeof(response));
> > +	ret = xe_sysctrl_send_command(&xe->sc, &command, &rlen);
> > +	if (ret) {
> > +		xe_err(xe, "[RAS]: Sysctrl error ret %d\n", ret);
> > +		return -EIO;
> 
> why not return ret? (same goes for the rest of the function and
> the store() function).
> 
> > +	}
> > +	if (rlen != sizeof(response)) {
> > +		xe_err(xe,
> > +		       "[RAS]: invalid Sysctrl response length %zu (expected %zu)\n",
> > +		       rlen, sizeof(response));
> > +		return -EIO;
> > +	}
> > +	if (response.current_health >= ARRAY_SIZE(gpu_health_states)) {
> > +		xe_err(xe, "[RAS]: invalid health state %u from Sysctrl\n",
> > +		       response.current_health);
> > +		return -EIO;
> > +	}
> 
> ...
> 
> > +static ssize_t gpu_health_store(struct device *dev, struct device_attribute *attr,
> > +				const char *buf, size_t count)
> > +{
> 
> ...
> 
> > +	if (IS_SRIOV_VF(xe)) {
> > +		xe_dbg(xe, "[RAS]: GPU health state update rejected on VF\n");
> > +		return -EPERM;
> > +	}
> 
> This is redundant as this function wouldn't be used for sriov.
> 
> > +	state = sysfs_match_string(gpu_health_states,
> > +				   buf);
> > +	if (state < 0)
> > +		return -EINVAL;
> > +
> > +	request.new_health = (xe_ras_health_status_t)state;
> > +
> > +	prepare_sysctrl_command(&command, XE_SYSCTRL_CMD_SET_HEALTH, &request,
> > +				sizeof(request), &response, sizeof(response));
> > +	ret = xe_sysctrl_send_command(&xe->sc, &command, &rlen);
> > +	if (ret) {
> > +		xe_err(xe, "[RAS]: Sysctrl error ret %d\n", ret);
> > +		return -EIO;
> > +	}
> > +	if (rlen != sizeof(response)) {
> > +		xe_err(xe,
> > +		       "[RAS]: invalid Sysctrl response length %zu (expected %zu)\n",
> > +		       rlen, sizeof(response));
> > +		return -EIO;
> > +	}
> > +	if (response.current_health >= ARRAY_SIZE(gpu_health_states)) {
> > +		xe_err(xe, "[RAS]: invalid health state %u from Sysctrl\n",
> > +		       response.current_health);
> > +		return -EIO;
> > +	}
> > +
> > +	health = response.current_health;
> > +
> > +	xe_dbg(xe, "[RAS]: current GPU health state=%d (%s)\n",
> > +	       health, gpu_health_states[health]);
> > +
> 
> BTW, why do we need the field response.operation_status that is
> not used at all here?
> 
> > +	return count;
> > +}
> > +
> > +static struct device_attribute dev_attr_gpu_health_rw =
> > +	__ATTR_RW_MODE(gpu_health, 0600);
> > +
> > +static struct device_attribute dev_attr_gpu_health_ro =
> > +	__ATTR_RO_MODE(gpu_health, 0400);
> > +
> > +static struct device_attribute *gpu_health_attr(struct xe_device *xe)
> > +{
> > +	return IS_SRIOV_VF(xe) ? &dev_attr_gpu_health_ro : &dev_attr_gpu_health_rw;
> > +}
> 
> ...
> 
> > +static void gpu_health_indicator_sysfs_init(struct xe_device *xe)
> > +{
> > +	struct device *dev = xe->drm.dev;
> > +	int err;
> > +
> > +	err = device_create_file(dev, gpu_health_attr(xe));
> > +	if (err)
> > +		goto err;
> 
> Please, don't use goto this way. If you need only one log out of
> the outcome of this function, print it in _init(): make this
> an int function, return err and check err in the calling
> function.
> 
> Andi
> 
> > +
> > +	err = devm_add_action_or_reset(dev, gpu_health_sysfs_fini, dev);
> > +	if (err)
> > +		goto err;
> > +
> > +	return;
> > +
> > +err:
> > +	xe_err(xe, "[RAS]: failed to initialize GPU health sysfs, err=%d\n", err);
> > +}
> > +
> > +/**
> > + * xe_ras_init - Initialize Xe RAS
> > + * @xe: xe device instance
> > + *
> > + * Initialize Xe RAS
> > + */
> > +void xe_ras_init(struct xe_device *xe)
> > +{
> > +	if (!xe->info.has_sysctrl)
> > +		return;
> > +
> > +	gpu_health_indicator_sysfs_init(xe);
> > +}