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

On Wed, Aug 19, 2026 at 12:35:43PM -0700, Matthew Brost wrote:
>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.

Looks like there are cases where it does get called but
__guc_exec_queue_process_msg_resume() handles it by checking the
guc_exec_queue_allowed_to_change_state().

I think it should be fine. We can drop the __suspend_fence_signal() here
and avoid the !suspend_pending assert by adding additonal condition that
the queue must not be in a killed/banned/wedged state.

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

I think we should be fine to drop __suspend_fence_pending() here. We can
just do the wakeup part here for now to ensure suspend_wait() gets woken
up properly as we are setting the state to 'killed'.

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

I agree, but by not calling __suspend_signal_fence() we would leave the
suspend_pending to true in those cases until the queue is teared down.
Perhaps it should be ok.

So, does dropping __suspend_fence_signal() from guc_exec_queue_kill() and
guc_exec_queue_suspend_timeout_ban() (with above adjustments) and keeping
the other 2 places under the lock looks ok?

Niranjana

>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 <[email protected]>
>> > > > > ---
>> > > > >  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
>> > > > >