Re: [PATCH v1] drm/xe/guc: Allow GuC CT for wedged device

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Removing the `xe_device_wedged` check from `__guc_ct_send_locked` causes a full 1-second delay in `xe_device_declare_wedged` when a GuC hang occurs.
- [Low] The kernel-doc comment for `xe_guc_ct_send()` is stale and incorrectly claims that non-blocking send helpers return `-ENOTRECOVERABLE` when the device is wedged.
--

--- Patch [1]: [PATCH v1] drm/xe/guc: Allow GuC CT for wedged device ---
commit baa99b44631241d9eb913ee6996cb7221b0a82f2
Author: Raag Jadav <[email protected]>

    drm/xe/guc: Allow GuC CT for wedged device
    
    Commit 50fa9acac26f ("drm/xe/guc: distinguish wedged from recoverable
    cancellation") introduced distinguishable error codes for g2h failure
    cases, but also blocked GuC CT for wedged device. This is problematic
    in cases where we want to prevent user from accessing the device but
    also keep GuC CT functioning on temporarily wedged device. First user
    of such requirement is PCIe FLR handling where we require uC firmware
    loading while the device is temporarily wedged.
    
    Fixes: 50fa9acac26f ("drm/xe/guc: distinguish wedged from recoverable cancellation")
    Signed-off-by: Raag Jadav <[email protected]>

> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index fe70c0fd85c52..541c74180bcaf 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> @@ -1065,11 +1065,6 @@ static int __guc_ct_send_locked(struct xe_guc_ct *ct, const u32 *action,
>  	xe_gt_assert(gt, g2h_len || !num_g2h);
>  	lockdep_assert_held(&ct->lock);
>  
> -	if (xe_device_wedged(ct_to_xe(ct))) {
> -		ret = -ENOTRECOVERABLE;
> -		goto out;
> -	}
> -

[Severity: Medium]
Will removing this check cause a 1-second delay during device wedging?

If guc_ct_send_locked() is executing when a GuC hang occurs and the CTB is full,
it enters a retry loop holding ct->lock. If xe_device_declare_wedged() executes
concurrently, it attempts to call xe_guc_ct_stop() and blocks waiting for
ct->lock.

Without this xe_device_wedged() check, the retry loop relies entirely on the
ct->state == XE_GUC_CT_STATE_STOPPED condition to abort early. However, because
the wedging thread is blocked on ct->lock, the state cannot be updated. This
causes the loop in guc_ct_send_locked() to retry until the 1-second timeout
expires.

[Severity: Low]
Should the kernel-doc for xe_guc_ct_send() be updated to reflect this change?

The documentation currently claims that non-blocking send helpers return
-ENOTRECOVERABLE when the device is wedged:

 * * -ENOTRECOVERABLE: the xe device is wedged. Stop submitting new GuC work; the
 *   request cannot make progress until the device is recovered.

Since the wedged check was removed from the send path, this error code is
no longer returned in that scenario.

>  	if (unlikely(ct->ctbs.h2g.info.broken)) {
>  		ret = -EPIPE;
>  		goto out;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1