Re: [PATCH v15 03/10] drm/xe/gt: Introduce FLR helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] drm/xe: scheduler teardown race and work_tdr livelock in
  xe_uc_flr_prepare()
- [Medium] drm/xe: race in xe_uc_flr_prepare() stopping queues before
  GuC CT
- [Medium] drm/xe/uc: race with gt_reset_worker in xe_uc_flr_prepare()
  underflows stopped
--

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

drm/xe/gt: Introduce FLR helpers

This commit introduces helper functions across the GT, uC, GuC, HuC, and
GSC layers to prepare for and re-initialize after a PCIe Function Level
Reset (FLR).

> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index 300126e6624f6..c4fb34e64cd6d 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -1736,6 +1736,35 @@ void xe_guc_sanitize(struct xe_guc *guc)
>  	xe_guc_submit_disable(guc);
>  }
>  
> +/**
> + * xe_guc_flr_prepare() - Prepare GuC for FLR
> + * @guc: The GuC object
> + *
> + * Stop GuC submission and tear down exec queues.
> + */
> +void xe_guc_flr_prepare(struct xe_guc *guc)
> +{
> +	if (!xe_uc_fw_is_loadable(&guc->fw))
> +		return;
> +
> +	xe_guc_submit_stop(guc);
> +	xe_guc_submit_pause_abort(guc);
> +}

[Severity: Medium]
As reported by Tauro, Riana on v10 [1], can stopping and aborting exec
queues here before stopping GuC CT in uc_flr_sanitize() cause
guc_exec_queue_destroy_async() to run twice on the same queue?

All other submission teardown paths (xe_guc_stop(), guc_submit_fini(), and
xe_guc_submit_wedge()) call xe_guc_ct_stop() to stop CT and cancel
ct->g2h_worker before calling xe_guc_submit_stop(), and call
xe_uc_fw_sanitize() before xe_guc_submit_pause_abort(). Here, ct->g2h_worker
is still active during uc_flr_prepare():

kworker (gt->ordered_wq)          kworker (ct->g2h_worker)
uc_flr_prepare()                  dequeue_one_g2h()
  xe_guc_flr_prepare()              g2h_read()
    xe_guc_submit_stop()
      guc_exec_queue_stop(q)
        /* clears REGISTERED,
         * keeps DESTROYED */
        guc_exec_queue_destroy_async(q)
                                    process_g2h_msg()
                                      xe_guc_deregister_done_handler()
                                        handle_deregister_done()
                                          guc_exec_queue_destroy_async(q)
                                            <-- double destroy

Because guc_exec_queue_stop() clears EXEC_QUEUE_STATE_REGISTERED (along
with PENDING_DISABLE, PENDING_ENABLE, and ENABLED) while leaving
EXEC_QUEUE_STATE_DESTROYED set, a concurrent
xe_guc_deregister_done_handler() passes its state check and queues
guc_exec_queue_destroy_async() a second time, corrupting the work item or
underflowing the queue refcount.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
> index 89040d9fa05ff..2ba57c5c9979b 100644
> --- a/drivers/gpu/drm/xe/xe_uc.c
> +++ b/drivers/gpu/drm/xe/xe_uc.c

[ ... ]

> @@ -279,6 +281,79 @@ static void uc_reset_wait(struct xe_uc *uc)
>  		goto again;
>  }
>  
> +static void uc_flr_prepare(struct work_struct *w)
> +{
> +	struct xe_uc_flr *flr = container_of(w, struct xe_uc_flr, work);
> +	struct xe_uc *uc = flr->uc;
> +
> +	xe_gt_assert(uc_to_gt(uc), xe_wedged(uc_to_xe(uc)));
> +
> +	xe_guc_flr_prepare(&uc->guc);
> +}
> +
> +static void uc_flr_sanitize(struct work_struct *w)
> +{
> +	struct xe_uc_flr *flr = container_of(w, struct xe_uc_flr, work);
> +	struct xe_uc *uc = flr->uc;
> +
> +	/* TODO: Sanitize GSC firmware */
> +	xe_gt_assert(uc_to_gt(uc), IS_DGFX(uc_to_xe(uc)));
> +	xe_gt_assert(uc_to_gt(uc), !xe_uc_fw_is_loadable(&uc->gsc.fw));
> +
> +	xe_uc_stop(uc);
> +	xe_uc_sanitize(uc);
> +}
> +
> +/**
> + * xe_uc_flr_prepare() - Prepare uCs for FLR
> + * @uc: The uC object
> + *
> + * Tear down pending work and stop all uCs.
> + */
> +void xe_uc_flr_prepare(struct xe_uc *uc)
> +{
> +	struct xe_uc_flr flr = { .uc = uc };
> +	struct xe_gt *gt = uc_to_gt(uc);
> +
> +	xe_uc_reset_prepare(uc);
> +
> +	/*
> +	 * We'll be tearing down exec queues which signals the fences and frees the
> +	 * jobs but all of that happens asynchronously, so make sure we don't disrupt
> +	 * the scheduler while jobs are still in-flight.
> +	 */
> +	INIT_WORK_ONSTACK(&flr.work, uc_flr_prepare);
> +	queue_work(gt->ordered_wq, &flr.work);

[Severity: Medium]
Can an in-progress gt_reset_worker on gt->ordered_wq clear
guc->submission_state.stopped before uc_flr_prepare() runs?

CPU0                              kworker (gt->ordered_wq)
                                  gt_reset_worker()
                                    /* passes xe_wedged() check */
xe_gt_flr_prepare()
  xe_uc_flr_prepare()
    xe_uc_reset_prepare()
      /* stopped is already 1 */
    queue_work(..., &flr.work)
                                    do_gt_restart()
                                      xe_uc_start()
                                        xe_guc_submit_start()
                                          atomic_dec(&...->stopped) /* -> 0 */
                                  uc_flr_prepare()
                                    xe_guc_flr_prepare()
                                      xe_guc_submit_stop()
                                        xe_gt_assert(..., stopped == 1)

When gt_reset_worker finishes before uc_flr_prepare() starts on
gt->ordered_wq, xe_guc_submit_start() decrements stopped back to 0. That
trips the xe_guc_read_stopped(guc) == 1 assert in xe_guc_submit_stop(), and
when xe_uc_start() runs after FLR it decrements stopped from 0 to -1,
leaving GuC submission permanently stopped. Calling xe_uc_reset_prepare(uc)
again inside uc_flr_prepare() (as xe_guc_submit_wedge() does on
gt->ordered_wq) would ensure stopped remains set.

> +	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);
> +}

[Severity: High]
Can queuing uc_flr_sanitize() immediately after uc_flr_prepare() on
gt->ordered_wq livelock work_tdr or leave pending jobs stranded when
in-flight jobs are being torn down?

Inside uc_flr_prepare(), xe_guc_submit_pause_abort() starts each scheduler
and calls guc_exec_queue_kill() -> drm_sched_tdr_queue_imm(), which sets
sched->base.timeout = 0 and queues work_tdr on gt->ordered_wq. When
drm_sched_job_timedout() runs ahead of uc_flr_sanitize(),
guc_exec_queue_timedout_job() marks the job's fence with an error via
async irq_work and returns DRM_GPU_SCHED_STAT_NO_HANG:

drivers/gpu/drm/scheduler/sched_main.c:drm_sched_job_timedout() {
	...
		if (status == DRM_GPU_SCHED_STAT_NO_HANG)
			drm_sched_job_reinsert_on_false_timeout(sched, job);
	} else {
		spin_unlock(&sched->job_list_lock);
	}

	if (status != DRM_GPU_SCHED_STAT_ENODEV)
		drm_sched_start_timeout_unlocked(sched);
	...
}

drm_sched_job_reinsert_on_false_timeout() puts the job back on
pending_list and queues work_free_job on sched->base.submit_wq (a separate
per-scheduler workqueue, not gt->ordered_wq), and
drm_sched_start_timeout_unlocked() immediately re-queues work_tdr with
delay 0 on gt->ordered_wq behind uc_flr_sanitize():

kworker (gt->ordered_wq)          kworker (sched->submit_wq)
uc_flr_prepare()
  ...
    drm_sched_tdr_queue_imm()
drm_sched_job_timedout()
  /* queues irq_work to signal fence */
  /* reinserts job on pending_list */
  drm_sched_run_free_queue()
  drm_sched_start_timeout_unlocked()
    /* re-queues work_tdr (delay 0)
     * behind uc_flr_sanitize */
uc_flr_sanitize()
  xe_uc_stop()
    ...
      drm_sched_wqueue_stop()
        sched->pause_submit = true
        cancel_work_sync(&work_free_job)
                                  /* work_free_job cancelled before
                                   * freeing reinserted job */
drm_sched_job_timedout()
  /* fence is now signaled: returns
   * NO_HANG early, reinserts job;
   * run_free_queue() is a no-op
   * because pause_submit is true;
   * re-queues work_tdr (delay 0) */  <-- infinite 0-delay work_tdr loop

If the re-queued work_tdr instead runs before irq_work signals the fence
(or if work_run_job on submit_wq pops an entity job and queues work_tdr
behind uc_flr_sanitize()), guc_exec_queue_timedout_job() calls
xe_sched_submission_start(sched) after uc_flr_sanitize(), leaving
pause_submit false and tripping the drm_sched_is_stopped() assert in
xe_sched_reinit().

[1] https://lore.kernel.org/r/fcd99d4a-b7b4-4bfd-9e78-65ff1ea2d7d7@intel.com

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