[PATCH v6 0/2] Unify PCI suspend and hibernate paths

[PATCH v6 0/2] Unify PCI suspend and hibernate paths

[Mario Limonciello (AMD)]

From: Mario Limonciello <superm1@kernel.org>

Many cycles ago I sent out a kernel series for using the S4 paths when
the system goes to S5.  Some parts of it got merged, and Rafael suggested
to split the other parts into smaller pieces across multiple kernel cycles
to make bisecting easier.

This series attempts to unify the PCI suspend and hibernate paths so that
PCIe bridges with downstream devices are put into D3 at hibernate the same
way they already are at S3 suspend.  I have been carrying it, rebasing it
and personally using it for a while now.

v6:
 * Fix a NEW recovery failure identified by Sashiko
 * rebase on pci/next

Mario Limonciello (AMD) (2):
  PCI/PM: Split out code from pci_pm_suspend_noirq() into helper
  PCI: Align hibernate poweroff flow with suspend flow for bridges

 drivers/pci/pci-driver.c | 100 +++++++++++++++++++++++++++------------
 1 file changed, 70 insertions(+), 30 deletions(-)

base-commit: bc80f6c85a8e97bdb69604f82af37179fcb01c38
-- 
2.53.0

[PATCH v6 1/2] PCI/PM: Split out code from pci_pm_suspend_noirq() into helper

[Mario Limonciello (AMD)]

In order to unify suspend and hibernate codepaths without code duplication
the common code should be in common helpers.  Move it from
pci_pm_suspend_noirq() into a helper.  No intended functional changes.

Reviewed-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>
Tested-by: Eric Naim <dnaim@cachyos.org>
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v5:
 * Add tag
v4:
 * Make pci_pm_suspend_noirq_common() bool instead (Rafael)
---
 drivers/pci/pci-driver.c | 77 +++++++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 28 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index e16aa59dd7ac8..8334214f8c1ed 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -818,6 +818,52 @@ static void pci_pm_complete(struct device *dev)
 
 #endif /* !CONFIG_PM_SLEEP */
 
+#if defined(CONFIG_SUSPEND)
+/**
+ * pci_pm_suspend_noirq_common - prepare a device to enter a low-power state
+ * @pci_dev: pci device
+ *
+ * Save the device state and decide whether bus-level power management should
+ * skipped. Returns true if bus-level power management should be skipped,
+ * false otherwise.
+ */
+static bool pci_pm_suspend_noirq_common(struct pci_dev *pci_dev)
+{
+	if (!pci_dev->state_saved) {
+		pci_save_state(pci_dev);
+
+		/*
+		 * If the device is a bridge with a child in D0 below it,
+		 * it needs to stay in D0, so check skip_bus_pm to avoid
+		 * putting it into a low-power state in that case.
+		 */
+		if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
+			pci_prepare_to_sleep(pci_dev);
+	}
+
+	pci_dbg(pci_dev, "PCI PM: Sleep power state: %s\n",
+		pci_power_name(pci_dev->current_state));
+
+	if (pci_dev->current_state == PCI_D0) {
+		pci_dev->skip_bus_pm = true;
+		/*
+		 * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
+		 * downstream device is in D0, so avoid changing the power state
+		 * of the parent bridge by setting the skip_bus_pm flag for it.
+		 */
+		if (pci_dev->bus->self)
+			pci_dev->bus->self->skip_bus_pm = true;
+	}
+
+	if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) {
+		pci_dbg(pci_dev, "PCI PM: Skipped\n");
+		return true;
+	}
+
+	return false;
+}
+#endif /* CONFIG_SUSPEND */
+
 #ifdef CONFIG_SUSPEND
 static void pcie_pme_root_status_cleanup(struct pci_dev *pci_dev)
 {
@@ -907,6 +953,7 @@ static int pci_pm_suspend_noirq(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	bool skip_bus_pm;
 
 	if (dev_pm_skip_suspend(dev))
 		return 0;
@@ -937,36 +984,10 @@ static int pci_pm_suspend_noirq(struct device *dev)
 		}
 	}
 
-	if (!pci_dev->state_saved) {
-		pci_save_state(pci_dev);
-
-		/*
-		 * If the device is a bridge with a child in D0 below it,
-		 * it needs to stay in D0, so check skip_bus_pm to avoid
-		 * putting it into a low-power state in that case.
-		 */
-		if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
-			pci_prepare_to_sleep(pci_dev);
-	}
+	skip_bus_pm = pci_pm_suspend_noirq_common(pci_dev);
 
-	pci_dbg(pci_dev, "PCI PM: Suspend power state: %s\n",
-		pci_power_name(pci_dev->current_state));
-
-	if (pci_dev->current_state == PCI_D0) {
-		pci_dev->skip_bus_pm = true;
-		/*
-		 * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
-		 * downstream device is in D0, so avoid changing the power state
-		 * of the parent bridge by setting the skip_bus_pm flag for it.
-		 */
-		if (pci_dev->bus->self)
-			pci_dev->bus->self->skip_bus_pm = true;
-	}
-
-	if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) {
-		pci_dbg(pci_dev, "PCI PM: Skipped\n");
+	if (skip_bus_pm)
 		goto Fixup;
-	}
 
 set_unknown:
 	pci_pm_set_unknown_state(pci_dev);
-- 
2.53.0

[PATCH v6 2/2] PCI: Align hibernate poweroff flow with suspend flow for bridges

[Mario Limonciello (AMD)]

During S3 suspend, pci_pm_suspend_noirq() puts PCIe bridges with
downstream devices into a low-power state (D3hot or D3cold) when the
platform allows it.  The hibernate poweroff_noirq path never did this:
it only called pci_prepare_to_sleep() for devices with no subordinate,
so bridges with active children were left in D0.

On many designs the platform firmware leaves bridges alone when the
system enters S4. This prevents being able to meet various energy
certification criteria for different parts of the world, particularly
in designs with a dGPU.

Align the hibernate flow with suspend by making pci_pm_poweroff_noirq()
use pci_pm_suspend_noirq_common() instead of the open-coded
pci_prepare_to_sleep() call.  This reuses exactly the logic the S3
suspend path uses, including the skip_bus_pm handling that keeps a
bridge in D0 when a downstream device must stay in D0 (e.g. a
configured wakeup source) and the pm_suspend_no_platform() bus-PM skip.
This alone is safe for the normal power-off-and-reboot path, since the
hibernation image is snapshotted before poweroff_noirq runs and
restoring it always goes through a full boot that re-enumerates and
retrains the PCIe links.

It isn't enough if hibernation_platform_enter() aborts after
dpm_suspend_end(PMSG_HIBERNATE) but before power-off: that recovers via
dpm_resume_start(PMSG_RESTORE) without rebooting, which calls
pci_pm_restore_noirq() instead of pci_pm_resume_noirq().  Give
pci_pm_restore_noirq() the same D3cold handling pci_pm_resume_noirq()
already has, so a bridge coming out of D3cold gets
pci_pm_bridge_power_up_actions() before its children's restore_noirq
callbacks run against a possibly untrained link.

Mirror the suspend_noirq guard for drivers as well: if a driver's
poweroff_noirq callback already left the device in a low-power state
without saving its configuration, skip pci_pm_suspend_noirq_common() and
go straight to the fixups, exactly as pci_pm_suspend_noirq() does.  This
avoids having the core call pci_save_state() on a device the driver has
already powered down, whose configuration space may no longer be
readable, which could otherwise corrupt the saved state used on a
poweroff abort.

Because the poweroff_noirq path now mirrors the already-shipping
suspend_noirq path and is guarded identically, bridges that must remain
in D0 are unaffected; only bridges that S3 suspend would have powered
down are now also powered down at hibernate.

Acked-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>
Cc: AceLan Kao <acelan.kao@canonical.com>
Cc: Kai-Heng Feng <kaihengf@nvidia.com>
Cc: Mark Pearson <mpearson-lenovo@squebb.ca>
Cc: Denis Benato <benato.denis96@gmail.com>
Cc: Merthan Karakaş <m3rthn.k@gmail.com>
Tested-by: Eric Naim <dnaim@cachyos.org>
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v6:
 * Restore secondary bus wait in pci_pm_restore_noirq() on hibernate abort
   Link: https://lore.kernel.org/linux-pci/20260916210522.3A1111F00899@smtp.kernel.org/
   (Sashiko)
v5:
 * Add a guard like suspend path has
 * Add tag for Rafael
 * Reword title
 * Clarify that not all designs leave bridges alone at S4
---
 drivers/pci/pci-driver.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 8334214f8c1ed..7efe808cdf0cc 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -818,7 +818,7 @@ static void pci_pm_complete(struct device *dev)
 
 #endif /* !CONFIG_PM_SLEEP */
 
-#if defined(CONFIG_SUSPEND)
+#if defined(CONFIG_SUSPEND) || defined(CONFIG_HIBERNATE_CALLBACKS)
 /**
  * pci_pm_suspend_noirq_common - prepare a device to enter a low-power state
  * @pci_dev: pci device
@@ -862,7 +862,7 @@ static bool pci_pm_suspend_noirq_common(struct pci_dev *pci_dev)
 
 	return false;
 }
-#endif /* CONFIG_SUSPEND */
+#endif /* CONFIG_SUSPEND || CONFIG_HIBERNATE_CALLBACKS */
 
 #ifdef CONFIG_SUSPEND
 static void pcie_pme_root_status_cleanup(struct pci_dev *pci_dev)
@@ -1217,6 +1217,8 @@ static int pci_pm_poweroff(struct device *dev)
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
+	pci_dev->skip_bus_pm = false;
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
 
@@ -1259,6 +1261,7 @@ static int pci_pm_poweroff_noirq(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	bool skip_bus_pm;
 
 	if (dev_pm_skip_suspend(dev))
 		return 0;
@@ -1272,16 +1275,26 @@ static int pci_pm_poweroff_noirq(struct device *dev)
 	}
 
 	if (pm->poweroff_noirq) {
+		pci_power_t prev = pci_dev->current_state;
 		int error;
 
 		error = pm->poweroff_noirq(dev);
 		suspend_report_result(dev, pm->poweroff_noirq, error);
 		if (error)
 			return error;
+
+		if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
+		    && pci_dev->current_state != PCI_UNKNOWN) {
+			pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
+				      "PCI PM: State of device not saved by %pS\n",
+				      pm->poweroff_noirq);
+			goto Fixup;
+		}
 	}
 
-	if (!pci_dev->state_saved && !pci_has_subordinate(pci_dev))
-		pci_prepare_to_sleep(pci_dev);
+	skip_bus_pm = pci_pm_suspend_noirq_common(pci_dev);
+	if (skip_bus_pm)
+		goto Fixup;
 
 	/*
 	 * The reason for doing this here is the same as for the analogous code
@@ -1290,6 +1303,7 @@ static int pci_pm_poweroff_noirq(struct device *dev)
 	if (pci_dev->class == PCI_CLASS_SERIAL_USB_EHCI)
 		pci_write_config_word(pci_dev, PCI_COMMAND, 0);
 
+Fixup:
 	pci_fixup_device(pci_fixup_suspend_late, pci_dev);
 
 	return 0;
@@ -1299,10 +1313,15 @@ static int pci_pm_restore_noirq(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	pci_power_t prev_state = pci_dev->current_state;
+	bool skip_bus_pm = pci_dev->skip_bus_pm;
 
 	pci_pm_default_resume_early(pci_dev);
 	pci_fixup_device(pci_fixup_resume_early, pci_dev);
 
+	if (!skip_bus_pm && prev_state == PCI_D3cold)
+		pci_pm_bridge_power_up_actions(pci_dev);
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return 0;
 
-- 
2.53.0

Re: [PATCH v6 1/2] PCI/PM: Split out code from pci_pm_suspend_noirq() into helper

[sashiko-bot]

> In order to unify suspend and hibernate codepaths without code duplication
> the common code should be in common helpers.  Move it from
> pci_pm_suspend_noirq() into a helper.  No intended functional changes.
> 
> Reviewed-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>
> Tested-by: Eric Naim <dnaim@cachyos.org>
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930213923.566846-1-superm1@kernel.org?part=1

Re: [PATCH v6 2/2] PCI: Align hibernate poweroff flow with suspend flow for bridges

[sashiko-bot]

> During S3 suspend, pci_pm_suspend_noirq() puts PCIe bridges with
> downstream devices into a low-power state (D3hot or D3cold) when the
> platform allows it.  The hibernate poweroff_noirq path never did this:
> it only called pci_prepare_to_sleep() for devices with no subordinate,
> so bridges with active children were left in D0.
> 
> On many designs the platform firmware leaves bridges alone when the
> system enters S4. This prevents being able to meet various energy
> certification criteria for different parts of the world, particularly
> in designs with a dGPU.
> 
> Align the hibernate flow with suspend by making pci_pm_poweroff_noirq()
> use pci_pm_suspend_noirq_common() instead of the open-coded
> pci_prepare_to_sleep() call.  This reuses exactly the logic the S3
> suspend path uses, including the skip_bus_pm handling that keeps a
> [ ... ]
> Cc: Mark Pearson <mpearson-lenovo@squebb.ca>
> Cc: Denis Benato <benato.denis96@gmail.com>
> Cc: Merthan Karakaş <m3rthn.k@gmail.com>
> Tested-by: Eric Naim <dnaim@cachyos.org>
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930213923.566846-1-superm1@kernel.org?part=2

Re: [PATCH v6 0/2] Unify PCI suspend and hibernate paths

[Bjorn Helgaas]

On Wed, Sep 30, 2026 at 04:39:21PM -0500, Mario Limonciello (AMD) wrote:
> From: Mario Limonciello <superm1@kernel.org>
> 
> Many cycles ago I sent out a kernel series for using the S4 paths when
> the system goes to S5.  Some parts of it got merged, and Rafael suggested
> to split the other parts into smaller pieces across multiple kernel cycles
> to make bisecting easier.
> 
> This series attempts to unify the PCI suspend and hibernate paths so that
> PCIe bridges with downstream devices are put into D3 at hibernate the same
> way they already are at S3 suspend.  I have been carrying it, rebasing it
> and personally using it for a while now.
> 
> v6:
>  * Fix a NEW recovery failure identified by Sashiko
>  * rebase on pci/next
> 
> Mario Limonciello (AMD) (2):
>   PCI/PM: Split out code from pci_pm_suspend_noirq() into helper
>   PCI: Align hibernate poweroff flow with suspend flow for bridges
> 
>  drivers/pci/pci-driver.c | 100 +++++++++++++++++++++++++++------------
>  1 file changed, 70 insertions(+), 30 deletions(-)

Applied to pci/pm for v7.4, thanks!