Re: [PATCH v12 08/10] drm/xe: Introduce temporary device wedging

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/xe: unmatched xe_wedge_put() in xe_pci_error_resume()
- [High] drm/xe: leaked wedge and runtime PM ref after xe_pci_error_slot_reset()
- [Medium] drm/xe: premature wedged.perm set before PCI reset suppresses xe_wedge_declare()
--

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

drm/xe: Introduce temporary device wedging

This commit converts xe->wedged.flag into a reference count (xe->wedged.ref)
for temporary device wedging and adds xe->wedged.perm to track permanent
device wedging.

> diff --git a/drivers/gpu/drm/xe/xe_pci_error.c b/drivers/gpu/drm/xe/xe_pci_error.c
> index 9bf0da3a3d152..6a942cb6f69d9 100644
> --- a/drivers/gpu/drm/xe/xe_pci_error.c
> +++ b/drivers/gpu/drm/xe/xe_pci_error.c
> @@ -21,13 +21,8 @@ static void prepare_device_for_reset(struct pci_dev *pdev)
>  	struct xe_gt *gt;
>  	u8 id;
>  
> -	/*
> -	 * Wedge the device to prevent userspace access but do not send the uevent.
> -	 * xe_wedge_fini() releases runtime pm if wedged flag is set, so acquire a runtime
> -	 * pm reference to avoid underflow.
> -	 */
> -	if (!atomic_xchg(&xe->wedged.flag, 1))
> -		xe_pm_runtime_get_noresume(xe);
> +	/* Wedge the device to prevent userspace access but do not send the uevent. */
> +	xe_wedge_get(xe);
>  
>  	xe_device_set_in_reset(xe);
>  
> @@ -41,10 +36,16 @@ static void prepare_device_for_reset(struct pci_dev *pdev)
>  
>  static pci_ers_result_t ras_action_to_pci_result(struct pci_dev *pdev, u8 action)
>  {
> +	struct xe_device *xe = xe_any_to_xe(pdev);
> +
>  	switch (action) {
>  	case XE_RAS_RECOVERY_ACTION_RECOVERED:
>  		return PCI_ERS_RESULT_RECOVERED;
>  	case XE_RAS_RECOVERY_ACTION_RESET:
> +		/* We should not be doing such ugly hacks, reuse FLR callbacks and drop this. */
> +		if (atomic_xchg(&xe->wedged.perm, 1))
> +			return PCI_ERS_RESULT_DISCONNECT;
> +
>  		prepare_device_for_reset(pdev);
>  		return PCI_ERS_RESULT_NEED_RESET;
>  	case XE_RAS_RECOVERY_ACTION_DISCONNECT:
> @@ -71,6 +72,10 @@ static pci_ers_result_t xe_pci_error_detected(struct pci_dev *pdev, pci_channel_
>  	case pci_channel_io_normal:
>  		return PCI_ERS_RESULT_CAN_RECOVER;
>  	case pci_channel_io_frozen:
> +		/* We should not be doing such ugly hacks, reuse FLR callbacks and drop this. */
> +		if (atomic_xchg(&xe->wedged.perm, 1))
> +			return PCI_ERS_RESULT_DISCONNECT;
> +
>  		prepare_device_for_reset(pdev);
>  		return PCI_ERS_RESULT_NEED_RESET;

[Severity: Medium]
What happens if the PCI reset fails before xe_pci_error_slot_reset() removes
the old driver instance?

Both ras_action_to_pci_result() and xe_pci_error_detected() set
xe->wedged.perm to 1 via atomic_xchg() before calling
prepare_device_for_reset() -> xe_wedge_get().

If reset_subordinates() fails in pcie_do_recovery(), or if pci_enable_device()
fails in xe_pci_error_slot_reset():

drivers/gpu/drm/xe/xe_pci_error.c:xe_pci_error_slot_reset() {
	...
	err = pci_enable_device(pdev);
	if (err) {
		xe_log_err_fatal(xe, PCI, err, "Cannot re-enable PCI device after reset\n");
		return PCI_ERS_RESULT_DISCONNECT;
	}

	xe_device_clear_in_reset(xe);
	pdev->driver->remove(pdev);
	...
}

the old xe_device remains bound with xe->wedged.perm == 1 and
xe->wedged.ref == 1.

Because xe->wedged.perm is already set to 1, xe_wedge_declare() was never
called and any subsequent call to xe_wedge_declare() will return early:

drivers/gpu/drm/xe/xe_wedge.c:xe_wedge_declare() {
	...
	if (atomic_xchg(&xe->wedged.perm, 1))
		return;

	xe_wedge_get(xe);
	xe->needs_flr_on_fini = true;
	...
	schedule_work(&xe->wedged.work);
}

Wouldn't this leave the device permanently blocking IOCTLs and GuC CT
communication with -ENOTRECOVERABLE while suppressing the drm_dev_wedged_event
uevent to userspace and leaving xe->needs_flr_on_fini unset?

[ ... ]

> @@ -129,7 +134,7 @@ static pci_ers_result_t xe_pci_error_slot_reset(struct pci_dev *pdev)
>  	xe = pdev_to_xe_device(pdev);
>  
>  	/* Wedge the device to prevent I/O operations till the resume callback */
> -	atomic_set(&xe->wedged.flag, 1);
> +	xe_wedge_get(xe);
>  
>  	return PCI_ERS_RESULT_RECOVERED;
>  }

[Severity: High]
As reported by Sashiko on v10 [1], does xe_pci_error_slot_reset() leak the
wedge reference and runtime PM reference if pcie_do_recovery() aborts before
calling xe_pci_error_resume()?

In xe_pci_error_slot_reset(), after removing the old device and probing a new
xe_device via pdev->driver->probe(), xe_wedge_get(xe) increments
xe->wedged.ref to 1 and acquires a runtime PM reference while xe->wedged.perm
remains 0.

In pcie_do_recovery(), pci_walk_bridge(bridge, report_slot_reset, &status)
walks all subordinate devices under the upstream bridge and merges their
results. If a sibling PCI device returns failure from slot_reset,
pcie_do_recovery() jumps to its failed path and broadcasts
report_perm_failure_detected() -> xe_pci_error_detected(pdev,
pci_channel_io_perm_failure) without calling report_resume(). Likewise,
report_slot_reset() releases device_lock(&pdev->dev) before report_resume()
re-acquires it, allowing driver unbind in between.

When the newly probed xe_device is later torn down, xe_wedge_fini() only puts
a reference if xe->wedged.perm is set:

drivers/gpu/drm/xe/xe_wedge.c:xe_wedge_fini() {
	...
	if (atomic_read(&xe->wedged.perm))
		xe_wedge_put(xe);

	xe_assert(xe, !xe_wedged(xe));
}

Wouldn't this leave the temporary reference from xe_pci_error_slot_reset()
unreleased, leaking the runtime PM reference and triggering the
xe_assert(xe, !xe_wedged(xe)) assertion on teardown?

Additionally, if pdev->driver->probe() in xe_pci_error_slot_reset() succeeds
by entering boot survivability mode in __xe_pci_probe(), it returns 0 before
xe_wedge_init() or xe_pm_init() are called, yet xe_pci_error_slot_reset()
unconditionally calls xe_wedge_get(xe).

> @@ -140,7 +145,8 @@ static void xe_pci_error_resume(struct pci_dev *pdev)
>  
>  	xe_info(xe, "PCI error: resume\n");
>  
> -	atomic_set(&xe->wedged.flag, 0);
> +	if (xe_wedged(xe))
> +		xe_wedge_put(xe);
>  }

[Severity: High]
Can xe_pci_error_resume() drop a wedge reference and runtime PM reference that
the PCI error recovery path never acquired?

When a non-fatal PCIe error occurs (state == pci_channel_io_normal),
pcie_do_recovery() runs:

  report_normal_detected()
    -> xe_pci_error_detected(pdev, pci_channel_io_normal)
         returns PCI_ERS_RESULT_CAN_RECOVER (without calling xe_wedge_get())
  report_mmio_enabled()
    -> xe_pci_error_mmio_enabled()
      -> xe_ras_process_errors()
        -> ras_action_to_pci_result(pdev, XE_RAS_RECOVERY_ACTION_RECOVERED)
             returns PCI_ERS_RESULT_RECOVERED (without calling xe_wedge_get())
  report_resume()
    -> xe_pci_error_resume()

Because xe_pci_error_detected() does not wedge the device or stop driver work
for pci_channel_io_normal, normal driver activity continues between
xe_pci_error_detected() and xe_pci_error_resume(). If a concurrent GT reset
failure, job timeout, or hardware error handler calls xe_wedge_declare() in
that window (or if xe_wedge_declare() was between atomic_xchg(&xe->wedged.perm,
1) and xe_wedge_get(xe) when xe_pci_error_detected() checked xe_wedged(xe)),
xe->wedged.ref becomes 1.

When xe_pci_error_resume() then checks xe_wedged(xe), it sees the global
xe->wedged.ref > 0 and calls xe_wedge_put(xe).

Wouldn't that consume the reference owned by xe_wedge_declare(), dropping
xe->wedged.ref back to 0 (unblocking IOCTLs on a permanently wedged device)
and later causing xe_wedge_fini() to underflow xe->wedged.ref to -1 and
underflow runtime PM when it sees xe->wedged.perm == 1?

[1] https://lore.kernel.org/r/20260821113758.006681F000E9@smtp.kernel.org

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