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