Re: [PATCH v2 1/1] drm/xe/guc: Fix wake_up race in handle_sched_done

[Maciej Patelczyk <maciej.patelczyk@intel.com>]

On 09/09/2026 09:08, Jakub Legowski wrote:

> Move wake_up_all() after clear_exec_queue_pending_disable()
>
> When a SCHED_DONE G2H arrives for a banned exec queue,
> handle_sched_done() calls wake_up_all() before
> clear_exec_queue_pending_disable(). This creates a race with the
> wait_event_timeout() in guc_exec_queue_timedout_job() that waits
> for pending_disable to be cleared:
>
>      smp_rmb();
>      ret = wait_event_timeout(guc->ct.wq,
>                   ...
>                   ...);
>
> This was observed as:
> [drm:guc_exec_queue_timedout_job [xe]] Tile0: GT0: Check job timeout: seqno=4294967169, lrc_seqno=4294967169, guc_id=4, running_time_ms=0, timeout_ms=5000, diff=0x00000001
>
> Moving the wake_up_all() (along with its smp_wmb())
> after all paths that clear pending_disable, ensuring that waiters
> always observe the updated state when woken.
>
> Same as for runnable_state == 1 in the beginning of
> handle_sched_done().
>
> Signed-off-by: Jakub Legowski <jakub.legowski@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_guc_submit.c | 30 ++++++++++++++++--------------
>   1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 99d8c807ff05..a181b5042293 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -3239,25 +3239,27 @@ static void handle_sched_done(struct xe_guc *guc, struct xe_exec_queue *q,
>   			clear_exec_queue_pending_disable(q);
>   			suspend_fence_signal(q);
>   		} else {
> +			bool destroyed = exec_queue_destroyed(q);
> +
> +			/*
> +			 * Make sure to clear the pending_disable only
> +			 * after sampling the destroyed state. We want
> +			 * to ensure we don't trigger the unregister too
> +			 * early with something intending to only
> +			 * disable scheduling. The caller doing the
> +			 * destroy must wait for an ongoing
> +			 * pending_disable before marking as destroyed.
> +			 */
> +			clear_exec_queue_pending_disable(q);
> +
>   			if (exec_queue_banned(q)) {
> +				/* Pairs with smp_rmb in guc_exec_queue_timedout_job */
>   				smp_wmb();
>   				wake_up_all(&guc->ct.wq);
>   			}
> -			if (exec_queue_destroyed(q)) {
> -				/*
> -				 * Make sure to clear the pending_disable only
> -				 * after sampling the destroyed state. We want
> -				 * to ensure we don't trigger the unregister too
> -				 * early with something intending to only
> -				 * disable scheduling. The caller doing the
> -				 * destroy must wait for an ongoing
> -				 * pending_disable before marking as destroyed.
> -				 */
> -				clear_exec_queue_pending_disable(q);
> +
> +			if (destroyed)
>   				deregister_exec_queue(guc, q);
> -			} else {
> -				clear_exec_queue_pending_disable(q);
> -			}
>   		}
>   	}
>   }

Apply suggested changes by Matthew.

Looks good.

Reviewed-by: Maciej Patelczyk <maciej.patelczyk@intel.com>