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

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.

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

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.

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.

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

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