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@gmail.com>
---
 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

On Tue, May 24, 2022 at 01:15:17AM +0800, Sheng Bi 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@gmail.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v4.17+

Code-wise, this LGTM.  There are a few things that could be
improved in the commit message, e.g. in the last paragraph,
"changing" (gerund form) is not proper English and the
imperative form "change" would be correct here.  However,
these details are difficult to get right for anyone who is
not an English native speaker and often Bjorn will wordsmith
the commit message to perfect it.

See here for some of the things Bjorn looks for:
https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com/

Bjorn may not find the time to look over your patch immediately,
so please be patient.

Thanks!

Lukas

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@gmail.com> 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@gmail.com>
> ---
>  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
>