Hi,
On Fri, Apr 21, 2023 at 10:51:14PM +0200, Lukas Wunner wrote:
> On Tue, Apr 18, 2023 at 10:28:08AM +0300, 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 brokine device so use the minimum
> ^^^^^^^
> broken
>
>
> > @@ -5027,14 +5032,29 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
> > if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT) {
> > 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");
> > - return -ENOTTY;
> > +
> > + /*
> > + * 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) {
> > + u16 status;
> > +
> > + pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &status);
> > + if (!(status & PCI_EXP_LNKSTA_DLLLA))
> > + return -ENOTTY;
> > }
> > +
> > + return pci_dev_wait(child, reset_type, PCI_RESET_WAIT - delay);
> > + }
>
> So above in the Gen1/Gen2 case (<= 5 GT/s), a delay of 100 msec is afforded
> and if the link isn't up by then, the function returns an error.
>
> Doesn't that violate PCIe r6.0.1 sec 6.6.1 that states:
>
> "system software must allow at least 1.0 s following exit from a
> Conventional Reset of a device, before determining that the device
> is broken if it fails to return a Successful Completion status for
> a valid Configuration Request. This period is independent of how
> quickly Link training completes."
Yes, it does :( Missed that last sentence.
> I think what we can do here is:
>
> if (!pci_dev_wait(child, reset_type, PCI_RESET_WAIT - delay))
> return 0;
>
> 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);
>
> In other words, if link active reporting is unsupported, we can only
> afford the 1 second prescribed by the spec and that's it. If the
> subordinate device is still inaccessible after that, reset recovery
> failed.
>
> If link active reporting is supported and the link is up, then we know
> the device is accessible but may need more time. In that case the
> full 60 seconds are afforded.
>
> Does that make sense?
Yes, it does, thanks! I will send an updated version with this (and the
typo) fixed after the merge window closes.
