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
