Re: [PATCH v5 4/7] drm/xe/guc: Cleanup error codes and handling for CT errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] xe_guc_ct: immediate failure on H2G corruption due to mismatched error code
--

commit 81ff170e2ca7e241924429f71845781f7d3f0d95
Author: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

drm/xe/guc: Cleanup error codes and handling for CT errors

This commit updates the GuC CT error handling to return more specific error
codes based on the error type rather than defaulting to -EPROTO. It also
inverts the error check in receive_g2h() to simplify reset handling.

> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index 5dbda0fe20f37..572a52773c56a 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c

[ ... ]

> @@ -1358,7 +1435,7 @@ int xe_guc_ct_send_g2h_handler(struct xe_guc_ct *ct, const u32 *action, u32 len)
>   */
>  static bool retry_failure(struct xe_guc_ct *ct, int ret)
>  {
> -	if (!(ret == -EDEADLK || ret == -EPIPE || ret == -ENODEV))
> +	if (!(ret == -EDEADLK || ret == -EPERM || ret == -ENODEV))
>  		return false;

[Severity: Medium]
When h2g_write() discovers descriptor corruption, it calls ct_corrupted() and
returns -EPIPE. Since this patch removes -EPIPE from the whitelist here in
favor of -EPERM, retry_failure() will return false and abort the send
instantly.

Does this deny the thread that discovers the corruption a chance to recover
if a GT reset occurs?

Later threads calling __guc_ct_send_locked() will observe the newly broken
state, return -EPERM, and correctly block waiting for recovery. Should the
initial -EPIPE error also be permitted to retry, or be escalated directly
to a reset?

[ ... ]

> @@ -2037,6 +2107,27 @@ static int dequeue_one_g2h(struct xe_guc_ct *ct)
>  	return 1;
>  }
>  
> +/*
> + * Errors reported by dequeue_one_g2h() come in two flavours: either the channel
> + * is simply not available right now, which is expected and handled gracefully,
> + * or the channel state or the message itself is inconsistent, in which case the
> + * only way forward is to declare the CT dead and reset the GuC. Since the
> + * former is a short and well known list, check against that and treat anything
> + * else as fatal, so that new error codes don't silently escape the escalation.
> + */
> +static bool g2h_err_is_fatal(int err)
> +{
> +	switch (err) {
> +	case -ENOTRECOVERABLE:	/* device wedged */
> +	case -ENODEV:		/* CT disabled */
> +	case -ECANCELED:	/* CT stopped */
> +	case -EPERM:		/* CT already declared broken */
> +		return false;
> +	default:
> +		return true;
> +	}
> +}
> +

[ ... ]

> @@ -2074,8 +2165,9 @@ static void receive_g2h(struct xe_guc_ct *ct)
>  		ret = dequeue_one_g2h(ct);
>  		mutex_unlock(&ct->lock);
>  
> -		if (unlikely(ret == -EPROTO || ret == -EOPNOTSUPP)) {
> -			xe_gt_err(ct_to_gt(ct), "CT dequeue failed: %d\n", ret);
> +		if (unlikely(ret < 0 && g2h_err_is_fatal(ret))) {
> +			xe_gt_err(ct_to_gt(ct), "CT dequeue failed, forcing GT reset(%pe)\n",
> +				  ERR_PTR(ret));
>  			CT_DEAD(ct, NULL, G2H_RECV);
>  			kick_reset(ct);
>  		}

[Severity: High]
This is a pre-existing issue, but this change to how errors are filtered
seems to leave a gap where a corrupted G2H channel silently escapes a GT
reset.

If corruption is detected during xe_guc_ct_fast_path(), g2h_read() calls
ct_corrupted(), marks the channel broken, and returns -EPIPE. The fast path
loop drops the negative length without resetting.

When the background worker later executes receive_g2h(), it sees the broken
channel and g2h_read() returns -EPERM. Because g2h_err_is_fatal(-EPERM)
explicitly returns false here, receive_g2h() ignores the error:

receive_g2h() {
    ...
    if (unlikely(ret < 0 && g2h_err_is_fatal(ret))) {
        // GT reset skipped
    }
}

Is there a mechanism that will trigger a recovery for the broken G2H channel,
or will it stay permanently wedged since neither path kicks a reset? Before
this patch, the fast path ignored -EPROTO and the worker ignored -EPIPE, so
the new error filtering inherits this behavior.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919050055.2092579-9-umesh.nerlige.ramappa@intel.com?part=4