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

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Aug 18, 2026 at 01:53:15PM -0700, Niranjana Vishwanathapura wrote:
> On Wed, Aug 19, 2026 at 02:38:39AM +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.
> > 
> > v2: Document that sched->msg_lock also protects
> >    guc->suspend_pending, which indicates a suspend message is in
> >    flight, in addition to the sched->msgs list (Niranjana)
> > 
> > Signed-off-by: Jagmeet Randhawa <jagmeet.randhawa@intel.com>
> 
> LGTM
> Reviewed-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> 

So my latest reply, I don't think patch is needed in its current form.

Matt

> > ---
> > drivers/gpu/drm/xe/xe_gpu_scheduler_types.h  |  5 +++-
> > drivers/gpu/drm/xe/xe_guc_exec_queue_types.h |  5 +++-
> > drivers/gpu/drm/xe/xe_guc_submit.c           | 29 ++++++++++++++++----
> > 3 files changed, 32 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h b/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
> > index 63d9bf92583c..78ef2e8ded4f 100644
> > --- a/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
> > +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
> > @@ -47,7 +47,10 @@ struct xe_gpu_scheduler {
> > 	const struct xe_sched_backend_ops	*ops;
> > 	/** @msgs: list of messages to be processed in @work_process_msg */
> > 	struct list_head			msgs;
> > -	/** @msg_lock: Message lock */
> > +	/**
> > +	 * @msg_lock: Protects @msgs and guc->suspend_pending (indicates a
> > +	 * suspend message is in flight) of exec queues on this scheduler.
> > +	 */
> > 	spinlock_t				msg_lock;
> > 	/** @work_process_msg: processes messages */
> > 	struct work_struct		work_process_msg;
> > diff --git a/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h b/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
> > index d27826b36649..74b711abe257 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
> > +++ b/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
> > @@ -52,7 +52,10 @@ struct xe_guc_exec_queue {
> > 	u16 id;
> > 	/** @suspend_wait: wait queue used to wait on pending suspends */
> > 	wait_queue_head_t suspend_wait;
> > -	/** @suspend_pending: a suspend of the exec_queue is pending */
> > +	/**
> > +	 * @suspend_pending: a suspend of the exec_queue is pending.
> > +	 * Protected by @sched.msg_lock.
> > +	 */
> > 	bool suspend_pending;
> > 	/**
> > 	 * @suspend_count: Reference count of active suspend requests. The
> > 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
> >