Re: [PATCH v3 7/7] drm/xe/xe_ras: Skip double-bit ECC page offlining on AER path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/xe/xe_ras: duplicate handle_page_offline() call on DB_ECC error path
--

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

drm/xe/xe_ras: Skip double-bit ECC page offlining on AER path

This commit skips double-bit ECC page offlining in
handle_device_memory_errors() when running on the AER path so pages are not
unnecessarily offlined and dropped from the firmware queue before a Secondary
Bus Reset clears them.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 1809c3c893dee..32aa376555bd5 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
> @@ -676,6 +676,23 @@ static u8 handle_device_memory_errors(struct xe_device *xe, struct xe_ras_error_
>  			    info->sw_address);
>  		/*
>  		 * Double-bit ECC errors are non-fatal errors and are reported via IRQ.
> +		 * On a race between AER for device fatal errors and non-fatal double-bit ECC
> +		 * errors, if AER pci error handlers execute first it will get all errors as
> +		 * a single response from system controller, which can include the double-bit
> +		 * ECC errors. This leads to unnecessary offlining of pages, since an SBR clears
> +		 * them on reload and firmware drops them from its queue once read.
> +		 * Skip handling of double-bit ECC errors on the AER path to avoid this.
> +		 *
> +		 * TODO: Remove this check once AER handling is optimized to retain soft
> +		 * offlined pages.
> +		 */
> +		if (xe_ras_is_aer_internal_error(xe)) {
> +			xe_info(xe, "[RAS]: Skipping double-bit ECC error handling on AER path\n");
> +			break;
> +		}
> +
> +		ret = handle_page_offline(xe, info->sw_address, true);

[Severity: High]
Was the existing call to handle_page_offline() above xe_log_comp() in
handle_device_memory_errors() meant to be removed or moved here?

drivers/gpu/drm/xe/xe_ras.c:handle_device_memory_errors() {
	...
	case XE_RAS_MEMORY_DB_ECC:
		ret = handle_page_offline(xe, info->sw_address, true);
		xe_log_comp(xe, ret ? cper_sev : CPER_SEV_INFORMATIONAL,
			    DEVICE_MEMORY, &arr->counter, sizeof(arr->counter),
			    "Double-bit ECC error detected at sw address 0x%llx\n",
			    info->sw_address);
		...
		if (xe_ras_is_aer_internal_error(xe)) {
			xe_info(xe, "[RAS]: Skipping double-bit ECC error handling on AER path\n");
			break;
		}

		ret = handle_page_offline(xe, info->sw_address, true);
	...
}

Because the earlier handle_page_offline() call is still present before the
xe_ras_is_aer_internal_error() check, the AER path still soft-offlines the
page and sends the page offline command to firmware before breaking out.

On the normal non-AER IRQ path, handle_page_offline() is now called twice in
a row for the same info->sw_address. When the first call soft-offlines a page
on its first error occurrence, the immediate second call sees -EEXIST from
xe_ttm_vram_handle_addr_fault():

drivers/gpu/drm/xe/xe_ras.c:handle_page_offline() {
	...
	case -EEXIST:
		if (xa_load(&state->offlined_pages, page_address >> XE_PTE_SHIFT))
			action = XE_RAS_PAGE_ACTION_REMOVE;
		else
			action = XE_RAS_PAGE_ACTION_OFFLINE;
	...
}

Wouldn't this cause the second call to treat a single first-occurrence ECC
error as a second occurrence and request XE_RAS_PAGE_ACTION_OFFLINE (or fail
in send_page_offline_cmd() and wedge the device)?

Also, note that xe_log_comp() evaluates ret ? cper_sev :
CPER_SEV_INFORMATIONAL, so if the first handle_page_offline() call is
removed, xe_log_comp() would either need to be moved after
handle_page_offline() or updated so it does not use an uninitialized ret.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103060929.1586783-9-riana.tauro@intel.com?part=7