Re: [PATCH v2] drm/xe: Add mnemonic error reason to page fault diagnostics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe: stale pagefault error codes mask true failure reasons on retries
--

commit 316fc8037f7fbbec7bf65ce4fb4107de7eac1d79
Author: Matthew Brost <matthew.brost@intel.com>
drm/xe: Add mnemonic error reason to page fault diagnostics

This patch adds enum xe_pagefault_error to encode failure reasons into the
reserved low bits of the page address. This allows xe_pagefault_print to
report a mnemonic failure reason.

> diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
> --- a/drivers/gpu/drm/xe/xe_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_pagefault.c

[ ... ]

> @@ -195,8 +203,10 @@ static int xe_pagefault_handle_vma(struct xe_gt *gt, struct xe_vma *vma,
>  						 needs_vram == 1);
>  			drm_exec_retry_on_contention(&exec);
>  			xe_validation_retry_on_oom(&ctx, &err);
> -			if (err)
> +			if (err) {
> +				xe_pagefault_set_error(pf, XE_PAGEFAULT_ERROR_VMA_VALIDATE);
>  				break;
> +			}

[Severity: Medium]
Does this leak the error state into subsequent retries if the failure is a
transient -EAGAIN?

If a transient failure occurs, xe_pagefault_set_error sets
XE_PAGEFAULT_ERROR_VMA_VALIDATE. When the outer loop retries, this error code
remains attached to pf. If a later retry fails for a different reason, the
stale error code might mask the true failure reason.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
> --- a/drivers/gpu/drm/xe/xe_svm.c
> +++ b/drivers/gpu/drm/xe/xe_svm.c

[ ... ]

> @@ -1480,11 +1488,15 @@ int xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
>  		 * may have been split by xe_svm_range_set_default_attr.
>  		 */
>  		vma = xe_vm_find_vma_by_addr(vm, fault_addr);
> -		if (!vma)
> +		if (!vma) {
> +			xe_pagefault_set_error(pf, XE_PAGEFAULT_ERROR_SVM_VMA_NOT_FOUND);
>  			return -EINVAL;
> +		}
>  
>  		goto retry;
>  	}
> +	if (ret && xe_pagefault_get_error(pf) == XE_PAGEFAULT_ERROR_NONE)
> +		xe_pagefault_set_error(pf, XE_PAGEFAULT_ERROR_SVM_SERVICE_FAILED);
>  	return ret;
>  }

[Severity: Medium]
Can a previous transient error mask the true failure reason here?

For example, if __xe_svm_handle_pagefault encounters a transient -EAGAIN
failure during xe_svm_garbage_collector, it sets
XE_PAGEFAULT_ERROR_SVM_GARBAGE_COLLECTOR.

When the retry happens, if it subsequently fails at a different point (such
as getting pages) that relies on this fallback error assignment, the
condition evaluates to false because the error state is no longer
XE_PAGEFAULT_ERROR_NONE.

Would it be safer to clear the error state before retrying, or
unconditionally overwrite the fallback error here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917193815.209362-1-matthew.brost@intel.com?part=1