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

Andi Shyti
2026-04-16T11:54:25Z

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