[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@xxxxxxxxx>
---
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);