Re: [PATCH v10 02/10] drm/xe/guc_submit: Introduce guc_exec_queue_reinit_kernel()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unprotected modification of scheduler timeout in xe_sched_reinit() races with active TDR work, and drm_sched_is_stopped() assert will falsely trigger.
- [Medium] Layering violation: generic `xe_gpu_scheduler` component directly references GuC-specific `xe_guc_exec_queue` via downcast.
--

--- Patch [2]: [PATCH v10 02/10] drm/xe/guc_submit: Introduce guc_exec_queue_reinit_kernel() ---
commit 9c5abd285bbf057d5cc8df0e74c9cc1e73a4d283
Author: Raag Jadav <[email protected]>

drm/xe/guc_submit: Introduce guc_exec_queue_reinit_kernel()

In preparation of usecases which require re-initializing GuC submission
after PCIe FLR, introduce guc_exec_queue_reinit_kernel() helper. This will
restore kernel queues which might have been killed before PCIe FLR.

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

[ ... ]

> @@ -84,6 +88,19 @@ int xe_sched_init(struct xe_gpu_scheduler *sched,
>  	return drm_sched_init(&sched->base, &args);
>  }
>  
> +void xe_sched_reinit(struct xe_gpu_scheduler *sched)
> +{
> +	struct xe_guc_exec_queue *ge = container_of(sched, struct xe_guc_exec_queue, sched);

[Severity: Medium]
Is it safe for the generic xe_gpu_scheduler component to directly downcast to
the GuC-specific xe_guc_exec_queue structure?

If xe_sched_reinit() is ever called by another submission backend (such as
execlists), couldn't this result in an invalid downcast and potential
memory corruption?

> +
> +	xe_gt_assert(ge->q->gt, drm_sched_is_stopped(&sched->base));
> +
> +	/*
> +	 * TODO: Implement drm_sched_reinit() instead of requiring the
> +	 * driver to restore individual fields.
> +	 */
> +	sched->base.timeout = sched->timeout;

[Severity: High]
Will the drm_sched_is_stopped() assert falsely trigger during PCIe FLR?

During hardware resets like PCIe FLR, xe_guc_submit_pause_abort() executes on
kernel queues, calling xe_sched_submission_start() (which sets pause_submit to
false) and queuing TDR via guc_exec_queue_kill(). If xe_sched_reinit() is
later called on the same queue, the scheduler is no longer stopped, causing
this assert to fail.

Additionally, does modifying sched->base.timeout here race with the concurrent
TDR work (work_tdr)? Modifying it without calling cancel_delayed_work_sync()
might leave the TDR work executing with an inconsistent timeout state on a
re-initialized kernel queue.

> +}
> +
>  void xe_sched_fini(struct xe_gpu_scheduler *sched)

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