Re: [PATCH v3 1/1] drm/xe/guc: Fix wake_up race in handle_sched_done

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] Misleading documentation and invalid memory barrier pairing between `handle_sched_done()` and `guc_exec_queue_timedout_job()`.

Pre-existing issues:
- [High] Waiters on `guc->ct.wq` will miss wakeups and timeout when a suspended queue completes its scheduling disable, because `handle_sched_done()` fails to wake up the wait queue.
- [High] Waiters on `guc->ct.wq` will miss wakeups and timeout because `handle_sched_done()` restricts `wake_up_all(&guc->ct.wq)` to banned queues only.
--

commit 93d97b23543a66d34d37e760d853aee106ea33dd
Author: Jakub Legowski <jakub.legowski@intel.com>

drm/xe/guc: Fix wake_up race in handle_sched_done

This commit moves the wake_up_all() call in handle_sched_done() after
clearing the pending_disable state. This ensures waiters in
guc_exec_queue_timedout_job() observe the updated state when woken.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 99d8c807ff056..a181b50422934 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
[ ... ]
> @@ -3239,25 +3239,27 @@ static void handle_sched_done(struct xe_guc *guc, struct xe_exec_queue *q,
>  		if (q->guc->suspend_pending) {
>  			clear_exec_queue_pending_disable(q);
>  			suspend_fence_signal(q);

[Severity: High]
This isn't a bug introduced by this patch, but it's a pre-existing issue.
Will waiters on guc->ct.wq miss wakeups here? When a suspended queue
completes its scheduling disable, this branch clears pending_disable but
completely bypasses calling wake_up_all(&guc->ct.wq).

For example, if disable_scheduling_deregister() is waiting on guc->ct.wq
for pending_disable to clear, and handle_sched_done() takes this
suspend_pending branch, it seems the waiter will hit the 5-second timeout
and trigger a spurious GT reset.

>  		} else {
> +			bool destroyed = exec_queue_destroyed(q);
> +
> +			/*
> +			 * Make sure to clear the pending_disable only
> +			 * after sampling the destroyed state. We want
> +			 * to ensure we don't trigger the unregister too
> +			 * early with something intending to only
> +			 * disable scheduling. The caller doing the
> +			 * destroy must wait for an ongoing
> +			 * pending_disable before marking as destroyed.
> +			 */
> +			clear_exec_queue_pending_disable(q);
> +
>  			if (exec_queue_banned(q)) {
> +				/* Pairs with smp_rmb in guc_exec_queue_timedout_job */
> +				smp_wmb();

[Severity: Low]
Is this memory barrier pairing valid? The smp_rmb() in
guc_exec_queue_timedout_job() is placed outside the wait_event_timeout()
loop:

guc_exec_queue_timedout_job() {
        ...
        smp_rmb();
        ret = wait_event_timeout(guc->ct.wq,
                                 !xe_uc_fw_is_running(&guc->fw) ||
                                 ...
}

Since wait_event_timeout() evaluates the condition in a loop, an external
smp_rmb() provides no ordering guarantees for subsequent evaluations of the
condition. The lock inside the waitqueue already provides the necessary full
memory barriers.

>  				wake_up_all(&guc->ct.wq);
>  			}

[Severity: High]
This isn't a bug introduced by this patch, but it's a pre-existing issue.
Does restricting the wakeup to only banned queues strand waiters on
non-banned queues?

In guc_exec_queue_timedout_job() for a multi-queue secondary, the secondary
queue is banned, but disable_scheduling() and the wait are performed on
the primary queue. Since the primary is not banned, when its SCHED_DONE
arrives, handle_sched_done() clears pending_disable but skips the wakeup
because exec_queue_banned(primary) is false.

This leaves threads waiting on guc->ct.wq to hit their timeout, leading to
spurious GT resets during multi-queue timeout recovery or normal teardown
races.

> -			if (exec_queue_destroyed(q)) {
[ ... ]
> -				clear_exec_queue_pending_disable(q);
> +
> +			if (destroyed)
>  				deregister_exec_queue(guc, q);
>  		}
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910121759.1835037-1-jakub.legowski@intel.com?part=1