| Message ID | 3118349.722IRLjr4b@kreacher (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: PM: Consolidate runtime resume and system resume paths | expand |
On Monday, October 14, 2019 12:51:31 PM CEST Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > There is an arbitrary difference between the system resume and > runtime resume code paths for PCI devices regarding the delay to > apply when switching the devices from D3cold to D0. > > Namely, pci_restore_standard_config() used in the runtime resume > code path calls pci_set_power_state() which in turn invokes > __pci_start_power_transition() to power up the device through the > platform firmware and that function applies the transition delay > (as per PCI Express Base Specification Revision 2.0, Section 6.6.1). > However, pci_pm_default_resume_early() used in the system resume > code path calls pci_power_up() which doesn't apply the delay at > all and that causes issues to occur during resume from > suspend-to-idle on some systems where the delay is required. > > Since there is no reason for that difference to exist, modify > pci_pm_default_resume_early() to invoke pci_restore_standard_config() > instead of pci_power_up() and drop the latter, but in order to > prevent the ACPI power state values (cached by the ACPI layer) from > becoming stale in some cases during resume from suspend-to-RAM > (ACPI S3), as per commit cc2893b6af52 ("PCI: Ensure we re-enable > devices on resume"), refresh the ACPI power state information in > pci_pm_default_resume_early() in that case. > > [Note that while this change should take the issue originally > addressed by commit cc2893b6af52 ("PCI: Ensure we re-enable devices > on resume") into account in a generally safer way, an alternative > would be to make pci_power_up() use __pci_start_power_transition() > instead of calling platform_pci_set_power_state() directly.] > > Fixes: db288c9c5f9d ("PCI / PM: restore the original behavior of pci_set_power_state()") > Reported-by: Daniel Drake <drake@endlessm.com> > Tested-by: Daniel Drake <drake@endlessm.com> > Link: https://lore.kernel.org/linux-pm/CAD8Lp44TYxrMgPLkHCqF9hv6smEurMXvmmvmtyFhZ6Q4SE+dig@mail.gmail.com/T/#m21be74af263c6a34f36e0fc5c77c5449d9406925 > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/pci/pci-driver.c | 8 +++++--- > drivers/pci/pci.c | 15 --------------- > drivers/pci/pci.h | 1 - > 3 files changed, 5 insertions(+), 19 deletions(-) > > Index: linux-pm/drivers/pci/pci-driver.c > =================================================================== > --- linux-pm.orig/drivers/pci/pci-driver.c > +++ linux-pm/drivers/pci/pci-driver.c > @@ -523,9 +523,10 @@ static int pci_restore_standard_config(s > > static void pci_pm_default_resume_early(struct pci_dev *pci_dev) > { > - pci_power_up(pci_dev); > - pci_restore_state(pci_dev); > - pci_pme_restore(pci_dev); > + if (pm_resume_via_firmware()) > + pci_refresh_power_state(pci_dev); Well, this is still not going to work if the ACPI power state after the update above is not D0, but the pci_update_current_state() in pci_restore_standard_config() returns D0, which was the case that triggered commit cc2893b6af52 IIRC. So scratch this one, please, and I'll submit the safer option. > + > + pci_restore_standard_config(pci_dev); > } > > /* > @@ -713,6 +714,7 @@ static void pci_pm_complete(struct devic > pci_power_t pre_sleep_state = pci_dev->current_state; > > pci_refresh_power_state(pci_dev); > + pci_update_current_state(pci_dev, pci_dev->current_state); > /* > * On platforms with ACPI this check may also trigger for > * devices sharing power resources if one of those power
Index: linux-pm/drivers/pci/pci-driver.c =================================================================== --- linux-pm.orig/drivers/pci/pci-driver.c +++ linux-pm/drivers/pci/pci-driver.c @@ -523,9 +523,10 @@ static int pci_restore_standard_config(s static void pci_pm_default_resume_early(struct pci_dev *pci_dev) { - pci_power_up(pci_dev); - pci_restore_state(pci_dev); - pci_pme_restore(pci_dev); + if (pm_resume_via_firmware()) + pci_refresh_power_state(pci_dev); + + pci_restore_standard_config(pci_dev); } /* @@ -713,6 +714,7 @@ static void pci_pm_complete(struct devic pci_power_t pre_sleep_state = pci_dev->current_state; pci_refresh_power_state(pci_dev); + pci_update_current_state(pci_dev, pci_dev->current_state); /* * On platforms with ACPI this check may also trigger for * devices sharing power resources if one of those power Index: linux-pm/drivers/pci/pci.c =================================================================== --- linux-pm.orig/drivers/pci/pci.c +++ linux-pm/drivers/pci/pci.c @@ -954,21 +954,6 @@ void pci_refresh_power_state(struct pci_ { if (platform_pci_power_manageable(dev)) platform_pci_refresh_power_state(dev); - - pci_update_current_state(dev, dev->current_state); -} - -/** - * pci_power_up - Put the given device into D0 forcibly - * @dev: PCI device to power up - */ -void pci_power_up(struct pci_dev *dev) -{ - if (platform_pci_power_manageable(dev)) - platform_pci_set_power_state(dev, PCI_D0); - - pci_raw_set_power_state(dev, PCI_D0); - pci_update_current_state(dev, PCI_D0); } /** Index: linux-pm/drivers/pci/pci.h =================================================================== --- linux-pm.orig/drivers/pci/pci.h +++ linux-pm/drivers/pci/pci.h @@ -85,7 +85,6 @@ struct pci_platform_pm_ops { int pci_set_platform_pm(const struct pci_platform_pm_ops *ops); void pci_update_current_state(struct pci_dev *dev, pci_power_t state); void pci_refresh_power_state(struct pci_dev *dev); -void pci_power_up(struct pci_dev *dev); void pci_disable_enabled_device(struct pci_dev *dev); int pci_finish_runtime_suspend(struct pci_dev *dev); void pcie_clear_root_pme_status(struct pci_dev *dev);