Re: [PATCH 09/13] drm/xe: Isolate a wedged device before notifying userspace

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] TOCTOU race in `xe_pci_suspend` allows concurrent execution of `xe_pm_suspend` and the asynchronous wedge worker, leading to unbalanced PCI enable counts and unsafe MMIO accesses.
- [High] The xe_pci_resume function unconditionally toggles D3Cold, leading to unbalanced power state changes when the device is wedged.
--

commit fc050bbcfed735c33773bd7fb0bbf0a5e4fe47a1
Author: Arvind Yadav <arvind.yadav@intel.com>

drm/xe: Isolate a wedged device before notifying userspace

This commit introduces a sleepable wedge worker to safely isolate a permanently
wedged device. It stops interrupt and DMA activity, invalidates VRAM mappings,
and updates the suspend and resume paths to wait for device isolation and skip
normal PM transitions when the device is wedged.

> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 1e04e8ef2611f..beaa79d136198 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -1331,6 +1331,16 @@ static int xe_pci_suspend(struct device *dev)
>  	if (xe_survivability_mode_is_boot_enabled(xe))
>  		return -EBUSY;
>  
> +	/*
> +	 * Wait until wedge work is queued, then wait for isolation to
> +	 * finish before skipping normal suspend.
> +	 */
> +	if (xe_device_wedged(xe)) {
> +		wait_for_completion(&xe->wedged.prepared);
> +		flush_work(&xe->wedged.work);
> +		return 0;
> +	}
> +

[Severity: High]
Does this lockless check open a TOCTOU race window before proceeding to the
normal PM suspend flow and pci_disable_device()?

If the device is declared wedged immediately after the lockless
xe_device_wedged(xe) check evaluates to false, xe_pci_suspend() will
proceed with executing xe_pm_suspend() and eventually calling
pci_disable_device(pdev). Meanwhile, xe_device_wedged_work() executes
concurrently with xe_pm_suspend().

Later during system resume, xe_pci_resume() will see xe_device_wedged(xe)
as true and return early without calling pci_enable_device(pdev). This
results in an unbalanced pci_disable_device() leaving enable_cnt decremented
without a matching increment.

Could this happen if a hardware timeout occurs during the suspend process
itself (e.g., during GT suspend or eviction timeout)?

Also, if the device is already wedged and xe_pci_suspend() returns 0 here,
it skips the d3cold_toggle(pdev, D3COLD_ENABLE) call which occurs later in
this function.

[ ... ]

> @@ -1352,6 +1362,7 @@ static int xe_pci_resume(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct xe_device *xe = pdev_to_xe_device(pdev);
>  	int err;
>  
>  	/* Give back the D3Cold decision to the runtime P M*/
>  	d3cold_toggle(pdev, D3COLD_DISABLE);
>  
>  	err = pci_set_power_state(pdev, PCI_D0);
>  	if (err)
>  		return err;
>  
>  	pci_restore_state(pdev);
>  
> +	/*
> +	 * Suspend skipped PCI disable for an already isolated device. Avoid
> +	 * incrementing enable_cnt and clear bus mastering restored from the
> +	 * saved configuration.
> +	 */
> +	if (xe_device_wedged(xe)) {
> +		pci_clear_master(pdev);
> +		return 0;
> +	}

[Severity: High]
Does this unconditionally toggle D3Cold before checking if the device is
wedged?

If the device is wedged before xe_pci_suspend() is called, it returns early
and skips the balancing d3cold_toggle(pdev, D3COLD_ENABLE) call. However,
on system resume, xe_pci_resume() unconditionally calls
d3cold_toggle(pdev, D3COLD_DISABLE) before checking if the device is wedged.

Can this lead to an unbalanced call to pci_d3cold_disable(root_pdev) (which
increments no_d3cold) on every suspend/resume cycle once the device has been
permanently wedged, eventually breaking D3Cold power management for the
root port?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827101801.1247654-1-arvind.yadav@intel.com?part=9