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 08:51:21PM -0700, Niranjana Vishwanathapura wrote:
> On Tue, Aug 18, 2026 at 06:01:20PM -0700, Matthew Brost wrote:
> > 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?
> > 
> 
> Below Sashiko report brought it up as a pre-existing issue.
> https://sashiko.dev/#/patchset/20260713202317.2187787-8-niranjana.vishwanathapura%40intel.com
> 

Ok.

> > > 
> > > 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.
> > 
> 
> If we do not call it, then it will leave queue as suspended forever, which
> can result in some other asserts down the line. Clearing it is part of the
> error handling recovery it looks like. As I mentioned, it will likely hit
> the !suspend_pending assert in guc_exec_queue_resume() under current code.
>

But the upper layers shouldn't call resume if suspend_wait fails, hence
we shouldn't hit that assert.

> > > 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.
> > 
> 
> If we do not wake here, then every kill happended during a suspend can
> leave the suspend_wait() wait for 5 seconds to see the queue has been killed.
> 

The suspend message should either issue a H2G that will result the
suspend fence signaling in the G2H or signal it directly *after* the
queue is off the hardware.

But I do see a potenial race. The kill really needs to be ordered behind
any suspends too.

We probably want a version of this patch which only sets the kill bit
inside the KILL message:

https://patchwork.freedesktop.org/patch/732703/?series=168398&rev=4

> > > 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.
> 
> The locking in this patch is more of a write side serialization lock
> (where supend_pending is written and where test-and-clear cases). So,
> I don't think we need locking here for reading suspend_pending.
> 
> > - 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().
> > 
> 
> Dropping __suspend_fence_signal() in those places will lead to above
> mentioned issues with current state of the driver. I am worried that
> fixing those might be beyond the scope of this patch. What do you suggest?
> 

My opinion is that this patch is trying to work around broken code in
the state machine, which we have to fix anyway. I'd rather audit
everything related to kill and suspend fences and get it right, rather
than adding locking on top that we'd have to unwind later anyway. If
this were a band-aid for a reported crash, then maybe. However, this is
a Sashiko report suggesting a fix for what I see as already broken code.

Matt

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