diff mbox series

[v1,2/2] PCI/ASPM: Save LTR Capability for suspend/resume

Message ID 154992574559.7864.8459750458248568194.stgit@bhelgaas-glaptop.roam.corp.google.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series PCI/ASPM: Fix LTR issues | expand

Commit Message

Bjorn Helgaas Feb. 11, 2019, 10:55 p.m. UTC 
From: Bjorn Helgaas <bhelgaas@google.com>

Latency Tolerance Reporting (LTR) allows Endpoints and Switch Upstream
Ports to report their latency requirements to upstream components.  If ASPM
L1 PM substates are enabled, the LTR information helps determine when a
Link enters L1.2 [1].

Software must set the maximum latency values in the LTR Capability based on
characteristics of the platform, then set LTR Mechanism Enable in the
Device Control 2 register in the PCIe Capability.  The device can then use
LTR to report its latency tolerance.

If the device reports a maximum latency value of zero, that means the
device requires the highest possible performance and the ASPM L1.2 substate
is effectively disabled.

We put devices in D3 for suspend, and we assume their internal state is
lost.  On resume, previously we did not restore the LTR Capability, but we
did restore the LTR Mechanism Enable bit, so devices would request the
highest possible performance and ASPM L1.2 wouldn't be used.

[1] PCIe r4.0, sec 5.5.1
Link: https://bugzilla.kernel.org/show_bug.cgi?id=201469
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

Comments

Vidya Sagar Feb. 13, 2019, 5:23 a.m. UTC | #1 
On 2/12/2019 4:25 AM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Latency Tolerance Reporting (LTR) allows Endpoints and Switch Upstream
> Ports to report their latency requirements to upstream components.  If ASPM
> L1 PM substates are enabled, the LTR information helps determine when a
> Link enters L1.2 [1].
>
> Software must set the maximum latency values in the LTR Capability based on
> characteristics of the platform, then set LTR Mechanism Enable in the
> Device Control 2 register in the PCIe Capability.  The device can then use
> LTR to report its latency tolerance.
>
> If the device reports a maximum latency value of zero, that means the
> device requires the highest possible performance and the ASPM L1.2 substate
> is effectively disabled.
>
> We put devices in D3 for suspend, and we assume their internal state is
> lost.  On resume, previously we did not restore the LTR Capability, but we
> did restore the LTR Mechanism Enable bit, so devices would request the
> highest possible performance and ASPM L1.2 wouldn't be used.
>
> [1] PCIe r4.0, sec 5.5.1
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=201469
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/pci.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index c9d8e3c837de..13d65991c77b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1233,7 +1233,6 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
>   	pcie_capability_write_word(dev, PCI_EXP_SLTCTL2, cap[i++]);
>   }
>   
> -
>   static int pci_save_pcix_state(struct pci_dev *dev)
>   {
>   	int pos;
> @@ -1270,6 +1269,45 @@ 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;
> +	u16 *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;
> +	}
> +
> +	cap = (u16 *)&save_state->cap.data[0];
> +	pci_read_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap++);
> +	pci_read_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, cap++);
> +}
> +
> +static void pci_restore_ltr_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	int ltr;
> +	u16 *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;
> +
> +	cap = (u16 *)&save_state->cap.data[0];
> +	pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++);
> +	pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++);
> +}
>   
>   /**
>    * pci_save_state - save the PCI configuration space of a device before suspending
> @@ -1291,6 +1329,7 @@ int pci_save_state(struct pci_dev *dev)
>   	if (i != 0)
>   		return i;
>   
> +	pci_save_ltr_state(dev);
>   	pci_save_dpc_state(dev);
>   	return pci_save_vc_state(dev);
>   }
> @@ -1390,7 +1429,12 @@ void pci_restore_state(struct pci_dev *dev)
>   	if (!dev->state_saved)
>   		return;
>   
> -	/* PCI Express register must be restored first */
> +	/*
> +	 * 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);
> @@ -2998,6 +3042,11 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
>   	if (error)
>   		pci_err(dev, "unable to preallocate PCI-X save buffer\n");
>   
> +	error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_LTR,
> +					    2 * sizeof(u16));
> +	if (error)
> +		pci_err(dev, "unable to allocate suspend buffer for LTR\n");
> +
>   	pci_allocate_vc_save_buffers(dev);
>   }
Don't we have to save and restore L1SS control registers (PCI_L1SS_CTL1 
& PCI_L1SS_CTL2) as well?
>   
>
Bjorn Helgaas Feb. 13, 2019, 5:40 p.m. UTC | #2 
On Wed, Feb 13, 2019 at 10:53:01AM +0530, Vidya Sagar wrote:
> On 2/12/2019 4:25 AM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Latency Tolerance Reporting (LTR) allows Endpoints and Switch Upstream
> > Ports to report their latency requirements to upstream components.  If ASPM
> > L1 PM substates are enabled, the LTR information helps determine when a
> > Link enters L1.2 [1].
> > 
> > Software must set the maximum latency values in the LTR Capability based on
> > characteristics of the platform, then set LTR Mechanism Enable in the
> > Device Control 2 register in the PCIe Capability.  The device can then use
> > LTR to report its latency tolerance.
> > 
> > If the device reports a maximum latency value of zero, that means the
> > device requires the highest possible performance and the ASPM L1.2 substate
> > is effectively disabled.
> > 
> > We put devices in D3 for suspend, and we assume their internal state is
> > lost.  On resume, previously we did not restore the LTR Capability, but we
> > did restore the LTR Mechanism Enable bit, so devices would request the
> > highest possible performance and ASPM L1.2 wouldn't be used.
> > 
> > [1] PCIe r4.0, sec 5.5.1
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=201469
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >   drivers/pci/pci.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 51 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index c9d8e3c837de..13d65991c77b 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1233,7 +1233,6 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
> >   	pcie_capability_write_word(dev, PCI_EXP_SLTCTL2, cap[i++]);
> >   }
> > -
> >   static int pci_save_pcix_state(struct pci_dev *dev)
> >   {
> >   	int pos;
> > @@ -1270,6 +1269,45 @@ 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;
> > +	u16 *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;
> > +	}
> > +
> > +	cap = (u16 *)&save_state->cap.data[0];
> > +	pci_read_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap++);
> > +	pci_read_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, cap++);
> > +}
> > +
> > +static void pci_restore_ltr_state(struct pci_dev *dev)
> > +{
> > +	struct pci_cap_saved_state *save_state;
> > +	int ltr;
> > +	u16 *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;
> > +
> > +	cap = (u16 *)&save_state->cap.data[0];
> > +	pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++);
> > +	pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++);
> > +}
> >   /**
> >    * pci_save_state - save the PCI configuration space of a device before suspending
> > @@ -1291,6 +1329,7 @@ int pci_save_state(struct pci_dev *dev)
> >   	if (i != 0)
> >   		return i;
> > +	pci_save_ltr_state(dev);
> >   	pci_save_dpc_state(dev);
> >   	return pci_save_vc_state(dev);
> >   }
> > @@ -1390,7 +1429,12 @@ void pci_restore_state(struct pci_dev *dev)
> >   	if (!dev->state_saved)
> >   		return;
> > -	/* PCI Express register must be restored first */
> > +	/*
> > +	 * 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);
> > @@ -2998,6 +3042,11 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
> >   	if (error)
> >   		pci_err(dev, "unable to preallocate PCI-X save buffer\n");
> > +	error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_LTR,
> > +					    2 * sizeof(u16));
> > +	if (error)
> > +		pci_err(dev, "unable to allocate suspend buffer for LTR\n");
> > +
> >   	pci_allocate_vc_save_buffers(dev);
> >   }

> Don't we have to save and restore L1SS control registers (PCI_L1SS_CTL1 &
> PCI_L1SS_CTL2) as well?

I think you're right!  It's getting embarrassing how much stuff we
missed.  I'll work on this, and look for other capabilities where we
might be missing save/restore.

Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c9d8e3c837de..13d65991c77b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1233,7 +1233,6 @@  static void pci_restore_pcie_state(struct pci_dev *dev)
 	pcie_capability_write_word(dev, PCI_EXP_SLTCTL2, cap[i++]);
 }
 
-
 static int pci_save_pcix_state(struct pci_dev *dev)
 {
 	int pos;
@@ -1270,6 +1269,45 @@  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;
+	u16 *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;
+	}
+
+	cap = (u16 *)&save_state->cap.data[0];
+	pci_read_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap++);
+	pci_read_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, cap++);
+}
+
+static void pci_restore_ltr_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	int ltr;
+	u16 *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;
+
+	cap = (u16 *)&save_state->cap.data[0];
+	pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++);
+	pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++);
+}
 
 /**
  * pci_save_state - save the PCI configuration space of a device before suspending
@@ -1291,6 +1329,7 @@  int pci_save_state(struct pci_dev *dev)
 	if (i != 0)
 		return i;
 
+	pci_save_ltr_state(dev);
 	pci_save_dpc_state(dev);
 	return pci_save_vc_state(dev);
 }
@@ -1390,7 +1429,12 @@  void pci_restore_state(struct pci_dev *dev)
 	if (!dev->state_saved)
 		return;
 
-	/* PCI Express register must be restored first */
+	/*
+	 * 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);
@@ -2998,6 +3042,11 @@  void pci_allocate_cap_save_buffers(struct pci_dev *dev)
 	if (error)
 		pci_err(dev, "unable to preallocate PCI-X save buffer\n");
 
+	error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_LTR,
+					    2 * sizeof(u16));
+	if (error)
+		pci_err(dev, "unable to allocate suspend buffer for LTR\n");
+
 	pci_allocate_vc_save_buffers(dev);
 }