PCI/PM: Do not save the config space of an inaccessible device

When a PCI device becomes inaccessible 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 to a device that answers again. On a MacBookPro14,3 this happens to the upstream bridge of a Thunderbolt 3 controller that drops off the bus while the system is
suspending: after resume the bridge has bus numbers ff/ff/ff and Secondary Bus Reset asserted, and the xHCI controllers behind it are removed. Resume also waits about 65 seconds for a device behind a dead bridge, because an all-ones Link Status reads as an active link.

Patch 2 makes pci_save_state() refuse to save an inaccessible device, using pci_dev_config_accessible() from commit e18d1abc3bff ("PCI: Avoid saving config space state if inaccessible"), so that system suspend is covered and not only resets. Patch 1 prepares the USB PCI HCD for it; without patch 1, patch 2 makes pci_pm_suspend_noirq() warn. Patch 3 stops the link wait code from taking an all-ones Link Status for an active link.

Patch 1 touches drivers/usb. Bjorn, if you take the series, it would need an ack from Greg or Alan.

Changes since v1:
- v1 2/4 and 3/4 took an all-ones Vendor and Device ID to mean that the device was inaccessible, but that is always the case for SR-IOV VFs, so they broke saving and restoring VFs (as I said in reply to v1). 2/3 now uses pci_dev_config_accessible(), which reads the Command and Status registers.
- Dropped v1 3/4 ("PCI/PM: Do not restore a config space snapshot that is all ones"): it would never restore a VF, and it did not protect what it claimed to, as pci_restore_state() restores the PCIe capability state before the standard header.
- 1/3: rewrote the commit message and moved the wakeup handling for a dead root hub ahead of the early return. Alan's Acked-by is dropped.
- 2/3: the second accessibility check now runs after the capabilities are saved, and state_saved is only set if both checks pass.
- 3/3: also cover pcie_wait_for_link_status().
- The v1 cover letter spoke of an earlier version; that version was never posted.
- Based on pci/next.

v1: https://lore.kernel.org/all/[email protected]/

Testing:
On a MacBookPro14,3 (two Alpine Ridge controllers), v6.18.49 with e18d1abc3bff backported and this series, S3 entered by closing the lid (158 s asleep), a USB disk on one controller and nothing on the other:

- In pci_pm_suspend_noirq() the bridges of both controllers, including the upstream bridge 04:00.0, were inaccessible and their state was not saved ("Device config space inaccessible; unable to save state").
- On resume the controller with nothing attached came back: 04:00.0 kept bus numbers 04/05/79 and Bridge Control 0x0002, the link came up at 8 GT/s and its xHCI controller resumed. Before the series the same bridge came back with ff/ff/ff and Bridge Control 0x005f (Secondary Bus Reset asserted), and both xHCI controllers were removed.
- The controller with the disk did not come back (its link does not train, which is a separate problem); resume waited 1 s for its xHCI controller instead of 65 s.
- No "State of device not saved" warning.

With the separate Alpine Ridge quirk applied, the xHCI controllers of an empty controller are inaccessible in hcd_pci_suspend_noirq(); four S3 cycles went through patch 1 without warnings and everything resumed.

When the machine wakes up again after a few seconds (with the lid open it does, after about 3.5 s), the empty controller does not come back either, with v1 as with v2, so there is nothing for the series to preserve. The "1 of 2 controllers instead of 0 of 2" in the v1 cover letter holds only for the longer sleeps.

I have no SR-IOV hardware, so the VF case is untested, and the machine never reaches the pcie_wait_for_link_status() change.

Francisco Beltrán Millalén (3):
usb: hcd-pci: Honour pci_save_state() failure PCI/PM: Do not save the config space of an inaccessible device
PCI: Do not mistake an absent device for an active link

 drivers/pci/pci.c          | 69 ++++++++++++++++++++++++++------------
 drivers/usb/core/hcd-pci.c | 15 +++++++--
 2 files changed, 61 insertions(+), 23 deletions(-)

hcd_pci_suspend_noirq() ignores the return value of pci_save_state() and goes on to pci_prepare_to_sleep().

The next patch makes pci_save_state() fail, without marking the state as saved, when the config space of the device is not accessible. For such a controller pci_prepare_to_sleep() does not work either:
pci_set_low_power_state() reads the PM control register as all ones and marks the device D3cold. pci_pm_suspend_noirq() then finds a device whose power state changed without its state being saved, and warns. With an earlier version of the next patch on a MacBookPro14,3, whose Thunderbolt xHCI controllers can be inaccessible at this point:

xhci_hcd 0000:7d:00.0: PCI PM: State of device not saved by hcd_pci_suspend_noirq+0x0/0x180

  WARNING: CPU: 7 PID: 96793 at drivers/pci/pci-driver.c:888 pci_pm_suspend_noirq+0x2f4/0x300

Check the return value and, if the state could not be saved, return 0 without putting the controller into a low-power state. The PCI core then handles it as it handles a driver without a suspend_noirq callback:
it tries to save the state and to put the device into a low-power state itself, which fails in the same way, but without the warning, as the power state did not change inside the driver callback.

Move the check that disables wakeup for a dead root hub ahead of it, so that it is not skipped.

Today pci_save_state() only fails if a capability save buffer is missing or the VC state cannot be saved. In that case the controller is now left in D0 instead of being put into a low-power state with an incomplete saved state.

---
v2:
- Rewrote the commit message: the failure handled here comes from the next patch, not from pci_save_state() as it is today.
- Disable wakeup for a dead root hub before the early return.
- Dropped Alan's Acked-by because of both changes.

 drivers/usb/core/hcd-pci.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

index cd2234759..7f8a56425 100644
@@ -541,8 +541,6 @@ static int hcd_pci_suspend_noirq(struct device *dev)
 	if (retval)
 		return retval;
 
-	pci_save_state(pci_dev);
-
 	/* If the root hub is dead rather than suspended, disallow remote
 	 * wakeup.  usb_hc_died() should ensure that both hosts are marked as
 	 * dying, so we only need to check the primary roothub.

@@ -551,6 +549,19 @@ static int hcd_pci_suspend_noirq(struct device *dev)
 		device_set_wakeup_enable(dev, 0);
 	dev_dbg(dev, "wakeup: %d\n", device_may_wakeup(dev));
 
+	/*
+	 * If the state could not be saved, most likely because the controller
+	 * is no longer accessible, putting it into a low-power state would
+	 * fail as well and leave it marked as D3cold, and the PCI core would
+	 * then warn that its state was not saved.  Leave it to the PCI core
+	 * instead, as for a driver without a suspend_noirq callback.
+	 */
+	retval = pci_save_state(pci_dev);
+	if (retval) {
+		dev_dbg(dev, "--> not suspending, state not saved\n");
+		return 0;
+	}
+
 	/* Possibly enable remote wakeup,
 	 * choose the appropriate low-power state, and go to that state.
 	 */

pci_save_state() reads the standard header into dev->saved_config_space and marks it valid without checking that the device answered. If the device is not accessible, every read returns all ones, the previous snapshot is overwritten, and pci_restore_state() writes the all-ones values back once the device answers again. 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.

On a MacBookPro14,3 this happens to the upstream bridge of a Thunderbolt controller that drops off the bus while the system is
suspending: after resume the bridge answers again, but with bus numbers ff/ff/ff and Secondary Bus Reset asserted, and the xHCI controllers behind it are removed.

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, where the state is saved by pci_pm_suspend_noirq() or by a driver's suspend_noirq callback, is covered as well. pci_dev_config_accessible() reads the Command and Status registers rather than the Vendor and Device IDs, which always read as all ones on SR-IOV VFs.

Check again after reading, as the device may become inaccessible in the meantime, and only then replace the previous snapshot of the header and set state_saved. The capabilities are saved straight into their own buffers and are not covered by the second check, and both checks are racy, as pci_dev_config_accessible() notes.

If the device is not accessible, return -EIO and leave state_saved as it was.

---
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. In the v1 thread I said I would use pci_device_is_present(), but for a VF that checks the PF and cannot tell whether the VF itself answers.
- Do the second check after saving the capabilities, and set state_saved only if both checks pass.
- The bus numbers are set to 0xff, not cleared.

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

index b2879a6be..94de99d31 100644
@@ -1776,31 +1776,52 @@ 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, nothing is saved and the
+ * previous snapshot of the standard header is kept, as writing back the
+ * all-ones values read from such a device would corrupt it once it is
+ * accessible again.
+ *
+ * Return: 0 on success, -EIO if @dev is not accessible, or another
+ * negative errno if a capability could not be saved.
  */
 int pci_save_state(struct pci_dev *dev)
 {
-	int i;
+	u32 config[16];
+	int i, ret;
+
+	if (!pci_dev_config_accessible(dev, "save state"))
+		return -EIO;
+
 	/* 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, &config[i]);
+		pci_dbg(dev, "save config %#04x: %#010x\n", i * 4, config[i]);
 	}
-	dev->state_saved = true;
 
-	i = pci_save_pcie_state(dev);
-	if (i != 0)
-		return i;
+	ret = pci_save_pcie_state(dev);
+	if (!ret)
+		ret = pci_save_pcix_state(dev);
+	if (!ret) {
+		pci_save_dpc_state(dev);
+		pci_save_aer_state(dev);
+		pci_save_ptm_state(dev);
+		pci_save_tph_state(dev);
+		ret = pci_save_vc_state(dev);
+	}
+
+	/*
+	 * The device may have become inaccessible while it was being read.
+	 * Keep the previous header snapshot in that case.  The capabilities
+	 * are saved directly into their buffers, so they are not protected.
+	 */
+	if (!pci_dev_config_accessible(dev, "save state"))
+		return -EIO;
 
-	i = pci_save_pcix_state(dev);
-	if (i != 0)
-		return i;
+	memcpy(dev->saved_config_space, config, sizeof(config));
+	dev->state_saved = true;
 
-	pci_save_dpc_state(dev);
-	pci_save_aer_state(dev);
-	pci_save_ptm_state(dev);
-	pci_save_tph_state(dev);
-	return pci_save_vc_state(dev);
+	return ret;
 }
 EXPORT_SYMBOL(pci_save_state);
 

[+cc Lukas, Farhan, Andreas, Mika, Yehezkel]

Apparently this is a reproducible issue on MacBookPro14,3. That makes me a little hesitant because we're not actually dealing with the fact that the Thunderbolt controller isn't responsive during suspend.

It seems worthwhile to me to skip pci_save_state() if the device isn't accessible, but I don't think it's a real solution to whatever is going on with Thunderbolt, and I don't think we should mention it here as though it is.

Hi Bjorn,

Thanks for the review, and for suggesting a simpler way to do this.

You're right that this patch doesn't fix the Thunderbolt problem itself; that is what the Alpine Ridge quirk is for. This patch only makes sure that, when a device stops responding, the PCI core doesn't save garbage and write it back later. I'll rewrite the commit message in v3 to say just that, and mention the MacBook only as the machine where I found it.

Yes, I'll change it that way in v3. While looking at it I found one small corner case worth mentioning: pci_save_state() can also fail when the kernel couldn't allocate memory for part of the saved state, back when the device was first found. The device itself works fine then, but with this change the reset would no longer disable it first. It is very unlikely to happen, so I don't think it matters much, but if you prefer, I can make the reset stop only when the device isn't responding.

I'll drop it in v3, so pci_save_state() goes back to what it was, plus the one check at the start.

I also wanted to see how your version behaves before sending it, so I built it and tested it on the MacBook today. It went through four suspend/resume cycles, three of them with a USB disk attached, with no warnings, and it skipped the controller that wasn't responding, just like v2 did. I also reset a USB controller by hand to try the pci_dev_save_and_disable() change, and that worked as before too.

Thanks again,
Francisco

Hi Bjorn,

Thanks for cc'ing me.

When I had initially looked into this, I had proposed adding the accessibility check in pci_save_state() as well [1]. Though Lukas had some valid concerns around the additional overhead from adding it in pci_save_state(). But maybe its worth re-visiting again?

Thanks

Farhan

[1] https://lore.kernel.org/all/[email protected]/

Hi Bjorn,

Thanks, Farhan, for the pointer to the earlier discussion. I hadn't seen it.

After reading that thread I went through the callers of pci_save_state(). When a device is already inaccessible while its driver's suspend callback runs, for example because it dropped off the bus and hasn't been removed yet, returning -EIO from pci_save_state() changes what some drivers do:

- Some return the error from their suspend callback, so system suspend is aborted: stmmac_pci_plat_suspend() (used by stmmac_pci, dwmac-loongson and dwmac-motorcomm), intel_eth_pci_suspend(), atl1e_suspend(), atl2_suspend() and amd_mp2_pci_suspend(). Today pci_save_state() returns 0 in that case, and these drivers go on.

- Many more ignore the return value and then put the device into D3hot, for example ata_pci_device_do_suspend(), which most libata PCI drivers use. On an inaccessible device that marks it D3cold, and since state_saved is now false, pci_legacy_suspend() or pci_pm_suspend() warns once that the state was not saved. This is the same warning that patch 1 avoids for the USB HCD in suspend_noirq.

I found all of this by reading the code; I can't test these drivers. I think this is what Lukas meant about callers that don't need the check.

The underlying problem is that pci_save_state() doesn't check that the device answered: for an inaccessible device it fills the saved header with all ones, marks the state saved and returns 0, and pci_restore_state() later writes those values back. What I have prepared for v3 fixes it in pci_save_state() itself, with the changes you suggested: it checks once that the device is accessible and returns -EIO if it isn't, and pci_dev_save_and_disable() uses the return value. v2 2/3 returns -EIO in the same way. The drivers above are the side effect of that: they now see a failure where they never saw one before.

I found this only recently, so I'd rather not send v3, and I'd ask you not to apply v2 2/3 as it is, until you've had a chance to look at this. Patch 3 doesn't depend on it. How would you like these drivers to be handled?

Thanks again,
Francisco

[1] https://lore.kernel.org/all/[email protected]/

When a bridge is no longer accessible, its Link Status register reads as 0xffff, which has both PCI_EXP_LNKSTA_DLLLA and PCI_EXP_LNKSTA_LT set. The code that waits for the link then takes a dead link for an active one:

- 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.

---
v2:
- Also check in pcie_wait_for_link_status() (faster ports and pcie_retrain_link()); not tested.
- Say that it is the bridge that is gone.

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

index 94de99d31..2e31d18b2 100644
@@ -4586,8 +4586,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)

@@ -4602,6 +4602,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);

@@ -4624,8 +4627,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)
 {

@@ -4858,8 +4862,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,

[...]

Hi Francisco,

I tested this series on another MacBookPro14,3, together with your Alpine Ridge SXFP quirk and the Apple native PME patch. On this machine the combination takes S3 from never resuming to working, and it fixes USB-C hotplug while the Thunderbolt xHCIs are runtime-suspended.

Hardware: MacBookPro14,3 (15", 2017, T1), two Alpine Ridge 4C controllers:

  upstream bridges    8086:1578   04:00.0, 7a:00.0
  downstream bridges  8086:15d3
  NHI                 8086:15d2   06:00.0, 7c:00.0
  xHCI                8086:15d4   07:00.0, 7d:00.0

The dGPU is a Radeon Pro 555 (1002:67ef, subsystem 106b:017a rev c7).

Kernel: 7.2.5 with the Omarchy distro patches (linux-omarchy 7.2.5-3), plus these, in this order:

e18d1abc3bff ("PCI: Avoid saving config space state if inaccessible") this series, v2 1/3-3/3
PCI: Extend Apple Thunderbolt power quirk to Alpine Ridge
ACPI: PCI: take native PME control on Apple machines a5be7ad8f5f0 + a22e5e4f2ebb (amdgpu VI reset quirk, incl. 017a)

Everything applied with offsets only (no fuzz) on 7.2.5. mem_sleep=deep, stock command line.

I applied and tested all of these together. I did not bisect them, so beyond what the logs show directly I can't pin a result on one patch.

Before (stock 7.2.5-3 on the same machine):

- pm_test=platform hard-hung 3 out of 3 times. That includes a fresh boot and runs with brcmfmac unloaded, and it did not recover after more than 3.5 minutes. pm_test=devices passed.
- Real S3 never resumed. Every attempt needed a forced power-off.
- With both TB xHCIs runtime-suspended (power/control=auto), plugging a USB 3 stick into any USB-C port produced no kernel messages at all. With power/control=on, it enumerated at SuperSpeed right away.

After (patched kernel):

- _OSC now reads "OS assumes control of [PCIeHotplug SHPCHotplug PME AER PCIeCapability LTR DPC]". PME is missing from that line on the stock kernel.
- Hotplug: with both xHCIs runtime-suspended, the same stick enumerated at SuperSpeed within about 1 s. 7d:00.0 resumed on its own and 07:00.0 stayed suspended. Nothing was forced.
- pm_test=platform passes. "quirk: cutting power to Thunderbolt controller..." is logged for both 04:00.0 and 7a:00.0.
- Real S3: every attempt resumed. That was about half a dozen real S3 cycles over one morning: rtcwake on AC, plus lid-close suspends on battery. amdgpu resumed in 1.2 s.
- With a device attached (lid-close S3 on battery, about 1 min asleep): a USB 3 stick was enumerated at SuperSpeed on 7d:00.0 (behind 7a:00.0) before suspend. The quirk logged for both 7a:00.0 and 04:00.0 with the stick attached. After resume the stick was still there with no USB disconnect logged, still at 5000 Mbps, and its filesystem mounted and listed fine. Unplugging it and plugging it back in about 2 min after resume re-enumerated it at SuperSpeed in about 3 s, with power/control=auto.
- On that cycle the xHCI of the other, empty controller logged "xhci_hcd 0000:07:00.0: xHC error in resume, USBSTS 0x401, Reinit" and recovered. I'm mentioning it only because it's on the path your quirk affects; I haven't looked into it further.
- noirq resume takes about 16 s (15.9 s on a real S3 cycle). About 11 s of that is in each upstream bridge (04:00.0, 7a:00.0), then about 5 s in the NHIs (06:00.0, 7c:00.0). Another 14,3 owner reports the same split in s2idle (stock kernel plus a local Thunderbolt workaround), and says it drops to about 0.5 s with the ACPICA change proposed in
https://github.com/open-acpica/acpica/pull/1235 . I haven't tried that here.

Not covered:

- The "plugging into USB-C no longer wakes it" trade-off is untested here.
- Only one S3 cycle had a device attached, and it was a USB stick. No real Thunderbolt devices and no SR-IOV.
- The stock kernel also lacked the amdgpu quirk, so the real-S3 before/after mixes both changes. The cleaner Thunderbolt-side comparison is pm_test=platform: the devices stage, amdgpu included, already passed on stock.

Two workarounds that have nothing to do with Thunderbolt were in place for every patched-kernel run, in case they show up in other reports:

- d3cold_allowed=0 on the NVMe SSD (02:00.0). On stock, the machine never even reached S3 without it. I have not retested without it on the patched kernel.
- brcmfmac (BCM43602, 03:00.0) unloaded before suspend and reloaded after. Left bound through a real S3, the Wi-Fi firmware state is lost.

Thanks for chasing this down. This is the first kernel on which this machine resumes from S3 at all.

[+cc Lukas]

If it would be safe to apply patches 2 and 3 without patch 1, I could go ahead and do that.

Patch 2 returns errors from pci_save_state() in more cases, but hcd_pci_suspend_noirq() doesn't check for errors anyway, so I think it's would be no worse off it we applied patch 2 without patch 1.

And patch 3 looks like it's probably safe by itself independent of the others.

Hi Bjorn,

Thanks for reviewing the series.

I tested that combination on the MacBookPro14,3: patch 2 as you suggest in your other reply, patch 1 disabled, the Alpine Ridge quirk applied, and nothing plugged into one of the two Thunderbolt controllers. The system resumed normally. The only difference was this warning, once per boot:

xhci_hcd 0000:07:00.0: Unable to change power state from D0 to D3hot, device inaccessible xhci_hcd 0000:07:00.0: PCI PM: State of device not saved by hcd_pci_suspend_noirq+0x0/0x1c0

  WARNING: CPU: 2 PID: 4344 at drivers/pci/pci-driver.c:888 pci_pm_suspend_noirq+0x2f4/0x300

hcd_pci_suspend_noirq() goes on to pci_prepare_to_sleep(), which fails on the inaccessible controller and leaves it marked D3cold, and pci_pm_suspend_noirq() then warns. With patch 1 there is no warning.

So 2 and 3 can go in first, with patch 1 following through your tree with an ack from Alan or Greg, or through the USB tree. Patch 1 builds on its own and has no effect in practice until patch 2 is in.

I'll send a v3 with your changes to patch 2.

Francisco