Re: [PATCH v3 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] xe_ras: unhandled page offlining failures bypass fallback GPU reset
- [Medium] xe_ras: out-of-bounds memory errors mischaracterized as user policy disable
--

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

    drm/xe/xe_ras: Handle page offline requests for device memory ecc errors
    
    Add basic support for sending page offline/remove requests to system
    controller and use it for device memory ECC error handling.
    Pages that belong to critical BOs cannot be handled by offlining and
    require a SBR (Secondary Bus Reset).

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 7a85735c57d58..96ad473388a95 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
[ ... ]
> @@ -201,6 +204,119 @@ static inline const char *comp_to_str(u8 component)
[ ... ]
> +static int handle_page_offline(struct xe_device *xe, u64 page_address, bool send_cmd)
> +{
[ ... ]
> +	switch (ret) {
> +	case 0:
> +		action = XE_RAS_PAGE_ACTION_REMOVE;
> +		xe_log_err(xe, DEVICE_MEMORY, ret,
> +			   "Poison detected at physical address 0x%llx, page soft-offlined\n",
> +			   page_address);
> +		break;
> +	/* User policy set to decline page offlining */
> +	case -EOPNOTSUPP:

[Severity: Medium]
When xe_ttm_vram_handle_addr_fault() encounters an out-of-bounds address, does
it also return -EOPNOTSUPP?

If firmware reports an error at an invalid physical address outside VRAM/GSM,
it looks like this will mischaracterize the error as a user policy choice and
still send a XE_RAS_PAGE_ACTION_REMOVE command.

> +		action = XE_RAS_PAGE_ACTION_REMOVE;
> +		xe_log_err(xe, DEVICE_MEMORY, ret,
> +			   "Poison detected at physical address 0x%llx, user policy set to decline soft-offlining\n",
> +			   page_address);
> +		break;
> +	case -EIO:
> +		xe_log_err(xe, DEVICE_MEMORY, ret,
> +			   "Poison detected at physical address 0x%llx, page belongs to critical BO and cannot be soft-offlined\n",
> +			   page_address);
> +		return ret;
> +	case -EEXIST:
> +		action = XE_RAS_PAGE_ACTION_OFFLINE;
> +		xe_log_err(xe, DEVICE_MEMORY, ret,
> +			   "Double-bit ECC error detected at physical address 0x%llx, page soft-offlined\n",
> +			   page_address);
> +		break;
> +	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 mask the error code from xe_ttm_vram_handle_addr_fault()?

If an internal error occurs (like -ENOMEM), returning 0 instead of the error
code causes the caller, handle_device_memory_errors(), to evaluate if (ret)
as false. This masks the offlining failure and bypasses the fallback hardware
reset, potentially leaving corrupted memory in use.

> +	}
> +
> +	if (send_cmd) {
> +		ret = send_page_offline_cmd(xe, page_address, action);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> @@ -384,10 +501,10 @@ static u8 handle_device_memory_errors(struct xe_device *xe, struct xe_ras_error_
>  		xe_info(xe, "[RAS]: Data parity error detected\n");
>  		break;
>  	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;
>  	default:
>  		return XE_RAS_RECOVERY_ACTION_RESET;
>  	}

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