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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/xe: unsynchronized scheduler queues in xe_device_suspend()
  FLR path
--

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

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

This commit factors out xe_device_suspend() and xe_device_resume() from
xe_pm_suspend() and xe_pm_resume(), adding an flr flag to handle 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 273e2c45fea9b..6911c2ac39f82 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
[ ... ]
> @@ -1384,3 +1385,157 @@ 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
> + * @hibernate: true if preparing for S4 hibernation, false for < S4 suspend
> + * @flr: indicate FLR prepare
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int xe_device_suspend(struct xe_device *xe, bool hibernate, bool flr)
> +{
> +	struct xe_gt *gt;
> +	u8 id;
> +	int err;
> +
> +	err = xe_pxp_pm_suspend(xe->pxp);
> +	if (err)
> +		return err;
> +
> +	xe_late_bind_pm_suspend(&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 xe_gt_flr_prepare() -> xe_uc_flr_prepare() race with
sched->base.submit_wq when aborting in-flight GuC jobs during FLR, causing
either an infinite zero-delay work_tdr livelock on gt->ordered_wq or a
failed drm_sched_is_stopped() assertion in xe_sched_reinit() on resume?

xe_uc_flr_prepare() queues and flushes uc_flr_prepare() on gt->ordered_wq,
and immediately queues and flushes uc_flr_sanitize() on the same
gt->ordered_wq:

drivers/gpu/drm/xe/xe_uc.c:xe_uc_flr_prepare() {
    ...
	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);
}

uc_flr_prepare() calls xe_guc_submit_pause_abort(), which starts scheduler
submission (queuing work_run_job and work_free_job on sched->base.submit_wq,
a separate workqueue from gt->ordered_wq) and kills each queue via
xe_sched_tdr_queue_imm(), setting sched->base.timeout = 0 and queuing
work_tdr with delay 0 on gt->ordered_wq:

gt->ordered_wq                    sched->base.submit_wq
--------------                    ---------------------
uc_flr_prepare()
  xe_guc_submit_pause_abort()
    xe_sched_submission_start()
    guc_exec_queue_kill()
      xe_sched_tdr_queue_imm()
        sched->base.timeout = 0
        queue work_tdr (delay 0)
drm_sched_job_timedout()
  guc_exec_queue_timedout_job()
    dma_fence_set_error(-ETIME)
    return DRM_GPU_SCHED_STAT_NO_HANG
  drm_sched_job_reinsert_on_false_timeout()
    list_add(&job->list, &pending_list)
    queue work_free_job --------->
  drm_sched_start_timeout_unlocked()
    queue work_tdr (delay 0)
uc_flr_sanitize()
  xe_guc_stop()
    drm_sched_wqueue_stop()
      sched->base.pause_submit = true
      cancel_work_sync(&work_free_job)
                                  (work_free_job cancelled before running)
drm_sched_job_timedout()
  guc_exec_queue_timedout_job()
    (fence already signaled, returns NO_HANG)
  drm_sched_job_reinsert_on_false_timeout()
    (drm_sched_is_stopped() is true, work_free_job not queued)
  drm_sched_start_timeout_unlocked()
    queue work_tdr (delay 0)      <-- infinite livelock on gt->ordered_wq

Conversely, if a job was still queued in entity->job_queue when
uc_flr_prepare() ran, work_run_job on sched->base.submit_wq can pop it into
pending_list and queue work_tdr on gt->ordered_wq behind uc_flr_sanitize().
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, tripping
xe_gt_assert(ge->q->gt, drm_sched_is_stopped(&sched->base)) in
xe_sched_reinit() during xe_device_resume(xe, true).

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