Re: [PATCH v6 2/3] drm/xe/sysctrl: Add RAS error injection debugfs interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] xe: missing little-endian conversion for RAS error injection payload
--

commit 7903eaec1791648b7046d617e95443efac765053
Author: Anoop Vijay <anoop.c.vijay@intel.com>

    drm/xe/sysctrl: Add RAS error injection debugfs interface
    
    Add debugfs interface for exercising System Controller's RAS error
    injection command, used to validate RAS error detection and recovery
    paths.

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c b/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c
> index 6e96b1190e268..272e51f24f7bc 100644
> --- a/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c

[ ... ]

> +static int sysctrl_parse_ras_error_inject(struct xe_device *xe, char *tmp,
> +					  struct xe_sysctrl_diag_ras_err_inj_req *req)
> +{
> +	unsigned int nfields = 0;
> +	char *token;
> +	unsigned long val;
> +
> +	while ((token = strsep(&tmp, " \t\n")) != NULL) {
> +		if (*token == '\0')
> +			continue;
> +
> +		if (kstrtoul(token, 0, &val))
> +			goto inval;
> +
> +		switch (nfields) {
> +		case 0:
> +			if (val > U16_MAX)
> +				goto inval;
> +			req->ras_block_id = val;
> +			break;
> +		case 1:
> +			if (val > U16_MAX)
> +				goto inval;
> +			req->ras_sub_block_id = val;
> +			break;
> +		case 2:
> +			if (val > U16_MAX)
> +				goto inval;
> +			req->err_type = val;
> +			break;
> +		case 3:
> +			if (val > U32_MAX)
> +				goto inval;
> +			req->params = val;
> +			break;

[Severity: Medium]
Do these assignments need cpu_to_le16() and cpu_to_le32() conversions?

In sysctrl_parse_ras_error_inject(), host-native values are assigned
directly to the payload struct. When writing to the ras_error_inject
debugfs file on big-endian architectures, the RAS error injection payload
is sent to the firmware with swapped bytes. This would cause the firmware
to receive incorrect block IDs and parameters.

> +		default:
> +			xe_err(xe, "sysctrl: too many ras_error_inject arguments\n");
> +			return -EINVAL;
> +		}
> +		nfields++;
> +	}

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h b/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
> index 691a699667788..f658c9c54b49f 100644
> --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h

[ ... ]

> +/**
> + * struct xe_sysctrl_diag_ras_err_inj_req - DIAG_RAS_ERR_INJECT request payload
> + *
> + * Request payload for XE_SYSCTRL_CMD_DIAG_RAS_ERR_INJECT. The mailbox layer
> + * prepends the application message header before sending.
> + *
> + * @ras_block_id: RAS block (subsystem) to inject the error into
> + * @ras_sub_block_id: RAS sub-block (IP) within @ras_block_id
> + * @err_type: Type of test error to inject
> + * @reserved: Must be zero
> + * @params: Optional injection parameters (default 0)
> + */
> +struct xe_sysctrl_diag_ras_err_inj_req {
> +	u16 ras_block_id;
> +	u16 ras_sub_block_id;
> +	u16 err_type;
> +	u16 reserved;
> +	u32 params;
> +} __packed;

[Severity: Medium]
Should these fields use __le16 and __le32 instead of u16 and u32?

The sysctrl mailbox interface expects little-endian data, but these hardware
payload fields are declared with host-native types in
struct xe_sysctrl_diag_ras_err_inj_req.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917193026.3234636-5-anoop.c.vijay@intel.com?part=2