Re: [PATCH] drm/xe/guc: Skip CLEANUP message when GuC is stopped

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

New issues:
- [High] An unprotected Time-of-Check to Time-of-Use (TOCTOU) race condition exists between checking the GuC stopped state and adding the CLEANUP message, leading to suspended hardware access.

Pre-existing issues:
- [High] Bypassing the CLEANUP message abandons pending messages in the scheduler queue, causing memory and runtime PM reference leaks.
--

--- Patch [1]: [PATCH] drm/xe/guc: Skip CLEANUP message when GuC is stopped ---
commit 3774f2ff22deecb1f1aaf595a02f4bc19900b790
Author: Varun Gupta <[email protected]>

drm/xe/guc: Skip CLEANUP message when GuC is stopped

Attempting to enqueue a CLEANUP message while the GuC is stopped
(during a device reset or suspend) triggers a missing outer runtime
PM protection warning in xe_pm_runtime_get_noresume().

Since the GuC cannot service Command Transport (CT) requests while
stopped, skip the message and route directly to local teardown.

Cc: [email protected]

> 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
> @@ -2180,12 +2180,14 @@ static bool guc_exec_queue_try_add_msg(struct xe_exec_queue *q,
>  #define STATIC_MSG_CGP_SYNC	3
>  static void guc_exec_queue_destroy(struct xe_exec_queue *q)
>  {
> +	struct xe_guc *guc = exec_queue_to_guc(q);
>  	struct xe_sched_msg *msg = q->guc->static_msgs + STATIC_MSG_CLEANUP;
>  
> -	if (!(q->flags & EXEC_QUEUE_FLAG_PERMANENT) && !exec_queue_wedged(q))
> +	if (!(q->flags & EXEC_QUEUE_FLAG_PERMANENT) && !exec_queue_wedged(q) &&
> +	    !xe_guc_read_stopped(guc))

[Severity: High]
Could this regression introduce a race condition between checking the GuC
stopped state and adding the CLEANUP message?

The lockless check of xe_guc_read_stopped(guc) creates a window before
xe_pm_runtime_get_noresume() is called inside guc_exec_queue_add_msg().
If a thread evaluates the check as true, but concurrently the device performs
a runtime PM suspend and reaches RPM_SUSPENDED, the thread will still proceed
to add the CLEANUP message.

This would increment the PM reference without waking the device, leading
to the worker executing hardware instructions against powered-down registers
when it processes the message.

>  		guc_exec_queue_add_msg(q, msg, CLEANUP);
>  	else
> -		__guc_exec_queue_destroy(exec_queue_to_guc(q), q);
> +		__guc_exec_queue_destroy(guc, q);
>  }

[Severity: High]
This is a pre-existing issue, but does bypassing the CLEANUP message abandon
pending messages in the scheduler queue?

By moving straight to local teardown without draining pending messages,
the queue is destroyed immediately. Later, xe_sched_fini() unconditionally
cancels the work_process_msg worker:

drivers/gpu/drm/xe/xe_gpu_scheduler.c:xe_sched_submission_stop() {
    ...
    drm_sched_wqueue_stop(&sched->base);
    cancel_work_sync(&sched->work_process_msg);
}

Any prior messages (like SUSPEND or SET_SCHED_PROPS) still pending in
sched->msgs would be stranded and never processed. This permanently leaks
the dynamically allocated messages and their associated runtime PM
references. 

A leaked PM reference permanently prevents the GPU from entering runtime
suspend (D3 states), resulting in continuous power drain until the system
is rebooted.

Will the CLEANUP message still be needed to act as a barrier to ensure
the queue is drained properly?

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