Re: [PATCH 0/5] Always build aspm.c

From: Bjorn Helgaas
Date: Fri Feb 23 2024 - 15:35:19 EST

Next message: Jesper Nilsson: "[PATCH net-next v3] net: stmmac: mmc_core: Drop interrupt registers from stats"
Previous message: Namhyung Kim: "Re: [solved] Re: linux-next: build failure after merge of the perf tree"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Jan 29, 2024 at 05:46:27PM -0600, Bjorn Helgaas wrote:
> On Sun, Jan 28, 2024 at 03:32:07PM -0800, David E. Box wrote:
> > Here is the series to always build aspm.c, add back L1SS save/restore, and
> > consolidate all ASPM related code in aspm.c.
> >
> > Patch 1 - Moves all PCI core functions running under CONFIG_PCIEASPM into
> > aspm.c and changes they Makefile to always build it. No functional changes.
> >
> > Patch 2 - Creates a separate function to save the L1SS offset and places it
> > outside of CONFIG_PCIEASPM in aspm.c so that the offset is available for
> > later use by the L1SS save/restore code which needs to run when
> > CONFIG_PCIEASPM=n.
> >
> > Patch 3 - Updated L1 Substate save/restore patch from previous V5 [1].
> >
> > Patch 4 - Moves the LTR save/restore state functions into aspm.c outside of
> > CONFIG_PCIEASPM.
> >
> > Patch 5 - Moves the LTR save/restore state calls into
> > pci_save/restore_pcie_state().
> >
> > The series does not continue any of the ASPM robustness changes proposed by
> > Ilpo [2]. But if we think it's worth combining with this series I can
> > add it and help continue the work.
> >
> > [1] https://lore.kernel.org/linux-pci/20231221011250.191599-1-david.e.box@xxxxxxxxxxxxxxx/
> > [2] https://lore.kernel.org/linux-pci/20230918131103.24119-1-ilpo.jarvinen@xxxxxxxxxxxxxxx/
> >
> > David E. Box (5):
> > PCI: Always build aspm.c
> > PCI: Create function to save L1SS offset
> > PCI/ASPM: Add back L1 PM Substate save and restore
> > PCI: Move pci_save/restore_ltr_state() to aspm.c
> > PCI: Call LTR save/restore state from PCIe save/restore
> >
> > drivers/pci/pci.c | 91 ++++----------
> > drivers/pci/pci.h | 10 +-
> > drivers/pci/pcie/Makefile | 2 +-
> > drivers/pci/pcie/aspm.c | 257 ++++++++++++++++++++++++++++++++++++--
> > drivers/pci/probe.c | 62 +--------
> > include/linux/pci.h | 4 +-
> > 6 files changed, 283 insertions(+), 143 deletions(-)
> >
> >
> > base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a
>
> Rebased to v6.8-rc1 and applied to pci/aspm for v6.9, thanks!

I reworked this to accommodate Vidya's "Update saved buffers with
latest ASPM configuration" patch:
https://lore.kernel.org/r/20230125133830.20620-1-vidyas@xxxxxxxxxx

It ended up being more work than I expected. The interdiff is below,
and I put this and Vidya's patch on a pci/aspm-rework branch
temporarily so we can take a look before saying it's done:
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=aspm-rework

The main changes are:

- Rename pci_save_aspm_state() to pci_save_aspm_l1ss_state() (this
is the main thing for Vidya's patch).

- 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.

Let me know if you spot anything that I broke.

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 00139fad1827..4ea98665172d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1623,7 +1623,7 @@ 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_state(dev);
+ pci_save_aspm_l1ss_state(dev);
pci_save_ltr_state(dev);

return 0;
@@ -1633,11 +1633,11 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
{
int i = 0;
struct pci_cap_saved_state *save_state;
- u16 *cap, val;
+ u16 *cap, lnkctl;

/*
* Restore max latencies (in the LTR capability) before enabling
- * LTR itself (in the PCIe capability).
+ * LTR itself in PCI_EXP_DEVCTL2.
*/
pci_restore_ltr_state(dev);

@@ -1655,19 +1655,22 @@ 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 only the LNKCTL register with the ASPM control field
- * clear. ASPM will be restored in pci_restore_aspm_state().
- */
- val = cap[i++] & ~PCI_EXP_LNKCTL_ASPMC;
- pcie_capability_write_word(dev, PCI_EXP_LNKCTL, val);
+ /* 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_state(dev);
+ 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)
@@ -3532,11 +3535,6 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
if (error)
pci_err(dev, "unable to allocate suspend buffer for LTR\n");

- error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_L1SS,
- 2 * sizeof(u32));
- if (error)
- pci_err(dev, "unable to allocate suspend buffer for ASPM-L1SS\n");
-
pci_allocate_vc_save_buffers(dev);
}

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index ecceb690fbbb..eca5938deb07 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -97,11 +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_aspm_get_l1ss(struct pci_dev *pdev);
-void pci_save_aspm_state(struct pci_dev *pdev);
-void pci_restore_aspm_state(struct pci_dev *pdev);
-void pci_save_ltr_state(struct pci_dev *dev);
-void pci_restore_ltr_state(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)
@@ -572,6 +567,14 @@ 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);
+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);
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 60716fbf40a9..977eca893b2a 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -24,126 +24,6 @@

#include "../pci.h"

-void pci_aspm_get_l1ss(struct pci_dev *pdev)
-{
- /* Read L1 PM substate capabilities */
- pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
-}
-
-void pci_save_aspm_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
- * is already saved in 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++);
-}
-
-static void pcie_restore_aspm_l1ss(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 after resume, 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);
- }
-}
-
-void pci_restore_aspm_state(struct pci_dev *pdev)
-{
- struct pci_cap_saved_state *save_state;
- u16 *cap, val;
-
- save_state = pci_find_saved_cap(pdev, PCI_CAP_ID_EXP);
-
- if (!save_state)
- return;
-
- cap = (u16 *)&save_state->cap.data[0];
- /* Must match the ordering in pci_save/restore_pcie_state() */
- val = cap[1] & PCI_EXP_LNKCTL_ASPMC;
- if (!val)
- return;
-
- /*
- * We restore L1 substate configuration first before enabling L1
- * as the PCIe spec 6.1 sec 5.5.4 suggests.
- */
- pcie_restore_aspm_l1ss(pdev);
-
- /* Re-enable L0s/L1 */
- pcie_capability_set_word(pdev, PCI_EXP_LNKCTL, val);
-}
-
void pci_save_ltr_state(struct pci_dev *dev)
{
int ltr;
@@ -184,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
@@ -1676,4 +1655,5 @@ bool pcie_aspm_support_enabled(void)
{
return aspm_support_enabled;
}
+
#endif /* CONFIG_PCIEASPM */
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b5ccf5a16dc1..1434bf495db3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2258,8 +2258,8 @@ static void pci_configure_device(struct pci_dev *dev)
pci_configure_mps(dev);
pci_configure_extended_tags(dev, NULL);
pci_configure_relaxed_ordering(dev);
- pci_aspm_get_l1ss(dev);
pci_configure_ltr(dev);
+ pci_configure_aspm_l1ss(dev);
pci_configure_eetlp_prefix(dev);
pci_configure_serr(dev);

Next message: Jesper Nilsson: "[PATCH net-next v3] net: stmmac: mmc_core: Drop interrupt registers from stats"
Previous message: Namhyung Kim: "Re: [solved] Re: linux-next: build failure after merge of the perf tree"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]