Re: [PATCH v2 1/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] Unhandled errors from `xe_ttm_vram_handle_addr_fault` are silently converted to success (return 0) in `handle_page_offline`, bypassing the required device reset for Double-bit ECC errors.
- [High] Mismatch between hardware page size (XE_PAGE_SIZE) and CPU page size (PAGE_SIZE) causes page offlining to fail on non-4KB page architectures.
--

commit b5f7912325b590064c71dc561b34bee2f792f1f0
Author: Riana Tauro <riana.tauro@intel.com>

drm/xe/xe_ras: Handle page offline requests for device memory ecc errors

This commit adds basic support for sending page offline/decline requests to
the system controller. It uses this to handle device memory ECC errors.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 7a85735c57d58..94ffd08529384 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
[ ... ]
> @@ -201,6 +204,110 @@ static inline const char *comp_to_str(u8 component)
[ ... ]
> +static int handle_page_offline(struct xe_device *xe, u64 page_address, bool send_cmd)
> +{
> +	enum xe_ras_page_action action;
> +	int ret = 0;
> +
> +	if (!IS_ALIGNED(page_address, XE_PAGE_SIZE)) {
> +		xe_log_err(xe, SYSCTRL, -EINVAL, "Unaligned physical page address: 0x%llx\n",
> +			   page_address);
> +		return -EINVAL;
> +	}
> +
> +	ret = xe_ttm_vram_handle_addr_fault(xe, page_address);

[Severity: High]
Can this validation cause page offlining to fail on architectures where the
CPU page size is not 4KB?

Here, the address is validated using XE_PAGE_SIZE (4KB). However, the
downstream function xe_ttm_vram_handle_addr_fault() rejects addresses that
are not aligned to the system's PAGE_SIZE:

xe_ttm_vram_handle_addr_fault() {
	/* Assert that the address is PAGE_SIZE aligned */
	if (WARN_ON_ONCE(!IS_ALIGNED(addr, PAGE_SIZE))) {
		...
		return -EINVAL;
	}
...
}

On architectures where the CPU page size is 16KB or 64KB, providing a
4KB-aligned page address will pass the XE_PAGE_SIZE check but fail in
xe_ttm_vram_handle_addr_fault(), returning -EINVAL.

> +
> +	/*
> +	 * Handle return code from address fault handling function:
[ ... ]
> +	switch (ret) {
[ ... ]
> +	default:
> +		xe_log_err(xe, DEVICE_MEMORY, ret, "Failed to handle address fault 0x%llx\n",
> +			   page_address);
> +		return 0;
> +	}

[Severity: High]
Does this default case incorrectly return 0 when an unhandled error occurs?

If xe_ttm_vram_handle_addr_fault() fails with an unexpected error (like
-ENOMEM or the -EINVAL from the alignment check above), returning 0 here
converts the failure into a success.

[ ... ]
> @@ -368,11 +475,12 @@ static u8 handle_soc_internal_errors(struct xe_device *xe, struct xe_ras_error_a
>  static u8 handle_device_memory_errors(struct xe_device *xe, struct xe_ras_error_array *arr)
>  {
>  	struct xe_ras_memory_error *info = (void *)arr->details;
> +	int ret;
>  
>  	/*
>  	 * For memory errors, the recovery action depends on the error category
[ ... ]
>  	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;

[Severity: High]
Since handle_page_offline() returns 0 for unhandled errors, does this bypass
the required device reset for Double-bit ECC errors?

The caller relies on a non-zero return value to trigger the GPU reset on
unhandled DB-ECC errors. If the error is swallowed in the default case above,
memory corruption might remain unmitigated without a reset.

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