Improvements to PCI hibernate path
cover Improvements to PCI hibernate path
A few cycles ago I sent out a kernel series for using the S4 paths when the system goes to S5. Some parts of it got merged, and Rafael suggested to split the other parts into smaller pieces across multiple kernel cycles to make bisecting easier.
This fell into my backlog behind other things, so I wanted to try again this cycle for the PCI pieces. I have been carrying it, rebasing it and personally using it for a while now though.
This series attempts to unify the PCI suspend and hibernate paths and to fix some things that I observed to be wrong with how I expect hibernate to work.
It is based off 7.1-rc1 + Lukas' patch:
"PCI: Stop setting cached power state to "unknown" on unbind"
Mario Limonciello (AMD) (5):
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
drivers/pci/pci-driver.c | 103 ++++++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 34 deletions(-)
Signature · 1 line
2.43.0
PATCH 1/5 PCI/PM: Disable device wakeups when halting or powering off system
PCI devices can be configured as wakeup sources from low power states. However, when the system is 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.
--- drivers/pci/pci-driver.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/pci/pci-driver.c +3 -0
@@ -1166,6 +1166,9 @@ 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_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
PATCH 2/5 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.
--- drivers/pci/pci-driver.c | 86 +++++++++++++++++++++++++--------------- 1 file changed, 54 insertions(+), 32 deletions(-)
drivers/pci/pci-driver.c +54 -32
@@ -788,6 +788,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) {
@@ -877,6 +925,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 ;
@@ -886,7 +935,8 @@ static int pci_pm_suspend_noirq(struct device *dev)if (!pm) {pci_save_state (pci_dev); - goto set_unknown; +pci_pm_set_unknown_state (pci_dev); + goto Ehci_workaround; }if (pm->suspend_noirq) {
@@ -907,40 +957,12 @@ 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; - } - -set_unknown: -pci_pm_set_unknown_state (pci_dev); +Ehci_workaround:/* * Some BIOSes from ASUS have a bug: If a USB EHCI host controller's * PCI COMMAND register isn't 0, the BIOS assumes that the controller
Signature · 1 line
2.43.0
PATCH 3/5 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.
--- drivers/pci/pci-driver.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/pci/pci-driver.c +5 -0
@@ -1273,10 +1273,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 4/5 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.
--- drivers/pci/pci-driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/pci-driver.c +1 -1
@@ -1254,7 +1254,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 5/5 PCI: Put PCIe bridges with downstream devices into D3 at hibernate
In reply to 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.
--- drivers/pci/pci-driver.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/pci/pci-driver.c +9 -4
@@ -788,7 +788,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
@@ -834,7 +834,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)
@@ -1191,6 +1191,8 @@ static int pci_pm_poweroff(struct device *dev)if (device_may_wakeup (dev) && 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 );
@@ -1233,6 +1235,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 ;
@@ -1254,8 +1257,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
@@ -1264,6 +1268,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
Quoted 24 lines · Mario Limonciello (AMD)
A few cycles ago I sent out a kernel series for using the S4 paths when the system goes to S5. Some parts of it got merged, and Rafael suggested to split the other parts into smaller pieces across multiple kernel cycles to make bisecting easier.
This fell into my backlog behind other things, so I wanted to try again this cycle for the PCI pieces. I have been carrying it, rebasing it and personally using it for a while now though.
This series attempts to unify the PCI suspend and hibernate paths and to fix some things that I observed to be wrong with how I expect hibernate to work.
It is based off 7.1-rc1 + Lukas' patch:
"PCI: Stop setting cached power state to "unknown" on unbind"Mario Limonciello (AMD) (5):
PCI/PM: Disable device wakeups when halting or powering off system 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
Quoted 4 lines
drivers/pci/pci-driver.c | 103 ++++++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 34 deletions(-)--
It would be good to CC this to linux-pm.
Quoted 26 lines · Rafael J. Wysocki
Quoted 24 lines · Mario Limonciello (AMD)
A few cycles ago I sent out a kernel series for using the S4 paths when the system goes to S5. Some parts of it got merged, and Rafael suggested to split the other parts into smaller pieces across multiple kernel cycles to make bisecting easier.
This fell into my backlog behind other things, so I wanted to try again this cycle for the PCI pieces. I have been carrying it, rebasing it and personally using it for a while now though.
This series attempts to unify the PCI suspend and hibernate paths and to fix some things that I observed to be wrong with how I expect hibernate to work.
It is based off 7.1-rc1 + Lukas' patch:
"PCI: Stop setting cached power state to "unknown" on unbind"Mario Limonciello (AMD) (5):
PCI/PM: Disable device wakeups when halting or powering off system 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
Quoted 6 lines
Quoted 4 lines
drivers/pci/pci-driver.c | 103 ++++++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 34 deletions(-)--
It would be good to CC this to linux-pm.
Sure thing - if there is a need for a v2, I will include linux-pm in CC.
Quoted 28 lines · Mario Limonciello
Quoted 26 lines · Rafael J. Wysocki
Quoted 24 lines · Mario Limonciello (AMD)
A few cycles ago I sent out a kernel series for using the S4 paths when the system goes to S5. Some parts of it got merged, and Rafael suggested to split the other parts into smaller pieces across multiple kernel cycles to make bisecting easier.
This fell into my backlog behind other things, so I wanted to try again this cycle for the PCI pieces. I have been carrying it, rebasing it and personally using it for a while now though.
This series attempts to unify the PCI suspend and hibernate paths and to fix some things that I observed to be wrong with how I expect hibernate to work.
It is based off 7.1-rc1 + Lukas' patch:
"PCI: Stop setting cached power state to "unknown" on unbind"Mario Limonciello (AMD) (5):
PCI/PM: Disable device wakeups when halting or powering off system 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
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
drivers/pci/pci-driver.c | 103 ++++++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 34 deletions(-)--
It would be good to CC this to linux-pm.
Sure thing - if there is a need for a v2, I will include linux-pm in CC.
The thing is, you may not know if there's a need for a v2 without resending.
Quoted 32 lines · Rafael J. Wysocki
Quoted 31 lines · Mario Limonciello
Quoted 29 lines · Rafael J. Wysocki
Quoted 27 lines · Mario Limonciello (AMD)
A few cycles ago I sent out a kernel series for using the S4 paths when the system goes to S5. Some parts of it got merged, and Rafael suggested to split the other parts into smaller pieces across multiple kernel cycles to make bisecting easier.
This fell into my backlog behind other things, so I wanted to try again this cycle for the PCI pieces. I have been carrying it, rebasing it and personally using it for a while now though.
This series attempts to unify the PCI suspend and hibernate paths and to fix some things that I observed to be wrong with how I expect hibernate to work.
It is based off 7.1-rc1 + Lukas' patch:
"PCI: Stop setting cached power state to "unknown" on unbind"Mario Limonciello (AMD) (5):
PCI/PM: Disable device wakeups when halting or powering off system 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 hibernatedrivers/pci/pci-driver.c | 103 ++++++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 34 deletions(-)
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
--
It would be good to CC this to linux-pm.
Sure thing - if there is a need for a v2, I will include linux-pm in CC.
The thing is, you may not know if there's a need for a v2 without resending.
I would think Lukas and Bjorn will at least have some comments and I want to give them some time before cluttering inboxes with a second copy of the series at the start of the dev cycle.
How about if it looks good to them as is I'll send a v2 with their tags added for linux-pm to get a chance to see and review as well.
And if they have feedback I'll incorporate and v2 will also go to linux-pm.
Quoted 38 lines · Mario Limonciello
Quoted 36 lines · Rafael J. Wysocki
Quoted 35 lines · Mario Limonciello
Quoted 32 lines · Rafael J. Wysocki
Quoted 24 lines · Mario Limonciello (AMD)
A few cycles ago I sent out a kernel series for using the S4 paths when the system goes to S5. Some parts of it got merged, and Rafael suggested to split the other parts into smaller pieces across multiple kernel cycles to make bisecting easier.
This fell into my backlog behind other things, so I wanted to try again this cycle for the PCI pieces. I have been carrying it, rebasing it and personally using it for a while now though.
This series attempts to unify the PCI suspend and hibernate paths and to fix some things that I observed to be wrong with how I expect hibernate to work.
It is based off 7.1-rc1 + Lukas' patch:
"PCI: Stop setting cached power state to "unknown" on unbind"Mario Limonciello (AMD) (5):
PCI/PM: Disable device wakeups when halting or powering off system 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 hibernateQuoted 4 lines
drivers/pci/pci-driver.c | 103 ++++++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 34 deletions(-)--
It would be good to CC this to linux-pm.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Sure thing - if there is a need for a v2, I will include linux-pm in CC.
The thing is, you may not know if there's a need for a v2 without resending.
I would think Lukas and Bjorn will at least have some comments and I want to give them some time before cluttering inboxes with a second copy of the series at the start of the dev cycle.
Fair enough.
Quoted 4 lines
How about if it looks good to them as is I'll send a v2 with their tags added for linux-pm to get a chance to see and review as well.
And if they have feedback I'll incorporate and v2 will also go to linux-pm.
That should work, thanks!
Quoted 9 lines · Mario Limonciello (AMD)
A few cycles ago I sent out a kernel series for using the S4 paths when the system goes to S5. Some parts of it got merged, and Rafael suggested to split the other parts into smaller pieces across multiple kernel cycles to make bisecting easier.
This fell into my backlog behind other things, so I wanted to try again this cycle for the PCI pieces. I have been carrying it, rebasing it and personally using it for a while now though.
Quoted 6 lines
This series attempts to unify the PCI suspend and hibernate paths and to fix some things that I observed to be wrong with how I expect hibernate to work.
It is based off 7.1-rc1 + Lukas' patch:
"PCI: Stop setting cached power state to "unknown" on unbind"
Hi Mario, would you mind posting a v2 that includes Lukas' patch, so the whole series applies cleanly so Sashiko can go through it?
I already applied Lukas' patch on pci/pm, but I guess Sashiko can't parse that dependency.
Quoted 13 lines
Mario Limonciello (AMD) (5):
PCI/PM: Disable device wakeups when halting or powering off system 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 hibernatedrivers/pci/pci-driver.c | 103 ++++++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 34 deletions(-)Signature · 1 line
2.43.0
Quoted 14 lines · Bjorn Helgaas
Quoted 13 lines · Mario Limonciello (AMD)
A few cycles ago I sent out a kernel series for using the S4 paths when the system goes to S5. Some parts of it got merged, and Rafael suggested to split the other parts into smaller pieces across multiple kernel cycles to make bisecting easier.
This fell into my backlog behind other things, so I wanted to try again this cycle for the PCI pieces. I have been carrying it, rebasing it and personally using it for a while now though.
This series attempts to unify the PCI suspend and hibernate paths and to fix some things that I observed to be wrong with how I expect hibernate to work.
Quoted 8 lines
Quoted 2 lines
It is based off 7.1-rc1 + Lukas' patch:
"PCI: Stop setting cached power state to "unknown" on unbind"Hi Mario, would you mind posting a v2 that includes Lukas' patch, so the whole series applies cleanly so Sashiko can go through it?
I already applied Lukas' patch on pci/pm, but I guess Sashiko can't parse that dependency.
Bjorn,
Sure. I'll repost with his patch front-loaded. I assume you'll just drop that when applying if/when everything else looks good right?
FWIW, I did run review-prompts offline on the series already with Claude models so hopefully no new surprises when it runs with Google's models :P
Thanks,
Quoted 14 lines
Quoted 14 lines
Mario Limonciello (AMD) (5):
PCI/PM: Disable device wakeups when halting or powering off system 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 hibernatedrivers/pci/pci-driver.c | 103 ++++++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 34 deletions(-)Signature · 1 line
2.43.0