[PATCH v7 0/5] PCI/ASPM: Save/restore L1 PM Substates for suspend/resume

[PATCH v7 0/5] PCI/ASPM: Save/restore L1 PM Substates for suspend/resume

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

This is some rework of David's series to preserve ASPM L1 substate
configuration across suspend/resume.

We've had several attempts to make this work:

  (unlabeled): https://lore.kernel.org/r/20240128233212.1139663-1-david.e.box@linux.intel.com
  v5: https://lore.kernel.org/r/20231221011250.191599-1-david.e.box@linux.intel.com
  v4: https://lore.kernel.org/all/20231002070044.2299644-1-mika.westerberg@linux.intel.com/
  v3: https://lore.kernel.org/linux-pci/20230925074636.2893747-1-mika.westerberg@linux.intel.com/
  v2: https://lore.kernel.org/linux-pci/20230911073352.3472918-1-mika.westerberg@linux.intel.com/
  v1: https://lore.kernel.org/linux-pci/20230627062442.54008-1-mika.westerberg@linux.intel.com/

The most recent posting is the unlabeled one mentioned above, and I'm
calling it v6 and this rework v7.

Changes since the unlabeled v6:

  - Rename pci_save_aspm_state() to pci_save_aspm_l1ss_state() (this
    is the reason for opening this again, because Vidya's patch [1]
    had to do some incidental renaming).

  - Rename pcie_restore_aspm_l1ss() to pci_restore_aspm_l1ss_state()
    to match.

  - Move the PCI_EXP_LNKCTL_ASPMC from pci_restore_aspm_state() to
    pci_restore_pcie_state() so both writes are in the same place.

  - Rename pci_aspm_get_l1ss() to pci_configure_aspm_l1ss() and add
    the save_buffer there as well.

  - Split [1/5] into two patches: move pci_configure_ltr() and
    pci_bridge_reconfigure_ltr() to aspm.c, and build aspm.c
    unconditionally.

  - Squash [2/5] and [3/5] since [2/5] didn't add any functionality
    itself so they seem like a single logical change.

[1] https://lore.kernel.org/r/20230125133830.20620-1-vidyas@nvidia.com

David E. Box (5):
  PCI/ASPM: Move pci_configure_ltr() to aspm.c
  PCI/ASPM: Always build aspm.c
  PCI/ASPM: Move pci_save_ltr_state() to aspm.c
  PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
  PCI/ASPM: Call pci_save_ltr_state() from pci_save_pcie_state()

 drivers/pci/pci.c         |  89 ++++------------
 drivers/pci/pci.h         |  13 ++-
 drivers/pci/pcie/Makefile |   2 +-
 drivers/pci/pcie/aspm.c   | 215 ++++++++++++++++++++++++++++++++++++++
 drivers/pci/probe.c       |  62 +----------
 include/linux/pci.h       |   2 +-
 6 files changed, 252 insertions(+), 131 deletions(-)

-- 
2.34.1

[PATCH v7 1/5] PCI/ASPM: Move pci_configure_ltr() to aspm.c

[Bjorn Helgaas]

From: "David E. Box" <david.e.box@linux.intel.com>

The Latency Tolerance Reporting (LTR) mechanism supports the ASPM L1.2
state and is only configured when CONFIG_PCIEASPM is set.

Move pci_configure_ltr() and pci_bridge_reconfigure_ltr() into aspm.c since
they only build when CONFIG_PCIEASPM is set.  No functional change
intended.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Link: https://lore.kernel.org/r/20240128233212.1139663-2-david.e.box@linux.intel.com
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
[bhelgaas: commit log, split build change from function moves]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c       | 18 ----------
 drivers/pci/pci.h       |  5 ++-
 drivers/pci/pcie/aspm.c | 75 +++++++++++++++++++++++++++++++++++++++++
 drivers/pci/probe.c     | 61 ---------------------------------
 4 files changed, 79 insertions(+), 80 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d8f11a078924..c783e0f1f2a9 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1626,24 +1626,6 @@ static int pci_save_pcie_state(struct pci_dev *dev)
 	return 0;
 }
 
-void pci_bridge_reconfigure_ltr(struct pci_dev *dev)
-{
-#ifdef CONFIG_PCIEASPM
-	struct pci_dev *bridge;
-	u32 ctl;
-
-	bridge = pci_upstream_bridge(dev);
-	if (bridge && bridge->ltr_path) {
-		pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, &ctl);
-		if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) {
-			pci_dbg(bridge, "re-enabling LTR\n");
-			pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
-						 PCI_EXP_DEVCTL2_LTR_EN);
-		}
-	}
-#endif
-}
-
 static void pci_restore_pcie_state(struct pci_dev *dev)
 {
 	int i = 0;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2336a8d1edab..9aeba82facc4 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -97,7 +97,6 @@ void pci_msi_init(struct pci_dev *dev);
 void pci_msix_init(struct pci_dev *dev);
 bool pci_bridge_d3_possible(struct pci_dev *dev);
 void pci_bridge_d3_update(struct pci_dev *dev);
-void pci_bridge_reconfigure_ltr(struct pci_dev *dev);
 int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
 
 static inline void pci_wakeup_event(struct pci_dev *dev)
@@ -573,11 +572,15 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev);
 void pcie_aspm_exit_link_state(struct pci_dev *pdev);
 void pcie_aspm_pm_state_change(struct pci_dev *pdev);
 void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
+void pci_configure_ltr(struct pci_dev *pdev);
+void pci_bridge_reconfigure_ltr(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) { }
 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_configure_ltr(struct pci_dev *pdev) { }
+static inline void pci_bridge_reconfigure_ltr(struct pci_dev *pdev) { }
 #endif
 
 #ifdef CONFIG_PCIE_ECRC
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 5a0066ecc3c5..d1538f73f2f9 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -938,6 +938,81 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 	up_read(&pci_bus_sem);
 }
 
+void pci_bridge_reconfigure_ltr(struct pci_dev *pdev)
+{
+	struct pci_dev *bridge;
+	u32 ctl;
+
+	bridge = pci_upstream_bridge(pdev);
+	if (bridge && bridge->ltr_path) {
+		pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, &ctl);
+		if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) {
+			pci_dbg(bridge, "re-enabling LTR\n");
+			pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
+						 PCI_EXP_DEVCTL2_LTR_EN);
+		}
+	}
+}
+
+void pci_configure_ltr(struct pci_dev *pdev)
+{
+	struct pci_host_bridge *host = pci_find_host_bridge(pdev->bus);
+	struct pci_dev *bridge;
+	u32 cap, ctl;
+
+	if (!pci_is_pcie(pdev))
+		return;
+
+	/* Read L1 PM substate capabilities */
+	pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
+
+	pcie_capability_read_dword(pdev, PCI_EXP_DEVCAP2, &cap);
+	if (!(cap & PCI_EXP_DEVCAP2_LTR))
+		return;
+
+	pcie_capability_read_dword(pdev, PCI_EXP_DEVCTL2, &ctl);
+	if (ctl & PCI_EXP_DEVCTL2_LTR_EN) {
+		if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) {
+			pdev->ltr_path = 1;
+			return;
+		}
+
+		bridge = pci_upstream_bridge(pdev);
+		if (bridge && bridge->ltr_path)
+			pdev->ltr_path = 1;
+
+		return;
+	}
+
+	if (!host->native_ltr)
+		return;
+
+	/*
+	 * Software must not enable LTR in an Endpoint unless the Root
+	 * Complex and all intermediate Switches indicate support for LTR.
+	 * PCIe r4.0, sec 6.18.
+	 */
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) {
+		pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
+					 PCI_EXP_DEVCTL2_LTR_EN);
+		pdev->ltr_path = 1;
+		return;
+	}
+
+	/*
+	 * If we're configuring a hot-added device, LTR was likely
+	 * disabled in the upstream bridge, so re-enable it before enabling
+	 * it in the new device.
+	 */
+	bridge = pci_upstream_bridge(pdev);
+	if (bridge && bridge->ltr_path) {
+		pci_bridge_reconfigure_ltr(pdev);
+		pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
+					 PCI_EXP_DEVCTL2_LTR_EN);
+		pdev->ltr_path = 1;
+	}
+}
+
 /* Recheck latencies and update aspm_capable for links under the root */
 static void pcie_update_aspm_capable(struct pcie_link_state *root)
 {
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b7335be56008..b809c0b0e0e5 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2209,67 +2209,6 @@ static void pci_configure_relaxed_ordering(struct pci_dev *dev)
 	}
 }
 
-static void pci_configure_ltr(struct pci_dev *dev)
-{
-#ifdef CONFIG_PCIEASPM
-	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
-	struct pci_dev *bridge;
-	u32 cap, ctl;
-
-	if (!pci_is_pcie(dev))
-		return;
-
-	/* Read L1 PM substate capabilities */
-	dev->l1ss = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_L1SS);
-
-	pcie_capability_read_dword(dev, PCI_EXP_DEVCAP2, &cap);
-	if (!(cap & PCI_EXP_DEVCAP2_LTR))
-		return;
-
-	pcie_capability_read_dword(dev, PCI_EXP_DEVCTL2, &ctl);
-	if (ctl & PCI_EXP_DEVCTL2_LTR_EN) {
-		if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
-			dev->ltr_path = 1;
-			return;
-		}
-
-		bridge = pci_upstream_bridge(dev);
-		if (bridge && bridge->ltr_path)
-			dev->ltr_path = 1;
-
-		return;
-	}
-
-	if (!host->native_ltr)
-		return;
-
-	/*
-	 * Software must not enable LTR in an Endpoint unless the Root
-	 * Complex and all intermediate Switches indicate support for LTR.
-	 * PCIe r4.0, sec 6.18.
-	 */
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
-		pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
-					 PCI_EXP_DEVCTL2_LTR_EN);
-		dev->ltr_path = 1;
-		return;
-	}
-
-	/*
-	 * If we're configuring a hot-added device, LTR was likely
-	 * disabled in the upstream bridge, so re-enable it before enabling
-	 * it in the new device.
-	 */
-	bridge = pci_upstream_bridge(dev);
-	if (bridge && bridge->ltr_path) {
-		pci_bridge_reconfigure_ltr(dev);
-		pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
-					 PCI_EXP_DEVCTL2_LTR_EN);
-		dev->ltr_path = 1;
-	}
-#endif
-}
-
 static void pci_configure_eetlp_prefix(struct pci_dev *dev)
 {
 #ifdef CONFIG_PCI_PASID
-- 
2.34.1

[PATCH v7 2/5] PCI/ASPM: Always build aspm.c

[Bjorn Helgaas]

From: "David E. Box" <david.e.box@linux.intel.com>

Some ASPM-related tasks, such as save and restore of LTR and L1SS
capabilities, still need to be performed when CONFIG_PCIEASPM is not
enabled. To prepare for these changes, wrap the current code in aspm.c
with an #ifdef and always build the file.

Link: https://lore.kernel.org/r/20240128233212.1139663-2-david.e.box@linux.intel.com
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
[bhelgaas: split build change from function moves]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/Makefile | 2 +-
 drivers/pci/pcie/aspm.c   | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index 8de4ed5f98f1..6461aa93fe76 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -6,7 +6,7 @@ pcieportdrv-y			:= portdrv.o rcec.o
 
 obj-$(CONFIG_PCIEPORTBUS)	+= pcieportdrv.o
 
-obj-$(CONFIG_PCIEASPM)		+= aspm.o
+obj-y				+= aspm.o
 obj-$(CONFIG_PCIEAER)		+= aer.o err.o
 obj-$(CONFIG_PCIEAER_INJECT)	+= aer_inject.o
 obj-$(CONFIG_PCIE_PME)		+= pme.o
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index d1538f73f2f9..d50c0f83430f 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -24,6 +24,8 @@
 
 #include "../pci.h"
 
+#ifdef CONFIG_PCIEASPM
+
 #ifdef MODULE_PARAM_PREFIX
 #undef MODULE_PARAM_PREFIX
 #endif
@@ -1517,3 +1519,5 @@ bool pcie_aspm_support_enabled(void)
 {
 	return aspm_support_enabled;
 }
+
+#endif /* CONFIG_PCIEASPM */
-- 
2.34.1

[PATCH v7 3/5] PCI/ASPM: Move pci_save_ltr_state() to aspm.c

[Bjorn Helgaas]

From: "David E. Box" <david.e.box@linux.intel.com>

Even when CONFIG_PCIEASPM is not set, we save and restore the LTR
Capability so that if ASPM L1.2 and LTR were configured by the platform,
ASPM L1.2 will still work after suspend/resume, when that platform
configuration may be lost. See dbbfadf23190 ("PCI/ASPM: Save LTR Capability
for suspend/resume").

Since ASPM L1.2 depends on the LTR Capability, move the save/restore code
to the part of aspm.c that is always compiled regardless of
CONFIG_PCIEASPM.  No functional change intended.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Link: https://lore.kernel.org/r/20240128233212.1139663-5-david.e.box@linux.intel.com
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
[bhelgaas: commit log, reorder to make this a pure move]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c       | 40 ----------------------------------------
 drivers/pci/pci.h       |  5 +++++
 drivers/pci/pcie/aspm.c | 40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 40 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c783e0f1f2a9..564e2cf2dde5 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1689,46 +1689,6 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
 	pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
 }
 
-static void pci_save_ltr_state(struct pci_dev *dev)
-{
-	int ltr;
-	struct pci_cap_saved_state *save_state;
-	u32 *cap;
-
-	if (!pci_is_pcie(dev))
-		return;
-
-	ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
-	if (!ltr)
-		return;
-
-	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
-	if (!save_state) {
-		pci_err(dev, "no suspend buffer for LTR; ASPM issues possible after resume\n");
-		return;
-	}
-
-	/* Some broken devices only support dword access to LTR */
-	cap = &save_state->cap.data[0];
-	pci_read_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap);
-}
-
-static void pci_restore_ltr_state(struct pci_dev *dev)
-{
-	struct pci_cap_saved_state *save_state;
-	int ltr;
-	u32 *cap;
-
-	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
-	ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
-	if (!save_state || !ltr)
-		return;
-
-	/* Some broken devices only support dword access to LTR */
-	cap = &save_state->cap.data[0];
-	pci_write_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap);
-}
-
 /**
  * pci_save_state - save the PCI configuration space of a device before
  *		    suspending
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9aeba82facc4..ad3add45345c 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -567,6 +567,11 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 
 bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
 int pcie_retrain_link(struct pci_dev *pdev, bool use_lt);
+
+/* ASPM-related functionality we need even without CONFIG_PCIEASPM */
+void pci_save_ltr_state(struct pci_dev *dev);
+void pci_restore_ltr_state(struct pci_dev *dev);
+
 #ifdef CONFIG_PCIEASPM
 void pcie_aspm_init_link_state(struct pci_dev *pdev);
 void pcie_aspm_exit_link_state(struct pci_dev *pdev);
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index d50c0f83430f..21731b232fb8 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -24,6 +24,46 @@
 
 #include "../pci.h"
 
+void pci_save_ltr_state(struct pci_dev *dev)
+{
+	int ltr;
+	struct pci_cap_saved_state *save_state;
+	u32 *cap;
+
+	if (!pci_is_pcie(dev))
+		return;
+
+	ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
+	if (!ltr)
+		return;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
+	if (!save_state) {
+		pci_err(dev, "no suspend buffer for LTR; ASPM issues possible after resume\n");
+		return;
+	}
+
+	/* Some broken devices only support dword access to LTR */
+	cap = &save_state->cap.data[0];
+	pci_read_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap);
+}
+
+void pci_restore_ltr_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	int ltr;
+	u32 *cap;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
+	ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
+	if (!save_state || !ltr)
+		return;
+
+	/* Some broken devices only support dword access to LTR */
+	cap = &save_state->cap.data[0];
+	pci_write_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap);
+}
+
 #ifdef CONFIG_PCIEASPM
 
 #ifdef MODULE_PARAM_PREFIX
-- 
2.34.1

[PATCH v7 4/5] PCI/ASPM: Save L1 PM Substates Capability for suspend/resume

[Bjorn Helgaas]

From: "David E. Box" <david.e.box@linux.intel.com>

4ff116d0d5fd ("PCI/ASPM: Save L1 PM Substates Capability for
suspend/resume") restored the L1 PM Substates Capability after resume,
which reduced power consumption by making the ASPM L1.x states work after
resume.

a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM Substates Capability for
suspend/resume"") reverted 4ff116d0d5fd because resume failed on some
systems, so power consumption after resume increased again.

a7152be79b62 mentioned that we restore L1 PM substate configuration even
though ASPM L1 may already be enabled. This is due the fact that the
pci_restore_aspm_l1ss_state() was called before pci_restore_pcie_state().

Save and restore the L1 PM Substates Capability, following PCIe r6.1, sec
5.5.4 more closely by:

  1) Do not restore ASPM configuration in pci_restore_pcie_state() but
     do that after PCIe capability is restored in pci_restore_aspm_state()
     following PCIe r6.1, sec 5.5.4.

  2) If BIOS reenables L1SS, particularly L1.2, we need to clear the
     enables in the right order, downstream before upstream. Defer
     restoring the L1SS config until we are at the downstream component.
     Then update the config for both ends of the link in the prescribed
     order.

  3) Program ASPM L1 PM substate configuration before L1 enables.

  4) Program ASPM L1 PM substate enables last, after rest of the fields
     in the capability are programmed.

Link: https://lore.kernel.org/r/20240128233212.1139663-3-david.e.box@linux.intel.com
Link: https://lore.kernel.org/r/20240128233212.1139663-4-david.e.box@linux.intel.com
Reported-by: Koba Ko <koba.ko@canonical.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217321
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216782
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216877
Co-developed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Co-developed-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
[bhelgaas: commit log, squash L1SS-related patches, do both LNKCTL restores
in pci_restore_pcie_state()]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Tasev Nikola <tasev.stefanoska@skynet.be>
Cc: Mark Enriquez <enriquezmark36@gmail.com>
Cc: Thomas Witt <kernel@witt.link>
Cc: Werner Sembach <wse@tuxedocomputers.com>
Cc: Vidya Sagar <vidyas@nvidia.com>
---
 drivers/pci/pci.c       |  17 ++++++-
 drivers/pci/pci.h       |   3 ++
 drivers/pci/pcie/aspm.c | 102 ++++++++++++++++++++++++++++++++++++++--
 drivers/pci/probe.c     |   1 +
 include/linux/pci.h     |   2 +-
 5 files changed, 119 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 564e2cf2dde5..ca6673588bc0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1623,6 +1623,8 @@ static int pci_save_pcie_state(struct pci_dev *dev)
 	pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]);
 	pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]);
 
+	pci_save_aspm_l1ss_state(dev);
+
 	return 0;
 }
 
@@ -1630,7 +1632,7 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
 {
 	int i = 0;
 	struct pci_cap_saved_state *save_state;
-	u16 *cap;
+	u16 *cap, lnkctl;
 
 	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
 	if (!save_state)
@@ -1645,12 +1647,23 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
 
 	cap = (u16 *)&save_state->cap.data[0];
 	pcie_capability_write_word(dev, PCI_EXP_DEVCTL, cap[i++]);
-	pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++]);
+
+	/* Restore LNKCTL register with ASPM control field clear */
+	lnkctl = cap[i++];
+	pcie_capability_write_word(dev, PCI_EXP_LNKCTL,
+				   lnkctl & ~PCI_EXP_LNKCTL_ASPMC);
+
 	pcie_capability_write_word(dev, PCI_EXP_SLTCTL, cap[i++]);
 	pcie_capability_write_word(dev, PCI_EXP_RTCTL, cap[i++]);
 	pcie_capability_write_word(dev, PCI_EXP_DEVCTL2, cap[i++]);
 	pcie_capability_write_word(dev, PCI_EXP_LNKCTL2, cap[i++]);
 	pcie_capability_write_word(dev, PCI_EXP_SLTCTL2, cap[i++]);
+
+	pci_restore_aspm_l1ss_state(dev);
+
+	/* Restore ASPM control after restoring L1SS state */
+	pcie_capability_set_word(dev, PCI_EXP_LNKCTL,
+				 lnkctl & PCI_EXP_LNKCTL_ASPMC);
 }
 
 static int pci_save_pcix_state(struct pci_dev *dev)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index ad3add45345c..eca5938deb07 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -571,6 +571,9 @@ int pcie_retrain_link(struct pci_dev *pdev, bool use_lt);
 /* ASPM-related functionality we need even without CONFIG_PCIEASPM */
 void pci_save_ltr_state(struct pci_dev *dev);
 void pci_restore_ltr_state(struct pci_dev *dev);
+void pci_configure_aspm_l1ss(struct pci_dev *dev);
+void pci_save_aspm_l1ss_state(struct pci_dev *dev);
+void pci_restore_aspm_l1ss_state(struct pci_dev *dev);
 
 #ifdef CONFIG_PCIEASPM
 void pcie_aspm_init_link_state(struct pci_dev *pdev);
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 21731b232fb8..977eca893b2a 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -64,6 +64,105 @@ void pci_restore_ltr_state(struct pci_dev *dev)
 	pci_write_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap);
 }
 
+void pci_configure_aspm_l1ss(struct pci_dev *pdev)
+{
+	int rc;
+
+	pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
+
+	rc = pci_add_ext_cap_save_buffer(pdev, PCI_EXT_CAP_ID_L1SS,
+					 2 * sizeof(u32));
+	if (rc)
+		pci_err(pdev, "unable to allocate ASPM L1SS save buffer (%pe)\n",
+			ERR_PTR(rc));
+}
+
+void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
+{
+	struct pci_cap_saved_state *save_state;
+	u16 l1ss = pdev->l1ss;
+	u32 *cap;
+
+	/*
+	 * Save L1 substate configuration. The ASPM L0s/L1 configuration
+	 * in PCI_EXP_LNKCTL_ASPMC is saved by pci_save_pcie_state().
+	 */
+	if (!l1ss)
+		return;
+
+	save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS);
+	if (!save_state)
+		return;
+
+	cap = &save_state->cap.data[0];
+	pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL2, cap++);
+	pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, cap++);
+}
+
+void pci_restore_aspm_l1ss_state(struct pci_dev *pdev)
+{
+	struct pci_cap_saved_state *pl_save_state, *cl_save_state;
+	struct pci_dev *parent = pdev->bus->self;
+	u32 *cap, pl_ctl1, pl_ctl2, pl_l1_2_enable;
+	u32 cl_ctl1, cl_ctl2, cl_l1_2_enable;
+
+	/*
+	 * In case BIOS enabled L1.2 when resuming, we need to disable it first
+	 * on the downstream component before the upstream. So, don't attempt to
+	 * restore either until we are at the downstream component.
+	 */
+	if (pcie_downstream_port(pdev) || !parent)
+		return;
+
+	if (!pdev->l1ss || !parent->l1ss)
+		return;
+
+	cl_save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS);
+	pl_save_state = pci_find_saved_ext_cap(parent, PCI_EXT_CAP_ID_L1SS);
+	if (!cl_save_state || !pl_save_state)
+		return;
+
+	cap = &cl_save_state->cap.data[0];
+	cl_ctl2 = *cap++;
+	cl_ctl1 = *cap;
+	cap = &pl_save_state->cap.data[0];
+	pl_ctl2 = *cap++;
+	pl_ctl1 = *cap;
+
+	/*
+	 * Disable L1.2 on this downstream endpoint device first, followed
+	 * by the upstream
+	 */
+	pci_clear_and_set_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL1,
+				       PCI_L1SS_CTL1_L1_2_MASK, 0);
+	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
+				       PCI_L1SS_CTL1_L1_2_MASK, 0);
+
+	/*
+	 * In addition, Common_Mode_Restore_Time and LTR_L1.2_THRESHOLD
+	 * in PCI_L1SS_CTL1 must be programmed *before* setting the L1.2
+	 * enable bits, even though they're all in PCI_L1SS_CTL1.
+	 */
+	pl_l1_2_enable = pl_ctl1 & PCI_L1SS_CTL1_L1_2_MASK;
+	pl_ctl1 &= ~PCI_L1SS_CTL1_L1_2_MASK;
+	cl_l1_2_enable = cl_ctl1 & PCI_L1SS_CTL1_L1_2_MASK;
+	cl_ctl1 &= ~PCI_L1SS_CTL1_L1_2_MASK;
+
+	/* Write back without enables first (above we cleared them in ctl1) */
+	pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, pl_ctl2);
+	pci_write_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL2, cl_ctl2);
+	pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, pl_ctl1);
+	pci_write_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL1, cl_ctl1);
+
+	/* Then write back the enables */
+	if (pl_l1_2_enable || cl_l1_2_enable) {
+		pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
+				       pl_ctl1 | pl_l1_2_enable);
+		pci_write_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL1,
+				       cl_ctl1 | cl_l1_2_enable);
+	}
+}
+
 #ifdef CONFIG_PCIEASPM
 
 #ifdef MODULE_PARAM_PREFIX
@@ -1005,9 +1104,6 @@ void pci_configure_ltr(struct pci_dev *pdev)
 	if (!pci_is_pcie(pdev))
 		return;
 
-	/* Read L1 PM substate capabilities */
-	pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
-
 	pcie_capability_read_dword(pdev, PCI_EXP_DEVCAP2, &cap);
 	if (!(cap & PCI_EXP_DEVCAP2_LTR))
 		return;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b809c0b0e0e5..1434bf495db3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2259,6 +2259,7 @@ static void pci_configure_device(struct pci_dev *dev)
 	pci_configure_extended_tags(dev, NULL);
 	pci_configure_relaxed_ordering(dev);
 	pci_configure_ltr(dev);
+	pci_configure_aspm_l1ss(dev);
 	pci_configure_eetlp_prefix(dev);
 	pci_configure_serr(dev);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index add9368e6314..6967ae7b4115 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -390,9 +390,9 @@ struct pci_dev {
 	unsigned int	d3hot_delay;	/* D3hot->D0 transition time in ms */
 	unsigned int	d3cold_delay;	/* D3cold->D0 transition time in ms */
 
+	u16		l1ss;		/* L1SS Capability pointer */
 #ifdef CONFIG_PCIEASPM
 	struct pcie_link_state	*link_state;	/* ASPM link state */
-	u16		l1ss;		/* L1SS Capability pointer */
 	unsigned int	ltr_path:1;	/* Latency Tolerance Reporting
 					   supported from root to here */
 #endif
-- 
2.34.1

[PATCH v7 5/5] PCI/ASPM: Call pci_save_ltr_state() from pci_save_pcie_state()

[Bjorn Helgaas]

From: "David E. Box" <david.e.box@linux.intel.com>

ASPM state is saved and restored from pci_save/restore_pcie_state().  Since
the LTR Capability is linked with ASPM, move the LTR save and restore calls
there as well.  No functional change intended.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Link: https://lore.kernel.org/r/20240128233212.1139663-6-david.e.box@linux.intel.com
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ca6673588bc0..4ea98665172d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1624,6 +1624,7 @@ static int pci_save_pcie_state(struct pci_dev *dev)
 	pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]);
 
 	pci_save_aspm_l1ss_state(dev);
+	pci_save_ltr_state(dev);
 
 	return 0;
 }
@@ -1634,6 +1635,12 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
 	struct pci_cap_saved_state *save_state;
 	u16 *cap, lnkctl;
 
+	/*
+	 * Restore max latencies (in the LTR capability) before enabling
+	 * LTR itself in PCI_EXP_DEVCTL2.
+	 */
+	pci_restore_ltr_state(dev);
+
 	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
 	if (!save_state)
 		return;
@@ -1726,7 +1733,6 @@ int pci_save_state(struct pci_dev *dev)
 	if (i != 0)
 		return i;
 
-	pci_save_ltr_state(dev);
 	pci_save_dpc_state(dev);
 	pci_save_aer_state(dev);
 	pci_save_ptm_state(dev);
@@ -1827,12 +1833,6 @@ void pci_restore_state(struct pci_dev *dev)
 	if (!dev->state_saved)
 		return;
 
-	/*
-	 * Restore max latencies (in the LTR capability) before enabling
-	 * LTR itself (in the PCIe capability).
-	 */
-	pci_restore_ltr_state(dev);
-
 	pci_restore_pcie_state(dev);
 	pci_restore_pasid_state(dev);
 	pci_restore_pri_state(dev);
-- 
2.34.1

Re: [PATCH v7 4/5] PCI/ASPM: Save L1 PM Substates Capability for suspend/resume

[Bjorn Helgaas]

On Fri, Feb 23, 2024 at 02:58:50PM -0600, Bjorn Helgaas wrote:
> From: "David E. Box" <david.e.box@linux.intel.com>
> 
> 4ff116d0d5fd ("PCI/ASPM: Save L1 PM Substates Capability for
> suspend/resume") restored the L1 PM Substates Capability after resume,
> which reduced power consumption by making the ASPM L1.x states work after
> resume.
> 
> a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM Substates Capability for
> suspend/resume"") reverted 4ff116d0d5fd because resume failed on some
> systems, so power consumption after resume increased again.
> 
> a7152be79b62 mentioned that we restore L1 PM substate configuration even
> though ASPM L1 may already be enabled. This is due the fact that the
> pci_restore_aspm_l1ss_state() was called before pci_restore_pcie_state().
> 
> Save and restore the L1 PM Substates Capability, following PCIe r6.1, sec
> 5.5.4 more closely by:
> 
>   1) Do not restore ASPM configuration in pci_restore_pcie_state() but
>      do that after PCIe capability is restored in pci_restore_aspm_state()
>      following PCIe r6.1, sec 5.5.4.
> 
>   2) If BIOS reenables L1SS, particularly L1.2, we need to clear the
>      enables in the right order, downstream before upstream. Defer
>      restoring the L1SS config until we are at the downstream component.
>      Then update the config for both ends of the link in the prescribed
>      order.
> 
>   3) Program ASPM L1 PM substate configuration before L1 enables.
> 
>   4) Program ASPM L1 PM substate enables last, after rest of the fields
>      in the capability are programmed.

> @@ -1645,12 +1647,23 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
>  
>  	cap = (u16 *)&save_state->cap.data[0];
>  	pcie_capability_write_word(dev, PCI_EXP_DEVCTL, cap[i++]);
> -	pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++]);
> +
> +	/* Restore LNKCTL register with ASPM control field clear */
> +	lnkctl = cap[i++];
> +	pcie_capability_write_word(dev, PCI_EXP_LNKCTL,
> +				   lnkctl & ~PCI_EXP_LNKCTL_ASPMC);
> +
>  	pcie_capability_write_word(dev, PCI_EXP_SLTCTL, cap[i++]);
>  	pcie_capability_write_word(dev, PCI_EXP_RTCTL, cap[i++]);
>  	pcie_capability_write_word(dev, PCI_EXP_DEVCTL2, cap[i++]);
>  	pcie_capability_write_word(dev, PCI_EXP_LNKCTL2, cap[i++]);
>  	pcie_capability_write_word(dev, PCI_EXP_SLTCTL2, cap[i++]);
> +
> +	pci_restore_aspm_l1ss_state(dev);
> +
> +	/* Restore ASPM control after restoring L1SS state */
> +	pcie_capability_set_word(dev, PCI_EXP_LNKCTL,
> +				 lnkctl & PCI_EXP_LNKCTL_ASPMC);
>  }

This part makes me wonder if the PCI_EXP_LNKCTL restore could be
simplified by moving the L0s/L1 disable into
pci_restore_aspm_l1ss_state() as in the patch below.

It would be nice if the knowledge about L0s/L1 being disabled while
restoring L1SS state were encapsulated in
pci_restore_aspm_l1ss_state() instead of doing this dance in
pci_restore_pcie_state().

I didn't include this in the v7 posting because it changes the
sequence and I don't have a way to test it, and this whole thing is
awfully tricky to get right.

Bjorn

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 4ea98665172d..5a4b501a3f41 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1633,13 +1633,14 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
 {
 	int i = 0;
 	struct pci_cap_saved_state *save_state;
-	u16 *cap, lnkctl;
+	u16 *cap;
 
 	/*
 	 * Restore max latencies (in the LTR capability) before enabling
 	 * LTR itself in PCI_EXP_DEVCTL2.
 	 */
 	pci_restore_ltr_state(dev);
+	pci_restore_aspm_l1ss_state(dev);
 
 	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
 	if (!save_state)
@@ -1654,23 +1655,12 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
 
 	cap = (u16 *)&save_state->cap.data[0];
 	pcie_capability_write_word(dev, PCI_EXP_DEVCTL, cap[i++]);
-
-	/* Restore LNKCTL register with ASPM control field clear */
-	lnkctl = cap[i++];
-	pcie_capability_write_word(dev, PCI_EXP_LNKCTL,
-				   lnkctl & ~PCI_EXP_LNKCTL_ASPMC);
-
+	pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++]);
 	pcie_capability_write_word(dev, PCI_EXP_SLTCTL, cap[i++]);
 	pcie_capability_write_word(dev, PCI_EXP_RTCTL, cap[i++]);
 	pcie_capability_write_word(dev, PCI_EXP_DEVCTL2, cap[i++]);
 	pcie_capability_write_word(dev, PCI_EXP_LNKCTL2, cap[i++]);
 	pcie_capability_write_word(dev, PCI_EXP_SLTCTL2, cap[i++]);
-
-	pci_restore_aspm_l1ss_state(dev);
-
-	/* Restore ASPM control after restoring L1SS state */
-	pcie_capability_set_word(dev, PCI_EXP_LNKCTL,
-				 lnkctl & PCI_EXP_LNKCTL_ASPMC);
 }
 
 static int pci_save_pcix_state(struct pci_dev *dev)
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index c89ef87ed1c4..46352132bb14 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -105,6 +105,7 @@ void pci_restore_aspm_l1ss_state(struct pci_dev *pdev)
 	struct pci_dev *parent = pdev->bus->self;
 	u32 *cap, pl_ctl1, pl_ctl2, pl_l1_2_enable;
 	u32 cl_ctl1, cl_ctl2, cl_l1_2_enable;
+	u16 clnkctl, plnkctl;
 
 	/*
 	 * In case BIOS enabled L1.2 when resuming, we need to disable it first
@@ -129,6 +130,17 @@ void pci_restore_aspm_l1ss_state(struct pci_dev *pdev)
 	pl_ctl2 = *cap++;
 	pl_ctl1 = *cap;
 
+	/* Make sure L0s/L1 are disabled before updating L1SS config */
+	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &clnkctl);
+	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &plnkctl);
+	if (FIELD_GET(PCI_EXP_LNKCTL_ASPMC, clnkctl) ||
+	    FIELD_GET(PCI_EXP_LNKCTL_ASPMC, plnkctl)) {
+		pcie_capability_write_word(pdev, PCI_EXP_LNKCTL,
+					   clnkctl & ~PCI_EXP_LNKCTL_ASPMC);
+		pcie_capability_write_word(parent, PCI_EXP_LNKCTL,
+					   plnkctl & ~PCI_EXP_LNKCTL_ASPMC);
+	}
+
 	/*
 	 * Disable L1.2 on this downstream endpoint device first, followed
 	 * by the upstream
@@ -161,6 +173,13 @@ void pci_restore_aspm_l1ss_state(struct pci_dev *pdev)
 		pci_write_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL1,
 				       cl_ctl1 | cl_l1_2_enable);
 	}
+
+	/* Restore L0s/L1 if they were enabled */
+	if (FIELD_GET(PCI_EXP_LNKCTL_ASPMC, clnkctl) ||
+	    FIELD_GET(PCI_EXP_LNKCTL_ASPMC, plnkctl)) {
+		pcie_capability_write_word(parent, PCI_EXP_LNKCTL, clnkctl);
+		pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, plnkctl);
+	}
 }
 
 #ifdef CONFIG_PCIEASPM

Re: [PATCH v7 2/5] PCI/ASPM: Always build aspm.c

[Kuppuswamy Sathyanarayanan]

On 2/23/24 12:58 PM, Bjorn Helgaas wrote:
> From: "David E. Box" <david.e.box@linux.intel.com>
>
> Some ASPM-related tasks, such as save and restore of LTR and L1SS
> capabilities, still need to be performed when CONFIG_PCIEASPM is not
> enabled. To prepare for these changes, wrap the current code in aspm.c
> with an #ifdef and always build the file.

Since save/restore needs to be called even if CONFIG_PCIEASPM is
not set, why not just leave it in pci.c?

>
> Link: https://lore.kernel.org/r/20240128233212.1139663-2-david.e.box@linux.intel.com
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> [bhelgaas: split build change from function moves]
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pcie/Makefile | 2 +-
>  drivers/pci/pcie/aspm.c   | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
> index 8de4ed5f98f1..6461aa93fe76 100644
> --- a/drivers/pci/pcie/Makefile
> +++ b/drivers/pci/pcie/Makefile
> @@ -6,7 +6,7 @@ pcieportdrv-y			:= portdrv.o rcec.o
>  
>  obj-$(CONFIG_PCIEPORTBUS)	+= pcieportdrv.o
>  
> -obj-$(CONFIG_PCIEASPM)		+= aspm.o
> +obj-y				+= aspm.o
>  obj-$(CONFIG_PCIEAER)		+= aer.o err.o
>  obj-$(CONFIG_PCIEAER_INJECT)	+= aer_inject.o
>  obj-$(CONFIG_PCIE_PME)		+= pme.o
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index d1538f73f2f9..d50c0f83430f 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -24,6 +24,8 @@
>  
>  #include "../pci.h"
>  
> +#ifdef CONFIG_PCIEASPM
> +
>  #ifdef MODULE_PARAM_PREFIX
>  #undef MODULE_PARAM_PREFIX
>  #endif
> @@ -1517,3 +1519,5 @@ bool pcie_aspm_support_enabled(void)
>  {
>  	return aspm_support_enabled;
>  }
> +
> +#endif /* CONFIG_PCIEASPM */

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v7 2/5] PCI/ASPM: Always build aspm.c

[Bjorn Helgaas]

On Sun, Feb 25, 2024 at 10:44:14PM -0800, Kuppuswamy Sathyanarayanan wrote:
> On 2/23/24 12:58 PM, Bjorn Helgaas wrote:
> > From: "David E. Box" <david.e.box@linux.intel.com>
> >
> > Some ASPM-related tasks, such as save and restore of LTR and L1SS
> > capabilities, still need to be performed when CONFIG_PCIEASPM is not
> > enabled. To prepare for these changes, wrap the current code in aspm.c
> > with an #ifdef and always build the file.
> 
> Since save/restore needs to be called even if CONFIG_PCIEASPM is
> not set, why not just leave it in pci.c?

We could do that, but we're accumulating various bits of ASPM-related
suspend/resume functionality (LTR, L1SS, etc) that seem like they fit
better in aspm.c.

Bjorn

Re: [PATCH v7 0/5] PCI/ASPM: Save/restore L1 PM Substates for suspend/resume

[Bjorn Helgaas]

On Fri, Feb 23, 2024 at 02:58:46PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> This is some rework of David's series to preserve ASPM L1 substate
> configuration across suspend/resume.
> 
> We've had several attempts to make this work:
> 
>   (unlabeled): https://lore.kernel.org/r/20240128233212.1139663-1-david.e.box@linux.intel.com
>   v5: https://lore.kernel.org/r/20231221011250.191599-1-david.e.box@linux.intel.com
>   v4: https://lore.kernel.org/all/20231002070044.2299644-1-mika.westerberg@linux.intel.com/
>   v3: https://lore.kernel.org/linux-pci/20230925074636.2893747-1-mika.westerberg@linux.intel.com/
>   v2: https://lore.kernel.org/linux-pci/20230911073352.3472918-1-mika.westerberg@linux.intel.com/
>   v1: https://lore.kernel.org/linux-pci/20230627062442.54008-1-mika.westerberg@linux.intel.com/
> 
> The most recent posting is the unlabeled one mentioned above, and I'm
> calling it v6 and this rework v7.
> 
> Changes since the unlabeled v6:
> 
>   - Rename pci_save_aspm_state() to pci_save_aspm_l1ss_state() (this
>     is the reason for opening this again, because Vidya's patch [1]
>     had to do some incidental renaming).
> 
>   - Rename pcie_restore_aspm_l1ss() to pci_restore_aspm_l1ss_state()
>     to match.
> 
>   - Move the PCI_EXP_LNKCTL_ASPMC from pci_restore_aspm_state() to
>     pci_restore_pcie_state() so both writes are in the same place.
> 
>   - Rename pci_aspm_get_l1ss() to pci_configure_aspm_l1ss() and add
>     the save_buffer there as well.
> 
>   - Split [1/5] into two patches: move pci_configure_ltr() and
>     pci_bridge_reconfigure_ltr() to aspm.c, and build aspm.c
>     unconditionally.
> 
>   - Squash [2/5] and [3/5] since [2/5] didn't add any functionality
>     itself so they seem like a single logical change.
> 
> [1] https://lore.kernel.org/r/20230125133830.20620-1-vidyas@nvidia.com
> 
> David E. Box (5):
>   PCI/ASPM: Move pci_configure_ltr() to aspm.c
>   PCI/ASPM: Always build aspm.c
>   PCI/ASPM: Move pci_save_ltr_state() to aspm.c
>   PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
>   PCI/ASPM: Call pci_save_ltr_state() from pci_save_pcie_state()
> 
>  drivers/pci/pci.c         |  89 ++++------------
>  drivers/pci/pci.h         |  13 ++-
>  drivers/pci/pcie/Makefile |   2 +-
>  drivers/pci/pcie/aspm.c   | 215 ++++++++++++++++++++++++++++++++++++++
>  drivers/pci/probe.c       |  62 +----------
>  include/linux/pci.h       |   2 +-
>  6 files changed, 252 insertions(+), 131 deletions(-)

I applied these as pci/aspm for v6.9, replacing the original unlabeled
v6 that has been in -next.

I also added the patch I suggested at
https://lore.kernel.org/r/20240223213733.GA115410@bhelgaas to disable
L1 inside pci_restore_aspm_l1ss_state() where we actually depend on it
being disabled.

Bjorn

Re: [PATCH v7 0/5] PCI/ASPM: Save/restore L1 PM Substates for suspend/resume

[Bjorn Helgaas]

[+cc Koba Ko]

On Tue, Mar 05, 2024 at 03:46:56PM -0600, Bjorn Helgaas wrote:
> On Fri, Feb 23, 2024 at 02:58:46PM -0600, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > This is some rework of David's series to preserve ASPM L1 substate
> > configuration across suspend/resume.
> > ...

> > David E. Box (5):
> >   PCI/ASPM: Move pci_configure_ltr() to aspm.c
> >   PCI/ASPM: Always build aspm.c
> >   PCI/ASPM: Move pci_save_ltr_state() to aspm.c
> >   PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> >   PCI/ASPM: Call pci_save_ltr_state() from pci_save_pcie_state()
> > 
> >  drivers/pci/pci.c         |  89 ++++------------
> >  drivers/pci/pci.h         |  13 ++-
> >  drivers/pci/pcie/Makefile |   2 +-
> >  drivers/pci/pcie/aspm.c   | 215 ++++++++++++++++++++++++++++++++++++++
> >  drivers/pci/probe.c       |  62 +----------
> >  include/linux/pci.h       |   2 +-
> >  6 files changed, 252 insertions(+), 131 deletions(-)
> 
> I applied these as pci/aspm for v6.9, replacing the original unlabeled
> v6 that has been in -next.

Would anybody be able to test this, particularly to make sure it works
for the bugs we're claiming to fix with this series?

  https://bugzilla.kernel.org/show_bug.cgi?id=217321
  https://bugzilla.kernel.org/show_bug.cgi?id=216782
  https://bugzilla.kernel.org/show_bug.cgi?id=216877

This series is headed for v6.9, and I hope we can finally claim
victory over these issues.

This is in -next as of the Mar 7 tree.  Or if you want just the ASPM
changes, based on v6.8-rc1, you can use the branch at
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=aspm

Bjorn

Re: [PATCH v7 0/5] PCI/ASPM: Save/restore L1 PM Substates for suspend/resume

[tasev.stefanoska]

Le 7/03/24 à 23:25, Bjorn Helgaas a écrit :

> [+cc Koba Ko]
>
> On Tue, Mar 05, 2024 at 03:46:56PM -0600, Bjorn Helgaas wrote:
>> On Fri, Feb 23, 2024 at 02:58:46PM -0600, Bjorn Helgaas wrote:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> This is some rework of David's series to preserve ASPM L1 substate
>>> configuration across suspend/resume.
>>> ...
>>> David E. Box (5):
>>>    PCI/ASPM: Move pci_configure_ltr() to aspm.c
>>>    PCI/ASPM: Always build aspm.c
>>>    PCI/ASPM: Move pci_save_ltr_state() to aspm.c
>>>    PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
>>>    PCI/ASPM: Call pci_save_ltr_state() from pci_save_pcie_state()
>>>
>>>   drivers/pci/pci.c         |  89 ++++------------
>>>   drivers/pci/pci.h         |  13 ++-
>>>   drivers/pci/pcie/Makefile |   2 +-
>>>   drivers/pci/pcie/aspm.c   | 215 ++++++++++++++++++++++++++++++++++++++
>>>   drivers/pci/probe.c       |  62 +----------
>>>   include/linux/pci.h       |   2 +-
>>>   6 files changed, 252 insertions(+), 131 deletions(-)
>> I applied these as pci/aspm for v6.9, replacing the original unlabeled
>> v6 that has been in -next.
> Would anybody be able to test this, particularly to make sure it works
> for the bugs we're claiming to fix with this series?
>
>    https://bugzilla.kernel.org/show_bug.cgi?id=217321
>    https://bugzilla.kernel.org/show_bug.cgi?id=216782
>    https://bugzilla.kernel.org/show_bug.cgi?id=216877
>
> This series is headed for v6.9, and I hope we can finally claim
> victory over these issues.
>
> This is in -next as of the Mar 7 tree.  Or if you want just the ASPM
> changes, based on v6.8-rc1, you can use the branch at
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=aspm
>
> Bjorn

I just tested the patch v7 from Bjorn, it works on my Asus UX305FA.
Tested on kernel v6.8-rc1.

Tasev

Re: [PATCH v7 0/5] PCI/ASPM: Save/restore L1 PM Substates for suspend/resume

[Bjorn Helgaas]

On Tue, Mar 12, 2024 at 06:03:21PM +0100, tasev.stefanoska wrote:
> Le 7/03/24 à 23:25, Bjorn Helgaas a écrit :
> > On Tue, Mar 05, 2024 at 03:46:56PM -0600, Bjorn Helgaas wrote:
> > > On Fri, Feb 23, 2024 at 02:58:46PM -0600, Bjorn Helgaas wrote:
> > > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > > 
> > > > This is some rework of David's series to preserve ASPM L1 substate
> > > > configuration across suspend/resume.
> > > > ...
> > > > David E. Box (5):
> > > >    PCI/ASPM: Move pci_configure_ltr() to aspm.c
> > > >    PCI/ASPM: Always build aspm.c
> > > >    PCI/ASPM: Move pci_save_ltr_state() to aspm.c
> > > >    PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> > > >    PCI/ASPM: Call pci_save_ltr_state() from pci_save_pcie_state()
> > > > 
> > > >   drivers/pci/pci.c         |  89 ++++------------
> > > >   drivers/pci/pci.h         |  13 ++-
> > > >   drivers/pci/pcie/Makefile |   2 +-
> > > >   drivers/pci/pcie/aspm.c   | 215 ++++++++++++++++++++++++++++++++++++++
> > > >   drivers/pci/probe.c       |  62 +----------
> > > >   include/linux/pci.h       |   2 +-
> > > >   6 files changed, 252 insertions(+), 131 deletions(-)
> > >
> > > I applied these as pci/aspm for v6.9, replacing the original unlabeled
> > > v6 that has been in -next.
> >
> > Would anybody be able to test this, particularly to make sure it works
> > for the bugs we're claiming to fix with this series?
> > 
> >    https://bugzilla.kernel.org/show_bug.cgi?id=217321
> >    https://bugzilla.kernel.org/show_bug.cgi?id=216782
> >    https://bugzilla.kernel.org/show_bug.cgi?id=216877
> > 
> > This series is headed for v6.9, and I hope we can finally claim
> > victory over these issues.
> > 
> > This is in -next as of the Mar 7 tree.  Or if you want just the ASPM
> > changes, based on v6.8-rc1, you can use the branch at
> > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=aspm
> 
> I just tested the patch v7 from Bjorn, it works on my Asus UX305FA.
> Tested on kernel v6.8-rc1.

Thank you very much!  I added the following to the "PCI/ASPM: Save L1
PM Substates Capability for suspend/resume" patch:

  Tested-by: Tasev Nikola <tasev.stefanoska@skynet.be> # Asus UX305FA