Re: [PATCH v4 7/7] drm/xe/guc: Report errors that cause a CT shutdown using SIGID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] xe: missed xe_log_err conversion in guc crash handlers
- [Medium] xe: mismatched error codes returned in parse_g2h_msg and parse_g2h_response
--

commit 3134241c4666d211fb1d7e98e3ce95c50000c2e9
Author: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Subject: drm/xe/guc: Report errors that cause a CT shutdown using SIGID

This patch converts errors that result in a CT being declared dead to use
the xe_log_err helper, which supports SIGID reporting. Escalate-to-caller
errors remain unchanged so callers can apply SIGID if needed. Error
messages are also updated to be clearer about the failure.

> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index 31ecddab30578..63d79987c5d28 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
[ ... ]
> @@ -1662,13 +1665,15 @@ static int parse_g2h_response(struct xe_guc_ct *ct, u32 *msg, u32 len)
>  	 */
>  	if (fence & CT_SEQNO_UNTRACKED) {
>  		if (type == GUC_HXG_TYPE_RESPONSE_FAILURE)
> -			xe_gt_err(gt, "FAST_REQ H2G fence 0x%x failed! e=0x%x, h=%u\n",
> -				  fence,
> -				  FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, hxg[0]),
> -				  FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, hxg[0]));
> +			xe_log_err(gt, GUC, -EINVAL,
> +				   "CT: FAST_REQ H2G fence 0x%x failed! e=0x%x, h=%u\n",
> +				   fence,
> +				   FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, hxg[0]),
> +				   FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, hxg[0]));
>  		else
> -			xe_gt_err(gt, "unexpected response %u for FAST_REQ H2G fence 0x%x!\n",
> -				  type, fence);
> +			xe_log_err(gt, GUC, -EPROTO,
> +				   "CT: unexpected response %u for FAST_REQ H2G fence 0x%x!\n",
> +				    type, fence);
>  
>  		fast_req_report(ct, fence);
>  
>  		/* FIXME: W/A race in the GuC, will get in firmware soon */
>  		if (xe_gt_recovery_pending(gt))
>  			return 0;
>  
>  		CT_DEAD(ct, NULL, PARSE_G2H_RESPONSE);
>  
>  		return -EPROTO;

[Severity: Medium]
Does this cause misleading double-logging when receive_g2h() processes the
return value?

Here in parse_g2h_response(), we log the specific -EINVAL error code, but
return -EPROTO to the caller. When the caller receive_g2h() receives this,
it will log the failure a second time as a generic -EPROTO error since the
error codes don't match.

>  	}
>  
[ ... ]
> @@ -1744,7 +1749,7 @@ static int parse_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
>  
>  	origin = FIELD_GET(GUC_HXG_MSG_0_ORIGIN, hxg[0]);
>  	if (unlikely(origin != GUC_HXG_ORIGIN_GUC)) {
> -		xe_gt_err(gt, "Invalid G2H origin=%u\n", origin);
> +		xe_log_err(gt, GUC, -EBADMSG, "CT: Invalid G2H origin=%u\n", origin);
>  		CT_DEAD(ct, &ct->ctbs.g2h, PARSE_G2H_ORIGIN);
>  
>  		return -EPROTO;

[Severity: Medium]
Similar to the check in parse_g2h_response(), does logging -EBADMSG here
but returning -EPROTO result in contradictory double-logging in the caller?

>  	}
>  
[ ... ]
> @@ -1862,8 +1867,8 @@ static int process_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
>  	}
>  
>  	if (ret) {
> -		xe_gt_err(gt, "G2H action %#04x failed (%pe) len %u msg %*ph\n",
> -			  action, ERR_PTR(ret), hxg_len, (int)sizeof(u32) * hxg_len, hxg);
> +		xe_log_err(gt, GUC, ret, "CT: G2H action %#04x failed len %u msg %*ph\n",
> +			   action, hxg_len, (int)sizeof(u32) * hxg_len, hxg);
>  		CT_DEAD(ct, NULL, PROCESS_FAILED);
>  	}

[Severity: Medium]
The commit message notes this converts any errors causing a CT shutdown to use
xe_log_err, but did it miss the GuC crash and exception handlers?

When a GuC crash or exception occurs, process_g2h_msg() calls
guc_handle_crash_dump_msg() and guc_handle_exception_msg(). Both of these
handlers call CT_DEAD directly and use the older xe_gt_err for logging:

guc_handle_crash_dump_msg()
    xe_gt_err(ct_to_gt(ct), "GuC Crash dump notification\n");
    CT_DEAD(ct, NULL, CRASH);

Should these be converted to xe_log_err as well to ensure these GuC crashes
receive a SIGID? As it stands, they are untagged, while the fallback generic
PROCESS_FAILED error above does get a SIGID.

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