Improvements to S5 power consumption
Subsystems (10)
v6 cover Improvements to S5 power consumption
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, run their shutdown() callbacks.
---v5->v6:
* Fix for LKP robot issue
* Some commit message changes
* Rebase on 6.17-rc2
Mario Limonciello (AMD) (11):
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() for poweroff with S4
flow
PCI: PM: Disable device wakeups when halting system through S4 flow
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
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 ++ 10 files changed, 103 insertions(+), 35 deletions(-)
Signature · 1 line
2.43.0
PATCH v6 1/11 PM: Introduce new PMSG_POWEROFF event
PMSG_POWEROFF will be used for the PM core to allow differentiating between an S4 or S5 shutdown sequence when re-using callbacks.
This event should not have wakeups enabled so update PMSG_NO_WAKEUP() to match it as well.
---
v5:
* Re-order and split
* Add tags
v4:
* https://lore.kernel.org/linux-pci/[email protected]/
--- drivers/base/power/main.c | 7 +++++++ include/linux/pm.h | 5 ++++- include/trace/events/power.h | 3 ++- 3 files changed, 13 insertions(+), 2 deletions(-)
drivers/base/power/main.c +7 -0
@@ -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 statcase 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 ; }
include/linux/pm.h +4 -1
@@ -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. *
include/trace/events/power.h +2 -1
@@ -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,
Signature · 1 line
2.43.0
PATCH v6 2/11 scsi: Add PM_EVENT_POWEROFF into suspend callbacks
In reply to 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.
---
v5:
* Re-order
v4:
* https://lore.kernel.org/linux-pci/[email protected]/
--- drivers/scsi/mesh.c | 1 + drivers/scsi/stex.c | 1 + 2 files changed, 2 insertions(+)
drivers/scsi/mesh.c +1 -0
@@ -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 ;
drivers/scsi/stex.c +1 -0
@@ -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 :
Signature · 1 line
2.43.0
PATCH v6 3/11 usb: sl811-hcd: Add PM_EVENT_POWEROFF into suspend callbacks
In reply to 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.
---
v5:
* Re-order
* Add tags
v4:
* https://lore.kernel.org/linux-pci/[email protected]/
--- drivers/usb/host/sl811-hcd.c | 1 + 1 file changed, 1 insertion(+)
drivers/usb/host/sl811-hcd.c +1 -0
@@ -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 ;
Signature · 1 line
2.43.0
PATCH v6 4/11 USB: Pass PMSG_POWEROFF event to suspend_common() for poweroff with S4 flow
In reply to Mario Limonciello (AMD)If powering off the system with the S4 flow USB wakeup sources should be ignored. Add a new callback hcd_pci_poweroff() which will differentiate whether target state is S5 and pass PMSG_POWEROFF as the message so that suspend_common() will avoid doing wakeups.
---
v6:
* Fix LKP robot issue without CONFIG_PM_SLEEP
v5:
* New patch
v4:
* https://lore.kernel.org/linux-pci/[email protected]/
Fix lkp robot issue
--- drivers/usb/core/hcd-pci.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
drivers/usb/core/hcd-pci.c +10 -1
@@ -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,
Signature · 1 line
2.43.0
Re: [PATCH v6 04/11] USB: Pass PMSG_POWEROFF event to suspend_common() for poweroff with S4 flow
Quoted 4 lines · Mario Limonciello (AMD)
If powering off the system with the S4 flow USB wakeup sources should be ignored. Add a new callback
hcd_pci_poweroff()which will differentiate whether target state is S5 and passPMSG_POWEROFFas the message so thatsuspend_common()will avoid doing wakeups.
Hi,
how will Wake-On-LAN work with this change?
Regards
Oliver
Re: [PATCH v6 04/11] USB: Pass PMSG_POWEROFF event to suspend_common() for poweroff with S4 flow
Quoted 6 lines · Oliver Neukum
Quoted 4 lines · Mario Limonciello (AMD)
If powering off the system with the S4 flow USB wakeup sources should be ignored. Add a new callback
hcd_pci_poweroff()which will differentiate whether target state is S5 and passPMSG_POWEROFFas the message so thatsuspend_common()will avoid doing wakeups.
Quoted 7 lines
Hi,
how will Wake-On-LAN work with this change?
Regards
Oliver
From S5 using a USB dongle? Is this a use case that is intended to work today? Does it?
I had expected this only intended from S4, S3 and s2idle.
PATCH v6 5/11 PCI: PM: Disable device wakeups when halting system through S4 flow
In reply to Mario Limonciello (AMD)PCI devices can be programmed as a wakeup source from low power states by sysfs. However when using the S4 flow to go into S5 these wakeup sources should be disabled to avoid what users would perceive as spurious wakeup events.
---
v5:
* Re-order
* Add tags
v4:
* https://lore.kernel.org/linux-pci/[email protected]/
--- drivers/pci/pci-driver.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/pci/pci-driver.c +4 -0
@@ -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 );
Signature · 1 line
2.43.0
Re: [PATCH v6 05/11] PCI: PM: Disable device wakeups when halting system through S4 flow
In subject, s|PCI: PM:|PCI/PM:| to follow previous practice.
Quoted 4 lines · Mario Limonciello (AMD)
PCI devices can be programmed as a wakeup source from low power states by sysfs. However when using the S4 flow to go into S5 these wakeup sources should be disabled to avoid what users would perceive as spurious wakeup events.
Is the "can be programmed vis sysfs" part relevant here?
I think S4 and S5 are ACPI sleep states not applicable to all platforms. Is it relevant that we got here via ACPI?
I assume non-ACPI systems can also exercise this path. Is there a way to describe this scenario in a way that would apply to all systems?
I'm not sure what "using the S4 flow to go in to S5" means.
It would be nice to have a spec reference or some sort of rationale for the requirement to disable all wakeup sources in SYSTEM_HALT and SYSTEM_POWER_OFF.
Quoted 17 lines
Tested-by Eric Naim <[email protected]>Signed-off-by Mario Limonciello (AMD) <[email protected]>---
v5:
* Re-order
* Add tags
v4:
* https://lore.kernel.org/linux-pci/[email protected]/--- drivers/pci/pci-driver.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/pci/pci-driver.c +0
-0
drivers/pci/pci-driver.c static int pci_pm_poweroff(struct device *dev) · line 1144
Quoted 6 lines
drivers/pci/pci-driver.c +3
-0@@ -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);
I guess the suggestion is that we can't wake up at all from SYSTEM_HALT or SYSTEM_POWER_OFF? Would both be considered S5?
Does this mean we need a physical power button push to start up again? I guess ACPI r6.5, sec 16.1.5 kind of suggests that: "hardware does allow a transition to S0 due to power button press or a Remote Start."
Quoted 6 lines
drivers/pci/pci-driver.c +0
-0if (pci_has_legacy_pm_support (pci_dev))return pci_legacy_suspend (dev,PMSG_HIBERNATE );Signature · 1 line
2.43.0
Re: [PATCH v6 05/11] PCI: PM: Disable device wakeups when halting system through S4 flow
Quoted 1 line · Bjorn Helgaas
In subject, s|PCI: PM:|PCI/PM:| to follow previous practice.
👍
Quoted 8 lines · Mario Limonciello (AMD)
Quoted 4 lines
PCI devices can be programmed as a wakeup source from low power states by sysfs. However when using the S4 flow to go into S5 these wakeup sources should be disabled to avoid what users would perceive as spurious wakeup events.
Is the "can be programmed vis sysfs" part relevant here?
No, I can drop that part of the sentence.
Quoted 3 lines
I think S4 and S5 are ACPI sleep states not applicable to all platforms. Is it relevant that we got here via ACPI?
But a non-ACPI system would still run the exact same callbacks for hibernation wouldn't it?
In general you can 's,S4,hibernation,; s,s5,shutdown,' and it would be accurate.
Quoted 5 lines
I assume non-ACPI systems can also exercise this path. Is there a way to describe this scenario in a way that would apply to all systems?
I'm not sure what "using the S4 flow to go in to S5" means.
It means to run the hibernate related device callbacks instead of the shutdown related device callbacks.
I suppose anywhere in the series that is not ACPI specific (besides this patch) I should clarify this as well.
Quoted 4 lines
It would be nice to have a spec reference or some sort of rationale for the requirement to disable all wakeup sources in
SYSTEM_HALTandSYSTEM_POWER_OFF.
I didn't observe it in the PCI path, but I did see that in USB that if you leave them enabled they stay enabled when the system is shutdown. For example if a USB mouse was connected and a wakeup source it could wakeup from S5 too.
So this patch was aiming for congruence between the two subsystems.
I would hypothesize that means that a dock could wake a system from S5 on a hotplug event, which I don't believe is intended behavior.
Quoted 21 lines
Quoted 21 lines
Tested-by Eric Naim <[email protected]>Signed-off-by Mario Limonciello (AMD) <[email protected]>---
v5:
* Re-order
* Add tags
v4:
* https://lore.kernel.org/linux-pci/[email protected]/--- drivers/pci/pci-driver.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/pci/pci-driver.c +0
-0@@ -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 ;
Quoted 6 lines
drivers/pci/pci-driver.c static int pci_pm_poweroff(struct device *dev) · line 1144
Quoted 3 lines
drivers/pci/pci-driver.c +3
-0+if (device_may_wakeup (dev) && + (system_state ==SYSTEM_HALT || system_state ==SYSTEM_POWER_OFF )) +device_set_wakeup_enable (dev, false);I guess the suggestion is that we can't wake up at all from
SYSTEM_HALTorSYSTEM_POWER_OFF? Would both be considered S5?
Correct.
Quoted 4 lines
Does this mean we need a physical power button push to start up again? I guess ACPI r6.5, sec 16.1.5 kind of suggests that: "hardware does allow a transition to S0 due to power button press or a Remote Start."
Correct.
Quoted 7 lines
Quoted 7 lines
drivers/pci/pci-driver.c +0
-0if (pci_has_legacy_pm_support (pci_dev))return pci_legacy_suspend (dev,PMSG_HIBERNATE );Signature · 1 line
2.43.0
PATCH v6 6/11 PCI: PM: Split out code from pci_pm_suspend_noirq() into helper
In reply to 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.
---
v5:
* Split from earlier patches
* Add tags
v4:
* https://lore.kernel.org/linux-pci/[email protected]/
--- drivers/pci/pci-driver.c | 81 +++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 30 deletions(-)
drivers/pci/pci-driver.c +51 -30
@@ -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
Signature · 1 line
2.43.0
PATCH v6 7/11 PCI: PM: Run bridge power up actions as part of restore phase
In reply to 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.
---
v5:
* Split out patch
--- drivers/pci/pci-driver.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/pci/pci-driver.c +5 -0
@@ -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 ;
Signature · 1 line
2.43.0
PATCH v6 8/11 PCI: PM: Use pci_power_manageable() in pci_pm_poweroff_noirq()
In reply to 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.
---
v5:
* Split out patch
--- drivers/pci/pci-driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/pci-driver.c +1 -1
@@ -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);/*
Signature · 1 line
2.43.0
PATCH v6 9/11 PCI: Put PCIe bridges with downstream devices into D3 at hibernate
In reply to Mario Limonciello (AMD)For the suspend flow PCIe bridges that have downstream devices are put into the appropriate low power state (D3hot or D3cold depending upon specific devices). For the hibernate flow, PCIe bridges with downstream devices stay in D0 however. This can lead to PCIe bridges that are remained powered on needlessly during hibernate.
Adjust the pci_pm_poweroff_noirq() to follow the same flow as pci_pm_suspend_noirq() by using pci_pm_suspend_noirq_common().
This introduces a functional change that the hibernate flow will now call pci_save_state() and unless bus PM is skipped will also set the PCIe device into an unknown state.
---
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/[email protected]/
v3:
* Split out common code between suspend_noirq() and poweroff_noirq()
to a helper function
* https://lore.kernel.org/linux-pm/[email protected]/T/#me6db0fb946e3d604a8f3d455128844ed802c82bb
--- drivers/pci/pci-driver.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/pci/pci-driver.c +9 -4
@@ -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 ;
Signature · 1 line
2.43.0
Re: [PATCH v6 09/11] PCI: Put PCIe bridges with downstream devices into D3 at hibernate
Quoted 5 lines · Mario Limonciello (AMD)
For the suspend flow PCIe bridges that have downstream devices are put into the appropriate low power state (D3hot or D3cold depending upon specific devices). For the hibernate flow, PCIe bridges with downstream devices stay in D0 however. This can lead to PCIe bridges that are remained powered on needlessly during hibernate.
s/are remained/remain/ I guess?
Quoted 6 lines
Adjust the
pci_pm_poweroff_noirq()to follow the same flow aspci_pm_suspend_noirq()by usingpci_pm_suspend_noirq_common().This introduces a functional change that the hibernate flow will now call
pci_save_state()and unless bus PM is skipped will also set the PCIe device into an unknown state.
Re: [PATCH v6 09/11] PCI: Put PCIe bridges with downstream devices into D3 at hibernate
Quoted 8 lines · Bjorn Helgaas
Quoted 5 lines · Mario Limonciello (AMD)
For the suspend flow PCIe bridges that have downstream devices are put into the appropriate low power state (D3hot or D3cold depending upon specific devices). For the hibernate flow, PCIe bridges with downstream devices stay in D0 however. This can lead to PCIe bridges that are remained powered on needlessly during hibernate.
s/are remained/remain/ I guess?
Yeah, I'll adjust.
Quoted 7 lines
Quoted 7 lines
Adjust the
pci_pm_poweroff_noirq()to follow the same flow aspci_pm_suspend_noirq()by usingpci_pm_suspend_noirq_common().This introduces a functional change that the hibernate flow will now call
pci_save_state()and unless bus PM is skipped will also set the PCIe device into an unknown state.
PATCH v6 10/11 drm/amd: Avoid evicting resources at S5
In reply to 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.
---
v5:
* No changes
v4:
* Add A-b tag for Alex
* https://lore.kernel.org/linux-pci/[email protected]/
--- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +4 -0
@@ -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" );
Signature · 1 line
2.43.0
PATCH v6 11/11 PM: Use hibernate flows for system power off
In reply to 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.
---
v5:
* split to multiple commits, re-order
v4:
* https://lore.kernel.org/linux-pci/[email protected]/
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/[email protected]/T/#me6db0fb946e3d604a8f3d455128844ed802c82bb
--- kernel/reboot.c | 6 ++++++ 1 file changed, 6 insertions(+)
kernel/reboot.c +6 -0
@@ -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 (); }/**
Signature · 1 line
2.43.0
Quoted 37 lines · 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 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, run their
shutdown()callbacks.Cc AceLan Kao <[email protected]>Cc Kai-Heng Feng <[email protected]>Cc Mark Pearson <[email protected]>Cc Merthan Karakaş <[email protected]>Cc Eric Naim <[email protected]>---
v5->v6:
* Fix for LKP robot issue
* Some commit message changes
* Rebase on 6.17-rc2Mario Limonciello (AMD) (11):
PM: Introduce newPMSG_POWEROFFevent
scsi: AddPM_EVENT_POWEROFFinto suspend callbacks
usb: sl811-hcd: AddPM_EVENT_POWEROFFinto suspend callbacks
USB: PassPMSG_POWEROFFevent tosuspend_common()for poweroff with S4
flow
PCI: PM: Disable device wakeups when halting system through S4 flow
PCI: PM: Split out code frompci_pm_suspend_noirq()into helper
PCI: PM: Run bridge power up actions as part of restore phase
PCI: PM: Usepci_power_manageable()inpci_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
Quoted 12 lines
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 ++ 10 files changed, 103 insertions(+), 35 deletions(-)
Rafael, Bjorn,
Any feedback for this series?
Thanks,
Rafael J. Wysocki
maintainerQuoted 52 lines · Mario Limonciello
Quoted 37 lines · 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 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, run their
shutdown()callbacks.Cc AceLan Kao <[email protected]>Cc Kai-Heng Feng <[email protected]>Cc Mark Pearson <[email protected]>Cc Merthan Karakaş <[email protected]>Cc Eric Naim <[email protected]>---
v5->v6:
* Fix for LKP robot issue
* Some commit message changes
* Rebase on 6.17-rc2Mario Limonciello (AMD) (11):
PM: Introduce newPMSG_POWEROFFevent
scsi: AddPM_EVENT_POWEROFFinto suspend callbacks
usb: sl811-hcd: AddPM_EVENT_POWEROFFinto suspend callbacks
USB: PassPMSG_POWEROFFevent tosuspend_common()for poweroff with S4
flow
PCI: PM: Disable device wakeups when halting system through S4 flow
PCI: PM: Split out code frompci_pm_suspend_noirq()into helper
PCI: PM: Run bridge power up actions as part of restore phase
PCI: PM: Usepci_power_manageable()inpci_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 offQuoted 12 lines
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 ++ 10 files changed, 103 insertions(+), 35 deletions(-)
Quoted 3 lines
Rafael, Bjorn,
Any feedback for this series?
I still have the same basic concern as before: It is an intrusive change likely to cause regressions to occur.
It also changes the driver ABI quite dramatically because different callbacks will now be used for system shutdown and kexec, for example, at least on some platforms.
Frankly, I'd like to know Greg's and Danilo's opinions on the direction here.
Greg Kroah-Hartman
maintainerQuoted 57 lines · Rafael J. Wysocki
Quoted 52 lines · Mario Limonciello
Quoted 37 lines · 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 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, run their
shutdown()callbacks.Cc AceLan Kao <[email protected]>Cc Kai-Heng Feng <[email protected]>Cc Mark Pearson <[email protected]>Cc Merthan Karakaş <[email protected]>Cc Eric Naim <[email protected]>---
v5->v6:
* Fix for LKP robot issue
* Some commit message changes
* Rebase on 6.17-rc2Mario Limonciello (AMD) (11):
PM: Introduce newPMSG_POWEROFFevent
scsi: AddPM_EVENT_POWEROFFinto suspend callbacks
usb: sl811-hcd: AddPM_EVENT_POWEROFFinto suspend callbacks
USB: PassPMSG_POWEROFFevent tosuspend_common()for poweroff with S4
flow
PCI: PM: Disable device wakeups when halting system through S4 flow
PCI: PM: Split out code frompci_pm_suspend_noirq()into helper
PCI: PM: Run bridge power up actions as part of restore phase
PCI: PM: Usepci_power_manageable()inpci_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 offQuoted 12 lines
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 ++ 10 files changed, 103 insertions(+), 35 deletions(-)Quoted 3 lines
Rafael, Bjorn,
Any feedback for this series?
Quoted 9 lines
I still have the same basic concern as before: It is an intrusive change likely to cause regressions to occur.
It also changes the driver ABI quite dramatically because different callbacks will now be used for system shutdown and kexec, for example, at least on some platforms.
Frankly, I'd like to know Greg's and Danilo's opinions on the direction here.
I've been ignoring this, sorry. Can someone resend the series and ask for my review then?
thanks,
greg k-h