PCI/ASPM: Disable ASPM when save/restore PCI state

Commit Message

Victor Ding Jan. 28, 2021, 12:27 p.m. UTC

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.

Therefore, this patch proposes to disable ASPM during a suspend/resume
process.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=211187
Signed-off-by: Victor Ding <[e-mail]>

---

 drivers/pci/pci.c       | 18 +++++++++++++++---
 drivers/pci/pci.h       |  6 ++++++
 drivers/pci/pcie/aspm.c | 26 ++++++++++++++++++++++++++
 include/linux/pci.h     |  1 +
 4 files changed, 48 insertions(+), 3 deletions(-)

Comments

Krzysztof Wilczyński Jan. 28, 2021, 1:44 p.m. UTC | #1

Hi Victor,

Thank you for working on this!

[...]
>  	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;
[...]
> +Exit:
> +	pcie_restore_aspm_control(dev);
> +	return i;
>  }
[...]

A silly thing, but the goto labels are customary lower-case.

Nonetheless, this is probably something that can be corrected when
applying, so that you don't need to unnecessarily send a new version
(unless you will eventually following other reviews, then don't forget
about it).

Krzysztof

Victor Ding Jan. 28, 2021, 3:07 p.m. UTC | #2

On Fri, Jan 29, 2021 at 12:44 AM Krzysztof Wilczyński <[e-mail]> wrote:
>
> Hi Victor,
>
> Thank you for working on this!
>
> [...]
> >       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;
> [...]
> > +Exit:
> > +     pcie_restore_aspm_control(dev);
> > +     return i;
> >  }
> [...]
>
> A silly thing, but the goto labels are customary lower-case.
>
> Nonetheless, this is probably something that can be corrected when
> applying, so that you don't need to unnecessarily send a new version
> (unless you will eventually following other reviews, then don't forget
> about it).
>
> Krzysztof

Thank you for reviewing. I am about to send out a V2 for a bug fix,
it will include this style change too.

Victor

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 32011b7b4c04..a925a7075063 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..519b9f1b067a 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -105,6 +105,12 @@  static const char *policy_str[] = {
 
 #define LINK_RETRAIN_TIMEOUT HZ
 
+void pcie_disable_aspm(struct pci_dev *pdev)
+{
+	pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
+						PCI_EXP_LNKCTL_ASPMC, 0);
+}
+
 static int policy_to_aspm_state(struct pcie_link_state *link)
 {
 	switch (aspm_policy) {
@@ -680,6 +686,26 @@  static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	}
 }
 
+void pcie_save_aspm_control(struct pci_dev *dev)
+{
+	u16 lnkctl;
+
+	if (!pci_is_pcie(dev))
+		return;
+
+	pci_read_config_word(dev, PCI_EXP_LNKCTL, &lnkctl);
+	dev->saved_aspm_ctl = lnkctl & PCI_EXP_LNKCTL_ASPMC;
+}
+
+void pcie_restore_aspm_control(struct pci_dev *dev)
+{
+	if (!pci_is_pcie(dev))
+		return;
+
+	pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
+					PCI_EXP_LNKCTL_ASPMC, dev->saved_aspm_ctl);
+}
+
 /* Configure the ASPM L1 substates */
 static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 {
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 */