On Sat, Dec 31, 2022 at 07:33:38PM +0100, Lukas Wunner wrote: > Sheng Bi reports that pci_bridge_secondary_bus_reset() may fail to wait > for devices on the secondary bus to become accessible after reset: > > Although it does call pci_dev_wait(), it erroneously passes the bridge's > pci_dev rather than that of a child. The bridge of course is always > accessible while its secondary bus is reset, so pci_dev_wait() returns > immediately. > > Sheng Bi proposes introducing a new pci_bridge_secondary_bus_wait() > function which is called from pci_bridge_secondary_bus_reset(): > > https://lore.kernel.org/linux-pci/20220523171517.32407-1-windy.bi.enflame@xxxxxxxxx/ > > However we already have pci_bridge_wait_for_secondary_bus() which does > almost exactly what we need. So far it's only called on resume from > D3cold (which implies a Fundamental Reset per PCIe r6.0 sec 5.8). > Re-using it for Secondary Bus Resets is a leaner and more rational > approach than introducing a new function. > > That only requires a few minor tweaks: > > - Amend pci_bridge_wait_for_secondary_bus() to await accessibility of > the first device on the secondary bus by calling pci_dev_wait() after > performing the prescribed delays. pci_dev_wait() needs two parameters, > a reset reason and a timeout, which callers must now pass to > pci_bridge_wait_for_secondary_bus(). The timeout is 1 sec for resume > (PCIe r6.0 sec 6.6.1) and 60 sec for reset (commit 821cdad5c46c ("PCI: > Wait up to 60 seconds for device to become ready after FLR")). > > - Amend pci_bridge_wait_for_secondary_bus() to return 0 on success or > -ENOTTY on error for consumption by pci_bridge_secondary_bus_reset(). > > - Drop an unnecessary 1 sec delay from pci_reset_secondary_bus() which > is now performed by pci_bridge_wait_for_secondary_bus(). A static > delay this long is only necessary for Conventional PCI, so modern > PCIe systems benefit from shorter reset times as a side effect. > > Fixes: 6b2f1351af56 ("PCI: Wait for device to become ready after secondary bus reset") > Reported-by: Sheng Bi <windy.bi.enflame@xxxxxxxxx> > Tested-by: Ravi Kishore Koppuravuri <ravi.kishore.koppuravuri@xxxxxxxxx> > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx # v4.17+ > Cc: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> > --- > drivers/pci/pci-driver.c | 2 +- > drivers/pci/pci.c | 50 ++++++++++++++++++---------------------- > drivers/pci/pci.h | 3 ++- > 3 files changed, 25 insertions(+), 30 deletions(-) > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index a2ceeacc33eb..02e84c87f41a 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -572,7 +572,7 @@ static void pci_pm_default_resume_early(struct pci_dev *pci_dev) > > static void pci_pm_bridge_power_up_actions(struct pci_dev *pci_dev) > { > - pci_bridge_wait_for_secondary_bus(pci_dev); > + pci_bridge_wait_for_secondary_bus(pci_dev, "resume", 1000); It sounds like this 1000 ms value is prescribed by sec 6.6.1, so we should have a #define for it. I know we didn't use one even before, but this seems like a a good opportunity to add it. > /** > * pci_bridge_wait_for_secondary_bus - Wait for secondary bus to be accessible > * @dev: PCI bridge > + * @reset_type: reset type in human-readable form > + * @timeout: maximum time to wait for devices on secondary bus I think we should mention here that the timeout is in milliseconds. Bjorn
