Re: [PATCH v4] PCI/PM: Shorten pci_bridge_wait_for_secondary_bus() wait time for slow links

[Lukas Wunner <lukas@wunner.de>]

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."

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?

Thanks,

Lukas