Re: [PATCH 1/3] drm/xe/wedge: add fault inspection with debugfs release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/xe/wedge: exec_queue ref leak on repeated xe_guc_submit_wedge()
- [High] drm/xe/wedge: suspend deadlock and RPM corruption after mode-3 skip
- [Medium] drm/xe/wedge: GT reset race in xe_gt_wedge_pause() and gt_reset_worker()
- [Medium] drm/xe/wedge: teardown race on shutting_down in xe_wedge_prepare_fini()
--

commit efc71b82b1b8d7ea492bbcdc755faa39da7e7570
Author: Alan Previn <alan.previn.teres.alexis@intel.com>

drm/xe/wedge: add fault inspection with debugfs release

This commit introduces wedged_mode=3 to pause GT submission and block new
Xe ioctls after a hang so hardware state can be inspected before releasing
via debugfs. It extends the GuC no-reset policy to cover mode 3 and adds a
worker that waits for a skip or continue command from wedged_pause_resume.

> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index be79a0da2f2ff..1633b203255a3 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -1207,10 +1207,25 @@ void xe_gt_wedge_declare(struct xe_gt *gt)
[ ... ]
> +void xe_gt_wedge_pause(struct xe_gt *gt)
> +{
> +	xe_gt_assert(gt, READ_ONCE(gt_to_xe(gt)->wedged.mode) ==
> +		     XE_WEDGED_MODE_UPON_ANY_HANG_NO_RESET_WAIT_FOR_RELEASE);
> +	xe_uc_reset_prepare(&gt->uc);
> +	xe_uc_wedge_declare(&gt->uc);
> +	xe_tlb_inval_reset(&gt->tlb_inval);
> +}
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 9e7c2778029ca..57e258283935a 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -1424,7 +1424,7 @@ void xe_guc_submit_wedge(struct xe_guc *guc)
[ ... ]
> @@ -1433,7 +1433,7 @@ void xe_guc_submit_wedge(struct xe_guc *guc)
>  	if (!guc->submission_state.initialized)
>  		return;
>  
> -	if (xe->wedged.mode == XE_WEDGED_MODE_UPON_ANY_HANG_NO_RESET) {
> +	if (xe_wedge_is_hanging_on_any(READ_ONCE(xe->wedged.mode))) {
>  		mutex_lock(&guc->submission_state.lock);
>  		xa_for_each(&guc->submission_state.exec_queue_lookup, index, q)
>  			if (xe_exec_queue_get_unless_zero(q))
[ ... ]
> @@ -1672,7 +1672,10 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>  	if (!exec_queue_killed(q))
>  		wedged = guc_submit_hint_wedged(exec_queue_to_guc(q));
>  	else
> -		wedged = xe_wedged(xe);
> +		wedged = xe_wedged(xe) ||
> +			 (READ_ONCE(xe->wedged.mode) ==
> +			  XE_WEDGED_MODE_UPON_ANY_HANG_NO_RESET_WAIT_FOR_RELEASE &&
> +			  exec_queue_wedged(q));
>  
>  	/*
>  	 * Only tag as GPU hang if this is the original timeout, not a
> @@ -1726,6 +1729,12 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>  			goto handle_vf_resume;
>  		if (!ret || xe_guc_read_stopped(guc)) {
>  trigger_reset:
> +			if (READ_ONCE(xe->wedged.mode) ==
> +			    XE_WEDGED_MODE_UPON_ANY_HANG_NO_RESET_WAIT_FOR_RELEASE &&
> +			    (exec_queue_wedged(q) || exec_queue_wedged(primary))) {
> +				wedged = true;
> +				goto complete_jobs;
> +			}
>  			if (!ret)
>  				xe_gt_warn(guc_to_gt(guc),
>  					   "Schedule disable failed to respond, guc_id=%d",

[Severity: Medium]
Can a concurrent job timeout on a killed queue still trigger a full hardware
GT reset during mode-3 inspection?

In xe_gt_wedge_pause(), xe_uc_reset_prepare() runs first, setting
guc->submission_state.stopped = 1 and waking guc->ct.wq. Then
xe_uc_wedge_declare() -> xe_guc_wedge_declare() calls xe_guc_ct_stop(),
which takes ct->lock and sleeps in cancel_work_sync() before
xe_guc_submit_wedge() sets EXEC_QUEUE_STATE_WEDGED on the queues:

xe_gt_wedge_pause()
  xe_uc_reset_prepare()
    xe_guc_reset_prepare()
      xe_guc_submit_reset_prepare() /* sets stopped = 1, wakes guc->ct.wq */
  xe_uc_wedge_declare()
    xe_guc_wedge_declare()
      xe_guc_ct_stop()              /* sleeps in cancel_work_sync() */
      xe_guc_submit_wedge()         /* sets EXEC_QUEUE_STATE_WEDGED */

If a killed queue is concurrently waiting in guc_exec_queue_timedout_job()
on guc->ct.wq, it wakes up as soon as xe_guc_read_stopped(guc) becomes true
and jumps to trigger_reset before xe_guc_submit_wedge() has marked the
queues wedged (and similarly if a killed queue enters
guc_exec_queue_timedout_job() after declare_pending is set but before
xe_guc_submit_wedge() runs). At trigger_reset, both exec_queue_wedged(q)
and exec_queue_wedged(primary) are still false, so it falls through to
xe_gt_reset_async(primary->gt).

In addition, gt_reset_worker() in drivers/gpu/drm/xe/xe_gt.c only checks
xe_wedged(gt_to_xe(gt)):

drivers/gpu/drm/xe/xe_gt.c:gt_reset_worker() {
	...
	if (xe_wedged(gt_to_xe(gt)))
		goto err_pm_put;
	...
}

Since xe_wedged(xe) remains false during mode-3 inspection and after a
skip reply, wouldn't gt_reset_worker() proceed to run do_gt_reset() and
do_gt_restart(), wiping out the fault state under inspection?

> diff --git a/drivers/gpu/drm/xe/xe_wedge.c b/drivers/gpu/drm/xe/xe_wedge.c
> index 2f35937c5e661..c22c3544adcaa 100644
> --- a/drivers/gpu/drm/xe/xe_wedge.c
> +++ b/drivers/gpu/drm/xe/xe_wedge.c
[ ... ]
> +static void wedge_pause_work(struct work_struct *work)
> +{
[ ... ]
> +	for_each_initialized_gt(gt, xe, id)
> +		xe_gt_wedge_pause(gt);
> +
> +	spin_lock_irqsave(&xe->wedged.mode_lock, flags);
> +	WRITE_ONCE(xe->wedged.reply, 0);
> +	xe->wedged.wait_active = true;
> +	spin_unlock_irqrestore(&xe->wedged.mode_lock, flags);
> +
> +	xe_err(xe, "GT submission stopped for mode-3 hardware debugging.\n"
> +	       "New DRM ioctls are blocked while waiting for 'wedged_pause_resume'.\n"
> +	       "Skip: 0x%08x; continue into permanent wedge: 0x%08x.\n"
> +	       "Skip does not restart GTs or recover the fault.\n",
> +	       XE_WEDGE_REPLY_SKIP, XE_WEDGE_REPLY_CONTINUE);
> +
> +	wait_event_idle(xe->wedged.pause_waitq,
> +			wedge_reply_ready(xe) || READ_ONCE(xe->wedged.shutting_down));
> +
> +	spin_lock_irqsave(&xe->wedged.mode_lock, flags);
> +	xe->wedged.wait_active = false;
> +	reply = READ_ONCE(xe->wedged.reply);
> +	spin_unlock_irqrestore(&xe->wedged.mode_lock, flags);
> +
> +	if (!READ_ONCE(xe->wedged.shutting_down) && reply == XE_WEDGE_REPLY_CONTINUE)
> +		wedge_declare_now(xe, DRM_WEDGE_RECOVERY_NONE, true);
> +	else
> +		xe_info(xe, "Inspection released without permanent wedge; GTs remain stopped\n");
> +put_runtime_ref:
> +	xe_pm_runtime_put(xe);
> +clear_pending:
> +	atomic_set(&xe->wedged.declare_pending, 0);
> +}

[Severity: High]
What happens if system suspend or runtime suspend runs during mode-3
inspection or after XE_WEDGE_REPLY_SKIP?

In wedge_pause_work(), xe_gt_wedge_pause() calls xe_uc_reset_prepare(),
which sets guc->submission_state.stopped = 1, and xe_uc_wedge_declare(),
which stops GuC CT and marks queues wedged. However, while waiting in
wait_event_idle(), or after the user writes XE_WEDGE_REPLY_SKIP,
wedge_declare_now() is not called, so xe_wedged(xe) remains false and
xe->needs_flr_on_fini is not set. On XE_WEDGE_REPLY_SKIP, wedge_pause_work()
also drops its runtime PM reference at put_runtime_ref.

If system suspend runs while paused or after XE_WEDGE_REPLY_SKIP (or D3cold
runtime suspend runs after the runtime PM reference is dropped),
xe_gt_suspend() -> xe_uc_suspend() -> uc_reset_wait() -> xe_guc_reset_wait()
calls xe_guc_submit_reset_wait():

drivers/gpu/drm/xe/xe_guc_submit.c:xe_guc_submit_reset_wait() {
	wait_event(guc->ct.wq, xe_wedged(guc_to_xe(guc)) ||
		   !xe_guc_read_stopped(guc));
}

Because xe_wedged(xe) is false and xe_guc_read_stopped(guc) stays 1
permanently, won't xe_guc_submit_reset_wait() (and the loop in
uc_reset_wait()) sleep uninterruptibly forever?

Also, on platforms where D3cold is disallowed, dropping the runtime PM
reference on XE_WEDGE_REPLY_SKIP allows xe_pm_runtime_suspend() ->
xe_gt_runtime_suspend() -> xe_guc_ct_runtime_suspend() to hit the
g2h->info.space == credits assert if any G2H credits were outstanding, and
a subsequent xe_pm_runtime_resume() -> xe_guc_runtime_resume() will re-enable
GuC CT and submission even though GTs are supposed to remain stopped.

> +
> +static void wedge_request(struct xe_device *xe, unsigned long method, bool mode3_only)
> +{
> +	unsigned long flags;
> +	enum xe_wedged_mode mode;
> +	bool pm_ref;
> +
> +	spin_lock_irqsave(&xe->wedged.mode_lock, flags);
> +	mode = READ_ONCE(xe->wedged.mode);
> +	if (READ_ONCE(xe->wedged.shutting_down) ||
> +	    (mode3_only && mode != XE_WEDGED_MODE_UPON_ANY_HANG_NO_RESET_WAIT_FOR_RELEASE)) {
> +		spin_unlock_irqrestore(&xe->wedged.mode_lock, flags);
> +		return;
> +	}
> +
> +	if (mode != XE_WEDGED_MODE_UPON_ANY_HANG_NO_RESET_WAIT_FOR_RELEASE) {
> +		spin_unlock_irqrestore(&xe->wedged.mode_lock, flags);
> +		wedge_declare_now(xe, method, false);
> +		return;
> +	}
> +
> +	if (atomic_read(&xe->wedged.declare_pending)) {
> +		spin_unlock_irqrestore(&xe->wedged.mode_lock, flags);
> +		return;
> +	}
> +
> +	atomic_set(&xe->wedged.declare_pending, 1);
> +	pm_ref = xe_pm_runtime_get_if_active(xe);
> +	if (WARN_ON_ONCE(!queue_work(xe->wedged.pause_workqueue, &xe->wedged.pause_work))) {
> +		xe_err(xe, "Mode-3 inspection work already queued\n");
> +		spin_unlock_irqrestore(&xe->wedged.mode_lock, flags);
> +		if (pm_ref)
> +			xe_pm_runtime_put(xe);
> +		return;
> +	}
> +	xe->wedged.pause_pm_ref = pm_ref;
> +	spin_unlock_irqrestore(&xe->wedged.mode_lock, flags);
> +}

[Severity: High]
Does this leak xe_exec_queue references if xe_guc_submit_wedge() runs more
than once?

When wedge_pause_work() finishes, it resets xe->wedged.declare_pending to 0:

1. If the user writes XE_WEDGE_REPLY_SKIP, xe->wedged.flag remains 0 and
   declare_pending becomes 0, which also re-opens the ioctl gate in
   xe_drm_ioctl() while GTs remain stopped. Any subsequent timeout on
   another non-killed queue in guc_exec_queue_timedout_job() sees
   !exec_queue_killed(q) (since xe_guc_submit_wedge() only sets
   EXEC_QUEUE_STATE_WEDGED) and calls guc_submit_hint_wedged() ->
   xe_wedge_declare() -> wedge_request(), queueing wedge_pause_work() a
   second time.
2. Even if the user writes XE_WEDGE_REPLY_CONTINUE (setting
   xe->wedged.flag = 1), wedge_request() does not check xe_wedged(xe)
   before handling mode 3, so any later xe_wedge_declare() or
   xe_wedge_declare_mode3() call after declare_pending is cleared will
   queue wedge_pause_work() again.

Each run of wedge_pause_work() calls xe_gt_wedge_pause() ->
xe_uc_wedge_declare() -> xe_guc_wedge_declare() -> xe_guc_submit_wedge():

drivers/gpu/drm/xe/xe_guc_submit.c:xe_guc_submit_wedge() {
	...
	if (xe_wedge_is_hanging_on_any(READ_ONCE(xe->wedged.mode))) {
		mutex_lock(&guc->submission_state.lock);
		xa_for_each(&guc->submission_state.exec_queue_lookup, index, q)
			if (xe_exec_queue_get_unless_zero(q))
				set_exec_queue_wedged(q);
		mutex_unlock(&guc->submission_state.lock);
	}
	...
}

Because xe_guc_submit_wedge() takes a new reference with
xe_exec_queue_get_unless_zero(q) without checking if exec_queue_wedged(q)
is already set, while guc_submit_fini() only calls xe_exec_queue_put(q) once
per wedged entry during teardown:

drivers/gpu/drm/xe/xe_guc_submit.c:guc_submit_fini() {
	...
	mutex_lock(&guc->submission_state.lock);
	xa_for_each(&guc->submission_state.exec_queue_lookup, index, q) {
		if (exec_queue_wedged(q)) {
			mutex_unlock(&guc->submission_state.lock);
			xe_exec_queue_put(q);
			mutex_lock(&guc->submission_state.lock);
		}
	}
	mutex_unlock(&guc->submission_state.lock);
}

won't every repeated call to xe_guc_submit_wedge() leak a reference on all
registered queues in exec_queue_lookup?

[ ... ]
> +void xe_wedge_prepare_fini(struct xe_device *xe)
> +{
> +	unsigned long flags;
> +	bool mode3;
> +
> +	mutex_lock(&xe->wedged.mode_mutex);
> +	spin_lock_irqsave(&xe->wedged.mode_lock, flags);
> +	mode3 = READ_ONCE(xe->wedged.mode) ==
> +		XE_WEDGED_MODE_UPON_ANY_HANG_NO_RESET_WAIT_FOR_RELEASE;
> +	if (mode3)
> +		WRITE_ONCE(xe->wedged.shutting_down, true);
> +	spin_unlock_irqrestore(&xe->wedged.mode_lock, flags);
> +	if (mode3 && xe->wedged.pause_workqueue) {
> +		wake_up_all(&xe->wedged.pause_waitq);
> +		flush_work(&xe->wedged.pause_work);
> +	}
> +	mutex_unlock(&xe->wedged.mode_mutex);
> +}

[Severity: Medium]
Why is xe->wedged.shutting_down only set when xe->wedged.mode is currently
mode 3?

xe_wedge_prepare_fini() is called at the start of xe_pci_remove() (before
drm_dev_unplug() removes debugfs), at the start of xe_device_shutdown()
(where debugfs is not removed), and in xe_device_probe() error cleanup
before drm_dev_unregister().

If xe->wedged.mode is not 3 when xe_wedge_prepare_fini() runs,
xe->wedged.shutting_down is left false. A concurrent write to the
wedged_mode debugfs file can then call xe_wedge_set_mode(), see
shutting_down == false, and switch the device into mode 3 after
xe_wedge_prepare_fini() has already returned. Similarly, if
wedge_pause_work() clears declare_pending just before exit and a concurrent
xe_wedge_set_mode() changes wedged.mode away from 3 before
xe_wedge_prepare_fini() acquires mode_mutex, xe_wedge_prepare_fini() will
see mode3 == false and skip flushing pause_work.

Should xe_wedge_prepare_fini() unconditionally set shutting_down = true and
flush pause_work whenever pause_workqueue is initialized?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102210242.635207-1-daniel.charles@intel.com?part=1