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

["Randhawa, Jagmeet" <jagmeet.randhawa@intel.com>]

On 8/19/2026 9:13 PM, Niranjana Vishwanathapura wrote:
> 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

Gentle ping on this one.

What would you like me to do here - respin along the lines of Niranjana's
suggestion above, or take a different route?

Thanks,
Jagmeet
>
>> 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
>>> > > > >