[PATCH v4 0/5] Improvements to S5 power consumption

[PATCH v4 0/5] Improvements to S5 power consumption

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

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 tries to use the S4 flow when the system is being
powered off.  This lines up the behavior with what other operating systems
do as well.  If for some reason that fails or is not supported, unwind and
do the previous S5 flow that will wake all devices and run their shutdown()
callbacks.

v3->v4:
 * Fix LKP robot failure
 * Rebase on v6.16-rc2

Previous submissions [1]:
Link: https://lore.kernel.org/linux-pm/CAJZ5v0hrKEJa8Ad7iiAvQ3d_0ysVhzZcXSYc5kkL=6vtseF+bg@mail.gmail.com/T/#m91e4eae868a7405ae579e89b135085f4906225d2
Link: https://lore.kernel.org/linux-pci/20250506041934.1409302-1-superm1@kernel.org/
Link: https://lore.kernel.org/linux-pci/20231213182656.6165-1-mario.limonciello@amd.com/ (v1)
Link: https://lore.kernel.org/linux-pm/20250514193406.3998101-1-superm1@kernel.org/ (v2)
Link: https://lore.kernel.org/linux-pm/20250609024619.407257-1-superm1@kernel.org/ (v3)

Mario Limonciello (5):
  PM: Use hibernate flows for system power off
  PCI: Put PCIe ports with downstream devices into D3 at hibernate
  drm/amd: Avoid evicting resources at S5
  scsi: Add PM_EVENT_POWEROFF into suspend callbacks
  usb: sl811-hcd: Add PM_EVENT_POWEROFF into suspend callbacks

 drivers/base/power/main.c                  |  7 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  4 +
 drivers/pci/pci-driver.c                   | 94 ++++++++++++++--------
 drivers/scsi/mesh.c                        |  1 +
 drivers/scsi/stex.c                        |  1 +
 drivers/usb/host/sl811-hcd.c               |  1 +
 include/linux/pm.h                         |  3 +
 include/trace/events/power.h               |  3 +-
 kernel/reboot.c                            |  6 ++
 9 files changed, 86 insertions(+), 34 deletions(-)

-- 
2.43.0

[PATCH v4 1/5] PM: Use hibernate flows for system power off

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

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, introduce a new PMSG_POWEROFF event that the PM core
will use. During shutdown the use the new 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: 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 <mario.limonciello@amd.com>
---
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
---
 drivers/base/power/main.c    | 7 +++++++
 include/linux/pm.h           | 3 +++
 include/trace/events/power.h | 3 ++-
 kernel/reboot.c              | 6 ++++++
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index bf77d28e959fd..4970a804afb6d 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -85,6 +85,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)";
 	}
@@ -355,6 +357,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:
@@ -389,6 +392,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:
@@ -423,6 +427,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:
@@ -1297,6 +1302,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 f0bd8fbae4f2c..cb66f47631a70 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -506,6 +506,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.
@@ -536,6 +537,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)
@@ -550,6 +552,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, })
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index 6c631eec23e32..8fa70f2397379 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h
@@ -199,7 +199,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,
 
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 v4 2/5] PCI: Put PCIe ports with downstream devices into D3 at hibernate

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

For the suspend flow PCIe ports that have downstream devices are put into
the appropriate D3 state when children are not in D0. For the hibernate
flow, PCIe ports with downstream devices stay in D0 however. This can
lead to PCIe ports that are remained powered on needlessly during
hibernate.

Adjust the pci_pm_poweroff_noirq() to follow the same flow as
pci_pm_suspend_noirq() in that PCIe ports that are power manageable should
without downstream devices in D0 should be put into their appropriate
sleep state.

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>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v4:
 * Use helper even when CONFIG_SUSPEND not set (LKP robot)
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 | 94 ++++++++++++++++++++++++++--------------
 1 file changed, 61 insertions(+), 33 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 0d4c67829958b..f7a0c23515718 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -759,6 +759,56 @@ static void pci_pm_complete(struct device *dev)
 
 #endif /* !CONFIG_PM_SLEEP */
 
+#if defined(CONFIG_SUSPEND) || defined(CONFIG_HIBERNATE_CALLBACKS)
+/**
+ * pci_pm_set_prepare_bus_pm
+ * @pci_dev: pci device
+ *
+ * Prepare the device to go into a low power state by saving state
+ * and configure bus PM policy.
+ *
+ * Return: TRUE for bus PM will be used
+ *         FALSE for bus PM will be skipped
+ */
+static bool pci_pm_set_prepare_bus_pm(struct pci_dev *pci_dev)
+{
+	if (!pci_dev->state_saved) {
+		pci_save_state(pci_dev);
+
+		/*
+		 * If the device is a bridge with a child in D0 below it,
+		 * it needs to stay in D0, so check skip_bus_pm to avoid
+		 * putting it into a low-power state in that case.
+		 */
+		if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
+			pci_prepare_to_sleep(pci_dev);
+	}
+
+	pci_dbg(pci_dev, "PCI PM: Sleep power state: %s\n",
+		pci_power_name(pci_dev->current_state));
+
+	if (pci_dev->current_state == PCI_D0) {
+		pci_dev->skip_bus_pm = true;
+		/*
+		 * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
+		 * downstream device is in D0, so avoid changing the power state
+		 * of the parent bridge by setting the skip_bus_pm flag for it.
+		 */
+		if (pci_dev->bus->self)
+			pci_dev->bus->self->skip_bus_pm = true;
+	}
+
+	if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) {
+		pci_dbg(pci_dev, "PCI PM: Skipped\n");
+		return FALSE;
+	}
+
+	pci_pm_set_unknown_state(pci_dev);
+
+	return TRUE;
+}
+#endif /* CONFIG_SUSPEND || CONFIG_HIBERNATE_CALLBACKS */
+
 #ifdef CONFIG_SUSPEND
 static void pcie_pme_root_status_cleanup(struct pci_dev *pci_dev)
 {
@@ -878,38 +928,8 @@ 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));
-
-	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 (!pci_pm_set_prepare_bus_pm(pci_dev))
 		goto Fixup;
-	}
-
-	pci_pm_set_unknown_state(pci_dev);
 
 	/*
 	 * Some BIOSes from ASUS have a bug: If a USB EHCI host controller's
@@ -1136,6 +1156,8 @@ static int pci_pm_poweroff(struct device *dev)
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
+	pci_dev->skip_bus_pm = false;
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
 
@@ -1199,8 +1221,8 @@ static int pci_pm_poweroff_noirq(struct device *dev)
 			return error;
 	}
 
-	if (!pci_dev->state_saved && !pci_has_subordinate(pci_dev))
-		pci_prepare_to_sleep(pci_dev);
+	if (!pci_pm_set_prepare_bus_pm(pci_dev))
+		goto Fixup;
 
 	/*
 	 * The reason for doing this here is the same as for the analogous code
@@ -1209,6 +1231,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;
@@ -1218,10 +1241,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 v4 3/5] drm/amd: Avoid evicting resources at S5

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

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>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v3: https://lore.kernel.org/linux-pm/20250609024619.407257-1-superm1@kernel.org/T/#me6db0fb946e3d604a8f3d455128844ed802c82bb
---
 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 8edd88328749b..c5d8f6d551238 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4966,6 +4966,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)
 		DRM_WARN("evicting device resources failed\n");
-- 
2.43.0

[PATCH v4 4/5] scsi: Add PM_EVENT_POWEROFF into suspend callbacks

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

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.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v3:
 * New patch
 * https://lore.kernel.org/linux-pm/20250609024619.407257-1-superm1@kernel.org/T/#me6db0fb946e3d604a8f3d455128844ed802c82bb
---
 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 v4 5/5] usb: sl811-hcd: Add PM_EVENT_POWEROFF into suspend callbacks

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

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.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v3: https://lore.kernel.org/linux-pm/20250609024619.407257-1-superm1@kernel.org/T/#me6db0fb946e3d604a8f3d455128844ed802c82bb
---
 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

Re: [PATCH v4 0/5] Improvements to S5 power consumption

[Rafael J. Wysocki]

On Mon, Jun 16, 2025 at 7:50 PM Mario Limonciello <superm1@kernel.org> wrote:
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> 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 tries to use the S4 flow when the system is being
> powered off.  This lines up the behavior with what other operating systems
> do as well.  If for some reason that fails or is not supported, unwind and
> do the previous S5 flow that will wake all devices and run their shutdown()
> callbacks.

I actually like this approach, but I think that it is risky.

It also requires more work/review from other people.

I'll be sending some comments on the individual patches going forward,
but I think the earliest it can go in is after 6.17-rc1 (given it is
reviewed properly till then).

Thanks!

> v3->v4:
>  * Fix LKP robot failure
>  * Rebase on v6.16-rc2
>
> Previous submissions [1]:
> Link: https://lore.kernel.org/linux-pm/CAJZ5v0hrKEJa8Ad7iiAvQ3d_0ysVhzZcXSYc5kkL=6vtseF+bg@mail.gmail.com/T/#m91e4eae868a7405ae579e89b135085f4906225d2
> Link: https://lore.kernel.org/linux-pci/20250506041934.1409302-1-superm1@kernel.org/
> Link: https://lore.kernel.org/linux-pci/20231213182656.6165-1-mario.limonciello@amd.com/ (v1)
> Link: https://lore.kernel.org/linux-pm/20250514193406.3998101-1-superm1@kernel.org/ (v2)
> Link: https://lore.kernel.org/linux-pm/20250609024619.407257-1-superm1@kernel.org/ (v3)
>
> Mario Limonciello (5):
>   PM: Use hibernate flows for system power off
>   PCI: Put PCIe ports with downstream devices into D3 at hibernate
>   drm/amd: Avoid evicting resources at S5
>   scsi: Add PM_EVENT_POWEROFF into suspend callbacks
>   usb: sl811-hcd: Add PM_EVENT_POWEROFF into suspend callbacks
>
>  drivers/base/power/main.c                  |  7 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  4 +
>  drivers/pci/pci-driver.c                   | 94 ++++++++++++++--------
>  drivers/scsi/mesh.c                        |  1 +
>  drivers/scsi/stex.c                        |  1 +
>  drivers/usb/host/sl811-hcd.c               |  1 +
>  include/linux/pm.h                         |  3 +
>  include/trace/events/power.h               |  3 +-
>  kernel/reboot.c                            |  6 ++
>  9 files changed, 86 insertions(+), 34 deletions(-)
>
> --
> 2.43.0
>

Re: [PATCH v4 0/5] Improvements to S5 power consumption

[Mario Limonciello]

On 7/3/25 10:29, Rafael J. Wysocki wrote:
> On Mon, Jun 16, 2025 at 7:50 PM Mario Limonciello <superm1@kernel.org> wrote:
>>
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> 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 tries to use the S4 flow when the system is being
>> powered off.  This lines up the behavior with what other operating systems
>> do as well.  If for some reason that fails or is not supported, unwind and
>> do the previous S5 flow that will wake all devices and run their shutdown()
>> callbacks.
> 
> I actually like this approach, but I think that it is risky.

Yeah; it does touch a lot of areas.

> 
> It also requires more work/review from other people.

I view patches 3-5 as being dependent upon patch 1, so until we have 
agreement on that one the others might not make a lot of sense.

Bjorn,

Can you take a look at patch 2?

> 
> I'll be sending some comments on the individual patches going forward,
> but I think the earliest it can go in is after 6.17-rc1 (given it is
> reviewed properly till then).

Thanks!

> 
> Thanks!
> 
>> v3->v4:
>>   * Fix LKP robot failure
>>   * Rebase on v6.16-rc2
>>
>> Previous submissions [1]:
>> Link: https://lore.kernel.org/linux-pm/CAJZ5v0hrKEJa8Ad7iiAvQ3d_0ysVhzZcXSYc5kkL=6vtseF+bg@mail.gmail.com/T/#m91e4eae868a7405ae579e89b135085f4906225d2
>> Link: https://lore.kernel.org/linux-pci/20250506041934.1409302-1-superm1@kernel.org/
>> Link: https://lore.kernel.org/linux-pci/20231213182656.6165-1-mario.limonciello@amd.com/ (v1)
>> Link: https://lore.kernel.org/linux-pm/20250514193406.3998101-1-superm1@kernel.org/ (v2)
>> Link: https://lore.kernel.org/linux-pm/20250609024619.407257-1-superm1@kernel.org/ (v3)
>>
>> Mario Limonciello (5):
>>    PM: Use hibernate flows for system power off
>>    PCI: Put PCIe ports with downstream devices into D3 at hibernate
>>    drm/amd: Avoid evicting resources at S5
>>    scsi: Add PM_EVENT_POWEROFF into suspend callbacks
>>    usb: sl811-hcd: Add PM_EVENT_POWEROFF into suspend callbacks
>>
>>   drivers/base/power/main.c                  |  7 ++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  4 +
>>   drivers/pci/pci-driver.c                   | 94 ++++++++++++++--------
>>   drivers/scsi/mesh.c                        |  1 +
>>   drivers/scsi/stex.c                        |  1 +
>>   drivers/usb/host/sl811-hcd.c               |  1 +
>>   include/linux/pm.h                         |  3 +
>>   include/trace/events/power.h               |  3 +-
>>   kernel/reboot.c                            |  6 ++
>>   9 files changed, 86 insertions(+), 34 deletions(-)
>>
>> --
>> 2.43.0
>>

Re: [PATCH v4 3/5] drm/amd: Avoid evicting resources at S5

[Alex Deucher]

On Mon, Jun 16, 2025 at 1:50 PM Mario Limonciello <superm1@kernel.org> wrote:
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> 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>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
> v3: https://lore.kernel.org/linux-pm/20250609024619.407257-1-superm1@kernel.org/T/#me6db0fb946e3d604a8f3d455128844ed802c82bb
> ---
>  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 8edd88328749b..c5d8f6d551238 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4966,6 +4966,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)
>                 DRM_WARN("evicting device resources failed\n");
> --
> 2.43.0
>

Re: [PATCH v4 2/5] PCI: Put PCIe ports with downstream devices into D3 at hibernate

[Bjorn Helgaas]

On Mon, Jun 16, 2025 at 12:50:16PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> For the suspend flow PCIe ports that have downstream devices are put into
> the appropriate D3 state when children are not in D0. For the hibernate
> flow, PCIe ports with downstream devices stay in D0 however. This can
> lead to PCIe ports that are remained powered on needlessly during
> hibernate.

I suppose by "appropriate D3 state", you mean the Port is put in
D3cold if all children are in D3cold, or D3hot if they are all in
D1-D3hot?  PM-illiterate folks like me need some help to know what is
"appropriate" :)

This refers specifically to "PCIe ports", but it looks like the code
applies to PCI bridges in general, so maybe it should just say
"bridges"?

s/ports that are remained powered on/ports that remain powered on/
(or "bridges that remain powered on")

> Adjust the pci_pm_poweroff_noirq() to follow the same flow as
> pci_pm_suspend_noirq() in that PCIe ports that are power manageable should
> without downstream devices in D0 should be put into their appropriate
> sleep state.

Extra "should" in this sentence, I guess?
(s/power manageable should/power manageable/)

Also "PCIe ports" here, maybe should be "bridges"?

> 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>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v4:
>  * Use helper even when CONFIG_SUSPEND not set (LKP robot)
> 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 | 94 ++++++++++++++++++++++++++--------------
>  1 file changed, 61 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 0d4c67829958b..f7a0c23515718 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -759,6 +759,56 @@ static void pci_pm_complete(struct device *dev)
>  
>  #endif /* !CONFIG_PM_SLEEP */
>  
> +#if defined(CONFIG_SUSPEND) || defined(CONFIG_HIBERNATE_CALLBACKS)
> +/**
> + * pci_pm_set_prepare_bus_pm
> + * @pci_dev: pci device
> + *
> + * Prepare the device to go into a low power state by saving state
> + * and configure bus PM policy.

I guess "configure bus PM policy" must mean "if this device is in D0,
set skip_bus_pm = true for the device and any upstream bridge so we
won't change their power state"?

> + * Return: TRUE for bus PM will be used
> + *         FALSE for bus PM will be skipped
> + */
> +static bool pci_pm_set_prepare_bus_pm(struct pci_dev *pci_dev)

The "pci_pm_set_prepare_bus_pm" name doesn't immediately suggest a
property that can be true or false.  It complicates things a bit when
a bool function has side effects in addition to giving a yes/no or
true/false answer.  Not sure whether or how this could be improved.

And it's a little confusing that we return false when we set
skip_bus_pm = true (mostly).  It's tough to keep track of what
true/false means.

> +{
> +	if (!pci_dev->state_saved) {
> +		pci_save_state(pci_dev);
> +
> +		/*
> +		 * If the device is a bridge with a child in D0 below it,
> +		 * it needs to stay in D0, so check skip_bus_pm to avoid
> +		 * putting it into a low-power state in that case.
> +		 */
> +		if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
> +			pci_prepare_to_sleep(pci_dev);
> +	}
> +
> +	pci_dbg(pci_dev, "PCI PM: Sleep power state: %s\n",
> +		pci_power_name(pci_dev->current_state));
> +
> +	if (pci_dev->current_state == PCI_D0) {
> +		pci_dev->skip_bus_pm = true;
> +		/*
> +		 * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
> +		 * downstream device is in D0, so avoid changing the power state
> +		 * of the parent bridge by setting the skip_bus_pm flag for it.
> +		 */
> +		if (pci_dev->bus->self)
> +			pci_dev->bus->self->skip_bus_pm = true;
> +	}
> +
> +	if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) {
> +		pci_dbg(pci_dev, "PCI PM: Skipped\n");
> +		return FALSE;
> +	}
> +
> +	pci_pm_set_unknown_state(pci_dev);
> +
> +	return TRUE;

"true" and "false" instead of "TRUE" and "FALSE".

> +}
> +#endif /* CONFIG_SUSPEND || CONFIG_HIBERNATE_CALLBACKS */
> +
>  #ifdef CONFIG_SUSPEND
>  static void pcie_pme_root_status_cleanup(struct pci_dev *pci_dev)
>  {
> @@ -878,38 +928,8 @@ 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));
> -
> -	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 (!pci_pm_set_prepare_bus_pm(pci_dev))
>  		goto Fixup;
> -	}
> -
> -	pci_pm_set_unknown_state(pci_dev);

This part looks like it's mostly factoring this code out to
pci_pm_set_prepare_bus_pm().  Would it be practical to split that
factoring to a patch that makes no functional change?  I'm wondering
if that would make the functional change smaller and easier to
understand.

>  	/*
>  	 * Some BIOSes from ASUS have a bug: If a USB EHCI host controller's
> @@ -1136,6 +1156,8 @@ static int pci_pm_poweroff(struct device *dev)
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  
> +	pci_dev->skip_bus_pm = false;
> +
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
>  
> @@ -1199,8 +1221,8 @@ static int pci_pm_poweroff_noirq(struct device *dev)
>  			return error;
>  	}
>  
> -	if (!pci_dev->state_saved && !pci_has_subordinate(pci_dev))
> -		pci_prepare_to_sleep(pci_dev);
> +	if (!pci_pm_set_prepare_bus_pm(pci_dev))
> +		goto Fixup;
>  
>  	/*
>  	 * The reason for doing this here is the same as for the analogous code
> @@ -1209,6 +1231,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;
> @@ -1218,10 +1241,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
> 

Re: [PATCH v4 2/5] PCI: Put PCIe ports with downstream devices into D3 at hibernate

[Mario Limonciello]

Thanks for looking.

On 7/17/25 4:55 PM, Bjorn Helgaas wrote:
> On Mon, Jun 16, 2025 at 12:50:16PM -0500, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> For the suspend flow PCIe ports that have downstream devices are put into
>> the appropriate D3 state when children are not in D0. For the hibernate
>> flow, PCIe ports with downstream devices stay in D0 however. This can
>> lead to PCIe ports that are remained powered on needlessly during
>> hibernate.
> 
> I suppose by "appropriate D3 state", you mean the Port is put in
> D3cold if all children are in D3cold, or D3hot if they are all in
> D1-D3hot?  PM-illiterate folks like me need some help to know what is
> "appropriate" :)

Yeah I was avoiding typing that all out; but I'll add the extra detail 
in the next spin.

> 
> This refers specifically to "PCIe ports", but it looks like the code
> applies to PCI bridges in general, so maybe it should just say
> "bridges"?

Yeah it applies to bridges in general, will reword.

> 
> s/ports that are remained powered on/ports that remain powered on/
> (or "bridges that remain powered on")
> 
👍

>> Adjust the pci_pm_poweroff_noirq() to follow the same flow as
>> pci_pm_suspend_noirq() in that PCIe ports that are power manageable should
>> without downstream devices in D0 should be put into their appropriate
>> sleep state.
> 
> Extra "should" in this sentence, I guess?
> (s/power manageable should/power manageable/)
> 
> Also "PCIe ports" here, maybe should be "bridges"?

👍

> 
>> 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>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v4:
>>   * Use helper even when CONFIG_SUSPEND not set (LKP robot)
>> 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 | 94 ++++++++++++++++++++++++++--------------
>>   1 file changed, 61 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> index 0d4c67829958b..f7a0c23515718 100644
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -759,6 +759,56 @@ static void pci_pm_complete(struct device *dev)
>>   
>>   #endif /* !CONFIG_PM_SLEEP */
>>   
>> +#if defined(CONFIG_SUSPEND) || defined(CONFIG_HIBERNATE_CALLBACKS)
>> +/**
>> + * pci_pm_set_prepare_bus_pm
>> + * @pci_dev: pci device
>> + *
>> + * Prepare the device to go into a low power state by saving state
>> + * and configure bus PM policy.
> 
> I guess "configure bus PM policy" must mean "if this device is in D0,
> set skip_bus_pm = true for the device and any upstream bridge so we
> won't change their power state"?
> 
>> + * Return: TRUE for bus PM will be used
>> + *         FALSE for bus PM will be skipped
>> + */
>> +static bool pci_pm_set_prepare_bus_pm(struct pci_dev *pci_dev)
> 
> The "pci_pm_set_prepare_bus_pm" name doesn't immediately suggest a
> property that can be true or false.  It complicates things a bit when
> a bool function has side effects in addition to giving a yes/no or
> true/false answer.  Not sure whether or how this could be improved.

Yeah it's an unfortunate side effect of the refactor.  I'll look through 
it once again and see if I can find a more logical way to do it.

> 
> And it's a little confusing that we return false when we set
> skip_bus_pm = true (mostly).  It's tough to keep track of what
> true/false means.

How about if it was:

static void pci_pm_prepare_bus_pm(struct pci_dev *pci_dev, bool *skip);

> 
>> +{
>> +	if (!pci_dev->state_saved) {
>> +		pci_save_state(pci_dev);
>> +
>> +		/*
>> +		 * If the device is a bridge with a child in D0 below it,
>> +		 * it needs to stay in D0, so check skip_bus_pm to avoid
>> +		 * putting it into a low-power state in that case.
>> +		 */
>> +		if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
>> +			pci_prepare_to_sleep(pci_dev);
>> +	}
>> +
>> +	pci_dbg(pci_dev, "PCI PM: Sleep power state: %s\n",
>> +		pci_power_name(pci_dev->current_state));
>> +
>> +	if (pci_dev->current_state == PCI_D0) {
>> +		pci_dev->skip_bus_pm = true;
>> +		/*
>> +		 * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
>> +		 * downstream device is in D0, so avoid changing the power state
>> +		 * of the parent bridge by setting the skip_bus_pm flag for it.
>> +		 */
>> +		if (pci_dev->bus->self)
>> +			pci_dev->bus->self->skip_bus_pm = true;
>> +	}
>> +
>> +	if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) {
>> +		pci_dbg(pci_dev, "PCI PM: Skipped\n");
>> +		return FALSE;
>> +	}
>> +
>> +	pci_pm_set_unknown_state(pci_dev);
>> +
>> +	return TRUE;
> 
> "true" and "false" instead of "TRUE" and "FALSE".
> 

👍

>> +}
>> +#endif /* CONFIG_SUSPEND || CONFIG_HIBERNATE_CALLBACKS */
>> +
>>   #ifdef CONFIG_SUSPEND
>>   static void pcie_pme_root_status_cleanup(struct pci_dev *pci_dev)
>>   {
>> @@ -878,38 +928,8 @@ 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));
>> -
>> -	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 (!pci_pm_set_prepare_bus_pm(pci_dev))
>>   		goto Fixup;
>> -	}
>> -
>> -	pci_pm_set_unknown_state(pci_dev);
> 
> This part looks like it's mostly factoring this code out to
> pci_pm_set_prepare_bus_pm().  Would it be practical to split that
> factoring to a patch that makes no functional change?  I'm wondering
> if that would make the functional change smaller and easier to
> understand.

Yeah I think you're right.  I'll try to split it in two patches.

> 
>>   	/*
>>   	 * Some BIOSes from ASUS have a bug: If a USB EHCI host controller's
>> @@ -1136,6 +1156,8 @@ static int pci_pm_poweroff(struct device *dev)
>>   	struct pci_dev *pci_dev = to_pci_dev(dev);
>>   	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>>   
>> +	pci_dev->skip_bus_pm = false;
>> +
>>   	if (pci_has_legacy_pm_support(pci_dev))
>>   		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
>>   
>> @@ -1199,8 +1221,8 @@ static int pci_pm_poweroff_noirq(struct device *dev)
>>   			return error;
>>   	}
>>   
>> -	if (!pci_dev->state_saved && !pci_has_subordinate(pci_dev))
>> -		pci_prepare_to_sleep(pci_dev);
>> +	if (!pci_pm_set_prepare_bus_pm(pci_dev))
>> +		goto Fixup;
>>   
>>   	/*
>>   	 * The reason for doing this here is the same as for the analogous code
>> @@ -1209,6 +1231,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;
>> @@ -1218,10 +1241,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
>>

Re: [PATCH v4 2/5] PCI: Put PCIe ports with downstream devices into D3 at hibernate

[Ville Syrjälä]

On Mon, Jun 16, 2025 at 12:50:16PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> For the suspend flow PCIe ports that have downstream devices are put into
> the appropriate D3 state when children are not in D0. For the hibernate
> flow, PCIe ports with downstream devices stay in D0 however. This can
> lead to PCIe ports that are remained powered on needlessly during
> hibernate.
> 
> Adjust the pci_pm_poweroff_noirq() to follow the same flow as
> pci_pm_suspend_noirq() in that PCIe ports that are power manageable should
> without downstream devices in D0 should be put into their appropriate
> sleep state.
> 
> 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>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v4:
>  * Use helper even when CONFIG_SUSPEND not set (LKP robot)
> 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 | 94 ++++++++++++++++++++++++++--------------
>  1 file changed, 61 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 0d4c67829958b..f7a0c23515718 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -759,6 +759,56 @@ static void pci_pm_complete(struct device *dev)
>  
>  #endif /* !CONFIG_PM_SLEEP */
>  
> +#if defined(CONFIG_SUSPEND) || defined(CONFIG_HIBERNATE_CALLBACKS)
> +/**
> + * pci_pm_set_prepare_bus_pm
> + * @pci_dev: pci device
> + *
> + * Prepare the device to go into a low power state by saving state
> + * and configure bus PM policy.
> + *
> + * Return: TRUE for bus PM will be used
> + *         FALSE for bus PM will be skipped
> + */
> +static bool pci_pm_set_prepare_bus_pm(struct pci_dev *pci_dev)
> +{
> +	if (!pci_dev->state_saved) {
> +		pci_save_state(pci_dev);
> +
> +		/*
> +		 * If the device is a bridge with a child in D0 below it,
> +		 * it needs to stay in D0, so check skip_bus_pm to avoid
> +		 * putting it into a low-power state in that case.
> +		 */
> +		if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
> +			pci_prepare_to_sleep(pci_dev);
> +	}
> +
> +	pci_dbg(pci_dev, "PCI PM: Sleep power state: %s\n",
> +		pci_power_name(pci_dev->current_state));
> +
> +	if (pci_dev->current_state == PCI_D0) {
> +		pci_dev->skip_bus_pm = true;
> +		/*
> +		 * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
> +		 * downstream device is in D0, so avoid changing the power state
> +		 * of the parent bridge by setting the skip_bus_pm flag for it.
> +		 */
> +		if (pci_dev->bus->self)
> +			pci_dev->bus->self->skip_bus_pm = true;
> +	}
> +
> +	if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) {
> +		pci_dbg(pci_dev, "PCI PM: Skipped\n");
> +		return FALSE;
> +	}
> +
> +	pci_pm_set_unknown_state(pci_dev);
> +
> +	return TRUE;
> +}
> +#endif /* CONFIG_SUSPEND || CONFIG_HIBERNATE_CALLBACKS */
> +
>  #ifdef CONFIG_SUSPEND
>  static void pcie_pme_root_status_cleanup(struct pci_dev *pci_dev)
>  {
> @@ -878,38 +928,8 @@ 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));
> -
> -	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 (!pci_pm_set_prepare_bus_pm(pci_dev))
>  		goto Fixup;
> -	}
> -
> -	pci_pm_set_unknown_state(pci_dev);
>  
>  	/*
>  	 * Some BIOSes from ASUS have a bug: If a USB EHCI host controller's
> @@ -1136,6 +1156,8 @@ static int pci_pm_poweroff(struct device *dev)
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  
> +	pci_dev->skip_bus_pm = false;
> +
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
>  
> @@ -1199,8 +1221,8 @@ static int pci_pm_poweroff_noirq(struct device *dev)
>  			return error;
>  	}
>  
> -	if (!pci_dev->state_saved && !pci_has_subordinate(pci_dev))
> -		pci_prepare_to_sleep(pci_dev);
> +	if (!pci_pm_set_prepare_bus_pm(pci_dev))
> +		goto Fixup;

This looks like it's doing similar stuff to what I wanted to do here:
https://lore.kernel.org/linux-pci/20240925144526.2482-2-ville.syrjala@linux.intel.com/

and a bunch of other stuff that seems to lack an explanation:
- the pci_has_subordinate() check is disappearing
- pci_save_state() is now getting called for the poweroff path
- same for pci_pm_set_unknown_state()
- the pci_pm_bridge_power_up_actions() call is being added to
  pci_pm_restore_noirq() for some reason

>  	/*
>  	 * The reason for doing this here is the same as for the analogous code
> @@ -1209,6 +1231,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;
> @@ -1218,10 +1241,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

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 0/5] Improvements to S5 power consumption

[Eric Naim]

On 6/17/25 00:50, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> 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 tries to use the S4 flow when the system is being
> powered off.  This lines up the behavior with what other operating systems
> do as well.  If for some reason that fails or is not supported, unwind and
> do the previous S5 flow that will wake all devices and run their shutdown()
> callbacks.
> 

Hi Mario,

I've been running this series on CachyOS since 6.16-rc3 and have no issues.

Feel free to add

Tested-by: Eric Naim <dnaim@cachyos.org>

-- 
Regards,
  Eric
> v3->v4:
>  * Fix LKP robot failure
>  * Rebase on v6.16-rc2
> 
> Previous submissions [1]:
> Link: https://lore.kernel.org/linux-pm/CAJZ5v0hrKEJa8Ad7iiAvQ3d_0ysVhzZcXSYc5kkL=6vtseF+bg@mail.gmail.com/T/#m91e4eae868a7405ae579e89b135085f4906225d2
> Link: https://lore.kernel.org/linux-pci/20250506041934.1409302-1-superm1@kernel.org/
> Link: https://lore.kernel.org/linux-pci/20231213182656.6165-1-mario.limonciello@amd.com/ (v1)
> Link: https://lore.kernel.org/linux-pm/20250514193406.3998101-1-superm1@kernel.org/ (v2)
> Link: https://lore.kernel.org/linux-pm/20250609024619.407257-1-superm1@kernel.org/ (v3)
> 
> Mario Limonciello (5):
>   PM: Use hibernate flows for system power off
>   PCI: Put PCIe ports with downstream devices into D3 at hibernate
>   drm/amd: Avoid evicting resources at S5
>   scsi: Add PM_EVENT_POWEROFF into suspend callbacks
>   usb: sl811-hcd: Add PM_EVENT_POWEROFF into suspend callbacks
> 
>  drivers/base/power/main.c                  |  7 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  4 +
>  drivers/pci/pci-driver.c                   | 94 ++++++++++++++--------
>  drivers/scsi/mesh.c                        |  1 +
>  drivers/scsi/stex.c                        |  1 +
>  drivers/usb/host/sl811-hcd.c               |  1 +
>  include/linux/pm.h                         |  3 +
>  include/trace/events/power.h               |  3 +-
>  kernel/reboot.c                            |  6 ++
>  9 files changed, 86 insertions(+), 34 deletions(-)
>