Re: [PATCH v5 2/7] drm/xe/guc: Handle CRASH and EXCEPTION G2H with separate helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/xe: guc_handle_*_msg helpers return -EHOSTDOWN instead of 0
- [Low] drm/xe: unused 'action' parameter in guc_handle_*_msg
--

commit 8c0a0c12a9abfb15d1eb3e3e87b08b081ebd3cd3
Author: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

drm/xe/guc: Handle CRASH and EXCEPTION G2H with separate helpers

This commit refactors the handling of CRASH and EXCEPTION G2H messages
by splitting them into separate helpers. It also removes unreachable
code that existed in the previous implementation.

> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index 5c4733da385c5..b3a6aa37808b4 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
[ ... ]
> @@ -1553,22 +1553,22 @@ static int parse_g2h_event(struct xe_guc_ct *ct, u32 *msg, u32 len)
>  	return 0;
>  }
>  
> -static int guc_crash_process_msg(struct xe_guc_ct *ct, u32 action)
> +static int guc_handle_crash_dump_msg(struct xe_guc_ct *ct, u32 action)

[Severity: Low]
Does this code still need the action parameter? It appears to be unused
within the guc_handle_crash_dump_msg() function body.

>  {
> -	struct xe_gt *gt = ct_to_gt(ct);
> +	xe_gt_err(ct_to_gt(ct), "GuC Crash dump notification\n");
> +	CT_DEAD(ct, NULL, CRASH);
> +	kick_reset(ct);
>  
> -	if (action == XE_GUC_ACTION_NOTIFY_CRASH_DUMP_POSTED)
> -		xe_gt_err(gt, "GuC Crash dump notification\n");
> -	else if (action == XE_GUC_ACTION_NOTIFY_EXCEPTION)
> -		xe_gt_err(gt, "GuC Exception notification\n");
> -	else
> -		xe_gt_err(gt, "Unknown GuC crash notification: 0x%04X\n", action);
> +	return -EHOSTDOWN;

[Severity: Medium]
Does returning -EHOSTDOWN here contradict the v2 changelog intent to "return 0
from the helpers to retain old behavior"? 

Returning a failure code here causes process_g2h_msg() to unintentionally
execute its error path:

	if (ret) {
		xe_gt_err(gt, "G2H action %#04x failed (%pe) len %u msg %*ph\n", ...
		CT_DEAD(ct, NULL, PROCESS_FAILED);
	}

This generates an extra false positive error log and overrides the
initial CT_DEAD(ct, NULL, CRASH) reason with PROCESS_FAILED.

> +}
>  
> +static int guc_handle_exception_msg(struct xe_guc_ct *ct, u32 action)

[Severity: Low]
Similar to guc_handle_crash_dump_msg(), is the action parameter still needed
here since it remains unused?

> +{
> +	xe_gt_err(ct_to_gt(ct), "GuC Exception notification\n");
>  	CT_DEAD(ct, NULL, CRASH);
> -
>  	kick_reset(ct);
>  
> -	return 0;
> +	return -EHOSTDOWN;

[Severity: Medium]
Does returning -EHOSTDOWN here contradict the v2 changelog intent to "return 0
from the helpers to retain old behavior"? 

Returning a failure code here causes process_g2h_msg() to unintentionally
execute its error path:

	if (ret) {
		xe_gt_err(gt, "G2H action %#04x failed (%pe) len %u msg %*ph\n", ...
		CT_DEAD(ct, NULL, PROCESS_FAILED);
	}

This generates an extra false positive error log and overrides the
initial CT_DEAD(ct, NULL, CRASH) reason with PROCESS_FAILED.

>  }

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