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

[Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>]

On Fri, Aug 14, 2026 at 04:18:48AM +0800, Jagmeet Randhawa wrote:
>q->guc->suspend_pending is accessed without any common lock.
>__suspend_fence_signal(), called from guc_exec_queue_kill() and the
>suspend-timeout ban path, clears the flag asynchronously. Meanwhile
>handle_sched_done(), guc_exec_queue_stop() and
>__guc_exec_queue_process_msg_suspend() check the flag and then call
>suspend_fence_signal(), which asserts that it is still set.
>
>As the check and suspend_fence_signal() are not atomic, the clear can
>land in between and trip the xe_gt_assert(q->guc->suspend_pending).
>
>The flag is already set and read under the per-queue msg_lock
>(xe_sched_msg_lock()) on the suspend and resume paths. Extend that same
>lock to the clear paths (kill and ban) and to the three check-then-act
>sites so the check and the signal are atomic with respect to the clear.
>In __guc_exec_queue_process_msg_suspend() only the non-sleeping branch is
>wrapped, since the other branch waits. In handle_sched_done() the flag is
>snapshotted under the lock and deregister_exec_queue() is kept outside it.
>

Looks good,
Though it looks like overloading of sched->msg_lock, it is not because
guc->suspend_pending is indeed indication of wating for message process
completion. Perhaps it would be good to update the documentation of
sched->msg_lock and mention what all it protects (sched->msgs list and
guc->suspend_pending which indicates suspend message is in fligt).

Niranjana

>Signed-off-by: Jagmeet Randhawa <jagmeet.randhawa@intel.com>
>---
> drivers/gpu/drm/xe/xe_guc_submit.c | 29 ++++++++++++++++++++++++-----
> 1 file changed, 24 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>index 9036f89dff7d..c565c1d32d3a 100644
>--- a/drivers/gpu/drm/xe/xe_guc_submit.c
>+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>@@ -1928,9 +1928,13 @@ static void __guc_exec_queue_process_msg_suspend(struct xe_sched_msg *msg)
> 			set_exec_queue_suspended(q);
> 			disable_scheduling(q, false);
> 		}
>-	} else if (q->guc->suspend_pending) {
>-		set_exec_queue_suspended(q);
>-		suspend_fence_signal(q);
>+	} else {
>+		xe_sched_msg_lock(&q->guc->sched);
>+		if (q->guc->suspend_pending) {
>+			set_exec_queue_suspended(q);
>+			suspend_fence_signal(q);
>+		}
>+		xe_sched_msg_unlock(&q->guc->sched);
> 	}
> }
>
>@@ -2130,7 +2134,9 @@ static void guc_exec_queue_kill(struct xe_exec_queue *q)
> {
> 	trace_xe_exec_queue_kill(q);
> 	set_exec_queue_killed(q);
>+	xe_sched_msg_lock(&q->guc->sched);
> 	__suspend_fence_signal(q);
>+	xe_sched_msg_unlock(&q->guc->sched);
> 	xe_guc_exec_queue_trigger_cleanup(q);
> }
>
>@@ -2392,11 +2398,15 @@ static void guc_exec_queue_suspend_timeout_ban(struct xe_exec_queue *q)
> 	 */
> 	if (xe_exec_queue_is_multi_queue(q)) {
> 		set_exec_queue_group_banned(q);
>+		xe_sched_msg_lock(&q->guc->sched);
> 		__suspend_fence_signal(q);
>+		xe_sched_msg_unlock(&q->guc->sched);
> 		xe_guc_exec_queue_group_trigger_cleanup(q);
> 	} else {
> 		set_exec_queue_banned(q);
>+		xe_sched_msg_lock(&q->guc->sched);
> 		__suspend_fence_signal(q);
>+		xe_sched_msg_unlock(&q->guc->sched);
> 		xe_guc_exec_queue_trigger_cleanup(q);
> 	}
> }
>@@ -2614,10 +2624,12 @@ static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
> 		if (exec_queue_destroyed(q))
> 			do_destroy = true;
> 	}
>+	xe_sched_msg_lock(sched);
> 	if (q->guc->suspend_pending) {
> 		set_exec_queue_suspended(q);
> 		suspend_fence_signal(q);
> 	}
>+	xe_sched_msg_unlock(sched);
> 	atomic_and(EXEC_QUEUE_STATE_WEDGED | EXEC_QUEUE_STATE_BANNED |
> 		   EXEC_QUEUE_STATE_KILLED | EXEC_QUEUE_STATE_DESTROYED |
> 		   EXEC_QUEUE_STATE_SUSPENDED,
>@@ -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);
>+
>+		if (!was_pending) {
> 			if (exec_queue_banned(q)) {
> 				smp_wmb();
> 				wake_up_all(&guc->ct.wq);
>-- 
>2.53.0
>