Re: [PATCH v12 1/9] drm/xe/gt: Use GT ordered workqueue for wedging

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/xe/gt: deadlock on gt->ordered_wq by deferring xe_guc_reset_prepare()
- [High] drm/xe/gt: race in xe_gt_init() leaves GT unwedged on early device wedge
--

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

drm/xe/gt: Use GT ordered workqueue for wedging

This commit defers GT wedge teardown to a worker queued on the GT ordered
workqueue and introduces for_each_initialized_gt() to skip uninitialized
GTs when wedging the device.

> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index bc9bf6f0b5a79..2ed67fe929e34 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c

[ ... ]

> @@ -768,7 +774,7 @@ int xe_gt_init(struct xe_gt *gt)
>  
>  	err = gt_init_with_all_forcewake(gt);
>  	if (err) {
> -		gt_reset_worker_disable(gt);
> +		gt_worker_disable(gt);
>  		return err;
>  	}
>  
> @@ -784,6 +790,8 @@ int xe_gt_init(struct xe_gt *gt)
>  			return err;
>  	}
>  
> +	/* Must be last */
> +	WRITE_ONCE(gt->initialized, true);
>  	return 0;
>  }

[Severity: High]
Can setting gt->initialized only at the end of xe_gt_init() cause a GT to
be left unwedged if xe_device_declare_wedged() runs while xe_gt_init() is
still in progress?

In xe_device_probe(), xe_irq_install() runs before the xe_gt_init() loop,
enabling top-level interrupts (such as xe_mert_irq_handler(),
xe_hw_error_irq_handler(), and xe_sysctrl_irq_handler()) and error workers
like csc_hw_error_work. Inside xe_gt_init(), gt_init_with_gt_forcewake()
and gt_init_with_all_forcewake() initialize GuC CT, GuC submission,
gt->tlb_inval, and the migration exec queue, enable GuC communication, and
submit GPU jobs via emit_job_sync() in xe_gt_record_default_lrcs(), all
while gt->initialized is still false.

If xe_device_declare_wedged() is called during this window (from an IRQ or
error worker, from another GT on a multi-GT device, or from a job timeout
or reset failure during xe_gt_record_default_lrcs()):

    xe_device_declare_wedged()
        atomic_xchg(&xe->wedged.flag, 1);
        for_each_initialized_gt(gt, xe, id)
            xe_gt_declare_wedged(gt);

for_each_initialized_gt() skips the GT currently inside xe_gt_init()
because gt->initialized is still false. Once xe->wedged.flag is set,
g2h_read() and __guc_ct_send_locked() reject all GuC CT traffic with
-ENOTRECOVERABLE, but xe_gt_declare_wedged() is never queued for this GT to
abort pending GuC CT requests, TLB invalidation fences, or kernel exec
queues. Furthermore, if xe_gt_init() finishes concurrently with an
asynchronous wedge event, it sets gt->initialized = true without checking
xe_device_wedged(xe), leaving the GT active and unwedged on a wedged
device.

> @@ -1204,6 +1212,15 @@ struct xe_hw_engine *xe_gt_any_hw_engine(struct xe_gt *gt)
>  	return NULL;
>  }
>  
> +static void gt_wedge_worker(struct work_struct *w)
> +{
> +	struct xe_gt *gt = container_of(w, typeof(*gt), wedge.worker);
> +
> +	xe_uc_declare_wedged(&gt->uc);
> +	xe_tlb_inval_reset(&gt->tlb_inval);
> +	xe_pm_runtime_put(gt_to_xe(gt));
> +}
> +
>  /**
>   * xe_gt_declare_wedged() - Declare GT wedged
>   * @gt: the GT object
> @@ -1213,8 +1230,11 @@ struct xe_hw_engine *xe_gt_any_hw_engine(struct xe_gt *gt)
>   */
>  void xe_gt_declare_wedged(struct xe_gt *gt)
>  {
> -	xe_gt_assert(gt, gt_to_xe(gt)->wedged.mode);
> +	struct xe_device *xe = gt_to_xe(gt);
>  
> -	xe_uc_declare_wedged(&gt->uc);
> -	xe_tlb_inval_reset(&gt->tlb_inval);
> +	xe_gt_assert(gt, xe->wedged.mode);
> +
> +	xe_pm_runtime_get_noresume(xe);
> +	if (!queue_work(gt->ordered_wq, &gt->wedge.worker))
> +		xe_pm_runtime_put(xe);
>  }

[Severity: High]
Does deferring xe_uc_reset_prepare() to gt_wedge_worker() risk a deadlock
and 5-second TDR timeouts on gt->ordered_wq?

When xe_device_declare_wedged() sets xe->wedged.flag, g2h_read() and
__guc_ct_send_locked() immediately begin returning -ENOTRECOVERABLE:

drivers/gpu/drm/xe/xe_guc_ct.c:g2h_read() {
    ...
    if (xe_device_wedged(xe))
        return -ENOTRECOVERABLE;
    ...
}

Unlike xe_gt_reset_async(), which calls xe_uc_reset_prepare(&gt->uc) ->
xe_guc_reset_prepare(guc) synchronously before queueing gt->reset.worker on
gt->ordered_wq, xe_gt_declare_wedged() now defers
xe_uc_declare_wedged(&gt->uc) -> xe_guc_declare_wedged(&uc->guc) ->
xe_guc_reset_prepare(guc) (which sets guc->submission_state.stopped and
wakes guc->ct.wq) to gt_wedge_worker() on gt->ordered_wq.

If an exec queue is processing a SUSPEND message on sched->work_process_msg
while q->guc->resume_time == RESUME_PENDING or exec_queue_pending_disable(q)
is true, __guc_exec_queue_process_msg_suspend() blocks in an unbounded
wait:

drivers/gpu/drm/xe/xe_guc_submit.c:__guc_exec_queue_process_msg_suspend() {
    ...
    wait_event(guc->ct.wq, vf_recovery(guc) ||
               ((q->guc->resume_time != RESUME_PENDING ||
               xe_guc_read_stopped(guc)) && !exec_queue_pending_disable(q)));
    ...
}

Because g2h_read() is blocked by xe_device_wedged(xe), handle_sched_done()
cannot run to clear RESUME_PENDING or PENDING_DISABLE, so work_process_msg
can only wake when xe_guc_read_stopped(guc) becomes true. If
guc_exec_queue_timedout_job() is running on gt->ordered_wq ahead of
gt->wedge.worker, it calls:

    guc_exec_queue_timedout_job()
        xe_sched_submission_stop(sched)
            cancel_work_sync(&sched->work_process_msg);

This blocks gt->ordered_wq waiting for work_process_msg, while
work_process_msg waits on guc->ct.wq for gt_wedge_worker() to set
xe_guc_read_stopped(guc), and gt_wedge_worker() cannot run because it is
queued behind guc_exec_queue_timedout_job() on the single-threaded
gt->ordered_wq.

Even without a concurrent SUSPEND message, when
guc_exec_queue_timedout_job() runs on gt->ordered_wq while xe->wedged.flag
is set, its wait_event_timeout(guc->ct.wq, ..., HZ * 5) calls for pending
enable/disable or G2H receive neither G2H nor xe_guc_read_stopped(guc),
stalling gt->ordered_wq for 5 seconds before taking a spurious devcoredump
and scheduling a GT reset.

Should xe_gt_declare_wedged() call xe_uc_reset_prepare(&gt->uc)
synchronously before queueing gt->wedge.worker, matching
xe_gt_reset_async()?

([ ... ])

 base-commit: 2efb7a11f1aa4d2dc285fe2680cca22cb9f72f64
-- 
2.51.2

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