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

[Andi Shyti <andi.shyti@linux.intel.com>]

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

...

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