Re: [PATCH v12 07/10] drm/xe/pm: Introduce xe_device_suspend/resume()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/xe: work_tdr livelock and scheduler race in xe_device_suspend() FLR path
- [High] drm/xe: missing ULLS teardown and synchronization across FLR in xe_device_suspend()
- [High] drm/xe: missing re-initialization of xe->cpu_bind->q in xe_device_resume()
--

commit c9d7243b4c02644f3740b46ba51d0f51d005d2ba
Author: Raag Jadav <raag.jadav@intel.com>

drm/xe/pm: Introduce xe_device_suspend/resume()

This commit extracts the system suspend and resume logic from xe_pm.c into
common xe_device_suspend() and xe_device_resume() helpers in xe_device.c,
adding an flr parameter to support PCIe FLR preparation and
re-initialization alongside system suspend and resume.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 07a7ab6ac0832..c54a42cdf9a47 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c

[ ... ]

> @@ -1384,3 +1386,150 @@ void xe_device_exit(void)
>  		wait_var_event(&xe_device_count, !atomic_read(&xe_device_count));
>  	}
>  }
> +
> +/**
> + * xe_device_suspend() - Common helper for system suspend and FLR prepare
> + * @xe: xe device instance
> + * @flr: indicate FLR prepare
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int xe_device_suspend(struct xe_device *xe, bool flr)
> +{
> +	struct xe_gt *gt;
> +	u8 id;
> +	int err;
> +
> +	err = xe_pxp_pm_suspend(xe->pxp);
> +	if (err)
> +		return err;
> +
> +	xe_late_bind_wait_for_worker_completion(&xe->late_bind);
> +
> +	for_each_gt(gt, xe, id)
> +		xe_gt_suspend_prepare(gt);
> +
> +	if (flr) {
> +		for_each_gt(gt, xe, id)
> +			xe_gt_flr_prepare(gt);

[Severity: High]
Can this FLR preparation path livelock on sched->base.work_tdr or leave the
scheduler running when xe_device_resume() later calls xe_sched_reinit()?

In xe_gt_flr_prepare() -> xe_uc_flr_prepare(), uc_flr_prepare() and
uc_flr_sanitize() are queued and flushed back-to-back on gt->ordered_wq:

	INIT_WORK_ONSTACK(&flr.work, uc_flr_prepare);
	queue_work(gt->ordered_wq, &flr.work);
	flush_work(&flr.work);
	destroy_work_on_stack(&flr.work);

	INIT_WORK_ONSTACK(&flr.work, uc_flr_sanitize);
	queue_work(gt->ordered_wq, &flr.work);
	flush_work(&flr.work);
	destroy_work_on_stack(&flr.work);

When an exec queue has jobs in flight, uc_flr_prepare() ->
xe_guc_flr_prepare() -> xe_guc_submit_pause_abort() starts submission and
calls guc_exec_queue_kill() -> xe_guc_exec_queue_trigger_cleanup() ->
xe_sched_tdr_queue_imm(), which sets sched->base.timeout = 0 and queues
sched->base.work_tdr (drm_sched_job_timedout) on gt->ordered_wq with delay 0.

When drm_sched_job_timedout() runs on gt->ordered_wq before
uc_flr_sanitize(), guc_exec_queue_timedout_job() marks the job fence with
-ETIME, calls xe_sched_submission_start(), and returns
DRM_GPU_SCHED_STAT_NO_HANG. drm_sched_job_timedout() then calls
drm_sched_job_reinsert_on_false_timeout(), which puts the signaled job back
on sched->base.pending_list and queues work_free_job on
sched->base.submit_wq (a separate workqueue from gt->ordered_wq), followed
immediately by drm_sched_start_timeout_unlocked(). Unless work_free_job on
submit_wq has already completed on another CPU and removed the job from
pending_list before that call, drm_sched_start_timeout() sees
sched->base.timeout == 0 and a non-empty pending_list, re-queueing work_tdr
with delay 0 on gt->ordered_wq behind uc_flr_sanitize().

Next on gt->ordered_wq, uc_flr_sanitize() -> xe_uc_stop() -> xe_guc_stop() ->
xe_guc_submit_stop() -> guc_exec_queue_stop() calls
xe_sched_submission_stop(), which sets sched->base.pause_submit = true and
cancels work_free_job. If work_free_job had not run yet on submit_wq, the
signaled job remains on sched->base.pending_list. When the re-queued
work_tdr runs, guc_exec_queue_timedout_job() sees DMA_FENCE_FLAG_SIGNALED_BIT
set and returns DRM_GPU_SCHED_STAT_NO_HANG without starting the scheduler;
drm_sched_job_reinsert_on_false_timeout() re-adds the job to pending_list
while drm_sched_run_free_queue() is a no-op because drm_sched_is_stopped() is
true, and drm_sched_start_timeout_unlocked() re-queues work_tdr with delay 0
in an infinite loop on gt->ordered_wq.

Conversely, if a job was still in entity->job_queue when uc_flr_prepare()
ran, work_run_job on sched->base.submit_wq can pop the job into pending_list
and queue work_tdr on gt->ordered_wq after uc_flr_sanitize() was queued.
After uc_flr_sanitize() stops the scheduler, work_tdr runs
guc_exec_queue_timedout_job(), which calls xe_sched_submission_start() and
leaves pause_submit == false, triggering:

	xe_gt_assert(ge->q->gt, drm_sched_is_stopped(&sched->base));

in xe_sched_reinit() during xe_device_resume(xe, true).

[Severity: High]
What stops tile->migrate->ulls.exit_work from racing with FLR preparation
and re-initialization or leaving stale ULLS state after reset?

On DGFX platforms, GPU page faults (xe_pagefault_service()) and SVM
prefetches (xe_vm_prefetch()) call xe_migrate_ulls_enter(), which sets
m->ulls.enabled = true under m->job_mutex, submits a ULLS_ENTER job to m->q,
and schedules m->ulls.exit_work (xe_migrate_ulls_exit()) on
system_percpu_wq. Neither xe_device_suspend(xe, true) nor
xe_device_resume(xe, true) (via xe_gt_reinit() -> xe_migrate_reinit())
disables or flushes m->ulls.exit_work, takes m->job_mutex, or resets
m->ulls.enabled (unlike xe_migrate_fini()).

This appears to allow three failure modes across FLR:

1. If m->ulls.exit_work fires while m->q->guc->sched is stopped (after
uc_flr_sanitize() during the FLR window, or in xe_device_resume(xe, true)
before xe_migrate_reinit()), xe_migrate_ulls_exit() sees
xe_exec_queue_is_idle(m->q, 1) and calls __xe_migrate_job_push(m, job,
ULLS_EXIT), which arms the job via xe_sched_job_arm() ->
xe_lrc_init_seqno_fence(), incrementing m->q->lrc[0]->fence_ctx.next_seqno
and queueing the job on m->q->entity. When xe_migrate_reinit() ->
xe_exec_queue_reinit(m->q) -> xe_lrc_ctx_init() runs without m->job_mutex,
it writes lrc->fence_ctx.next_seqno - 1 into __xe_lrc_seqno_map(lrc) and
__xe_lrc_start_seqno_map(lrc), marking the unexecuted ULLS_EXIT job's seqno
as completed in the hardware seqno page before the job is emitted in
xe_guc_submit_start().

2. If m->ulls.exit_work fires during xe_uc_flr_prepare() between
xe_guc_submit_pause_abort() (which sets EXEC_QUEUE_STATE_KILLED on m->q) and
uc_flr_sanitize(), guc_exec_queue_run_job() sees
exec_queue_killed_or_banned_or_wedged(m->q), skips q->ring_ops->emit_job(job)
(leaving job->ptrs[0].head == 0), and places the unsignaled ULLS_EXIT job on
sched->base.pending_list. During xe_device_resume(xe, true), after
xe_lrc_ctx_init() writes MI_ARB_ON_OFF into the ring, xe_guc_submit_start()
-> guc_exec_queue_start(m->q) finds that pending job and rewinds
lrc->ring.tail, RING_HEAD, and RING_TAIL to job->ptrs[0].head (0),
overwriting the ring state initialized by xe_lrc_ctx_init().

3. If m->ulls.exit_work has not fired before FLR resume completes,
m->ulls.enabled remains true even though the hardware reset wiped engine
state, causing subsequent xe_migrate_ulls_enter() calls to skip submitting a
ULLS_ENTER job.

[ ... ]

> +int xe_device_resume(struct xe_device *xe, bool flr)
> +{
> +	struct xe_tile *tile;
> +	struct xe_gt *gt;
> +	u8 id;
> +	int err;
> +
> +	err = xe_pcode_ready(xe, true);
> +	if (err)
> +		return err;
> +
> +	assert_lmem_ready(xe);
> +
> +	for_each_tile(tile, xe, id)
> +		xe_wa_apply_tile_workarounds(tile);
> +
> +	if (flr) {
> +		err = xe_bo_restore_map(xe);
> +		if (err)
> +			return err;
> +
> +		for_each_gt(gt, xe, id) {
> +			err = xe_gt_reinit(gt);
> +			if (err)
> +				return err;
> +		}
> +	}

[Severity: High]
Does the FLR re-initialization branch also need to re-initialize
xe->cpu_bind->q?

During probe, xe_device_probe() -> xe_cpu_bind_init() creates the device-wide
kernel bind queue xe->cpu_bind->q with EXEC_QUEUE_FLAG_KERNEL |
EXEC_QUEUE_FLAG_MIGRATE, and guc_exec_queue_init() inserts it into
guc->submission_state.exec_queue_lookup.

During xe_device_suspend(xe, true), xe_gt_flr_prepare() ->
xe_uc_flr_prepare() -> xe_guc_flr_prepare() -> xe_guc_submit_pause_abort()
iterates over guc->submission_state.exec_queue_lookup and calls
guc_exec_queue_kill() on every queue, which sets EXEC_QUEUE_STATE_KILLED on
xe->cpu_bind->q and zeroes its scheduler timeout.

In xe_device_resume(xe, true), xe_gt_reinit() only re-initializes
tile->migrate->q via xe_migrate_reinit() -> xe_exec_queue_reinit(m->q),
leaving xe->cpu_bind->q with EXEC_QUEUE_STATE_KILLED set. When a subsequent
pipelined page-table update is scheduled on xe->cpu_bind->q (via
xe_vm_rebind(), xe_vma_rebind(), xe_vm_range_rebind(), or
xe_vm_range_unbind() -> xe_cpu_bind_update_pgtables_job()),
guc_exec_queue_run_job() sees exec_queue_killed_or_banned_or_wedged(q) as
true:

	if (is_pt_job(job)) {
		xe_gt_assert(guc_to_gt(guc), !exec_queue_registered(q));

		run_pt_job(guc_to_xe(guc), job,
			   killed_or_banned_or_wedged_or_error);

and passes force_clear = true to run_pt_job() ->
xe_cpu_bind_update_pgtables_execute(), clearing page table entries instead of
populating them while still committing the VMA as present.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103060649.1015468-1-raag.jadav@intel.com?part=7