[PATCH v7 00/12] Improvements to S5 power consumption

[PATCH v7 00/12] Improvements to S5 power consumption

[Mario Limonciello (AMD)]

A variety of issues both in function and in power consumption have been
raised as a result of devices not being put into a low power state when
the system is powered off.

There have been some localized changes[1] to PCI core to help these issues,
but they have had various downsides.

This series instead uses the driver hibernate flows when the system is
being powered off or halted.  This lines up the behavior with what other
operating systems do as well.  If for some reason that fails or is not
supported, run driver shutdown() callbacks.

Rafael did mention in earlier versions of the series concerns about
regression risk.  He was looking for thoughts from Greg who isn't against
it but also isn't sure about how to maintain it. [1]

This has been validated by me and several others in AMD
on a variety of AMD hardware platforms. It's been validated by some
community members on their Intel hardware. To my knowledge it has not
been validated on non-x86.

On my development laptop I have also contrived failures in the hibernation
callbacks to make sure that the fallback to shutdown callback works.

In order to assist with potential regressions the series also includes
documentation to help with getting a kernel log at shutdown after
the disk is unmounted.

Cc: AceLan Kao <acelan.kao@canonical.com>
Cc: Kai-Heng Feng <kaihengf@nvidia.com>
Cc: Mark Pearson <mpearson-lenovo@squebb.ca>
Cc: Merthan Karakaş <m3rthn.k@gmail.com>
Cc: Eric Naim <dnaim@cachyos.org>
Link: https://lore.kernel.org/linux-usb/2025090852-coma-tycoon-9f37@gregkh/ [1]
---
v6->v7:
 * Add documentation on how to debug a shutdown hang
 * Adjust commit messages per feedback from Bjorn

Mario Limonciello (AMD) (12):
  PM: Introduce new PMSG_POWEROFF event
  scsi: Add PM_EVENT_POWEROFF into suspend callbacks
  usb: sl811-hcd: Add PM_EVENT_POWEROFF into suspend callbacks
  USB: Pass PMSG_POWEROFF event to suspend_common()
  PCI/PM: Disable device wakeups when halting or powering off system
  PCI/PM: Split out code from pci_pm_suspend_noirq() into helper
  PCI/PM: Run bridge power up actions as part of restore phase
  PCI/PM: Use pci_power_manageable() in pci_pm_poweroff_noirq()
  PCI: Put PCIe bridges with downstream devices into D3 at hibernate
  drm/amd: Avoid evicting resources at S5
  PM: Use hibernate flows for system power off
  Documentation: power: Add document on debugging shutdown hangs

 Documentation/power/index.rst              |  1 +
 Documentation/power/shutdown-debugging.rst | 55 ++++++++++++
 drivers/base/power/main.c                  |  7 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  4 +
 drivers/pci/pci-driver.c                   | 99 +++++++++++++++-------
 drivers/scsi/mesh.c                        |  1 +
 drivers/scsi/stex.c                        |  1 +
 drivers/usb/core/hcd-pci.c                 | 11 ++-
 drivers/usb/host/sl811-hcd.c               |  1 +
 include/linux/pm.h                         |  5 +-
 include/trace/events/power.h               |  3 +-
 kernel/reboot.c                            |  6 ++
 12 files changed, 159 insertions(+), 35 deletions(-)
 create mode 100644 Documentation/power/shutdown-debugging.rst


base-commit: 76eeb9b8de9880ca38696b2fb56ac45ac0a25c6c
-- 
2.43.0

[PATCH v7 01/12] PM: Introduce new PMSG_POWEROFF event

[Mario Limonciello (AMD)]

PMSG_POWEROFF will be used for the PM core to allow differentiating between
a hibernation or shutdown sequence when re-using callbacks.

This event should not have wakeups enabled so update PMSG_NO_WAKEUP() to
match it as well.

Tested-by: Eric Naim <dnaim@cachyos.org>
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v7:
 * Reword commit
v5:
 * Re-order and split
 * Add tags
v4:
 * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
---
 drivers/base/power/main.c    | 7 +++++++
 include/linux/pm.h           | 5 ++++-
 include/trace/events/power.h | 3 ++-
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 2ea6e05e6ec90..86661c94e8cef 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -99,6 +99,8 @@ static const char *pm_verb(int event)
 		return "restore";
 	case PM_EVENT_RECOVER:
 		return "recover";
+	case PM_EVENT_POWEROFF:
+		return "poweroff";
 	default:
 		return "(unknown PM event)";
 	}
@@ -369,6 +371,7 @@ static pm_callback_t pm_op(const struct dev_pm_ops *ops, pm_message_t state)
 	case PM_EVENT_FREEZE:
 	case PM_EVENT_QUIESCE:
 		return ops->freeze;
+	case PM_EVENT_POWEROFF:
 	case PM_EVENT_HIBERNATE:
 		return ops->poweroff;
 	case PM_EVENT_THAW:
@@ -403,6 +406,7 @@ static pm_callback_t pm_late_early_op(const struct dev_pm_ops *ops,
 	case PM_EVENT_FREEZE:
 	case PM_EVENT_QUIESCE:
 		return ops->freeze_late;
+	case PM_EVENT_POWEROFF:
 	case PM_EVENT_HIBERNATE:
 		return ops->poweroff_late;
 	case PM_EVENT_THAW:
@@ -437,6 +441,7 @@ static pm_callback_t pm_noirq_op(const struct dev_pm_ops *ops, pm_message_t stat
 	case PM_EVENT_FREEZE:
 	case PM_EVENT_QUIESCE:
 		return ops->freeze_noirq;
+	case PM_EVENT_POWEROFF:
 	case PM_EVENT_HIBERNATE:
 		return ops->poweroff_noirq;
 	case PM_EVENT_THAW:
@@ -1370,6 +1375,8 @@ static pm_message_t resume_event(pm_message_t sleep_state)
 		return PMSG_RECOVER;
 	case PM_EVENT_HIBERNATE:
 		return PMSG_RESTORE;
+	case PM_EVENT_POWEROFF:
+		return PMSG_ON;
 	}
 	return PMSG_ON;
 }
diff --git a/include/linux/pm.h b/include/linux/pm.h
index cc7b2dc28574c..892bd93f13dad 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -507,6 +507,7 @@ const struct dev_pm_ops name = { \
  * RECOVER	Creation of a hibernation image or restoration of the main
  *		memory contents from a hibernation image has failed, call
  *		->thaw() and ->complete() for all devices.
+ * POWEROFF	System will poweroff, call ->poweroff() for all devices.
  *
  * The following PM_EVENT_ messages are defined for internal use by
  * kernel subsystems.  They are never issued by the PM core.
@@ -537,6 +538,7 @@ const struct dev_pm_ops name = { \
 #define PM_EVENT_USER		0x0100
 #define PM_EVENT_REMOTE		0x0200
 #define PM_EVENT_AUTO		0x0400
+#define PM_EVENT_POWEROFF	0x0800
 
 #define PM_EVENT_SLEEP		(PM_EVENT_SUSPEND | PM_EVENT_HIBERNATE)
 #define PM_EVENT_USER_SUSPEND	(PM_EVENT_USER | PM_EVENT_SUSPEND)
@@ -551,6 +553,7 @@ const struct dev_pm_ops name = { \
 #define PMSG_QUIESCE	((struct pm_message){ .event = PM_EVENT_QUIESCE, })
 #define PMSG_SUSPEND	((struct pm_message){ .event = PM_EVENT_SUSPEND, })
 #define PMSG_HIBERNATE	((struct pm_message){ .event = PM_EVENT_HIBERNATE, })
+#define PMSG_POWEROFF	((struct pm_message){ .event = PM_EVENT_POWEROFF, })
 #define PMSG_RESUME	((struct pm_message){ .event = PM_EVENT_RESUME, })
 #define PMSG_THAW	((struct pm_message){ .event = PM_EVENT_THAW, })
 #define PMSG_RESTORE	((struct pm_message){ .event = PM_EVENT_RESTORE, })
@@ -568,7 +571,7 @@ const struct dev_pm_ops name = { \
 
 #define PMSG_IS_AUTO(msg)	(((msg).event & PM_EVENT_AUTO) != 0)
 #define PMSG_NO_WAKEUP(msg)	(((msg).event & \
-				(PM_EVENT_FREEZE | PM_EVENT_QUIESCE)) != 0)
+				(PM_EVENT_FREEZE | PM_EVENT_QUIESCE | PM_EVENT_POWEROFF)) != 0)
 /*
  * Device run-time power management status.
  *
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index 82904291c2b81..370f8df2fdb4b 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h
@@ -179,7 +179,8 @@ TRACE_EVENT(pstate_sample,
 		{ PM_EVENT_HIBERNATE, "hibernate" }, \
 		{ PM_EVENT_THAW, "thaw" }, \
 		{ PM_EVENT_RESTORE, "restore" }, \
-		{ PM_EVENT_RECOVER, "recover" })
+		{ PM_EVENT_RECOVER, "recover" }, \
+		{ PM_EVENT_POWEROFF, "poweroff" })
 
 DEFINE_EVENT(cpu, cpu_frequency,
 
-- 
2.43.0

[PATCH v7 02/12] scsi: Add PM_EVENT_POWEROFF into suspend callbacks

[Mario Limonciello (AMD)]

When the ACPI core uses hibernation callbacks for shutdown drivers
will receive PM_EVENT_POWEROFF and should handle it the same as
PM_EVENT_HIBERNATE would have been used.

Tested-by: Eric Naim <dnaim@cachyos.org>
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v5:
 * Re-order
v4:
 * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
---
 drivers/scsi/mesh.c | 1 +
 drivers/scsi/stex.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/scsi/mesh.c b/drivers/scsi/mesh.c
index 1c15cac41d805..768b85eecc8fd 100644
--- a/drivers/scsi/mesh.c
+++ b/drivers/scsi/mesh.c
@@ -1762,6 +1762,7 @@ static int mesh_suspend(struct macio_dev *mdev, pm_message_t mesg)
 	case PM_EVENT_SUSPEND:
 	case PM_EVENT_HIBERNATE:
 	case PM_EVENT_FREEZE:
+	case PM_EVENT_POWEROFF:
 		break;
 	default:
 		return 0;
diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
index 63ed7f9aaa937..ee9372e1f7f07 100644
--- a/drivers/scsi/stex.c
+++ b/drivers/scsi/stex.c
@@ -1965,6 +1965,7 @@ static int stex_choice_sleep_mic(struct st_hba *hba, pm_message_t state)
 	case PM_EVENT_SUSPEND:
 		return ST_S3;
 	case PM_EVENT_HIBERNATE:
+	case PM_EVENT_POWEROFF:
 		hba->msi_lock = 0;
 		return ST_S4;
 	default:
-- 
2.43.0

[PATCH v7 03/12] usb: sl811-hcd: Add PM_EVENT_POWEROFF into suspend callbacks

[Mario Limonciello (AMD)]

When the ACPI core uses hibernation callbacks for shutdown drivers
will receive PM_EVENT_POWEROFF and should handle it the same as
PM_EVENT_HIBERNATE would have been used.

Tested-by: Eric Naim <dnaim@cachyos.org>
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v5:
 * Re-order
 * Add tags
v4:
 * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
---
 drivers/usb/host/sl811-hcd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/sl811-hcd.c b/drivers/usb/host/sl811-hcd.c
index ea3cab99c5d40..5d6dba681e503 100644
--- a/drivers/usb/host/sl811-hcd.c
+++ b/drivers/usb/host/sl811-hcd.c
@@ -1748,6 +1748,7 @@ sl811h_suspend(struct platform_device *dev, pm_message_t state)
 		break;
 	case PM_EVENT_SUSPEND:
 	case PM_EVENT_HIBERNATE:
+	case PM_EVENT_POWEROFF:
 	case PM_EVENT_PRETHAW:		/* explicitly discard hw state */
 		port_power(sl811, 0);
 		break;
-- 
2.43.0

[PATCH v7 04/12] USB: Pass PMSG_POWEROFF event to suspend_common()

[Mario Limonciello (AMD)]

If powering off the system USB wakeup sources should be ignored.
Add a new callback hcd_pci_poweroff() which will differentiate whether
target state is halt or power off and pass PMSG_POWEROFF as the
message so that suspend_common() will avoid doing wakeups.

Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v7:
 * Reword commit mesasge
v6:
 * Fix LKP robot issue without CONFIG_PM_SLEEP
v5:
 * New patch
v4:
 * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/

Fix lkp robot issue
---
 drivers/usb/core/hcd-pci.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index cd223475917ef..921d1d0940016 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -6,6 +6,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/pm.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
 
@@ -531,6 +532,13 @@ static int hcd_pci_freeze(struct device *dev)
 	return suspend_common(dev, PMSG_FREEZE);
 }
 
+static int hcd_pci_poweroff(struct device *dev)
+{
+	if (system_state == SYSTEM_HALT || system_state == SYSTEM_POWER_OFF)
+		return suspend_common(dev, PMSG_POWEROFF);
+	return suspend_common(dev, PMSG_SUSPEND);
+}
+
 static int hcd_pci_suspend_noirq(struct device *dev)
 {
 	struct pci_dev		*pci_dev = to_pci_dev(dev);
@@ -602,6 +610,7 @@ static int hcd_pci_restore(struct device *dev)
 #define hcd_pci_suspend		NULL
 #define hcd_pci_freeze			NULL
 #define hcd_pci_suspend_noirq	NULL
+#define hcd_pci_poweroff	NULL
 #define hcd_pci_poweroff_late	NULL
 #define hcd_pci_resume_noirq	NULL
 #define hcd_pci_resume		NULL
@@ -639,7 +648,7 @@ const struct dev_pm_ops usb_hcd_pci_pm_ops = {
 	.freeze_noirq	= check_root_hub_suspended,
 	.thaw_noirq	= NULL,
 	.thaw		= hcd_pci_resume,
-	.poweroff	= hcd_pci_suspend,
+	.poweroff	= hcd_pci_poweroff,
 	.poweroff_late	= hcd_pci_poweroff_late,
 	.poweroff_noirq	= hcd_pci_suspend_noirq,
 	.restore_noirq	= hcd_pci_resume_noirq,
-- 
2.43.0

[PATCH v7 05/12] PCI/PM: Disable device wakeups when halting or powering off system

[Mario Limonciello (AMD)]

PCI devices can be configured as wakeup sources from low power states.
However, when the system is halting or powering off such wakeups are
not expected and may lead to spurious behavior.

ACPI r6.5, section 16.1.5 notes:

    "Hardware does allow a transition to S0 due to power button press
     or a Remote Start."

This implies that wakeups from PCI devices should not be relied upon
in these states. To align with this expectation and avoid unintended
wakeups, disable device wakeup capability during these transitions.

Tested-by: Eric Naim <dnaim@cachyos.org>
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v7:
 * Reword title
 * Reword commit
v5:
 * Re-order
 * Add tags
v4:
 * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
---
 drivers/pci/pci-driver.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 63665240ae87f..f201d298d7173 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1139,6 +1139,10 @@ 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;
 
+	if (device_may_wakeup(dev) &&
+	    (system_state == SYSTEM_HALT || system_state == SYSTEM_POWER_OFF))
+		device_set_wakeup_enable(dev, false);
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
 
-- 
2.43.0

[PATCH v7 06/12] 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.

Tested-by: Eric Naim <dnaim@cachyos.org>
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v7:
 * Reword title
v5:
 * Split from earlier patches
 * Add tags
v4:
 * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
---
 drivers/pci/pci-driver.c | 81 +++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 30 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index f201d298d7173..571a3809f163a 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -762,6 +762,54 @@ static void pci_pm_complete(struct device *dev)
 
 #endif /* !CONFIG_PM_SLEEP */
 
+#if defined(CONFIG_SUSPEND)
+/**
+ * pci_pm_suspend_noirq_common
+ * @pci_dev: pci device
+ * @skip_bus_pm: pointer to a boolean indicating whether to skip bus PM
+ *
+ * Prepare the device to go into a low power state by saving state and
+ * deciding whether to skip bus PM.
+ *
+ */
+static void pci_pm_suspend_noirq_common(struct pci_dev *pci_dev, bool *skip_bus_pm)
+{
+	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");
+		*skip_bus_pm = true;
+		return;
+	}
+
+	pci_pm_set_unknown_state(pci_dev);
+}
+#endif /* CONFIG_SUSPEND */
+
 #ifdef CONFIG_SUSPEND
 static void pcie_pme_root_status_cleanup(struct pci_dev *pci_dev)
 {
@@ -851,6 +899,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 = false;
 
 	if (dev_pm_skip_suspend(dev))
 		return 0;
@@ -881,38 +930,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);
-	}
-
-	pci_dbg(pci_dev, "PCI PM: Suspend power state: %s\n",
-		pci_power_name(pci_dev->current_state));
+	pci_pm_suspend_noirq_common(pci_dev, &skip_bus_pm);
 
-	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;
-	}
-
-	pci_pm_set_unknown_state(pci_dev);
 
 	/*
 	 * Some BIOSes from ASUS have a bug: If a USB EHCI host controller's
-- 
2.43.0

[PATCH v7 07/12] PCI/PM: Run bridge power up actions as part of restore phase

[Mario Limonciello (AMD)]

Suspend resume actions will check the state of the device and whether
bus PM should be skipped.  These same actions make sense during hibernation
image restore.  Apply them there as well.

Tested-by: Eric Naim <dnaim@cachyos.org>
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v7:
 * Reword title
v5:
 * Split out patch
---
 drivers/pci/pci-driver.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 571a3809f163a..fb6f1f60b2f1f 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1246,10 +1246,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.43.0

[PATCH v7 08/12] PCI/PM: Use pci_power_manageable() in pci_pm_poweroff_noirq()

[Mario Limonciello (AMD)]

Devices with no subordinate should be put into D3 during hibernate, but
devices that have bridge_d3 set should also be put to sleep during
hibernate. Adjust the check in pci_pm_poweroff_noirq() to use
pci_power_manageable() to cover those as well.

Tested-by: Eric Naim <dnaim@cachyos.org>
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v7:
 * Reword title
v5:
 * Split out patch
---
 drivers/pci/pci-driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index fb6f1f60b2f1f..c563fd6af979d 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1227,7 +1227,7 @@ static int pci_pm_poweroff_noirq(struct device *dev)
 			return error;
 	}
 
-	if (!pci_dev->state_saved && !pci_has_subordinate(pci_dev))
+	if (!pci_dev->state_saved && pci_power_manageable(pci_dev))
 		pci_prepare_to_sleep(pci_dev);
 
 	/*
-- 
2.43.0

[PATCH v7 09/12] PCI: Put PCIe bridges with downstream devices into D3 at hibernate

[Mario Limonciello (AMD)]

During suspend, PCIe bridges with downstream devices are transitioned into
a low power state (D3hot or D3cold) depending on platform capabilities.
However, during hibernate, these bridges remain in D0, which can lead to
unnecessary power consumption.

Align the hibernate flow with suspend by updating pci_pm_poweroff_noirq()
to use pci_pm_suspend_noirq_common(). This ensures that PCIe bridges with
active downstream devices are properly transitioned to a low power state
during hibernate.

This change introduces a functional update: the hibernate path will now
invoke pci_save_state(), and — unless bus-level power management is
skipped — will transition the bridge into D3hot or D3cold as appropriate.

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>
---
v7:
 * reword commit text
v6:
 * s/port/bridge/
v5:
 * Split out into more patches
 * Rewrite commit
 * Add tag
v4:
 * Use helper even when CONFIG_SUSPEND not set (LKP robot)
 * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
v3:
 * Split out common code between suspend_noirq() and poweroff_noirq()
   to a helper function
 * https://lore.kernel.org/linux-pm/20250609024619.407257-1-superm1@kernel.org/T/#me6db0fb946e3d604a8f3d455128844ed802c82bb
---
 drivers/pci/pci-driver.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index c563fd6af979d..5eedfbb0be8a4 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -762,7 +762,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
  * @pci_dev: pci device
@@ -808,7 +808,7 @@ static void pci_pm_suspend_noirq_common(struct pci_dev *pci_dev, bool *skip_bus_
 
 	pci_pm_set_unknown_state(pci_dev);
 }
-#endif /* CONFIG_SUSPEND */
+#endif /* CONFIG_SUSPEND || CONFIG_HIBERNATE_CALLBACKS */
 
 #ifdef CONFIG_SUSPEND
 static void pcie_pme_root_status_cleanup(struct pci_dev *pci_dev)
@@ -1164,6 +1164,8 @@ static int pci_pm_poweroff(struct device *dev)
 	    (system_state == SYSTEM_HALT || system_state == SYSTEM_POWER_OFF))
 		device_set_wakeup_enable(dev, false);
 
+	pci_dev->skip_bus_pm = false;
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
 
@@ -1206,6 +1208,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 = false;
 
 	if (dev_pm_skip_suspend(dev))
 		return 0;
@@ -1227,8 +1230,9 @@ static int pci_pm_poweroff_noirq(struct device *dev)
 			return error;
 	}
 
-	if (!pci_dev->state_saved && pci_power_manageable(pci_dev))
-		pci_prepare_to_sleep(pci_dev);
+	pci_pm_suspend_noirq_common(pci_dev, &skip_bus_pm);
+	if (skip_bus_pm)
+		goto Fixup;
 
 	/*
 	 * The reason for doing this here is the same as for the analogous code
@@ -1237,6 +1241,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;
-- 
2.43.0

[PATCH v7 10/12] drm/amd: Avoid evicting resources at S5

[Mario Limonciello (AMD)]

Normally resources are evicted on dGPUs at suspend or hibernate and
on APUs at hibernate.  These steps are unnecessary when using the S4
callbacks to put the system into S5.

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>
Acked-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v5:
 * No changes
v4:
 * Add A-b tag for Alex
 * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index f9b4c4321f67c..4e4b7a63cc61e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -5017,6 +5017,10 @@ static int amdgpu_device_evict_resources(struct amdgpu_device *adev)
 	if (!adev->in_s4 && (adev->flags & AMD_IS_APU))
 		return 0;
 
+	/* No need to evict when going to S5 through S4 callbacks */
+	if (system_state == SYSTEM_HALT || system_state == SYSTEM_POWER_OFF)
+		return 0;
+
 	ret = amdgpu_ttm_evict_resources(adev, TTM_PL_VRAM);
 	if (ret) {
 		dev_warn(adev->dev, "evicting device resources failed\n");
-- 
2.43.0

[PATCH v7 11/12] PM: Use hibernate flows for system power off

[Mario Limonciello (AMD)]

When the system is powered off the kernel will call device_shutdown()
which will issue callbacks into PCI core to wake up a device and call
it's shutdown() callback.  This will leave devices in ACPI D0 which can
cause some devices to misbehave with spurious wakeups and also leave some
devices on which will consume power needlessly.

The issue won't happen if the device is in D3 before system shutdown, so
putting device to low power state before shutdown solves the issue.

ACPI Spec 6.5, "7.4.2.5 System \_S4 State" says "Devices states are
compatible with the current Power Resource states. In other words, all
devices are in the D3 state when the system state is S4."

The following "7.4.2.6 System \_S5 State (Soft Off)" states "The S5
state is similar to the S4 state except that OSPM does not save any
context." so it's safe to assume devices should be at D3 for S5.

To accomplish this, use the PMSG_POWEROFF event to call all the device
hibernate callbacks when the kernel is compiled with hibernate support.
If compiled without hibernate support or hibernate fails fall back into
the previous shutdown flow.

Cc: AceLan Kao <acelan.kao@canonical.com>
Cc: Kai-Heng Feng <kaihengf@nvidia.com>
Cc: Mark Pearson <mpearson-lenovo@squebb.ca>
Cc: Merthan Karakaş <m3rthn.k@gmail.com>
Tested-by: Eric Naim <dnaim@cachyos.org>
Tested-by: Denis Benato <benato.denis96@gmail.com>
Link: https://lore.kernel.org/linux-pci/20231213182656.6165-1-mario.limonciello@amd.com/
Link: https://lore.kernel.org/linux-pci/20250506041934.1409302-1-superm1@kernel.org/
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v5:
 * split to multiple commits, re-order
v4:
 * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
v3:
 * Add new PMSG_POWEROFF and PM_EVENT_POWEROFF which alias to poweroff
   callbacks
 * Don't try to cleanup on dpm_suspend_start() or dpm_suspend_end() failures
   Jump right into normal shutdown flow instead.
 * https://lore.kernel.org/linux-pm/20250609024619.407257-1-superm1@kernel.org/T/#me6db0fb946e3d604a8f3d455128844ed802c82bb
---
 kernel/reboot.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/reboot.c b/kernel/reboot.c
index ec087827c85cd..c8835f8e5f271 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -13,6 +13,7 @@
 #include <linux/kexec.h>
 #include <linux/kmod.h>
 #include <linux/kmsg_dump.h>
+#include <linux/pm.h>
 #include <linux/reboot.h>
 #include <linux/suspend.h>
 #include <linux/syscalls.h>
@@ -305,6 +306,11 @@ static void kernel_shutdown_prepare(enum system_states state)
 		(state == SYSTEM_HALT) ? SYS_HALT : SYS_POWER_OFF, NULL);
 	system_state = state;
 	usermodehelper_disable();
+#ifdef CONFIG_HIBERNATE_CALLBACKS
+	if (!dpm_suspend_start(PMSG_POWEROFF) && !dpm_suspend_end(PMSG_POWEROFF))
+		return;
+	pr_emerg("Failed to power off devices, using shutdown instead.\n");
+#endif
 	device_shutdown();
 }
 /**
-- 
2.43.0

[PATCH v7 12/12] Documentation: power: Add document on debugging shutdown hangs

[Mario Limonciello (AMD)]

The kernel will attempt hibernation callbacks before shutdown callbacks.
If there is any problem with this, ideally a UART log should be captured
to debug the problem.  However if one isn't available users can use the
pstore functionality to retrieve logs.  Add a document explaining how
this works.

Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v7:
 * New patch
---
 Documentation/power/index.rst              |  1 +
 Documentation/power/shutdown-debugging.rst | 55 ++++++++++++++++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 Documentation/power/shutdown-debugging.rst

diff --git a/Documentation/power/index.rst b/Documentation/power/index.rst
index a0f5244fb4279..ea70633d9ce6c 100644
--- a/Documentation/power/index.rst
+++ b/Documentation/power/index.rst
@@ -19,6 +19,7 @@ Power Management
     power_supply_class
     runtime_pm
     s2ram
+    shutdown-debugging
     suspend-and-cpuhotplug
     suspend-and-interrupts
     swsusp-and-swap-files
diff --git a/Documentation/power/shutdown-debugging.rst b/Documentation/power/shutdown-debugging.rst
new file mode 100644
index 0000000000000..d4bf12000c1cd
--- /dev/null
+++ b/Documentation/power/shutdown-debugging.rst
@@ -0,0 +1,55 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Debugging Kernel Shutdown Hangs with pstore
++++++++++++++++++++++++++++++++++++++++++++
+
+Overview
+========
+When the system is shut down to either a halt or power off, the kernel will
+attempt to run hibernation calls for all devices. If this fails, the kernel will
+fall back to shutdown callbacks. If this process fails and the system hangs
+the kernel logs will need to be retrieved to debug the issue.
+
+On systems that have a UART available, it is best to configure the kernel to use
+this UART for kernel console output.
+
+If a UART isn't available, the ``pstore`` subsystem provides a mechanism to
+persist this data across a system reset, allowing it to be retrieved on the next
+boot.
+
+Kernel Configuration
+====================
+To enable ``pstore`` and enable saving kernel ring buffer logs, set the
+following kernel configuration options:
+
+* ``CONFIG_PSTORE=y``
+* ``CONFIG_PSTORE_CONSOLE=y``
+
+Additionally, enable a backend to store the data. Depending upon your platform
+some options include:
+
+* ``CONFIG_EFI_VARS_PSTORE=y``
+* ``CONFIG_PSTORE_RAM=y``
+* ``CONFIG_PSTORE_FIRMWARE=y``
+* ``CONFIG_PSTORE_BLK=y``
+
+Kernel Command-line Parameters
+==============================
+Add these parameters to your kernel command line:
+
+* ``printk.always_kmsg_dump=Y``
+	* Forces the kernel to dump the entire message buffer to pstore during
+		shutdown
+* ``efi_pstore.pstore_disable=N``
+	* For EFI-based systems, ensures the EFI backend is active
+
+Userspace Interaction and Log Retrieval
+=======================================
+On the next boot after a hang, pstore logs will be available in the pstore
+filesystem (``/sys/fs/pstore``) and can be retrieved by userspace.
+
+On systemd systems, the ``systemd-pstore`` service will help do the following:
+
+#. Locate pstore data in ``/sys/fs/pstore``
+#. Read and save it to ``/var/lib/systemd/pstore``
+#. Clear pstore data for the next event
-- 
2.43.0

Re: [PATCH v7 02/12] scsi: Add PM_EVENT_POWEROFF into suspend callbacks

[Martin K. Petersen]

Mario,

> When the ACPI core uses hibernation callbacks for shutdown drivers
> will receive PM_EVENT_POWEROFF and should handle it the same as
> PM_EVENT_HIBERNATE would have been used.

No objections from me wrt. the SCSI changes.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen

Re: [PATCH v7 01/12] PM: Introduce new PMSG_POWEROFF event

[Rafael J. Wysocki]

On Tue, Sep 9, 2025 at 9:16 PM Mario Limonciello (AMD)
<superm1@kernel.org> wrote:
>
> PMSG_POWEROFF will be used for the PM core to allow differentiating between
> a hibernation or shutdown sequence when re-using callbacks.
>
> This event should not have wakeups enabled

Why?

It surely is valid to wake up the system while it is being powered
off, especially in the hibernation case.

The "poweroff" transition is generally not recoverable, however, so it
may be better to complete it and trigger a reboot if wakeup has been
signaled.

> so update PMSG_NO_WAKEUP() to match it as well.

No, please.

> Tested-by: Eric Naim <dnaim@cachyos.org>
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
> v7:
>  * Reword commit
> v5:
>  * Re-order and split
>  * Add tags
> v4:
>  * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
> ---
>  drivers/base/power/main.c    | 7 +++++++
>  include/linux/pm.h           | 5 ++++-
>  include/trace/events/power.h | 3 ++-
>  3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 2ea6e05e6ec90..86661c94e8cef 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -99,6 +99,8 @@ static const char *pm_verb(int event)
>                 return "restore";
>         case PM_EVENT_RECOVER:
>                 return "recover";
> +       case PM_EVENT_POWEROFF:
> +               return "poweroff";
>         default:
>                 return "(unknown PM event)";
>         }
> @@ -369,6 +371,7 @@ static pm_callback_t pm_op(const struct dev_pm_ops *ops, pm_message_t state)
>         case PM_EVENT_FREEZE:
>         case PM_EVENT_QUIESCE:
>                 return ops->freeze;
> +       case PM_EVENT_POWEROFF:
>         case PM_EVENT_HIBERNATE:
>                 return ops->poweroff;
>         case PM_EVENT_THAW:
> @@ -403,6 +406,7 @@ static pm_callback_t pm_late_early_op(const struct dev_pm_ops *ops,
>         case PM_EVENT_FREEZE:
>         case PM_EVENT_QUIESCE:
>                 return ops->freeze_late;
> +       case PM_EVENT_POWEROFF:
>         case PM_EVENT_HIBERNATE:
>                 return ops->poweroff_late;
>         case PM_EVENT_THAW:
> @@ -437,6 +441,7 @@ static pm_callback_t pm_noirq_op(const struct dev_pm_ops *ops, pm_message_t stat
>         case PM_EVENT_FREEZE:
>         case PM_EVENT_QUIESCE:
>                 return ops->freeze_noirq;
> +       case PM_EVENT_POWEROFF:
>         case PM_EVENT_HIBERNATE:
>                 return ops->poweroff_noirq;
>         case PM_EVENT_THAW:
> @@ -1370,6 +1375,8 @@ static pm_message_t resume_event(pm_message_t sleep_state)
>                 return PMSG_RECOVER;
>         case PM_EVENT_HIBERNATE:
>                 return PMSG_RESTORE;
> +       case PM_EVENT_POWEROFF:
> +               return PMSG_ON;
>         }
>         return PMSG_ON;
>  }
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index cc7b2dc28574c..892bd93f13dad 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -507,6 +507,7 @@ const struct dev_pm_ops name = { \
>   * RECOVER     Creation of a hibernation image or restoration of the main
>   *             memory contents from a hibernation image has failed, call
>   *             ->thaw() and ->complete() for all devices.
> + * POWEROFF    System will poweroff, call ->poweroff() for all devices.
>   *
>   * The following PM_EVENT_ messages are defined for internal use by
>   * kernel subsystems.  They are never issued by the PM core.
> @@ -537,6 +538,7 @@ const struct dev_pm_ops name = { \
>  #define PM_EVENT_USER          0x0100
>  #define PM_EVENT_REMOTE                0x0200
>  #define PM_EVENT_AUTO          0x0400
> +#define PM_EVENT_POWEROFF      0x0800
>
>  #define PM_EVENT_SLEEP         (PM_EVENT_SUSPEND | PM_EVENT_HIBERNATE)
>  #define PM_EVENT_USER_SUSPEND  (PM_EVENT_USER | PM_EVENT_SUSPEND)
> @@ -551,6 +553,7 @@ const struct dev_pm_ops name = { \
>  #define PMSG_QUIESCE   ((struct pm_message){ .event = PM_EVENT_QUIESCE, })
>  #define PMSG_SUSPEND   ((struct pm_message){ .event = PM_EVENT_SUSPEND, })
>  #define PMSG_HIBERNATE ((struct pm_message){ .event = PM_EVENT_HIBERNATE, })
> +#define PMSG_POWEROFF  ((struct pm_message){ .event = PM_EVENT_POWEROFF, })
>  #define PMSG_RESUME    ((struct pm_message){ .event = PM_EVENT_RESUME, })
>  #define PMSG_THAW      ((struct pm_message){ .event = PM_EVENT_THAW, })
>  #define PMSG_RESTORE   ((struct pm_message){ .event = PM_EVENT_RESTORE, })
> @@ -568,7 +571,7 @@ const struct dev_pm_ops name = { \
>
>  #define PMSG_IS_AUTO(msg)      (((msg).event & PM_EVENT_AUTO) != 0)
>  #define PMSG_NO_WAKEUP(msg)    (((msg).event & \
> -                               (PM_EVENT_FREEZE | PM_EVENT_QUIESCE)) != 0)
> +                               (PM_EVENT_FREEZE | PM_EVENT_QUIESCE | PM_EVENT_POWEROFF)) != 0)
>  /*
>   * Device run-time power management status.
>   *
> diff --git a/include/trace/events/power.h b/include/trace/events/power.h
> index 82904291c2b81..370f8df2fdb4b 100644
> --- a/include/trace/events/power.h
> +++ b/include/trace/events/power.h
> @@ -179,7 +179,8 @@ TRACE_EVENT(pstate_sample,
>                 { PM_EVENT_HIBERNATE, "hibernate" }, \
>                 { PM_EVENT_THAW, "thaw" }, \
>                 { PM_EVENT_RESTORE, "restore" }, \
> -               { PM_EVENT_RECOVER, "recover" })
> +               { PM_EVENT_RECOVER, "recover" }, \
> +               { PM_EVENT_POWEROFF, "poweroff" })
>
>  DEFINE_EVENT(cpu, cpu_frequency,
>
> --
> 2.43.0
>

Re: [PATCH v7 06/12] PCI/PM: Split out code from pci_pm_suspend_noirq() into helper

[Bjorn Helgaas]

On Tue, Sep 09, 2025 at 02:16:13PM -0500, Mario Limonciello (AMD) wrote:
> 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.
> 
> Tested-by: Eric Naim <dnaim@cachyos.org>
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

If you have other reason to repost this, ...

> +	if (pci_dev->current_state == PCI_D0) {
> +		pci_dev->skip_bus_pm = true;

Add a blank line here.

> +		/*
> +		 * 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;
> +	}

Re: [PATCH v7 05/12] PCI/PM: Disable device wakeups when halting or powering off system

[Bjorn Helgaas]

On Tue, Sep 09, 2025 at 02:16:12PM -0500, Mario Limonciello (AMD) wrote:
> PCI devices can be configured as wakeup sources from low power states.
> However, when the system is halting or powering off such wakeups are
> not expected and may lead to spurious behavior.

I'm a little unclear on the nomenclature for these low power states,
so I think it would be helpful to connect to the user action, e.g.,
suspend/hibernate/etc, and the ACPI state, e.g.,

  ... when the system is hibernating (e.g., transitioning to ACPI S4
  and halting) or powering off (e.g., transitioning to ACPI S5 soft
  off), such wakeups are not expected ...

When I suspend or power off my laptop from the GUI user interface, I
want to know if keyboard or mouse activity will resume or if I need to
press the power button.

> ACPI r6.5, section 16.1.5 notes:
> 
>     "Hardware does allow a transition to S0 due to power button press
>      or a Remote Start."

Important to note here that sec 16.1.5 is specifically for "S5 Soft
Off State".

S4 is a sleeping state and presumably sec 16.1.6 ("Transitioning from
the Working to the Sleeping State") applies.  That section mentions
wakeup devices, so it's not obvious to me that PCI device wakeup
should be disabled for S4.

> This implies that wakeups from PCI devices should not be relied upon
> in these states. To align with this expectation and avoid unintended
> wakeups, disable device wakeup capability during these transitions.
> 
> Tested-by: Eric Naim <dnaim@cachyos.org>
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
> v7:
>  * Reword title
>  * Reword commit
> v5:
>  * Re-order
>  * Add tags
> v4:
>  * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
> ---
>  drivers/pci/pci-driver.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 63665240ae87f..f201d298d7173 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1139,6 +1139,10 @@ 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;
>  
> +	if (device_may_wakeup(dev) &&
> +	    (system_state == SYSTEM_HALT || system_state == SYSTEM_POWER_OFF))
> +		device_set_wakeup_enable(dev, false);
> +
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
>  
> -- 
> 2.43.0
> 

Re: [PATCH v7 11/12] PM: Use hibernate flows for system power off

[Bjorn Helgaas]

On Tue, Sep 09, 2025 at 02:16:18PM -0500, Mario Limonciello (AMD) wrote:
> When the system is powered off the kernel will call device_shutdown()
> which will issue callbacks into PCI core to wake up a device and call
> it's shutdown() callback.  This will leave devices in ACPI D0 which can
> cause some devices to misbehave with spurious wakeups and also leave some
> devices on which will consume power needlessly.

The connection between this change and spurious wakeups seems pretty
tenuous.  If we don't want wakeups, powering off the device seems like
a sledgehammer approach.

s/it's/its/

> The issue won't happen if the device is in D3 before system shutdown, so
> putting device to low power state before shutdown solves the issue.
> 
> ACPI Spec 6.5, "7.4.2.5 System \_S4 State" says "Devices states are
> compatible with the current Power Resource states. In other words, all
> devices are in the D3 state when the system state is S4."

Re patch 05/12, also interesting that this section mentions "devices
that are enabled to wake the system and that can do so from their
device state in S4 can initiate a hardware event that transitions the
system state to S0."

So it looks like wakeup from S4 should work in at least some cases.

> The following "7.4.2.6 System \_S5 State (Soft Off)" states "The S5
> state is similar to the S4 state except that OSPM does not save any
> context." so it's safe to assume devices should be at D3 for S5.
> 
> To accomplish this, use the PMSG_POWEROFF event to call all the device
> hibernate callbacks when the kernel is compiled with hibernate support.
> If compiled without hibernate support or hibernate fails fall back into
> the previous shutdown flow.
> 
> Cc: AceLan Kao <acelan.kao@canonical.com>
> Cc: Kai-Heng Feng <kaihengf@nvidia.com>
> Cc: Mark Pearson <mpearson-lenovo@squebb.ca>
> Cc: Merthan Karakaş <m3rthn.k@gmail.com>
> Tested-by: Eric Naim <dnaim@cachyos.org>
> Tested-by: Denis Benato <benato.denis96@gmail.com>
> Link: https://lore.kernel.org/linux-pci/20231213182656.6165-1-mario.limonciello@amd.com/
> Link: https://lore.kernel.org/linux-pci/20250506041934.1409302-1-superm1@kernel.org/
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
> v5:
>  * split to multiple commits, re-order
> v4:
>  * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
> v3:
>  * Add new PMSG_POWEROFF and PM_EVENT_POWEROFF which alias to poweroff
>    callbacks
>  * Don't try to cleanup on dpm_suspend_start() or dpm_suspend_end() failures
>    Jump right into normal shutdown flow instead.
>  * https://lore.kernel.org/linux-pm/20250609024619.407257-1-superm1@kernel.org/T/#me6db0fb946e3d604a8f3d455128844ed802c82bb
> ---
>  kernel/reboot.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index ec087827c85cd..c8835f8e5f271 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -13,6 +13,7 @@
>  #include <linux/kexec.h>
>  #include <linux/kmod.h>
>  #include <linux/kmsg_dump.h>
> +#include <linux/pm.h>
>  #include <linux/reboot.h>
>  #include <linux/suspend.h>
>  #include <linux/syscalls.h>
> @@ -305,6 +306,11 @@ static void kernel_shutdown_prepare(enum system_states state)
>  		(state == SYSTEM_HALT) ? SYS_HALT : SYS_POWER_OFF, NULL);
>  	system_state = state;
>  	usermodehelper_disable();
> +#ifdef CONFIG_HIBERNATE_CALLBACKS
> +	if (!dpm_suspend_start(PMSG_POWEROFF) && !dpm_suspend_end(PMSG_POWEROFF))
> +		return;
> +	pr_emerg("Failed to power off devices, using shutdown instead.\n");
> +#endif
>  	device_shutdown();
>  }
>  /**
> -- 
> 2.43.0
> 

Re: [PATCH v7 11/12] PM: Use hibernate flows for system power off

[Mario Limonciello]

On 9/10/25 10:18 AM, Bjorn Helgaas wrote:
> On Tue, Sep 09, 2025 at 02:16:18PM -0500, Mario Limonciello (AMD) wrote:
>> When the system is powered off the kernel will call device_shutdown()
>> which will issue callbacks into PCI core to wake up a device and call
>> it's shutdown() callback.  This will leave devices in ACPI D0 which can
>> cause some devices to misbehave with spurious wakeups and also leave some
>> devices on which will consume power needlessly.
> 
> The connection between this change and spurious wakeups seems pretty
> tenuous.  If we don't want wakeups, powering off the device seems like
> a sledgehammer approach.

It seems I'm confusing the issue the intent of the series by mentioning 
wakeups here.  The reason that they were mentioned is my series and Kai 
Heng's series merged and they fixed his issue too which AER caused a 
spurious wakeup [1].

My main focus for the series is power consumption.

Link: 
https://lore.kernel.org/linux-pci/20250506041934.1409302-1-superm1@kernel.org/ 
[1]

> 
> s/it's/its/

👍

> 
>> The issue won't happen if the device is in D3 before system shutdown, so
>> putting device to low power state before shutdown solves the issue.
>>
>> ACPI Spec 6.5, "7.4.2.5 System \_S4 State" says "Devices states are
>> compatible with the current Power Resource states. In other words, all
>> devices are in the D3 state when the system state is S4."
> 
> Re patch 05/12, also interesting that this section mentions "devices
> that are enabled to wake the system and that can do so from their
> device state in S4 can initiate a hardware event that transitions the
> system state to S0."
> 
> So it looks like wakeup from S4 should work in at least some cases.

Yes; Wake-ups do work from S4.

> 
>> The following "7.4.2.6 System \_S5 State (Soft Off)" states "The S5
>> state is similar to the S4 state except that OSPM does not save any
>> context." so it's safe to assume devices should be at D3 for S5.
>>
>> To accomplish this, use the PMSG_POWEROFF event to call all the device
>> hibernate callbacks when the kernel is compiled with hibernate support.
>> If compiled without hibernate support or hibernate fails fall back into
>> the previous shutdown flow.
>>
>> Cc: AceLan Kao <acelan.kao@canonical.com>
>> Cc: Kai-Heng Feng <kaihengf@nvidia.com>
>> Cc: Mark Pearson <mpearson-lenovo@squebb.ca>
>> Cc: Merthan Karakaş <m3rthn.k@gmail.com>
>> Tested-by: Eric Naim <dnaim@cachyos.org>
>> Tested-by: Denis Benato <benato.denis96@gmail.com>
>> Link: https://lore.kernel.org/linux-pci/20231213182656.6165-1-mario.limonciello@amd.com/
>> Link: https://lore.kernel.org/linux-pci/20250506041934.1409302-1-superm1@kernel.org/
>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> ---
>> v5:
>>   * split to multiple commits, re-order
>> v4:
>>   * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
>> v3:
>>   * Add new PMSG_POWEROFF and PM_EVENT_POWEROFF which alias to poweroff
>>     callbacks
>>   * Don't try to cleanup on dpm_suspend_start() or dpm_suspend_end() failures
>>     Jump right into normal shutdown flow instead.
>>   * https://lore.kernel.org/linux-pm/20250609024619.407257-1-superm1@kernel.org/T/#me6db0fb946e3d604a8f3d455128844ed802c82bb
>> ---
>>   kernel/reboot.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/kernel/reboot.c b/kernel/reboot.c
>> index ec087827c85cd..c8835f8e5f271 100644
>> --- a/kernel/reboot.c
>> +++ b/kernel/reboot.c
>> @@ -13,6 +13,7 @@
>>   #include <linux/kexec.h>
>>   #include <linux/kmod.h>
>>   #include <linux/kmsg_dump.h>
>> +#include <linux/pm.h>
>>   #include <linux/reboot.h>
>>   #include <linux/suspend.h>
>>   #include <linux/syscalls.h>
>> @@ -305,6 +306,11 @@ static void kernel_shutdown_prepare(enum system_states state)
>>   		(state == SYSTEM_HALT) ? SYS_HALT : SYS_POWER_OFF, NULL);
>>   	system_state = state;
>>   	usermodehelper_disable();
>> +#ifdef CONFIG_HIBERNATE_CALLBACKS
>> +	if (!dpm_suspend_start(PMSG_POWEROFF) && !dpm_suspend_end(PMSG_POWEROFF))
>> +		return;
>> +	pr_emerg("Failed to power off devices, using shutdown instead.\n");
>> +#endif
>>   	device_shutdown();
>>   }
>>   /**
>> -- 
>> 2.43.0
>>

Re: [PATCH v7 05/12] PCI/PM: Disable device wakeups when halting or powering off system

[Mario Limonciello]

On 9/10/25 10:06 AM, Bjorn Helgaas wrote:
> On Tue, Sep 09, 2025 at 02:16:12PM -0500, Mario Limonciello (AMD) wrote:
>> PCI devices can be configured as wakeup sources from low power states.
>> However, when the system is halting or powering off such wakeups are
>> not expected and may lead to spurious behavior.
> 
> I'm a little unclear on the nomenclature for these low power states,
> so I think it would be helpful to connect to the user action, e.g.,
> suspend/hibernate/etc, and the ACPI state, e.g.,
> 
>    ... when the system is hibernating (e.g., transitioning to ACPI S4
>    and halting) or powering off (e.g., transitioning to ACPI S5 soft
>    off), such wakeups are not expected ...

I will try to firm it up in the commit message.  But yes you're getting 
the intent, having a wakeup occur at S5 would be unexpected, and would 
likely change semantics of what people "think" powering off a machine means.

> 
> When I suspend or power off my laptop from the GUI user interface, I
> want to know if keyboard or mouse activity will resume or if I need to
> press the power button.

The way the kernel is set up today you get a single wakeup sysfs file 
for a device and that wakeup file means 3 things:
* abort the process of entering a suspend state or hibernate
* wake up the machine from a suspend state
* wake up the machine from hibernate

> 
>> ACPI r6.5, section 16.1.5 notes:
>>
>>      "Hardware does allow a transition to S0 due to power button press
>>       or a Remote Start."
> 
> Important to note here that sec 16.1.5 is specifically for "S5 Soft
> Off State".
> 
> S4 is a sleeping state and presumably sec 16.1.6 ("Transitioning from
> the Working to the Sleeping State") applies.  That section mentions
> wakeup devices, so it's not obvious to me that PCI device wakeup
> should be disabled for S4.

It actually /shouldn't/ be disabled for S4 - it should only be disabled 
for S5.

Are you implying a bug in the flow?  I didn't think there was one:

During entering hibernate the poweroff() call will have system_state = 
SYSTEM_SUSPEND so wakeups would be enabled.

For powering off the system using hibernate flows poweroff() call would 
have system_state = SYSTEM_HALT or SYSTEM_POWER_OFF.

> 
>> This implies that wakeups from PCI devices should not be relied upon
>> in these states. To align with this expectation and avoid unintended
>> wakeups, disable device wakeup capability during these transitions.
>>
>> Tested-by: Eric Naim <dnaim@cachyos.org>
>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> ---
>> v7:
>>   * Reword title
>>   * Reword commit
>> v5:
>>   * Re-order
>>   * Add tags
>> v4:
>>   * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
>> ---
>>   drivers/pci/pci-driver.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> index 63665240ae87f..f201d298d7173 100644
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -1139,6 +1139,10 @@ 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;
>>   
>> +	if (device_may_wakeup(dev) &&
>> +	    (system_state == SYSTEM_HALT || system_state == SYSTEM_POWER_OFF))
>> +		device_set_wakeup_enable(dev, false);
>> +
>>   	if (pci_has_legacy_pm_support(pci_dev))
>>   		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
>>   
>> -- 
>> 2.43.0
>>

Re: [PATCH v7 06/12] PCI/PM: Split out code from pci_pm_suspend_noirq() into helper

[Mario Limonciello]

On 9/10/25 9:46 AM, Bjorn Helgaas wrote:
> On Tue, Sep 09, 2025 at 02:16:13PM -0500, Mario Limonciello (AMD) wrote:
>> 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.
>>
>> Tested-by: Eric Naim <dnaim@cachyos.org>
>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> If you have other reason to repost this, ...
> 
>> +	if (pci_dev->current_state == PCI_D0) {
>> +		pci_dev->skip_bus_pm = true;
> 
> Add a blank line here.

Ack, thanks.

> 
>> +		/*
>> +		 * 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;
>> +	}

Re: [PATCH v7 05/12] PCI/PM: Disable device wakeups when halting or powering off system

[Bjorn Helgaas]

On Wed, Sep 10, 2025 at 11:52:00AM -0500, Mario Limonciello wrote:
> On 9/10/25 10:06 AM, Bjorn Helgaas wrote:
> > On Tue, Sep 09, 2025 at 02:16:12PM -0500, Mario Limonciello (AMD) wrote:
> > > PCI devices can be configured as wakeup sources from low power states.
> > > However, when the system is halting or powering off such wakeups are
> > > not expected and may lead to spurious behavior.
> > 
> > I'm a little unclear on the nomenclature for these low power states,
> > so I think it would be helpful to connect to the user action, e.g.,
> > suspend/hibernate/etc, and the ACPI state, e.g.,
> > 
> >    ... when the system is hibernating (e.g., transitioning to ACPI S4
> >    and halting) or powering off (e.g., transitioning to ACPI S5 soft
> >    off), such wakeups are not expected ...
> 
> I will try to firm it up in the commit message.  But yes you're getting the
> intent, having a wakeup occur at S5 would be unexpected, and would likely
> change semantics of what people "think" powering off a machine means.
> 
> > When I suspend or power off my laptop from the GUI user interface, I
> > want to know if keyboard or mouse activity will resume or if I need to
> > press the power button.
> 
> The way the kernel is set up today you get a single wakeup sysfs file for a
> device and that wakeup file means 3 things:
> * abort the process of entering a suspend state or hibernate
> * wake up the machine from a suspend state
> * wake up the machine from hibernate
> 
> > > ACPI r6.5, section 16.1.5 notes:
> > > 
> > >      "Hardware does allow a transition to S0 due to power button press
> > >       or a Remote Start."
> > 
> > Important to note here that sec 16.1.5 is specifically for "S5
> > Soft Off State".
> > 
> > S4 is a sleeping state and presumably sec 16.1.6 ("Transitioning
> > from the Working to the Sleeping State") applies.  That section
> > mentions wakeup devices, so it's not obvious to me that PCI device
> > wakeup should be disabled for S4.
> 
> It actually /shouldn't/ be disabled for S4 - it should only be
> disabled for S5.
> 
> Are you implying a bug in the flow?  I didn't think there was one:
> 
> During entering hibernate the poweroff() call will have system_state
> = SYSTEM_SUSPEND so wakeups would be enabled.
> 
> For powering off the system using hibernate flows poweroff() call
> would have system_state = SYSTEM_HALT or SYSTEM_POWER_OFF.

OK.  I assumed that since you check for two states (SYSTEM_HALT or
SYSTEM_POWER_OFF), one must be hibernate (ending up in S4?) and the
other a soft power off (ending up in S5?).

But it sounds like there are two ways to power off.  I'm just confused
about the correspondence between hibernate, soft poweroff, S4, S5,
SYSTEM_HALT, and SYSTEM_POWER_OFF.

*Do* both SYSTEM_HALT and SYSTEM_POWER_OFF lead to S5 on an ACPI
system?  If so, what's the difference between them?

> > > This implies that wakeups from PCI devices should not be relied upon
> > > in these states. To align with this expectation and avoid unintended
> > > wakeups, disable device wakeup capability during these transitions.
> > > 
> > > Tested-by: Eric Naim <dnaim@cachyos.org>
> > > Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> > > ---
> > > v7:
> > >   * Reword title
> > >   * Reword commit
> > > v5:
> > >   * Re-order
> > >   * Add tags
> > > v4:
> > >   * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
> > > ---
> > >   drivers/pci/pci-driver.c | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > index 63665240ae87f..f201d298d7173 100644
> > > --- a/drivers/pci/pci-driver.c
> > > +++ b/drivers/pci/pci-driver.c
> > > @@ -1139,6 +1139,10 @@ 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;
> > > +	if (device_may_wakeup(dev) &&
> > > +	    (system_state == SYSTEM_HALT || system_state == SYSTEM_POWER_OFF))
> > > +		device_set_wakeup_enable(dev, false);
> > > +
> > >   	if (pci_has_legacy_pm_support(pci_dev))
> > >   		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
> > > -- 
> > > 2.43.0
> > > 
> 

Re: [PATCH v7 05/12] PCI/PM: Disable device wakeups when halting or powering off system

[Mario Limonciello]

On 9/10/25 12:11 PM, Bjorn Helgaas wrote:
> On Wed, Sep 10, 2025 at 11:52:00AM -0500, Mario Limonciello wrote:
>> On 9/10/25 10:06 AM, Bjorn Helgaas wrote:
>>> On Tue, Sep 09, 2025 at 02:16:12PM -0500, Mario Limonciello (AMD) wrote:
>>>> PCI devices can be configured as wakeup sources from low power states.
>>>> However, when the system is halting or powering off such wakeups are
>>>> not expected and may lead to spurious behavior.
>>>
>>> I'm a little unclear on the nomenclature for these low power states,
>>> so I think it would be helpful to connect to the user action, e.g.,
>>> suspend/hibernate/etc, and the ACPI state, e.g.,
>>>
>>>     ... when the system is hibernating (e.g., transitioning to ACPI S4
>>>     and halting) or powering off (e.g., transitioning to ACPI S5 soft
>>>     off), such wakeups are not expected ...
>>
>> I will try to firm it up in the commit message.  But yes you're getting the
>> intent, having a wakeup occur at S5 would be unexpected, and would likely
>> change semantics of what people "think" powering off a machine means.
>>
>>> When I suspend or power off my laptop from the GUI user interface, I
>>> want to know if keyboard or mouse activity will resume or if I need to
>>> press the power button.
>>
>> The way the kernel is set up today you get a single wakeup sysfs file for a
>> device and that wakeup file means 3 things:
>> * abort the process of entering a suspend state or hibernate
>> * wake up the machine from a suspend state
>> * wake up the machine from hibernate
>>
>>>> ACPI r6.5, section 16.1.5 notes:
>>>>
>>>>       "Hardware does allow a transition to S0 due to power button press
>>>>        or a Remote Start."
>>>
>>> Important to note here that sec 16.1.5 is specifically for "S5
>>> Soft Off State".
>>>
>>> S4 is a sleeping state and presumably sec 16.1.6 ("Transitioning
>>> from the Working to the Sleeping State") applies.  That section
>>> mentions wakeup devices, so it's not obvious to me that PCI device
>>> wakeup should be disabled for S4.
>>
>> It actually /shouldn't/ be disabled for S4 - it should only be
>> disabled for S5.
>>
>> Are you implying a bug in the flow?  I didn't think there was one:
>>
>> During entering hibernate the poweroff() call will have system_state
>> = SYSTEM_SUSPEND so wakeups would be enabled.
>>
>> For powering off the system using hibernate flows poweroff() call
>> would have system_state = SYSTEM_HALT or SYSTEM_POWER_OFF.
> 
> OK.  I assumed that since you check for two states (SYSTEM_HALT or
> SYSTEM_POWER_OFF), one must be hibernate (ending up in S4?) and the
> other a soft power off (ending up in S5?).
> 
> But it sounds like there are two ways to power off.  I'm just confused
> about the correspondence between hibernate, soft poweroff, S4, S5,
> SYSTEM_HALT, and SYSTEM_POWER_OFF.
> 
> *Do* both SYSTEM_HALT and SYSTEM_POWER_OFF lead to S5 on an ACPI
> system?  If so, what's the difference between them?

The two functions are kernel_halt() and kernel_power_off().

And looking again, Ahhhh!  kernel_power_off() is the only thing that 
actually leads to machine_power_off().  Halt just stops the CPUs.

I think we should only be using the hibernate flows for SYSTEM_POWER_OFF.

This has implications for a lot of the patches.  Thanks a lot for 
pointing this out. I'll walk the series again and change accordingly.

> 
>>>> This implies that wakeups from PCI devices should not be relied upon
>>>> in these states. To align with this expectation and avoid unintended
>>>> wakeups, disable device wakeup capability during these transitions.
>>>>
>>>> Tested-by: Eric Naim <dnaim@cachyos.org>
>>>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>>> ---
>>>> v7:
>>>>    * Reword title
>>>>    * Reword commit
>>>> v5:
>>>>    * Re-order
>>>>    * Add tags
>>>> v4:
>>>>    * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
>>>> ---
>>>>    drivers/pci/pci-driver.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>>>> index 63665240ae87f..f201d298d7173 100644
>>>> --- a/drivers/pci/pci-driver.c
>>>> +++ b/drivers/pci/pci-driver.c
>>>> @@ -1139,6 +1139,10 @@ 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;
>>>> +	if (device_may_wakeup(dev) &&
>>>> +	    (system_state == SYSTEM_HALT || system_state == SYSTEM_POWER_OFF))
>>>> +		device_set_wakeup_enable(dev, false);
>>>> +
>>>>    	if (pci_has_legacy_pm_support(pci_dev))
>>>>    		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
>>>> -- 
>>>> 2.43.0
>>>>
>>

Re: [PATCH v7 06/12] PCI/PM: Split out code from pci_pm_suspend_noirq() into helper

[Rafael J. Wysocki]

On Tue, Sep 9, 2025 at 9:16 PM Mario Limonciello (AMD)
<superm1@kernel.org> wrote:
>
> 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.

You should say why you need/want to unify the suspend and hibernate
codepaths because this is kind of relevant for this patch.

It isn't entirely valid to use suspend-specific code in the
hibernate/power down code paths.

Also, I'd consider reordering the series so that this patch goes
immediately before patch [9/12] in which the new function is used for
the first time.

> Tested-by: Eric Naim <dnaim@cachyos.org>
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
> v7:
>  * Reword title
> v5:
>  * Split from earlier patches
>  * Add tags
> v4:
>  * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
> ---
>  drivers/pci/pci-driver.c | 81 +++++++++++++++++++++++++---------------
>  1 file changed, 51 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index f201d298d7173..571a3809f163a 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -762,6 +762,54 @@ static void pci_pm_complete(struct device *dev)
>
>  #endif /* !CONFIG_PM_SLEEP */
>
> +#if defined(CONFIG_SUSPEND)
> +/**
> + * pci_pm_suspend_noirq_common
> + * @pci_dev: pci device
> + * @skip_bus_pm: pointer to a boolean indicating whether to skip bus PM
> + *
> + * Prepare the device to go into a low power state by saving state and
> + * deciding whether to skip bus PM.
> + *
> + */
> +static void pci_pm_suspend_noirq_common(struct pci_dev *pci_dev, bool *skip_bus_pm)

I guess you want to propagate pci_dev->skip_bus_pm to the parent
bridge if set for pci_dev in the hibernation code path so that bridges
can go into D3hot/cold in that case.

Fair enough, but I'd give the common function a somewhat less neutral
name, like for example pci_pm_noirq_prepare_to_sleep().

> +{
> +       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()) {

And pm_suspend_no_platform() is suspend-specific and in the
hibernation/power down cases it is always false because the platform
is going to take over.

This means that *skip_bus_pm will not be updated in those cases, so
why do you need it in this function in the first place?

> +               pci_dbg(pci_dev, "PCI PM: Skipped\n");
> +               *skip_bus_pm = true;
> +               return;
> +       }
> +
> +       pci_pm_set_unknown_state(pci_dev);

So this should stay outside the common part I think.

> +}
> +#endif /* CONFIG_SUSPEND */
> +
>  #ifdef CONFIG_SUSPEND
>  static void pcie_pme_root_status_cleanup(struct pci_dev *pci_dev)
>  {
> @@ -851,6 +899,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 = false;

And it doesn't look like this variable is necessary after all.

>
>         if (dev_pm_skip_suspend(dev))
>                 return 0;
> @@ -881,38 +930,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);
> -       }
> -
> -       pci_dbg(pci_dev, "PCI PM: Suspend power state: %s\n",
> -               pci_power_name(pci_dev->current_state));
> +       pci_pm_suspend_noirq_common(pci_dev, &skip_bus_pm);
>
> -       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;
> -       }
> -
> -       pci_pm_set_unknown_state(pci_dev);
>
>         /*
>          * Some BIOSes from ASUS have a bug: If a USB EHCI host controller's
> --

Re: [PATCH v7 05/12] PCI/PM: Disable device wakeups when halting or powering off system

[Rafael J. Wysocki]

On Wed, Sep 10, 2025 at 7:24 PM Mario Limonciello <superm1@kernel.org> wrote:
>
> On 9/10/25 12:11 PM, Bjorn Helgaas wrote:
> > On Wed, Sep 10, 2025 at 11:52:00AM -0500, Mario Limonciello wrote:
> >> On 9/10/25 10:06 AM, Bjorn Helgaas wrote:
> >>> On Tue, Sep 09, 2025 at 02:16:12PM -0500, Mario Limonciello (AMD) wrote:
> >>>> PCI devices can be configured as wakeup sources from low power states.
> >>>> However, when the system is halting or powering off such wakeups are
> >>>> not expected and may lead to spurious behavior.
> >>>
> >>> I'm a little unclear on the nomenclature for these low power states,
> >>> so I think it would be helpful to connect to the user action, e.g.,
> >>> suspend/hibernate/etc, and the ACPI state, e.g.,
> >>>
> >>>     ... when the system is hibernating (e.g., transitioning to ACPI S4
> >>>     and halting) or powering off (e.g., transitioning to ACPI S5 soft
> >>>     off), such wakeups are not expected ...
> >>
> >> I will try to firm it up in the commit message.  But yes you're getting the
> >> intent, having a wakeup occur at S5 would be unexpected, and would likely
> >> change semantics of what people "think" powering off a machine means.
> >>
> >>> When I suspend or power off my laptop from the GUI user interface, I
> >>> want to know if keyboard or mouse activity will resume or if I need to
> >>> press the power button.
> >>
> >> The way the kernel is set up today you get a single wakeup sysfs file for a
> >> device and that wakeup file means 3 things:
> >> * abort the process of entering a suspend state or hibernate
> >> * wake up the machine from a suspend state
> >> * wake up the machine from hibernate
> >>
> >>>> ACPI r6.5, section 16.1.5 notes:
> >>>>
> >>>>       "Hardware does allow a transition to S0 due to power button press
> >>>>        or a Remote Start."
> >>>
> >>> Important to note here that sec 16.1.5 is specifically for "S5
> >>> Soft Off State".
> >>>
> >>> S4 is a sleeping state and presumably sec 16.1.6 ("Transitioning
> >>> from the Working to the Sleeping State") applies.  That section
> >>> mentions wakeup devices, so it's not obvious to me that PCI device
> >>> wakeup should be disabled for S4.
> >>
> >> It actually /shouldn't/ be disabled for S4 - it should only be
> >> disabled for S5.
> >>
> >> Are you implying a bug in the flow?  I didn't think there was one:
> >>
> >> During entering hibernate the poweroff() call will have system_state
> >> = SYSTEM_SUSPEND so wakeups would be enabled.
> >>
> >> For powering off the system using hibernate flows poweroff() call
> >> would have system_state = SYSTEM_HALT or SYSTEM_POWER_OFF.
> >
> > OK.  I assumed that since you check for two states (SYSTEM_HALT or
> > SYSTEM_POWER_OFF), one must be hibernate (ending up in S4?) and the
> > other a soft power off (ending up in S5?).
> >
> > But it sounds like there are two ways to power off.  I'm just confused
> > about the correspondence between hibernate, soft poweroff, S4, S5,
> > SYSTEM_HALT, and SYSTEM_POWER_OFF.
> >
> > *Do* both SYSTEM_HALT and SYSTEM_POWER_OFF lead to S5 on an ACPI
> > system?  If so, what's the difference between them?
>
> The two functions are kernel_halt() and kernel_power_off().
>
> And looking again, Ahhhh!  kernel_power_off() is the only thing that
> actually leads to machine_power_off().  Halt just stops the CPUs.
>
> I think we should only be using the hibernate flows for SYSTEM_POWER_OFF.

That's correct.

Re: [PATCH v7 01/12] PM: Introduce new PMSG_POWEROFF event

[Mario Limonciello]

On 9/10/25 8:58 AM, Rafael J. Wysocki wrote:
> On Tue, Sep 9, 2025 at 9:16 PM Mario Limonciello (AMD)
> <superm1@kernel.org> wrote:
>>
>> PMSG_POWEROFF will be used for the PM core to allow differentiating between
>> a hibernation or shutdown sequence when re-using callbacks.
>>
>> This event should not have wakeups enabled
> 
> Why?
> 
> It surely is valid to wake up the system while it is being powered
> off, especially in the hibernation case.

In the hibernation case - yes you want wakeups.

But in what is perceived as powering off the machine it's not expected 
that you would have wakeups.

If I have a USB mouse connected and set as a wakeup source, I can click 
the mouse and the machine wakes up.  A user wouldn't expect that happens 
with a powered off machine.

That's certainly not how it works today at least.
> 
> The "poweroff" transition is generally not recoverable, however, so it
> may be better to complete it and trigger a reboot if wakeup has been
> signaled.

Hmm, I'm not sure about that.  Back to hypothesizing on the USB mouse case:
If I'm entering suspend, the mouse is an enabled wakeup source and I'm 
moving the mouse the suspend should be aborted.

But if I requested the machine to be powered off and and I clicked the 
mouse while powering off that would be an "aborted power off?.
Wouldn't that be really counterintuitive to reboot instead?

IE as a user you expect that pressing the power button gets you an off 
machine, not a different result based on other activity.

> 
>> so update PMSG_NO_WAKEUP() to match it as well.
> 
> No, please.
> 
>> Tested-by: Eric Naim <dnaim@cachyos.org>
>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> ---
>> v7:
>>   * Reword commit
>> v5:
>>   * Re-order and split
>>   * Add tags
>> v4:
>>   * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@kernel.org/
>> ---
>>   drivers/base/power/main.c    | 7 +++++++
>>   include/linux/pm.h           | 5 ++++-
>>   include/trace/events/power.h | 3 ++-
>>   3 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> index 2ea6e05e6ec90..86661c94e8cef 100644
>> --- a/drivers/base/power/main.c
>> +++ b/drivers/base/power/main.c
>> @@ -99,6 +99,8 @@ static const char *pm_verb(int event)
>>                  return "restore";
>>          case PM_EVENT_RECOVER:
>>                  return "recover";
>> +       case PM_EVENT_POWEROFF:
>> +               return "poweroff";
>>          default:
>>                  return "(unknown PM event)";
>>          }
>> @@ -369,6 +371,7 @@ static pm_callback_t pm_op(const struct dev_pm_ops *ops, pm_message_t state)
>>          case PM_EVENT_FREEZE:
>>          case PM_EVENT_QUIESCE:
>>                  return ops->freeze;
>> +       case PM_EVENT_POWEROFF:
>>          case PM_EVENT_HIBERNATE:
>>                  return ops->poweroff;
>>          case PM_EVENT_THAW:
>> @@ -403,6 +406,7 @@ static pm_callback_t pm_late_early_op(const struct dev_pm_ops *ops,
>>          case PM_EVENT_FREEZE:
>>          case PM_EVENT_QUIESCE:
>>                  return ops->freeze_late;
>> +       case PM_EVENT_POWEROFF:
>>          case PM_EVENT_HIBERNATE:
>>                  return ops->poweroff_late;
>>          case PM_EVENT_THAW:
>> @@ -437,6 +441,7 @@ static pm_callback_t pm_noirq_op(const struct dev_pm_ops *ops, pm_message_t stat
>>          case PM_EVENT_FREEZE:
>>          case PM_EVENT_QUIESCE:
>>                  return ops->freeze_noirq;
>> +       case PM_EVENT_POWEROFF:
>>          case PM_EVENT_HIBERNATE:
>>                  return ops->poweroff_noirq;
>>          case PM_EVENT_THAW:
>> @@ -1370,6 +1375,8 @@ static pm_message_t resume_event(pm_message_t sleep_state)
>>                  return PMSG_RECOVER;
>>          case PM_EVENT_HIBERNATE:
>>                  return PMSG_RESTORE;
>> +       case PM_EVENT_POWEROFF:
>> +               return PMSG_ON;
>>          }
>>          return PMSG_ON;
>>   }
>> diff --git a/include/linux/pm.h b/include/linux/pm.h
>> index cc7b2dc28574c..892bd93f13dad 100644
>> --- a/include/linux/pm.h
>> +++ b/include/linux/pm.h
>> @@ -507,6 +507,7 @@ const struct dev_pm_ops name = { \
>>    * RECOVER     Creation of a hibernation image or restoration of the main
>>    *             memory contents from a hibernation image has failed, call
>>    *             ->thaw() and ->complete() for all devices.
>> + * POWEROFF    System will poweroff, call ->poweroff() for all devices.
>>    *
>>    * The following PM_EVENT_ messages are defined for internal use by
>>    * kernel subsystems.  They are never issued by the PM core.
>> @@ -537,6 +538,7 @@ const struct dev_pm_ops name = { \
>>   #define PM_EVENT_USER          0x0100
>>   #define PM_EVENT_REMOTE                0x0200
>>   #define PM_EVENT_AUTO          0x0400
>> +#define PM_EVENT_POWEROFF      0x0800
>>
>>   #define PM_EVENT_SLEEP         (PM_EVENT_SUSPEND | PM_EVENT_HIBERNATE)
>>   #define PM_EVENT_USER_SUSPEND  (PM_EVENT_USER | PM_EVENT_SUSPEND)
>> @@ -551,6 +553,7 @@ const struct dev_pm_ops name = { \
>>   #define PMSG_QUIESCE   ((struct pm_message){ .event = PM_EVENT_QUIESCE, })
>>   #define PMSG_SUSPEND   ((struct pm_message){ .event = PM_EVENT_SUSPEND, })
>>   #define PMSG_HIBERNATE ((struct pm_message){ .event = PM_EVENT_HIBERNATE, })
>> +#define PMSG_POWEROFF  ((struct pm_message){ .event = PM_EVENT_POWEROFF, })
>>   #define PMSG_RESUME    ((struct pm_message){ .event = PM_EVENT_RESUME, })
>>   #define PMSG_THAW      ((struct pm_message){ .event = PM_EVENT_THAW, })
>>   #define PMSG_RESTORE   ((struct pm_message){ .event = PM_EVENT_RESTORE, })
>> @@ -568,7 +571,7 @@ const struct dev_pm_ops name = { \
>>
>>   #define PMSG_IS_AUTO(msg)      (((msg).event & PM_EVENT_AUTO) != 0)
>>   #define PMSG_NO_WAKEUP(msg)    (((msg).event & \
>> -                               (PM_EVENT_FREEZE | PM_EVENT_QUIESCE)) != 0)
>> +                               (PM_EVENT_FREEZE | PM_EVENT_QUIESCE | PM_EVENT_POWEROFF)) != 0)
>>   /*
>>    * Device run-time power management status.
>>    *
>> diff --git a/include/trace/events/power.h b/include/trace/events/power.h
>> index 82904291c2b81..370f8df2fdb4b 100644
>> --- a/include/trace/events/power.h
>> +++ b/include/trace/events/power.h
>> @@ -179,7 +179,8 @@ TRACE_EVENT(pstate_sample,
>>                  { PM_EVENT_HIBERNATE, "hibernate" }, \
>>                  { PM_EVENT_THAW, "thaw" }, \
>>                  { PM_EVENT_RESTORE, "restore" }, \
>> -               { PM_EVENT_RECOVER, "recover" })
>> +               { PM_EVENT_RECOVER, "recover" }, \
>> +               { PM_EVENT_POWEROFF, "poweroff" })
>>
>>   DEFINE_EVENT(cpu, cpu_frequency,
>>
>> --
>> 2.43.0
>>

Re: [PATCH v7 07/12] PCI/PM: Run bridge power up actions as part of restore phase

[Rafael J. Wysocki]

On Tue, Sep 9, 2025 at 9:16 PM Mario Limonciello (AMD)
<superm1@kernel.org> wrote:
>
> Suspend resume actions will check the state of the device and whether
> bus PM should be skipped.  These same actions make sense during hibernation
> image restore.

Not really (see below).

They kind of would have made sense in the error code path attempting
to roll back the power-off transition, but I'm not sure if this is
worth the hassle because it would require ->restore() to be able to
handle two different cases without knowing which case it is handling.

>  Apply them there as well.
>
> Tested-by: Eric Naim <dnaim@cachyos.org>
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
> v7:
>  * Reword title
> v5:
>  * Split out patch
> ---
>  drivers/pci/pci-driver.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 571a3809f163a..fb6f1f60b2f1f 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1246,10 +1246,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;

In the hibernation restore case, pci_dev->current_state is irrelevant
because the system has gone through the entire init in the BIOS and
boot loader, and the boot (restore) kernel before the control goes
back to the image kernel which runs the ->restore() callbacks.  It may
have changed three times since it was set during power-off.

> +       bool skip_bus_pm = pci_dev->skip_bus_pm;

This one is irrelevant either in that case.

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

Re: [PATCH v7 01/12] PM: Introduce new PMSG_POWEROFF event

[Rafael J. Wysocki]

On Wed, Sep 10, 2025 at 7:48 PM Mario Limonciello <superm1@kernel.org> wrote:
>
> On 9/10/25 8:58 AM, Rafael J. Wysocki wrote:
> > On Tue, Sep 9, 2025 at 9:16 PM Mario Limonciello (AMD)
> > <superm1@kernel.org> wrote:
> >>
> >> PMSG_POWEROFF will be used for the PM core to allow differentiating between
> >> a hibernation or shutdown sequence when re-using callbacks.
> >>
> >> This event should not have wakeups enabled
> >
> > Why?
> >
> > It surely is valid to wake up the system while it is being powered
> > off, especially in the hibernation case.
>
> In the hibernation case - yes you want wakeups.
>
> But in what is perceived as powering off the machine it's not expected
> that you would have wakeups.
>
> If I have a USB mouse connected and set as a wakeup source, I can click
> the mouse and the machine wakes up.  A user wouldn't expect that happens
> with a powered off machine.
>
> That's certainly not how it works today at least.

Sure, but you want to use the same transition mechanism to handle
those two cases.  In one of them, you don't want wakeup events to be
disabled, so you cannot do that at the transition type level.

> >
> > The "poweroff" transition is generally not recoverable, however, so it
> > may be better to complete it and trigger a reboot if wakeup has been
> > signaled.
>
> Hmm, I'm not sure about that.  Back to hypothesizing on the USB mouse case:
> If I'm entering suspend, the mouse is an enabled wakeup source and I'm
> moving the mouse the suspend should be aborted.
>
> But if I requested the machine to be powered off and and I clicked the
> mouse while powering off that would be an "aborted power off?.
> Wouldn't that be really counterintuitive to reboot instead?

I was talking about the last stage of hibernation, sorry if that
wasn't clear enough.

So in the last stage of hibernation, which also carries out a
"power-off" transition (if run in the "platform" mode that is), you
want wakeups to be enabled, don't you?

But the "power-off" transition is not recoverable in general (IOW, it
cannot be reliably rolled back, at least as of today), it may be
better to complete it and make the kernel load the just created
hibernation image and restore from it in response to a wakeup event.
Something like the latter is already done in the "test_resume"
hibernation mode.

> IE as a user you expect that pressing the power button gets you an off
> machine, not a different result based on other activity.

That's a slightly different case, but if you press the power button
once while hibernation is in progress, you may actually expect to get
back to the pre-hibernation state (or at least hope that this is going
to happen).

Re: [PATCH v7 00/12] Improvements to S5 power consumption

[Rafael J. Wysocki]

Hi Mario,

On Tue, Sep 9, 2025 at 9:16 PM Mario Limonciello (AMD)
<superm1@kernel.org> wrote:
>
> A variety of issues both in function and in power consumption have been
> raised as a result of devices not being put into a low power state when
> the system is powered off.
>
> There have been some localized changes[1] to PCI core to help these issues,
> but they have had various downsides.
>
> This series instead uses the driver hibernate flows when the system is
> being powered off or halted.  This lines up the behavior with what other
> operating systems do as well.  If for some reason that fails or is not
> supported, run driver shutdown() callbacks.
>
> Rafael did mention in earlier versions of the series concerns about
> regression risk.  He was looking for thoughts from Greg who isn't against
> it but also isn't sure about how to maintain it. [1]
>
> This has been validated by me and several others in AMD
> on a variety of AMD hardware platforms. It's been validated by some
> community members on their Intel hardware. To my knowledge it has not
> been validated on non-x86.

Still, the patches need more work (see my replies to the relevant patches).

> On my development laptop I have also contrived failures in the hibernation
> callbacks to make sure that the fallback to shutdown callback works.
>
> In order to assist with potential regressions the series also includes
> documentation to help with getting a kernel log at shutdown after
> the disk is unmounted.
>
> Cc: AceLan Kao <acelan.kao@canonical.com>
> Cc: Kai-Heng Feng <kaihengf@nvidia.com>
> Cc: Mark Pearson <mpearson-lenovo@squebb.ca>
> Cc: Merthan Karakaş <m3rthn.k@gmail.com>
> Cc: Eric Naim <dnaim@cachyos.org>
> Link: https://lore.kernel.org/linux-usb/2025090852-coma-tycoon-9f37@gregkh/ [1]
> ---
> v6->v7:
>  * Add documentation on how to debug a shutdown hang
>  * Adjust commit messages per feedback from Bjorn
>
> Mario Limonciello (AMD) (12):
>   PM: Introduce new PMSG_POWEROFF event
>   scsi: Add PM_EVENT_POWEROFF into suspend callbacks
>   usb: sl811-hcd: Add PM_EVENT_POWEROFF into suspend callbacks
>   USB: Pass PMSG_POWEROFF event to suspend_common()
>   PCI/PM: Disable device wakeups when halting or powering off system
>   PCI/PM: Split out code from pci_pm_suspend_noirq() into helper
>   PCI/PM: Run bridge power up actions as part of restore phase
>   PCI/PM: Use pci_power_manageable() in pci_pm_poweroff_noirq()
>   PCI: Put PCIe bridges with downstream devices into D3 at hibernate
>   drm/amd: Avoid evicting resources at S5
>   PM: Use hibernate flows for system power off
>   Documentation: power: Add document on debugging shutdown hangs

If I were you, I'd split this series into 3 parts.

The first part would be the addition of PMSG_POWEROFF just for
hibernation, which should not be objectionable (the first 4 patches
above).

The next one would be changes to allow PCI bridges to go into
D3hot/cold during the last stage of hibernation (the "power-off"
transition).  This can be justified by itself even before starting to
use the same power-off flow for the last stage of hibernation and for
system power-down.

The last one would be the hibernation/power-down integration.

Each of the above can be posted separately and arguably you need to
get the first part in before the other two and the second part in
before the third one, preferably not in the same cycle.

This way, if there are any regressions in the first two parts, there
will be at least some time to address them before the last part goes
in.

Thanks!

Re: [PATCH v7 00/12] Improvements to S5 power consumption

[Mario Limonciello]

On 9/10/25 1:11 PM, Rafael J. Wysocki wrote:
> Hi Mario,
> 
> On Tue, Sep 9, 2025 at 9:16 PM Mario Limonciello (AMD)
> <superm1@kernel.org> wrote:
>>
>> A variety of issues both in function and in power consumption have been
>> raised as a result of devices not being put into a low power state when
>> the system is powered off.
>>
>> There have been some localized changes[1] to PCI core to help these issues,
>> but they have had various downsides.
>>
>> This series instead uses the driver hibernate flows when the system is
>> being powered off or halted.  This lines up the behavior with what other
>> operating systems do as well.  If for some reason that fails or is not
>> supported, run driver shutdown() callbacks.
>>
>> Rafael did mention in earlier versions of the series concerns about
>> regression risk.  He was looking for thoughts from Greg who isn't against
>> it but also isn't sure about how to maintain it. [1]
>>
>> This has been validated by me and several others in AMD
>> on a variety of AMD hardware platforms. It's been validated by some
>> community members on their Intel hardware. To my knowledge it has not
>> been validated on non-x86.
> 
> Still, the patches need more work (see my replies to the relevant patches).

Yes, thanks for the review.
> 
>> On my development laptop I have also contrived failures in the hibernation
>> callbacks to make sure that the fallback to shutdown callback works.
>>
>> In order to assist with potential regressions the series also includes
>> documentation to help with getting a kernel log at shutdown after
>> the disk is unmounted.
>>
>> Cc: AceLan Kao <acelan.kao@canonical.com>
>> Cc: Kai-Heng Feng <kaihengf@nvidia.com>
>> Cc: Mark Pearson <mpearson-lenovo@squebb.ca>
>> Cc: Merthan Karakaş <m3rthn.k@gmail.com>
>> Cc: Eric Naim <dnaim@cachyos.org>
>> Link: https://lore.kernel.org/linux-usb/2025090852-coma-tycoon-9f37@gregkh/ [1]
>> ---
>> v6->v7:
>>   * Add documentation on how to debug a shutdown hang
>>   * Adjust commit messages per feedback from Bjorn
>>
>> Mario Limonciello (AMD) (12):
>>    PM: Introduce new PMSG_POWEROFF event
>>    scsi: Add PM_EVENT_POWEROFF into suspend callbacks
>>    usb: sl811-hcd: Add PM_EVENT_POWEROFF into suspend callbacks
>>    USB: Pass PMSG_POWEROFF event to suspend_common()
>>    PCI/PM: Disable device wakeups when halting or powering off system
>>    PCI/PM: Split out code from pci_pm_suspend_noirq() into helper
>>    PCI/PM: Run bridge power up actions as part of restore phase
>>    PCI/PM: Use pci_power_manageable() in pci_pm_poweroff_noirq()
>>    PCI: Put PCIe bridges with downstream devices into D3 at hibernate
>>    drm/amd: Avoid evicting resources at S5
>>    PM: Use hibernate flows for system power off
>>    Documentation: power: Add document on debugging shutdown hangs
> 
> If I were you, I'd split this series into 3 parts.
> 
> The first part would be the addition of PMSG_POWEROFF just for
> hibernation, which should not be objectionable (the first 4 patches
> above).
> 
> The next one would be changes to allow PCI bridges to go into
> D3hot/cold during the last stage of hibernation (the "power-off"
> transition).  This can be justified by itself even before starting to
> use the same power-off flow for the last stage of hibernation and for
> system power-down.
> 
> The last one would be the hibernation/power-down integration.
> 
> Each of the above can be posted separately and arguably you need to
> get the first part in before the other two and the second part in
> before the third one, preferably not in the same cycle.
> 
> This way, if there are any regressions in the first two parts, there
> will be at least some time to address them before the last part goes
> in.
> 
> Thanks!

Thanks for this proposal.

I do like the idea of splitting it in 3 parts to give time for 
regression control.

It's getting close to the end of this cycle, would you be opposed to a 
re-spun first 4 patches for 6.18?

Thanks,

Re: [PATCH v7 00/12] Improvements to S5 power consumption

[Rafael J. Wysocki]

On Wed, Sep 10, 2025 at 8:19 PM Mario Limonciello <superm1@kernel.org> wrote:
>
> On 9/10/25 1:11 PM, Rafael J. Wysocki wrote:
> > Hi Mario,
> >
> > On Tue, Sep 9, 2025 at 9:16 PM Mario Limonciello (AMD)
> > <superm1@kernel.org> wrote:
> >>
> >> A variety of issues both in function and in power consumption have been
> >> raised as a result of devices not being put into a low power state when
> >> the system is powered off.
> >>
> >> There have been some localized changes[1] to PCI core to help these issues,
> >> but they have had various downsides.
> >>
> >> This series instead uses the driver hibernate flows when the system is
> >> being powered off or halted.  This lines up the behavior with what other
> >> operating systems do as well.  If for some reason that fails or is not
> >> supported, run driver shutdown() callbacks.
> >>
> >> Rafael did mention in earlier versions of the series concerns about
> >> regression risk.  He was looking for thoughts from Greg who isn't against
> >> it but also isn't sure about how to maintain it. [1]
> >>
> >> This has been validated by me and several others in AMD
> >> on a variety of AMD hardware platforms. It's been validated by some
> >> community members on their Intel hardware. To my knowledge it has not
> >> been validated on non-x86.
> >
> > Still, the patches need more work (see my replies to the relevant patches).
>
> Yes, thanks for the review.
> >
> >> On my development laptop I have also contrived failures in the hibernation
> >> callbacks to make sure that the fallback to shutdown callback works.
> >>
> >> In order to assist with potential regressions the series also includes
> >> documentation to help with getting a kernel log at shutdown after
> >> the disk is unmounted.
> >>
> >> Cc: AceLan Kao <acelan.kao@canonical.com>
> >> Cc: Kai-Heng Feng <kaihengf@nvidia.com>
> >> Cc: Mark Pearson <mpearson-lenovo@squebb.ca>
> >> Cc: Merthan Karakaş <m3rthn.k@gmail.com>
> >> Cc: Eric Naim <dnaim@cachyos.org>
> >> Link: https://lore.kernel.org/linux-usb/2025090852-coma-tycoon-9f37@gregkh/ [1]
> >> ---
> >> v6->v7:
> >>   * Add documentation on how to debug a shutdown hang
> >>   * Adjust commit messages per feedback from Bjorn
> >>
> >> Mario Limonciello (AMD) (12):
> >>    PM: Introduce new PMSG_POWEROFF event
> >>    scsi: Add PM_EVENT_POWEROFF into suspend callbacks
> >>    usb: sl811-hcd: Add PM_EVENT_POWEROFF into suspend callbacks
> >>    USB: Pass PMSG_POWEROFF event to suspend_common()
> >>    PCI/PM: Disable device wakeups when halting or powering off system
> >>    PCI/PM: Split out code from pci_pm_suspend_noirq() into helper
> >>    PCI/PM: Run bridge power up actions as part of restore phase
> >>    PCI/PM: Use pci_power_manageable() in pci_pm_poweroff_noirq()
> >>    PCI: Put PCIe bridges with downstream devices into D3 at hibernate
> >>    drm/amd: Avoid evicting resources at S5
> >>    PM: Use hibernate flows for system power off
> >>    Documentation: power: Add document on debugging shutdown hangs
> >
> > If I were you, I'd split this series into 3 parts.
> >
> > The first part would be the addition of PMSG_POWEROFF just for
> > hibernation, which should not be objectionable (the first 4 patches
> > above).
> >
> > The next one would be changes to allow PCI bridges to go into
> > D3hot/cold during the last stage of hibernation (the "power-off"
> > transition).  This can be justified by itself even before starting to
> > use the same power-off flow for the last stage of hibernation and for
> > system power-down.
> >
> > The last one would be the hibernation/power-down integration.
> >
> > Each of the above can be posted separately and arguably you need to
> > get the first part in before the other two and the second part in
> > before the third one, preferably not in the same cycle.
> >
> > This way, if there are any regressions in the first two parts, there
> > will be at least some time to address them before the last part goes
> > in.
> >
> > Thanks!
>
> Thanks for this proposal.
>
> I do like the idea of splitting it in 3 parts to give time for
> regression control.
>
> It's getting close to the end of this cycle, would you be opposed to a
> re-spun first 4 patches for 6.18?

No, I wouldn't.

I think that they have been reviewed already.