[PATCH v3 0/2] PCI/PM: Keep the saved config space of an inaccessible device

[PATCH v3 0/2] PCI/PM: Keep the saved config space of an inaccessible device

[Francisco Beltrán Millalén]

When a PCI device stops responding while the system is suspending,
pci_save_state() stores all ones over its saved config space, and
pci_restore_state() writes that back on resume once the device responds
again.  For a bridge, that means bus numbers ff/ff/ff and Secondary Bus
Reset asserted, which cuts off everything below it.  Resume can also
wait about 65 seconds for a device behind a dead bridge, because an
all-ones Link Status reads as an active link.  I ran into both on a
MacBookPro14,3.

Patch 1 makes pci_save_state() keep the previous snapshot of a device
that is not accessible, instead of overwriting it with all ones.  Patch
2 stops the link wait code from taking an all-ones Link Status for an
active link.

Patch 2 does not depend on patch 1 and is unchanged from v2, so it can
be applied on its own, as you offered on v2.

Why patch 1 changed
-------------------
On v2 I described [1] what returning -EIO from pci_save_state() does to
some drivers: stmmac_pci_plat_suspend(), intel_eth_pci_suspend(),
atl1e, atl2 and amd_mp2 abort system suspend, and libata and others
trigger the "State of device not saved" warning.  I asked how you would
like those drivers handled, and said I would hold v3 until then.

The Thunderbolt work exposed the same PCI problem in another path.  I
have sent a Thunderbolt patch [2] that calls the SXFP ACPI method on
Apple systems with Alpine Ridge before system sleep.  SXFP also powers
down the xHCI on the side of the controller with nothing connected.
The automated review of that patch pointed out that if this happens
before the xHCI saves its state, pci_save_state() stores all ones and
resume writes them back.  I found the same by reading the code; I have
not run that case without this series.

On v2 you wrote that skipping pci_save_state() when the device is not
accessible seemed worthwhile.  That preserves a usable saved state, but
does not make the Thunderbolt controller resume.  The Thunderbolt part
now has its own patch in drivers/thunderbolt [2].  This version only
does the skipping: pci_save_state() keeps the previous snapshot, marks
the state as saved and returns 0, as it does today.  Callers see no
change, and there is nothing for the PM core to warn about.  The device
gets the config space it had when it was last accessible back on
resume.  This makes v2 patch 1 (usb: hcd-pci) unnecessary, so it is
dropped.  The values written back can be older than the last ones the
device had; patch 1 describes that.

If you would still rather have pci_save_state() report the failure, I'd
like to know how you want the drivers above handled.

[1] https://lore.kernel.org/all/20261011210451.12129-1-fbeltranmillalen@gmail.com/
[2] https://lore.kernel.org/all/20261019134851.7496-1-fbeltranmillalen@gmail.com/

Changes since v2:
- 1/2 (was 2/3): keep the previous snapshot and return 0 instead of
  returning -EIO, with the check once at the start of pci_save_state()
  as you suggested.  On v2 I said I would make pci_dev_save_and_disable()
  use the return value of pci_save_state(); with this version that no
  longer works, so it keeps its own check (see the notes in patch 1).
- Dropped v2 1/3 (usb: hcd-pci).
- 2/2 (was 3/3): no code change.  Added Darrell's Tested-by from v2.
- Based on pci/next as of 2026-10-15 (732c4f7d2c04).

v2: https://lore.kernel.org/all/20260930141914.6678-1-fbeltranmillalen@gmail.com/
v1: https://lore.kernel.org/all/20260924124221.12374-1-fbeltranmillalen@gmail.com/

Testing:
I built patch 1 into a v6.18.49 kernel with patch 2, a backport of
e18d1abc3bff and other local changes for the MacBookPro14,3, including
the next version of the Thunderbolt patch above, with the changes Mika
asked for in his review.  The USB HCD patch was not included.  I tested
it on that machine with 30 second suspend/resume cycles and a USB 3
disk on the Type-C ports, which sit behind the two Thunderbolt
controllers:

- Five cycles: three with the disk on one side or the other, one
  removing the disk while the system was suspended, and one with
  nothing connected.  All resumed, and both Thunderbolt controllers
  came back.  The disk was read after each cycle in which it stayed
  connected, and after the other two it worked when plugged in again
  (on both sides after the first, on one after the second).  In these
  cycles the xHCI saved its state before SXFP powered it down, so the
  new check in pci_save_state() did not trigger.  The order of the two
  is not fixed, as both suspend asynchronously.
- To hit the case above, I also called SXFP earlier, in the PCI suspend
  phase, through a test-only PCI quirk that is otherwise disabled.  In 3
  of 3 cycles the xHCI on the empty side (each side at least once) was
  reported as "Device config space inaccessible; unable to save state",
  the snapshot from before was written back on resume instead of all
  ones, and the xHCI came back.  A disk plugged into that xHCI
  afterwards worked at USB 3 speed, on both sides.  In all of them the
  snapshot was one taken earlier while the xHCI was accessible.

There was no kernel WARNING and no "State of device not saved" during
the whole boot.  The series itself was built on pci/next with W=1.  I
have no SR-IOV hardware, so the VF case is untested, and the machine
never reaches the pcie_wait_for_link_status() change.

Thanks to Bjorn for the review and the simpler check, to Farhan for the
pointer to the 2025 discussion, and to Darrell Gum for testing v2 on a
second MacBookPro14,3.

Francisco Beltrán Millalén (2):
  PCI/PM: Keep the saved config space of an inaccessible device
  PCI: Do not mistake an absent device for an active link

 drivers/pci/pci.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

[PATCH v3 1/2] PCI/PM: Keep the saved config space of an inaccessible device

[Francisco Beltrán Millalén]

pci_save_state() saves the standard header without checking whether the
device responded.  For an inaccessible device, each read returns all
ones, overwriting the previous snapshot, which pci_restore_state()
writes back after resume.  On a bridge, that sets every writable bit of
the Bridge Control register, Secondary Bus Reset included, and sets the
primary, secondary and subordinate bus numbers to 0xff, which cuts off
everything below it.  This was seen on a MacBookPro14,3, where a bridge
that stopped responding during system suspend came back after resume
with bus numbers ff/ff/ff.

Commit e18d1abc3bff ("PCI: Avoid saving config space state if
inaccessible") added pci_dev_config_accessible() and checks it before a
reset.  Check it in pci_save_state() itself, so that system suspend is
covered as well.

If it is not accessible, keep the previous snapshot, mark the state as
saved and return 0.  This avoids changing callers or making the PM core
warn about a low-power device without saved state.  pci_bus_add_device()
saves the state when the device is added, so normally a snapshot exists.
That snapshot can be older than the last values the device had, for
example if its driver changed the Command register after the last
successful save.

Assisted-by: LLM
Signed-off-by: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com>
---
v3:
- Keep the previous snapshot and return 0 instead of returning -EIO, so
  that callers see no change.  Returning -EIO changed what some drivers
  do, as described on v2:
  https://lore.kernel.org/all/20261011210451.12129-1-fbeltranmillalen@gmail.com/
- Check accessibility only once, at the start of pci_save_state(), and
  drop the temporary copy of the header and the second check, as Bjorn
  suggested.
- pci_dev_save_and_disable() is left as it is.  Bjorn suggested using
  the return value of pci_save_state() there, and on v2 I said I would.
  This version returns 0 for an inaccessible device, so the existing
  check stays.  It also keeps that function from clearing the Command
  register of a device that does not respond.
- The commit message no longer presents this as a fix for the
  Thunderbolt controller dropping off during suspend.
- Darrell's Tested-by from v2 is not carried over, as the code changed.
- Farhan pointed out [1] that in 2025 Lukas was concerned about checking
  in pci_save_state() itself, as callers that don't need the check pay
  for it [2].  The check is one config read on top of the 16 for the
  header and those for the capabilities.  System suspend saves the state
  both from the PCI core and from the suspend callbacks of many drivers,
  so checking in every caller isn't practical.

About the snapshot that is kept, which I have not tested beyond the
case in the cover letter:
- For a bridge, bus numbers and windows changed after the last
  successful save, for example by hotplug, would be written back with
  their older values.
- The snapshot from pci_bus_add_device() is taken before the driver
  probes, so a device that was never saved again since then gets the
  pre-probe values.
- A device that became inaccessible before it was added has no real
  snapshot: the saved header is all zeros, which is what would be
  written back.

[1] https://lore.kernel.org/all/7ed02ff5-d7f8-42e4-bca9-b3d3ce7b6795@linux.ibm.com/
[2] https://lore.kernel.org/all/aOQX6ZTMvekd6gWy@wunner.de/

v2:
- Use pci_dev_config_accessible() instead of reading the Vendor ID,
  which always reads as all ones on SR-IOV VFs: v1 refused to save the
  state of every VF.
- Do the second check after saving the capabilities, and set
  state_saved only if both checks pass.

 drivers/pci/pci.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a62af7320539..eacaa1e71a83 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1794,10 +1794,23 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
  * pci_save_state - save the PCI configuration space of a device before
  *		    suspending
  * @dev: PCI device that we're dealing with
+ *
+ * If the config space of @dev is not accessible, the previous snapshot is
+ * kept and reported as saved, as writing back the all-ones values read from
+ * such a device would corrupt it once it is accessible again.
+ *
+ * Return: 0 on success, or a negative errno if a capability could not be
+ * saved.
  */
 int pci_save_state(struct pci_dev *dev)
 {
 	int i;
+
+	if (!pci_dev_config_accessible(dev, "save state")) {
+		dev->state_saved = true;
+		return 0;
+	}
+
 	/* XXX: 100% dword access ok here? */
 	for (i = 0; i < 16; i++) {
 		pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]);

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

[Francisco Beltrán Millalén]

When an inaccessible bridge's Link Status register reads as 0xffff,
both PCI_EXP_LNKSTA_DLLLA and PCI_EXP_LNKSTA_LT appear set.  The link
wait code then takes the inaccessible bridge for an active link:

  - pci_bridge_wait_for_secondary_bus(), for ports up to 5 GT/s, decides
    that the link is up and waits PCIE_RESET_READY_POLL_MS for the device
    below it;

  - pcie_wait_for_link_status(), used for faster ports and by
    pcie_retrain_link(), reports success at once when waiting for an
    active link, and times out after PCIE_LINK_RETRAIN_TIMEOUT_MS when
    waiting for LT to clear.

Treat an all-ones read as the device being gone.  All callers of
pcie_wait_for_link_status() and pcie_retrain_link() treat any non-zero
return as failure.

On a MacBookPro14,3 whose Thunderbolt controller does not come back from
S3, the downstream ports of the controller are 2.5 GT/s ports that no
longer answer.  Resume spent about 65 seconds waiting for the xHCI
controller behind one of them ("not ready 65535ms after resume"); with
the pci_bridge_wait_for_secondary_bus() change it gives up on that
controller after about one second.  That machine never reaches
pcie_wait_for_link_status() in this situation, so that part has not been
tested.

Assisted-by: LLM
Signed-off-by: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com>
Tested-by: Darrell Gum <d@rrell.co>
---
v3:
- No change, apart from adding Darrell's Tested-by from v2 (was 3/3).

v2:
- Also cover pcie_wait_for_link_status().

 drivers/pci/pci.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index eacaa1e71a83..24b51dd0f9cc 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4604,8 +4604,8 @@ static int pci_pm_reset(struct pci_dev *dev, bool probe)
  * @use_lt: Use the LT bit if TRUE, or the DLLLA bit if FALSE.
  * @active: Waiting for active or inactive?
  *
- * Return 0 if successful, or -ETIMEDOUT if status has not changed within
- * PCIE_LINK_RETRAIN_TIMEOUT_MS milliseconds.
+ * Return 0 if successful, -ENODEV if @pdev is not accessible, or -ETIMEDOUT
+ * if status has not changed within PCIE_LINK_RETRAIN_TIMEOUT_MS milliseconds.
  */
 static int pcie_wait_for_link_status(struct pci_dev *pdev,
 				     bool use_lt, bool active)
@@ -4620,6 +4620,9 @@ static int pcie_wait_for_link_status(struct pci_dev *pdev,
 	end_jiffies = jiffies + msecs_to_jiffies(PCIE_LINK_RETRAIN_TIMEOUT_MS);
 	do {
 		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnksta);
+		/* All ones means @pdev is gone; it would look like an active link */
+		if (PCI_POSSIBLE_ERROR(lnksta))
+			return -ENODEV;
 		if ((lnksta & lnksta_mask) == lnksta_match)
 			return 0;
 		msleep(1);
@@ -4642,8 +4645,9 @@ static int pcie_wait_for_link_status(struct pci_dev *pdev,
  * according to @use_lt.  It is not verified whether the use of the DLLLA
  * bit is valid.
  *
- * Return 0 if successful, or -ETIMEDOUT if training has not completed
- * within PCIE_LINK_RETRAIN_TIMEOUT_MS milliseconds.
+ * Return 0 if successful, -ENODEV if @pdev is not accessible, or -ETIMEDOUT
+ * if training has not completed within PCIE_LINK_RETRAIN_TIMEOUT_MS
+ * milliseconds.
  */
 int pcie_retrain_link(struct pci_dev *pdev, bool use_lt)
 {
@@ -4876,8 +4880,10 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
 		if (!dev->link_active_reporting)
 			return -ENOTTY;
 
+		/* All ones means @dev is gone; it would look like an active link */
 		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,