Hi Bjorn, Alex, Lukas,
Is this acceptable or anything needs to be improved?
Thanks
windy
On Tue, May 24, 2022 at 1:15 AM Sheng Bi <windy.bi.enflame@xxxxxxxxx> wrote:
>
> pci_bridge_secondary_bus_reset() triggers SBR followed by 1 second sleep,
> and then uses pci_dev_wait() for waiting device ready. The dev parameter
> passes to the wait function is currently the bridge itself, but not the
> device been reset.
>
> If we call pci_bridge_secondary_bus_reset() to trigger SBR to a device,
> there is 1 second sleep but not waiting device ready, since the bridge
> is always ready while resetting downstream devices. pci_dev_wait() here
> is a no-op actually. This would be risky in the case which the device
> becomes ready after more than 1 second, especially while hotplug enabled.
> The late coming hotplug event after 1 second will trigger hotplug module
> to remove/re-insert the device.
>
> Instead of waiting ready of bridge itself, changing to wait all the
> downstream devices become ready with timeout PCIE_RESET_READY_POLL_MS
> after SBR, considering all downstream devices are affected during SBR.
> Once one of the devices doesn't reappear within the timeout, return
> -ENOTTY to indicate SBR doesn't complete successfully.
>
> Fixes: 6b2f1351af56 ("PCI: Wait for device to become ready after secondary bus reset")
> Signed-off-by: Sheng Bi <windy.bi.enflame@xxxxxxxxx>
> ---
> drivers/pci/pci.c | 30 +++++++++++++++++++++++++++++-
> 1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index eb7c0a08ff57..4653a9ae6e5b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5049,6 +5049,34 @@ void pci_bridge_wait_for_secondary_bus(struct pci_dev *dev)
> }
> }
>
> +static int pci_bridge_secondary_bus_wait(struct pci_dev *bridge, int timeout)
> +{
> + struct pci_dev *dev;
> + unsigned long start_jiffies;
> +
> + down_read(&pci_bus_sem);
> +
> + if (!bridge->subordinate || list_empty(&bridge->subordinate->devices)) {
> + up_read(&pci_bus_sem);
> + return 0;
> + }
> +
> + list_for_each_entry(dev, &bridge->subordinate->devices, bus_list) {
> + start_jiffies = jiffies;
> +
> + if (timeout < 0 || pci_dev_wait(dev, "bus reset", timeout)) {
> + up_read(&pci_bus_sem);
> + return -ENOTTY;
> + }
> +
> + timeout -= jiffies_to_msecs(jiffies - start_jiffies);
> + }
> +
> + up_read(&pci_bus_sem);
> +
> + return 0;
> +}
> +
> void pci_reset_secondary_bus(struct pci_dev *dev)
> {
> u16 ctrl;
> @@ -5092,7 +5120,7 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
> {
> pcibios_reset_secondary_bus(dev);
>
> - return pci_dev_wait(dev, "bus reset", PCIE_RESET_READY_POLL_MS);
> + return pci_bridge_secondary_bus_wait(dev, PCIE_RESET_READY_POLL_MS);
> }
> EXPORT_SYMBOL_GPL(pci_bridge_secondary_bus_reset);
>
>
> base-commit: 617c8a1e527fadaaec3ba5bafceae7a922ebef7e
> --
> 2.36.1
>
