| Message ID | 20180905203546.21921-4-keith.busch@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI, error handling and hot plug | expand |
On Wed, Sep 05, 2018 at 02:35:29PM -0600, Keith Busch wrote: > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4485,21 +4485,33 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active) > bool ret; > u16 lnk_status; > > + /* > + * PCIe 4.0r1 6.6.1, a component must enter LTSSM Detect within 20ms, > + * after which we should expect an link active if the reset was > + * successful. If so, software must wait a minimum 100ms before sending > + * configuration requests to devices downstream this port. > + * > + * If the link fails to activate, either the device was physically > + * removed or the link is permanently failed. > + */ > + if (active) > + msleep(20); This function is also called by pciehp when bringing up the slot, yet I assume the above delay only applies to a Fundamental Reset, not hotplug? > + if (active && ret) > + msleep(100); This delay is already observed by pciehp_check_link_status() after calling pcie_wait_link_active(). Thanks, Lukas
On Thu, Sep 06, 2018 at 01:42:15PM +0200, Lukas Wunner wrote: > On Wed, Sep 05, 2018 at 02:35:29PM -0600, Keith Busch wrote: > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -4485,21 +4485,33 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active) > > bool ret; > > u16 lnk_status; > > > > + /* > > + * PCIe 4.0r1 6.6.1, a component must enter LTSSM Detect within 20ms, > > + * after which we should expect an link active if the reset was > > + * successful. If so, software must wait a minimum 100ms before sending > > + * configuration requests to devices downstream this port. > > + * > > + * If the link fails to activate, either the device was physically > > + * removed or the link is permanently failed. > > + */ > > + if (active) > > + msleep(20); > > This function is also called by pciehp when bringing up the slot, > yet I assume the above delay only applies to a Fundamental Reset, > not hotplug? Should be any conventional reset, which includes the "cold reset" applying power to a hot add component. > > + if (active && ret) > > + msleep(100); > > This delay is already observed by pciehp_check_link_status() > after calling pcie_wait_link_active(). Yeah, that's gone in patch 13. This series became larger than I originally expected, and there might be a more logical shuffling.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 30b260332a10..8eb1d869fc98 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4485,21 +4485,33 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active) bool ret; u16 lnk_status; + /* + * PCIe 4.0r1 6.6.1, a component must enter LTSSM Detect within 20ms, + * after which we should expect an link active if the reset was + * successful. If so, software must wait a minimum 100ms before sending + * configuration requests to devices downstream this port. + * + * If the link fails to activate, either the device was physically + * removed or the link is permanently failed. + */ + if (active) + msleep(20); for (;;) { pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status); ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA); if (ret == active) - return true; + break; if (timeout <= 0) break; msleep(10); timeout -= 10; } - - pci_info(pdev, "Data Link Layer Link Active not %s in 1000 msec\n", - active ? "set" : "cleared"); - - return false; + if (active && ret) + msleep(100); + else if (ret != active) + pci_info(pdev, "Data Link Layer Link Active not %s in 1000 msec\n", + active ? "set" : "cleared"); + return ret == active; } void pci_reset_secondary_bus(struct pci_dev *dev)
The spec has hard timing requirements when waiting for a link to become active. This patch implements those hard delays when waiting for an active link so each caller doesn't have to respect those timings. Signed-off-by: Keith Busch <keith.busch@intel.com> --- drivers/pci/pci.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)