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 Fri, Apr 14, 2023 at 01:11:47PM +0300, Mika Westerberg wrote:
> To summarize the v4 patch would look something like below. Only compile
> tested but I will run real testing later today. I think it now includes
> the 1s optimization and also checking of the active link reporting
> support for the devices behind slow links. Let me know is I missed
> something.

The patch seems to be based on a branch which has the v3 patch applied
instead of on pci.git/reset, and that makes it slightly more difficult
to review, but from a first glance it LGTM.


> It is getting rather complex unfortunately :(

I disagree. :)  Basically the Gen1/Gen2 situation becomes a special case
because it has specific timing requirements (need to observe a delay
before accessing the Secondary Bus, instead of waiting for the link)
and it doesn't necessarily support link active reporting.  So special
casing it seems fair to me.


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

It *might* be worth avoiding the rewrapping of the first 3 lines to
make the patch smaller, your choice.


> +
> +		/*
> +		 * 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.
> +		 */

Nit:  Drop the "one more time" because it seems this is actually the
first time the link is checked.


Somewhat tangentially, I note that pcie_wait_for_link_delay() has a
"if (!pdev->link_active_reporting)" branch right at its top, however
pci_bridge_wait_for_secondary_bus() only calls the function in the
Gen3+ (> 5 GT/s) case, which always supports link active reporting.

Thus the branch is never taken when pcie_wait_for_link_delay() is called
from pci_bridge_wait_for_secondary_bus().  There's only one other caller,
pcie_wait_for_link().  So moving the "if (!pdev->link_active_reporting)"
branch to pcie_wait_for_link() *might* make the code more readable.
Just a thought.

Thanks,

Lukas