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

["Purkait, Soham" <soham.purkait@intel.com>]

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