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

On 8/13/2026 8:51 PM, Niranjana Vishwanathapura wrote:
> On Fri, Aug 14, 2026 at 04:18:48AM +0800, Jagmeet Randhawa wrote:
>> q->guc->suspend_pending is accessed without any common lock.
>> __suspend_fence_signal(), called from guc_exec_queue_kill() and the
>> 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.
>>
>
> Looks good,
> Though it looks like overloading of sched->msg_lock, it is not because
> guc->suspend_pending is indeed indication of wating for message process
> completion. Perhaps it would be good to update the documentation of
> sched->msg_lock and mention what all it protects (sched->msgs list and
> guc->suspend_pending which indicates suspend message is in fligt).
>
> Niranjana

Thanks Niranjana. Good point, I'll document that msg_lock also covers 
suspend_pending (the in-flight suspend indication), alongside the msgs 
list, and send a v2.

Jagmeet
>
>> Signed-off-by: Jagmeet Randhawa <[email protected]>
>> ---
>> drivers/gpu/drm/xe/xe_guc_submit.c | 29 ++++++++++++++++++++++++-----
>> 1 file changed, 24 insertions(+), 5 deletions(-)
>>
>> 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
>>