Re: [PATCH v4 3/5] drm/xe/ras: Add support for error threshold

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `threshold` and `reserved` fields are incorrectly swapped in `struct xe_ras_set_threshold_response`.
--

commit b88a93d0104e4bd53859ba2c4b041330bc44ceef
Author: Raag Jadav <raag.jadav@intel.com>

drm/xe/ras: Add support for error threshold

System controller allows getting/setting per counter threshold for
correctable errors, which it uses to raise error events to the driver.
Get/set it using the respective mailbox command.

> diff --git a/drivers/gpu/drm/xe/xe_ras_types.h b/drivers/gpu/drm/xe/xe_ras_types.h
> index 6688e11f57a86..747b651880cd5 100644
> --- a/drivers/gpu/drm/xe/xe_ras_types.h
> +++ b/drivers/gpu/drm/xe/xe_ras_types.h
[ ... ]
> +struct xe_ras_get_threshold_response {
> +	/** @counter: Counter ID */
> +	struct xe_ras_error_class counter;
> +	/** @threshold: Current threshold of the counter */
> +	u32 threshold;
> +	/** @reserved: Reserved for future use */
> +	u32 reserved[4];
> +} __packed;
[ ... ]
> +struct xe_ras_set_threshold_request {
> +	/** @counter: Counter to set threshold for */
> +	struct xe_ras_error_class counter;
> +	/** @threshold: Threshold to be set */
> +	u32 threshold;
> +	/** @reserved: Reserved for future use */
> +	u32 reserved;
> +} __packed;
> +
> +/**
> + * struct xe_ras_set_threshold_response - Response structure for set threshold
> + */
> +struct xe_ras_set_threshold_response {
> +	/** @counter: Counter ID */
> +	struct xe_ras_error_class counter;
> +	/** @reserved: Reserved */
> +	u32 reserved;
> +	/** @threshold: Updated threshold */
> +	u32 threshold;
> +	/** @status: Operation status */
> +	u32 status;

[Severity: Medium]
Are the reserved and threshold fields swapped in this structure compared to
the firmware specification?

In xe_ras_set_threshold_request and xe_ras_get_threshold_response, the
threshold payload immediately follows the counter struct. Placing reserved
before threshold here shifts the threshold and status fields to different
offsets.

If this is a mistake, it could cause ras_status_to_errno() to read the
wrong offset and silently ignore firmware errors when xe_ras_set_threshold()
checks response.status, as well as log an incorrect threshold value.

> +	/** @reserved1: Reserved for future use */
> +	u32 reserved1[2];
> +} __packed;
> +
>  #endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623101043.255897-1-raag.jadav@intel.com?part=3