Certain PCIe devices (e.g. GL9750) have high penalties (e.g. high Port T_POWER_ON) when exiting L1 but enter L1 aggressively. As a result, such devices enter and exit L1 frequently during pci_save_state and pci_restore_state; eventually causing poor suspend/resume performance.
Based on the observation that PCI accesses dominance pci_save_state/ pci_restore_state plus these accesses are fairly close to each other, the actual time the device could stay in low power states is negligible. Therefore, the little power-saving benefit from ASPM during suspend/resume does not overweight the performance degradation caused by high L1 exit penalties. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=211187 Signed-off-by: Victor Ding <victord...@google.com> --- Changes in v2: - Updated commit message to remove unnecessary information - Fixed a bug reading wrong register in pcie_save_aspm_control - Updated to reuse existing pcie_config_aspm_dev where possible - Fixed goto label style drivers/pci/pci.c | 18 +++++++++++++++--- drivers/pci/pci.h | 6 ++++++ drivers/pci/pcie/aspm.c | 27 +++++++++++++++++++++++++++ include/linux/pci.h | 1 + 4 files changed, 49 insertions(+), 3 deletions(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 32011b7b4c04..9ea88953f90b 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1542,6 +1542,10 @@ static void pci_restore_ltr_state(struct pci_dev *dev) int pci_save_state(struct pci_dev *dev) { int i; + + pcie_save_aspm_control(dev); + pcie_disable_aspm(dev); + /* XXX: 100% dword access ok here? */ for (i = 0; i < 16; i++) { pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]); @@ -1552,18 +1556,22 @@ int pci_save_state(struct pci_dev *dev) i = pci_save_pcie_state(dev); if (i != 0) - return i; + goto exit; i = pci_save_pcix_state(dev); if (i != 0) - return i; + goto exit; pci_save_ltr_state(dev); pci_save_aspm_l1ss_state(dev); pci_save_dpc_state(dev); pci_save_aer_state(dev); pci_save_ptm_state(dev); - return pci_save_vc_state(dev); + i = pci_save_vc_state(dev); + +exit: + pcie_restore_aspm_control(dev); + return i; } EXPORT_SYMBOL(pci_save_state); @@ -1661,6 +1669,8 @@ void pci_restore_state(struct pci_dev *dev) if (!dev->state_saved) return; + pcie_disable_aspm(dev); + /* * Restore max latencies (in the LTR capability) before enabling * LTR itself (in the PCIe capability). @@ -1689,6 +1699,8 @@ void pci_restore_state(struct pci_dev *dev) pci_enable_acs(dev); pci_restore_iov_state(dev); + pcie_restore_aspm_control(dev); + dev->state_saved = false; } EXPORT_SYMBOL(pci_restore_state); diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index a81459159f6d..e074a0cbe73c 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -584,6 +584,9 @@ void pcie_aspm_pm_state_change(struct pci_dev *pdev); void pcie_aspm_powersave_config_link(struct pci_dev *pdev); void pci_save_aspm_l1ss_state(struct pci_dev *dev); void pci_restore_aspm_l1ss_state(struct pci_dev *dev); +void pcie_save_aspm_control(struct pci_dev *dev); +void pcie_restore_aspm_control(struct pci_dev *dev); +void pcie_disable_aspm(struct pci_dev *pdev); #else static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { } static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { } @@ -591,6 +594,9 @@ static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { } static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { } static inline void pci_save_aspm_l1ss_state(struct pci_dev *dev) { } static inline void pci_restore_aspm_l1ss_state(struct pci_dev *dev) { } +static inline void pcie_save_aspm_control(struct pci_dev *dev) { } +static inline void pcie_restore_aspm_control(struct pci_dev *dev) { } +static inline void pcie_disable_aspm(struct pci_dev *pdev) { } #endif #ifdef CONFIG_PCIE_ECRC diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index a08e7d6dc248..e1e97db32e8b 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -784,6 +784,33 @@ static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val) PCI_EXP_LNKCTL_ASPMC, val); } +void pcie_disable_aspm(struct pci_dev *pdev) +{ + if (!pci_is_pcie(pdev)) + return; + + pcie_config_aspm_dev(pdev, 0); +} + +void pcie_save_aspm_control(struct pci_dev *pdev) +{ + u16 lnkctl; + + if (!pci_is_pcie(pdev)) + return; + + pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &lnkctl); + pdev->saved_aspm_ctl = lnkctl & PCI_EXP_LNKCTL_ASPMC; +} + +void pcie_restore_aspm_control(struct pci_dev *pdev) +{ + if (!pci_is_pcie(pdev)) + return; + + pcie_config_aspm_dev(pdev, pdev->saved_aspm_ctl); +} + static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) { u32 upstream = 0, dwstream = 0; diff --git a/include/linux/pci.h b/include/linux/pci.h index b32126d26997..a21bfd6e3f89 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -387,6 +387,7 @@ struct pci_dev { unsigned int ltr_path:1; /* Latency Tolerance Reporting supported from root to here */ u16 l1ss; /* L1SS Capability pointer */ + u16 saved_aspm_ctl; /* ASPM Control saved at suspend time */ #endif unsigned int eetlp_prefix_path:1; /* End-to-End TLP Prefix */ -- 2.30.0.280.ga3ce27912f-goog
