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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Mon, Oct 19, 2026 at 05:24:43PM -0300, Francisco Beltrán Millalén wrote:
> 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

Both patches look good to me and they improve PCI config space handling in
general too,

Reviewed-by: Mika Westerberg (Intel) <westeri@kernel.org>