On 4/24/23 11:47 PM, Mika Westerberg wrote: > With slow links (<= 5GT/s) active link reporting is not mandatory, so if > a device is disconnected during system sleep we might end up waiting for > it to respond for ~60s slowing down resume time. PCIe spec r6.0 sec > 6.6.1 mandates that the system software must wait for at least 1s before > it can determine the device as broken device so use the minimum > requirement for slow links and bail out if we do not get reply within > 1s. However, if the port supports active link reporting we can continue > the wait following what we do with the fast links. > > This should make system resume time faster for slow links as well while > still following the PCIe spec. > > While there move the PCI_RESET_WAIT constant into pci.c because it is > not used outside of that file anymore. > > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> > --- Looks good to me. Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> > Hi all, > > The previous version of the patch can be found here: > > https://lore.kernel.org/linux-pci/20230418072808.10431-1-mika.westerberg@xxxxxxxxxxxxxxx/ > > Changes from the previous version: > > * Observe the mandatory 1s delay before looking at the active link > reporting as suggesteed by Lukas. > > drivers/pci/pci.c | 49 +++++++++++++++++++++++++++++++++++------------ > drivers/pci/pci.h | 7 ------- > 2 files changed, 37 insertions(+), 19 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 0b4f3b08f780..6bc0eeeb37fa 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -64,6 +64,13 @@ struct pci_pme_device { > > #define PME_TIMEOUT 1000 /* How long between PME checks */ > > +/* > + * Following exit from Conventional Reset, devices must be ready within 1 sec > + * (PCIe r6.0 sec 6.6.1). A D3cold to D0 transition implies a Conventional > + * Reset (PCIe r6.0 sec 5.8). > + */ > +#define PCI_RESET_WAIT 1000 /* msec */ > + > /* > * Devices may extend the 1 sec period through Request Retry Status > * completions (PCIe r6.0 sec 2.3.1). The spec does not provide an upper > @@ -5012,11 +5019,9 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type) > * > * However, 100 ms is the minimum and the PCIe spec says the > * software must allow at least 1s before it can determine that the > - * device that did not respond is a broken device. There is > - * evidence that 100 ms is not always enough, for example certain > - * Titan Ridge xHCI controller does not always respond to > - * configuration requests if we only wait for 100 ms (see > - * https://bugzilla.kernel.org/show_bug.cgi?id=203885). > + * device that did not respond is a broken device. Also device can > + * take longer than that to respond if it indicates so through Request > + * Retry Status completions. > * > * Therefore we wait for 100 ms and check for the device presence > * until the timeout expires. > @@ -5025,16 +5030,36 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type) > return 0; > > if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT) { > + u16 status; > + > pci_dbg(dev, "waiting %d ms for downstream link\n", delay); > msleep(delay); > - } else { > - pci_dbg(dev, "waiting %d ms for downstream link, after activation\n", > - delay); > - if (!pcie_wait_for_link_delay(dev, true, delay)) { > - /* Did not train, no need to wait any further */ > - pci_info(dev, "Data Link Layer Link Active not set in 1000 msec\n"); > + > + if (!pci_dev_wait(child, reset_type, PCI_RESET_WAIT - delay)) > + return 0; > + > + /* > + * If the port supports active link reporting we now check > + * whether the link is active and if not bail out early with > + * the assumption that the device is not present anymore. > + */ > + if (!dev->link_active_reporting) > return -ENOTTY; > - } > + > + pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &status); > + if (!(status & PCI_EXP_LNKSTA_DLLLA)) > + return -ENOTTY; > + > + return pci_dev_wait(child, reset_type, > + PCIE_RESET_READY_POLL_MS - PCI_RESET_WAIT); > + } > + > + pci_dbg(dev, "waiting %d ms for downstream link, after activation\n", > + delay); > + if (!pcie_wait_for_link_delay(dev, true, delay)) { > + /* Did not train, no need to wait any further */ > + pci_info(dev, "Data Link Layer Link Active not set in 1000 msec\n"); > + return -ENOTTY; > } > > return pci_dev_wait(child, reset_type, > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 67005a0ee381..42ce4c6e738f 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -64,13 +64,6 @@ struct pci_cap_saved_state *pci_find_saved_ext_cap(struct pci_dev *dev, > #define PCI_PM_D3HOT_WAIT 10 /* msec */ > #define PCI_PM_D3COLD_WAIT 100 /* msec */ > > -/* > - * Following exit from Conventional Reset, devices must be ready within 1 sec > - * (PCIe r6.0 sec 6.6.1). A D3cold to D0 transition implies a Conventional > - * Reset (PCIe r6.0 sec 5.8). > - */ > -#define PCI_RESET_WAIT 1000 /* msec */ > - > void pci_update_current_state(struct pci_dev *dev, pci_power_t state); > void pci_refresh_power_state(struct pci_dev *dev); > int pci_power_up(struct pci_dev *dev); -- Sathyanarayanan Kuppuswamy Linux Kernel Developer
