Re: [PATCH v3] PCI/PM: Bail out early in pci_bridge_wait_for_secondary_bus() if link is not trained

[Lukas Wunner <lukas@wunner.de>]

On Thu, Apr 13, 2023 at 01:16:42PM +0300, Mika Westerberg wrote:
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5037,6 +5037,22 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
>  		}
>  	}
>  
> +	/*
> +	 * Everything above is handling the delays mandated by the PCIe r6.0
> +	 * sec 6.6.1.
> +	 *
> +	 * If the port supports active link reporting we now check one more
> +	 * time if 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,
>  			    PCIE_RESET_READY_POLL_MS - delay);
>  }

Hm, shouldn't the added code live in the

	if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT)

branch?  For the else branch (Gen3+ devices with > 5 GT/s),
we've already waited for the link to become active, so the
additional check seems superfluous.  (But maybe I'm missing
something.)

I also note that this documentation change has been dropped
vis-à-vis v1 of the patch, not sure if that's intentional:

-	* 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).
+	* 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. Also device can take longer than
+	* that to respond if it indicates so through Request Retry Status
+	* completions.

Thanks,

Lukas