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

On Tue, Aug 18, 2026 at 06:51:42PM +0000, [email protected] wrote:
>Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
>Pre-existing issues:
>- [High] TDR (Timeout Delay Recovery) races with asynchronous queue suspend, leading to misattributed SCHED_DONE messages, an assertion failure, and a TDR hang.
>--
>
>--- Patch [1]: [PATCH v2] drm/xe/guc: Fix race around q->guc->suspend_pending access ---
>commit 256a06cec5d42e79b9711d9f1861b3717e23b08f
>Author: Jagmeet Randhawa <[email protected]>
>
>drm/xe/guc: Fix race around q->guc->suspend_pending access
>
>This commit wraps accesses to q->guc->suspend_pending with xe_sched_msg_lock
>in the suspend, resume, kill, and ban paths. This ensures that checking the
>flag and calling suspend_fence_signal() is atomic with respect to the
>asynchronous clears in the kill and ban paths.
>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>> index 9036f89dff7d9..c565c1d32d3a4 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>[ ... ]
>> @@ -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);
>
>[Severity: High]
>This is a pre-existing issue, but does this logic misattribute a SCHED_DONE
>message if TDR races with an asynchronous queue suspend?
>
>If a queue is being suspended (setting q->guc->suspend_pending = true) and
>concurrently a job times out, the TDR handler stops the DRM scheduler,
>trapping the SUSPEND message before it can execute. TDR then calls
>disable_scheduling(), which directly sends a DISABLE command to the GuC.
>
>When the GuC responds to the DISABLE command with a SCHED_DONE message,
>won't handle_sched_done() incorrectly attribute it to the trapped SUSPEND
>message because was_pending is still true?
>
>It looks like this would call suspend_fence_signal(q) and trigger an
>assertion failure, because the queue was never actually suspended (the
>message was trapped) or killed (TDR only bans it). Additionally, skipping
>the wake_up_all() call in the other branch might cause the TDR thread to
>wait and hang for 5 seconds until it times out.
>

As the report says, it it unrelated, pre-existing issue which should
be addressed separately.

Niranjana

>> +
>> +		if (!was_pending) {
>>  			if (exec_queue_banned(q)) {
>>  				smp_wmb();
>>  				wake_up_all(&guc->ct.wq);
>
>-- 
>Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1