Re: [PATCH v4 2/6] drm/xe/xe_ras: Handle page offline requests for device memory ecc errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/xe: handle_page_offline() masks address fault errors, preventing GPU reset
- [Medium] drm/xe: missing __le64/__le32 in xe_ras_page_offline_request corrupts data on BE
--

commit dfae5aa8d25c44eda9036e3a3cde9ebb95f9803e
Author: Riana Tauro <riana.tauro@intel.com>
Subject: drm/xe/xe_ras: Handle page offline requests for device memory ecc errors

This commit adds support for sending page offline/remove requests to the
system controller and uses it for device memory ECC error handling. Pages
are soft-offlined depending on whether they belong to critical buffer
objects or standard user space.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
[ ... ]
> @@ -201,6 +204,127 @@ static inline const char *comp_to_str(u8 component)
[ ... ]
> +static int handle_page_offline(struct xe_device *xe, u64 page_address, bool notify_fw)
> +{
[ ... ]
> +	switch (ret) {
> +	case 0:
[ ... ]
> +	case -ENOMEM:
> +		xe_log_err(xe, DEVICE_MEMORY, ret,
> +			   "Poison detected at physical address 0x%llx, failed to soft-offline\n",
> +			   page_address);
> +		return ret;
> +	default:
> +		xe_log_err(xe, DEVICE_MEMORY, ret, "Failed to handle address fault at physical address 0x%llx\n",
> +			   page_address);
> +		return 0;

[Severity: High]
Does returning 0 here unintentionally mask unexpected errors from
xe_ttm_vram_handle_addr_fault?

If soft-offlining a page fails with an unexpected error such as -EINVAL
or -ENOENT, returning 0 causes the caller in handle_device_memory_errors
to assume the recovery was successful. This skips the required Secondary
Bus Reset, which leaves the system running with an active corrupted memory
page. Should this propagate the error to ensure a reset is triggered?

> +	}
[ ... ]
> @@ -368,11 +492,12 @@ static u8 handle_soc_internal_errors(struct xe_device *xe, struct xe_ras_error_a
[ ... ]
>  	case XE_RAS_MEMORY_DB_ECC:
> -		xe_info(xe, "[RAS]: Double-bit ECC error detected at sw address 0x%llx\n",
> -			info->sw_address);
> -		/* TODO: Add page offlining for Double-bit ECC error */
> -		fallthrough;
> +		ret = handle_page_offline(xe, info->sw_address, true);
> +		if (ret)
> +			return XE_RAS_RECOVERY_ACTION_RESET;
> +		break;
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_ras_types.h b/drivers/gpu/drm/xe/xe_ras_types.h
> --- a/drivers/gpu/drm/xe/xe_ras_types.h
> +++ b/drivers/gpu/drm/xe/xe_ras_types.h
[ ... ]
> @@ -295,6 +308,28 @@ struct xe_ras_memory_error {
[ ... ]
> +struct xe_ras_page_offline_request {
> +	/** @page_address: Page address (4KB aligned) */
> +	u64 page_address;
> +	/** @action: Action to be performed, see &enum xe_ras_page_action */
> +	u32 action;

[Severity: Medium]
Could this struct layout cause data corruption on big-endian architectures?

The struct uses native host types for the 64-bit page_address and 32-bit
action instead of explicitly specifying __le64 and __le32. When the sysctrl
mailbox transmits this data via MMIO writel in 32-bit chunks, a big-endian
system will word-swap the 64-bit value, sending the most-significant 32 bits
where the firmware expects the least-significant 32 bits.

Since send_page_offline_cmd assigns these fields directly without any
cpu_to_le64 conversion, should this struct definition use little-endian types
to safely maintain the hardware ABI contract?

> +	/** @reserved: Reserved for future use */
> +	u32 reserved;
> +} __packed;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005090156.1233748-8-riana.tauro@intel.com?part=2