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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] TDR firing concurrently with a queue suspend request routes the GuC DISABLE response incorrectly, leading to an assertion failure and an unnecessary GT reset.
--

--- Patch [1]: [PATCH] drm/xe/guc: Fix race around q->guc->suspend_pending access ---
commit 9e45449078caaa7428f1073720ba630cb43d9ca9
Author: Jagmeet Randhawa <[email protected]>

drm/xe/guc: Fix race around q->guc->suspend_pending access

This patch wraps accesses to q->guc->suspend_pending with the msg_lock to
prevent concurrent clear and check-then-act sequences from tripping assertions.
The lock is extended across suspend and resume paths, as well as kill and ban.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c

[ ... ]

> @@ -3092,13 +3104,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);

[Severity: High]
This is a pre-existing issue, but does routing the GuC DISABLE response based
solely on was_pending fail to account for TDR-initiated DISABLEs?

If a Timeout Detection and Recovery (TDR) event fires concurrently with a queue
suspend request, it seems the following sequence could occur:

1. TDR stops the scheduler but hasn't yet marked the queue as BANNED.
2. A suspend request sets q->guc->suspend_pending = true. The SUSPEND message
   is never processed because the scheduler is stopped.
3. TDR marks the queue as BANNED and sends a DISABLE action to GuC.
4. When GuC replies to the TDR's DISABLE, handle_sched_done() sees
   was_pending == true and calls suspend_fence_signal(q).

Will this trip the assertion in suspend_fence_signal(q) because the queue is
BANNED, not SUSPENDED, and also skip the !was_pending block, causing a missed
wakeup for TDR and an unnecessary full GT reset?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1