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 <[email protected]> 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/[email protected]?part=9
