Improvements to PCI hibernate path
v2 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.
Based off v7.1-rc1
v2:
- Include Lukas' patch from pci/pm directly in series so Sashiko can better review.
- Include linux-pm in CC.
Lukas Wunner (1):
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 | 109 +++++++++++++++++++++++++-------------- 1 file changed, 69 insertions(+), 40 deletions(-)
Signature · 1 line
2.53.0
PATCH v2 1/6 PCI: Stop setting cached power state to "unknown" on unbind
From: Lukas Wunner <[email protected]>
When a PCI device is unbound from its driver, pci_device_remove() sets the cached power state in pci_dev->current_state to PCI_UNKNOWN. This was introduced by commit 2449e06a5696 ("PCI: reset pci device state to unknown state for resume") to invalidate the cached power state in case the system is subsequently put to sleep.
For bound devices, the cached power state is set to PCI_UNKNOWN in pci_pm_suspend_noirq(), immediately before entering system sleep.
Extend to unbound devices for consistency.
This obviates the need to change the cached power state on unbind, so stop doing so.
--- drivers/pci/pci-driver.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
drivers/pci/pci-driver.c +2 -8
@@ -512,13 +512,6 @@ static void pci_device_remove(struct device *dev)/* Undo the runtime PM settings in local_pci_probe() */ pm_runtime_put_sync (dev); -/* -* If the device is still on, set the power state as "unknown", -* since it might change by the next time we load the driver. -*/ -if (pci_dev->current_state ==PCI_D0 ) - pci_dev->current_state =PCI_UNKNOWN ; -/* * We would love to complain here if pci_dev->is_enabled is set, that * the driver should have called pci_disable_device(), but the
@@ -893,7 +886,7 @@ static int pci_pm_suspend_noirq(struct device *dev)if (!pm) {pci_save_state (pci_dev); - goto Fixup; + goto set_unknown; }if (pm->suspend_noirq) {
@@ -945,6 +938,7 @@ static int pci_pm_suspend_noirq(struct device *dev) goto Fixup; } +set_unknown:pci_pm_set_unknown_state (pci_dev);/*
Signature · 1 line
2.53.0
Re: [PATCH v2 1/6] PCI: Stop setting cached power state to "unknown" on unbind
Quoted 12 lines · Mario Limonciello (AMD)
From: Lukas Wunner <[email protected]>
When a PCI device is unbound from its driver,
pci_device_remove()sets the cached power state inpci_dev->current_statetoPCI_UNKNOWN. This was introduced by commit2449e06a5696("PCI: reset pci device state to unknown state for resume") to invalidate the cached power state in case the system is subsequently put to sleep.For bound devices, the cached power state is set to
PCI_UNKNOWNinpci_pm_suspend_noirq(), immediately before entering system sleep.
Quoted 7 lines
Extend to unbound devices for consistency.
This obviates the need to change the cached power state on unbind, so stop doing so.
Signed-off-by Lukas Wunner <[email protected]>Reviewed-by Mario Limonciello (AMD) <[email protected]>
Quoted 42 lines
--- drivers/pci/pci-driver.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)drivers/pci/pci-driver.c +2
-9@@ -512,13 +512,6 @@ static void pci_device_remove(struct device *dev)/* Undo the runtime PM settings in local_pci_probe() */ pm_runtime_put_sync (dev); -/* -* If the device is still on, set the power state as "unknown", -* since it might change by the next time we load the driver. -*/ -if (pci_dev->current_state ==PCI_D0 ) - pci_dev->current_state =PCI_UNKNOWN ; -/* * We would love to complain here if pci_dev->is_enabled is set, that * the driver should have called pci_disable_device(), but the @@ -893,7 +886,7 @@ static int pci_pm_suspend_noirq(struct device *dev)if (!pm) {pci_save_state (pci_dev); - goto Fixup; + goto set_unknown; }if (pm->suspend_noirq) {@@ -945,6 +938,7 @@ static int pci_pm_suspend_noirq(struct device *dev) goto Fixup; } +set_unknown:pci_pm_set_unknown_state (pci_dev);/* -- 2.53.0
PATCH v2 2/6 PCI/PM: Disable device wakeups when halting or powering off system
In reply to Mario Limonciello (AMD)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.53.0
Re: [PATCH v2 2/6] PCI/PM: Disable device wakeups when halting or powering off system
Quoted 5 lines · Mario Limonciello (AMD)
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.
Quoted 7 lines
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.
Does it? And what about the Remote Start part? Wake-on-LAN etc?
Quoted 14 lines
To align with this expectation and avoid unintended wakeups, disable device wakeup capability during these transitions.
Tested-by Eric Naim <[email protected]>Signed-off-by Mario Limonciello (AMD) <[email protected]>--- drivers/pci/pci-driver.c | 3 +++ 1 file changed, 3 insertions(+)drivers/pci/pci-driver.c +0
-0
drivers/pci/pci-driver.c static int pci_pm_poweroff(struct device *dev) · line 1170
Quoted 5 lines
drivers/pci/pci-driver.c +2
-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);
1. device_wakeup_disable() can be used here.
2. Do you really want to unregister the wakeup source class device here?
If you need to roll back the poweroff transition, user space will see this change.
3. What if the device is a NIC that needs to be set up for WoL?
Quoted 5 lines
drivers/pci/pci-driver.c +1
-1+if (pci_has_legacy_pm_support (pci_dev))return pci_legacy_suspend (dev,PMSG_HIBERNATE ); --
PATCH v2 3/6 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.53.0
Re: [PATCH v2 3/6] PCI/PM: Split out code from pci_pm_suspend_noirq() into helper
Quoted 54 lines · Mario Limonciello (AMD)
In order to unify suspend and hibernate codepaths without code duplication the common code should be in common helpers. Move it from
pci_pm_suspend_noirq()into a helper. No intended functional changes.Tested-by Eric Naim <[email protected]>Signed-off-by Mario Limonciello (AMD) <[email protected]>--- drivers/pci/pci-driver.c | 86 +++++++++++++++++++++++++--------------- 1 file changed, 54 insertions(+), 32 deletions(-)drivers/pci/pci-driver.c +35
-0@@ -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)
drivers/pci/pci-driver.c static void pci_pm_complete(struct device *dev) · line 835
Quoted 10 lines
drivers/pci/pci-driver.c +10
-0+ 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);
Can this go back to pci_pm_suspend_noirq()? It wouldn't be necessary to change labels then.
Quoted 71 lines
drivers/pci/pci-driver.c +9
-33+} +#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 -- 2.53.0
PATCH v2 4/6 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.53.0
Re: [PATCH v2 4/6] PCI/PM: Run bridge power up actions as part of restore phase
Quoted 4 lines · 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.
Doing things because they can be done may not be a good idea.
Please say why you need them to happen here.
Quoted 29 lines
Tested-by Eric Naim <[email protected]>Signed-off-by Mario Limonciello (AMD) <[email protected]>--- drivers/pci/pci-driver.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/pci/pci-driver.c +5
-1@@ -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 ; --2.53.0
PATCH v2 5/6 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.53.0
Re: [PATCH v2 5/6] PCI/PM: Use pci_power_manageable() in pci_pm_poweroff_noirq()
Quoted 4 lines · Mario Limonciello (AMD)
Devices with no subordinate should be put into D3 during hibernate, but devices that have
bridge_d3set should also be put to sleep during hibernate.
Why should they?
Quoted 2 lines
Adjust the check in
pci_pm_poweroff_noirq()to usepci_power_manageable()to cover those as well.
Please say what actual problem is addressed by this change.
Quoted 22 lines
Tested-by Eric Naim <[email protected]>Signed-off-by Mario Limonciello (AMD) <[email protected]>--- drivers/pci/pci-driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pci/pci-driver.c +1
-2@@ -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);/* -- 2.53.0
PATCH v2 6/6 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.53.0
Re: [PATCH v2 6/6] PCI: Put PCIe bridges with downstream devices into D3 at hibernate
Quoted 5 lines · 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.
The platform firmware is supposed to power them down though when the platform is going into S4 unless wakeup devices (that can wake up from S4) are configured below them.
Quoted 8 lines
Align the hibernate flow with suspend by updating
pci_pm_poweroff_noirq()to usepci_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.
This is somewhat risky because it wasn't done previously and, again, it would be good to have some explanation of why it is needed now.
Quoted 73 lines
Cc AceLan Kao <[email protected]>Cc Kai-Heng Feng <[email protected]>Cc Mark Pearson <[email protected]>Cc Denis Benato <[email protected]>Cc Merthan Karakaş <[email protected]>Tested-by Eric Naim <[email protected]>Signed-off-by Mario Limonciello (AMD) <[email protected]>--- drivers/pci/pci-driver.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)drivers/pci/pci-driver.c +9
-5@@ -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 ; --2.53.0
Quoted 4 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.
Thanks for posting this again. Sashiko had a few questions:
https://sashiko.dev/#/patchset/20260427205024.254677-1-superm1%40kernel.org
I didn't look at them in detail, although I did think the ACPI r6.5 sec 16.1.5 reference was a little bit obscure. The diagram in sec 16.1 certainly implies that wakeups only occur in S1-S4 and not in S5.
Sec 16.1.5 does mention "Remote Start", which is completely undefined by the spec but searching does find sec 7.4.2.6, which clearly says the system requires a complete boot when awakened. Possibly a little misleading to refer to Remote Start as "awakening" when it's apparently not a "wakeup".
That section also says "OSPM does not disable wake events before setting the SLP_EN bit when entering the S5 system state."
Quoted 13 lines · Bjorn Helgaas
Quoted 4 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.
Thanks for posting this again. Sashiko had a few questions:
https://sashiko.dev/#/patchset/20260427205024.254677-1-superm1%40kernel.orgI didn't look at them in detail, although I did think the ACPI r6.5 sec 16.1.5 reference was a little bit obscure. The diagram in sec 16.1 certainly implies that wakeups only occur in S1-S4 and not in S5.
Quoted 8 lines
Sec 16.1.5 does mention "Remote Start", which is completely undefined by the spec but searching does find sec 7.4.2.6, which clearly says the system requires a complete boot when awakened. Possibly a little misleading to refer to Remote Start as "awakening" when it's apparently not a "wakeup".
That section also says "OSPM does not disable wake events before setting the
SLP_ENbit when entering the S5 system state."
Yeah; especially the comments on patch #2 I'm not in agreement with it.
How exactly do you want to handle the rest of the Sashiko comments? I suppose one option is to copy and paste them all to refute the ones I agree or disagree with.
But I was thinking let you and Lukas provide comments and then I'll rev for your comments (if necessary) and then ones that I agree with Sashiko, ignore the rest.
Quoted 25 lines · Mario Limonciello
Quoted 13 lines · Bjorn Helgaas
Quoted 4 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.
Thanks for posting this again. Sashiko had a few questions:
https://sashiko.dev/#/patchset/20260427205024.254677-1-superm1%40kernel.orgI didn't look at them in detail, although I did think the ACPI r6.5 sec 16.1.5 reference was a little bit obscure. The diagram in sec 16.1 certainly implies that wakeups only occur in S1-S4 and not in S5.
Quoted 8 lines
Sec 16.1.5 does mention "Remote Start", which is completely undefined by the spec but searching does find sec 7.4.2.6, which clearly says the system requires a complete boot when awakened. Possibly a little misleading to refer to Remote Start as "awakening" when it's apparently not a "wakeup".
That section also says "OSPM does not disable wake events before setting the
SLP_ENbit when entering the S5 system state."Yeah; especially the comments on patch #2 I'm not in agreement with it.
Quoted 7 lines
How exactly do you want to handle the rest of the Sashiko comments? I suppose one option is to copy and paste them all to refute the ones I agree or disagree with.
But I was thinking let you and Lukas provide comments and then I'll rev for your comments (if necessary) and then ones that I agree with Sashiko, ignore the rest.
It's a fair question. From my perspective I don't think it scales very well for me to go through all the Sashiko comments and figure out whether they make sense.
The trivial kernel-doc comment is definitely legit. The others would take me significant legwork to evaluate.
Quoted 3 lines
It's a fair question. From my perspective I don't think it scales very well for me to go through all the Sashiko comments and figure out whether they make sense.
They are absolutely a lot of work to go through these flows and validate for me too. I'll do it; but I'd trust Lukas' thoughts first before I spend the time validating Sashiko's.
Quoted 3 lines
The trivial kernel-doc comment is definitely legit. The others would take me significant legwork to evaluate.
Yup agreed on that one too, it's in my tree already for the next rev.