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

Purkait, Soham
2026-04-20T16:19:50Z

Hi Andi,

On 16-04-2026 17:24, 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?

Sure. I missed it in this series. I think it is better to be in
cover-letter isn't it  ?

FYI the sysfs file (gpu_health) is placed in the device level and behaves as
follows:

$ cat /sys/.../device/gpu_health
[ok] warning critical

$ echo critical > /sys/.../device/gpu_health

$ cat /sys/.../device/gpu_health
ok warning [critical]

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

This is just for the debugging purposes. "response.current_health" is
the health status after this change.

Thanks,
Soham

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