Re: [PATCH v2] drm/xe/guc: Fix race around q->guc->suspend_pending access

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

Pre-existing issues:
- [High] TDR (Timeout Delay Recovery) races with asynchronous queue suspend, leading to misattributed SCHED_DONE messages, an assertion failure, and a TDR hang.
--

--- Patch [1]: [PATCH v2] drm/xe/guc: Fix race around q->guc->suspend_pending access ---
commit 256a06cec5d42e79b9711d9f1861b3717e23b08f
Author: Jagmeet Randhawa <[email protected]>

drm/xe/guc: Fix race around q->guc->suspend_pending access

This commit wraps accesses to q->guc->suspend_pending with xe_sched_msg_lock
in the suspend, resume, kill, and ban paths. This ensures that checking the
flag and calling suspend_fence_signal() is atomic with respect to the
asynchronous clears in the kill and ban paths.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 9036f89dff7d9..c565c1d32d3a4 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
[ ... ]
> @@ -3222,13 +3234,20 @@ static void handle_sched_done(struct xe_guc *guc, struct xe_exec_queue *q,
>  		smp_wmb();
>  		wake_up_all(&guc->ct.wq);
>  	} else {
> +		bool was_pending;
> +
>  		xe_gt_assert(guc_to_gt(guc), runnable_state == 0);
>  		xe_gt_assert(guc_to_gt(guc), exec_queue_pending_disable(q));
>  
> -		if (q->guc->suspend_pending) {
> +		xe_sched_msg_lock(&q->guc->sched);
> +		was_pending = q->guc->suspend_pending;
> +		if (was_pending) {
>  			clear_exec_queue_pending_disable(q);
>  			suspend_fence_signal(q);
> -		} else {
> +		}
> +		xe_sched_msg_unlock(&q->guc->sched);

[Severity: High]
This is a pre-existing issue, but does this logic misattribute a SCHED_DONE
message if TDR races with an asynchronous queue suspend?

If a queue is being suspended (setting q->guc->suspend_pending = true) and
concurrently a job times out, the TDR handler stops the DRM scheduler,
trapping the SUSPEND message before it can execute. TDR then calls
disable_scheduling(), which directly sends a DISABLE command to the GuC.

When the GuC responds to the DISABLE command with a SCHED_DONE message,
won't handle_sched_done() incorrectly attribute it to the trapped SUSPEND
message because was_pending is still true?

It looks like this would call suspend_fence_signal(q) and trigger an
assertion failure, because the queue was never actually suspended (the
message was trapped) or killed (TDR only bans it). Additionally, skipping
the wake_up_all() call in the other branch might cause the TDR thread to
wait and hang for 5 seconds until it times out.

> +
> +		if (!was_pending) {
>  			if (exec_queue_banned(q)) {
>  				smp_wmb();
>  				wake_up_all(&guc->ct.wq);

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