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

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

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