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 04:27:14PM -0700, Niranjana Vishwanathapura wrote:
> On Tue, Aug 18, 2026 at 02:05:10PM -0700, Matthew Brost wrote:
> > On Wed, Aug 19, 2026 at 02:38:39AM +0800, Jagmeet Randhawa wrote:
> > 
> > This is designed to be lockless.
> > 
> > > q->guc->suspend_pending is accessed without any common lock.
> > > __suspend_fence_signal(), called from guc_exec_queue_kill() and the
> > 
> > This is actually the problem. __suspend_fence_signal shouldn't be called
> > from guc_exec_queue_kill(). This can prematurely signal a suspend fence
> > while the hardware is still executing.
> > 
> > __suspend_fence_signal should be called in two possible places:
> > 
> > - Naturally in G2H handler (handle_sched_done)
> > - Or in global event that takes down the GuC firmware
> >  (guc_exec_queue_stop)
> > 
> > With that, a lock isn't need because the state machine / firmware
> > interaction ensures everything is race free.
> > 
> > So I think the solution is ensure __suspend_fence_signal is called in
> > the correct places rather than adding protection via a lock.
> > 
> 
> Matt,
> 
> I think it is probably not as trivial as dropping __suspend_fence_signal()
> from guc_exec_queue_kill() for following reasons.

Let's take a step back, has existing code been linked to any bugs?

> 
> 1. __suspend_fence_signal() is also called from
>    guc_exec_queue_suspend_timeout_ban() if suspend_wait times out.
> 

Yes, this is another example of where this should not be called.

> 2. Calling __suspend_fence_signal() wakes up any suspend_wait(),
>    which otherwise will have to wait 5 seconds before timing out.
>    If in guc_exec_queue_kill(), if we just try to wake up suspend_wait,
>    without clearing suspend_pending, then a resume() might run before
>    TDR kicks in and hits the !suspend_pending assert.
> 

Don't do a wake here.

> 3. Even if we drop __suspend_pending_signal() from guc_exec_queue_kill()
>    and guc_exec_queue_suspend_timeout_ban(), we still have
>    handle_sched_done() and guc_exec_queue_stop() which can race against
>    each other in accessing suspend_pending and clearing it.

I don't think this part can race.

 - __guc_exec_queue_process_msg_suspend, this is only there for GT
   resets racing (I think). This should be executed before or after
   guc_exec_queue_stop() but not in parallel.
 - guc_exec_queue_wait_suspend_done(), this on wait queue and we don't
   have lock upon reading, at least in this patch. So if justification
   is all readers need a lock, then this is missing in this patch.
 - guc_exec_queue_stop() touch this but this code is only reachable
   when GuC exec queue is stopped and CTs are down. I guess a new
   suspend could come in and race, so maybe in a lock is needed here.

> 
> So, probably this locking extention patch here might be simpler and
> effective.

I'm thinking this need a bit more rework and would like to get this
right in single patch. I'm fine with a lock, but let's at least make
guc_exec_queue_wait_suspend_done() consistent in using a lock and remove
the two places we should not be calling __suspend_pending_signal().

Matt

> 
> Niranjana
> 
> > Matt
> > 
> > > 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>
> > > ---
> > >  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
> > >