[PATCH 0/4] PCI/PM: Do not save or restore the config space of an inaccessible device

[PATCH 0/4] PCI/PM: Do not save or restore the config space of an inaccessible device

[Francisco Beltrán Millalén]

When a PCI device becomes inaccessible while the system is suspending,
pci_save_state() happily stores 0xFFFFFFFF into all 16 dwords of the
saved config space, and pci_restore_state() writes that back on resume --
to a device that by then *is* responding again.

On a MacBookPro14,3 (Intel Alpine Ridge Thunderbolt 3) this is not
theoretical.  The upstream bridge of the Thunderbolt switch stops
responding during suspend, and on resume the restore leaves it with:

  - the Secondary Bus Reset bit asserted (0xFF contains
    PCI_BRIDGE_CTL_BUS_RESET),
  - primary/secondary/subordinate bus numbers set to ff/ff/ff,
  - the link retrained down from 8 GT/s to 2.5 GT/s.

The visible effect is a 65-second resume while the kernel waits for
devices that can no longer be reached, followed by the removal of both
xHCI controllers: every USB-C port on the machine is gone until reboot.

The series makes the save path refuse to snapshot a device that is not
there, the restore path refuse to write back a snapshot that is all
ones, and teaches one more caller not to mistake an absent device for a
working link.

Patch 1 makes the USB PCI HCD honour the pci_save_state() return value,
which it currently ignores.  It is a fix in its own right --
pci_save_state() can already fail today -- and it is placed first so
that no patch in the series introduces an error condition before its
caller knows how to handle it.

Measured on the affected machine, comparing the same suspend/resume
cycle with and without the series:

  - resume time for the affected bridge: 65 s -> 1 s;
  - the bridge keeps its bus numbers (04/05/79) and its 8 GT/s link,
    with no Secondary Bus Reset asserted;
  - the xHCI behind it survives the cycle: 1 of 2 controllers instead of
    0 of 2, and 4 USB buses instead of 2.

v2 of this work fixes two defects found in review of v1: a
pci_WARN_ONCE() that the first version could trigger in
pci_pm_suspend_noirq(), and a partially written snapshot -- v1 could
leave dev->saved_config_space half updated if the device disappeared
while it was being read.  v2 reads into a temporary buffer and only
commits it after re-checking that the device is still there.  Both were
verified on hardware: the warning count went from 1 and 1 to 0 and 0,
and the per-device resume timings are unchanged to within 0.01%.

Tested on 6.18.49 on the machine described above.  I do not have other
affected hardware, so wider testing of the PCI core changes would be
welcome.

[PATCH 1/4] usb: hcd-pci: Honour pci_save_state() failure

[Francisco Beltrán Millalén]

hcd_pci_suspend_noirq() calls pci_save_state() and ignores its return
value, then goes on to call pci_prepare_to_sleep().  pci_save_state()
can fail -- it already propagates failures from pci_save_pcie_state()
and friends -- and when it does there is no saved state to restore
later, so putting the device into a low-power state only makes matters
worse: the subsequent transition fails too, and the PCI core then warns
that the callback returned without saving the state.

Check the return value.  If the state could not be saved, leave the
device alone and return success, so the rest of the system can still
suspend.  Leaving the power state untouched also means the PCI core has
nothing to complain about.

Signed-off-by: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com>
---
diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -541,7 +541,17 @@
 	if (retval)
 		return retval;
 
-	pci_save_state(pci_dev);
+	/*
+	 * If the controller is already inaccessible, there is no state to
+	 * save and nothing to put into a low-power state.  Leaving the power
+	 * state alone also keeps the PCI core from warning that this callback
+	 * returned without saving the state.
+	 */
+	retval = pci_save_state(pci_dev);
+	if (retval) {
+		dev_dbg(dev, "--> not suspending, device inaccessible\n");
+		return 0;
+	}
 
 	/* If the root hub is dead rather than suspended, disallow remote
 	 * wakeup.  usb_hc_died() should ensure that both hosts are marked as

[PATCH 2/4] PCI/PM: Do not save the config space of an inaccessible device

[Francisco Beltrán Millalén]

pci_save_state() reads sixteen dwords in a bare loop and then marks the
snapshot valid unconditionally.  If the device is already inaccessible,
every read returns all ones, and that garbage replaces a previously good
snapshot.

Restoring it later does not merely fail to help.  On a bridge that is
still alive, writing all ones sets every writable bit of BRIDGE_CONTROL,
which asserts Secondary Bus Reset, and clears the primary, secondary and
subordinate bus numbers, which unmaps everything behind the bridge.  On
a MacBookPro14,3 this is what removes the Thunderbolt USB controllers
after a suspend/resume cycle: the bridge keeps answering, but the kernel
has just written 0xffffffff over its configuration.

Check whether the device answers before saving, and again afterwards,
because it can disappear while the loop is running -- on this machine
the window between a successful read and a failing one has been measured
at a few microseconds.  Read into a temporary buffer so the previous,
known-good snapshot survives if either check fails.

Signed-off-by: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com>
---
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1740,13 +1740,43 @@
  */
 int pci_save_state(struct pci_dev *dev)
 {
+	u32 buf[16];
 	int i;
+	u32 val;
+
+	/*
+	 * If the device is already inaccessible, every config read returns
+	 * all ones.  Saving that would replace a previously good snapshot
+	 * with garbage, and restoring the garbage later does not merely fail
+	 * to help, it actively damages the device: on a bridge it asserts
+	 * Secondary Bus Reset and clears the bus numbers, which unmaps
+	 * everything behind it.  Keep the old snapshot instead.
+	 */
+	pci_read_config_dword(dev, PCI_VENDOR_ID, &val);
+	if (PCI_POSSIBLE_ERROR(val)) {
+		pci_warn(dev, "not saving config space, device inaccessible\n");
+		return -EIO;
+	}
+
+	/*
+	 * Read into a temporary buffer: the device can become inaccessible
+	 * while we are reading, and then only part of the snapshot is all
+	 * ones.  The previous snapshot must stay intact until we know the
+	 * new one is good.
+	 */
 	/* XXX: 100% dword access ok here? */
 	for (i = 0; i < 16; i++) {
-		pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]);
-		pci_dbg(dev, "save config %#04x: %#010x\n",
-			i * 4, dev->saved_config_space[i]);
+		pci_read_config_dword(dev, i * 4, &buf[i]);
+		pci_dbg(dev, "save config %#04x: %#010x\n", i * 4, buf[i]);
 	}
+
+	pci_read_config_dword(dev, PCI_VENDOR_ID, &val);
+	if (PCI_POSSIBLE_ERROR(val)) {
+		pci_warn(dev, "not saving config space, device became inaccessible\n");
+		return -EIO;
+	}
+
+	memcpy(dev->saved_config_space, buf, sizeof(buf));
 	dev->state_saved = true;
 
 	i = pci_save_pcie_state(dev);

[PATCH 3/4] PCI/PM: Do not restore a config space snapshot that is all ones

[Francisco Beltrán Millalén]

Belt and braces for the previous patch: even if a snapshot of all ones
is somehow recorded, never write it back.  The vendor ID of a device
that was present can never be 0xffffffff, so a snapshot whose first
dword is all ones is known to be garbage.

Signed-off-by: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com>
---
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1833,6 +1833,17 @@
 
 static void pci_restore_config_space(struct pci_dev *pdev)
 {
+	/*
+	 * Vendor and Device ID are read-only, so a snapshot whose first
+	 * dword reads as all ones was taken from an inaccessible device and
+	 * is garbage.  Writing it back would corrupt a device that has since
+	 * become accessible again, so leave the hardware alone.
+	 */
+	if (PCI_POSSIBLE_ERROR(pdev->saved_config_space[0])) {
+		pci_warn(pdev, "not restoring config space, saved state is invalid\n");
+		return;
+	}
+
 	if (pdev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
 		pci_restore_config_space_range(pdev, 10, 15, 0, false);
 		/* Restore BARs before the command register. */

[PATCH 4/4] PCI: Do not mistake an absent device for an active link

[Francisco Beltrán Millalén]

pci_bridge_wait_for_secondary_bus() reads PCI_EXP_LNKSTA and treats
PCI_EXP_LNKSTA_DLLLA as "the link is up".  When the device is gone the
read returns 0xffff, which has that bit set, so an absent device looks
like an active link and the code waits PCIE_RESET_READY_POLL_MS -- 60
seconds -- before giving up.

Treat an all-ones read as "no link", which is what it means.  On a
MacBookPro14,3 whose Thunderbolt controller does not come back from S3
this takes the resume from 65 seconds down to one.

Signed-off-by: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com>
---
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4773,8 +4773,16 @@
 		if (!dev->link_active_reporting)
 			return -ENOTTY;
 
+		/*
+		 * A device that is gone answers config reads with all ones,
+		 * and all ones has DLLLA set, so an unchecked read here is
+		 * indistinguishable from an active link.  That makes the
+		 * code below wait the full PCIE_RESET_READY_POLL_MS for a
+		 * device that is not there.  Treat it as absent instead.
+		 */
 		pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &status);
-		if (!(status & PCI_EXP_LNKSTA_DLLLA))
+		if (PCI_POSSIBLE_ERROR(status) ||
+		    !(status & PCI_EXP_LNKSTA_DLLLA))
 			return -ENOTTY;
 
 		return pci_dev_wait(child, reset_type,

Re: [PATCH 1/4] usb: hcd-pci: Honour pci_save_state() failure

[Alan Stern]

On Thu, Sep 24, 2026 at 09:42:18AM -0300, Francisco Beltrán Millalén wrote:
> hcd_pci_suspend_noirq() calls pci_save_state() and ignores its return
> value, then goes on to call pci_prepare_to_sleep().  pci_save_state()
> can fail -- it already propagates failures from pci_save_pcie_state()
> and friends -- and when it does there is no saved state to restore
> later, so putting the device into a low-power state only makes matters
> worse: the subsequent transition fails too, and the PCI core then warns
> that the callback returned without saving the state.
> 
> Check the return value.  If the state could not be saved, leave the
> device alone and return success, so the rest of the system can still
> suspend.  Leaving the power state untouched also means the PCI core has
> nothing to complain about.
> 
> Signed-off-by: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com>
> ---

Acked-by: Alan Stern <stern@rowland.harvard.edu>

> diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
> --- a/drivers/usb/core/hcd-pci.c
> +++ b/drivers/usb/core/hcd-pci.c
> @@ -541,7 +541,17 @@
>  	if (retval)
>  		return retval;
>  
> -	pci_save_state(pci_dev);
> +	/*
> +	 * If the controller is already inaccessible, there is no state to
> +	 * save and nothing to put into a low-power state.  Leaving the power
> +	 * state alone also keeps the PCI core from warning that this callback
> +	 * returned without saving the state.
> +	 */
> +	retval = pci_save_state(pci_dev);
> +	if (retval) {
> +		dev_dbg(dev, "--> not suspending, device inaccessible\n");
> +		return 0;
> +	}
>  
>  	/* If the root hub is dead rather than suspended, disallow remote
>  	 * wakeup.  usb_hc_died() should ensure that both hosts are marked as
>