[PATCH 1/1] PCI: Wait for Link before restoring Downstream Buses

[PATCH 1/1] PCI: Wait for Link before restoring Downstream Buses

[Ilpo Järvinen]

__pci_reset_bus() calls pci_bridge_secondary_bus_reset() to perform the
reset and also waits for the Secondary Bus to become again accessible.
__pci_reset_bus() then calls pci_bus_restore_locked() that restores the
PCI devices connected to the bus, and if necessary, recursively restores
also the subordinate buses and their devices.

The logic in pci_bus_restore_locked() does not take into account that
after restoring a device on one level, there might be another Link
Downstream that can only start to come up after restore has been
performed for its Downstream Port device. That is, the Link may
require additional wait until it becomes accessible.

Similarly, pci_slot_restore_locked() lacks wait.

Amend pci_bus_restore_locked() and pci_slot_restore_locked() to wait
for the Secondary Bus before recursively performing the restore of that
bus.

Fixes: 090a3c5322e9 ("PCI: Add pci_reset_slot() and pci_reset_bus()")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

NOTE TO MAINTAINER: I've not seen anything to actually trigger this issue
but only realized this problem exist while looking into the other issues
related to bus reset/restore. The fix regardless seems to make sense so
sending it out.

 drivers/pci/pci.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e3a49f66982d..98c7b732998a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5671,8 +5671,10 @@ static void pci_bus_restore_locked(struct pci_bus *bus)
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		pci_dev_restore(dev);
-		if (dev->subordinate)
+		if (dev->subordinate) {
+			pci_bridge_wait_for_secondary_bus(dev, "bus reset");
 			pci_bus_restore_locked(dev->subordinate);
+		}
 	}
 }
 
@@ -5706,8 +5708,10 @@ static void pci_slot_restore_locked(struct pci_slot *slot)
 		if (!dev->slot || dev->slot != slot)
 			continue;
 		pci_dev_restore(dev);
-		if (dev->subordinate)
+		if (dev->subordinate) {
+			pci_bridge_wait_for_secondary_bus(dev, "slot reset");
 			pci_bus_restore_locked(dev->subordinate);
+		}
 	}
 }
 
-- 
2.39.2

Re: [PATCH 1/1] PCI: Wait for Link before restoring Downstream Buses

[Bjorn Helgaas]

On Thu, Aug 08, 2024 at 03:17:07PM +0300, Ilpo Järvinen wrote:
> __pci_reset_bus() calls pci_bridge_secondary_bus_reset() to perform the
> reset and also waits for the Secondary Bus to become again accessible.
> __pci_reset_bus() then calls pci_bus_restore_locked() that restores the
> PCI devices connected to the bus, and if necessary, recursively restores
> also the subordinate buses and their devices.
> 
> The logic in pci_bus_restore_locked() does not take into account that
> after restoring a device on one level, there might be another Link
> Downstream that can only start to come up after restore has been
> performed for its Downstream Port device. That is, the Link may
> require additional wait until it becomes accessible.
> 
> Similarly, pci_slot_restore_locked() lacks wait.
> 
> Amend pci_bus_restore_locked() and pci_slot_restore_locked() to wait
> for the Secondary Bus before recursively performing the restore of that
> bus.
> 
> Fixes: 090a3c5322e9 ("PCI: Add pci_reset_slot() and pci_reset_bus()")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Seems reasonable to me, applied to pci/reset for v6.12, thanks!

> ---
> 
> NOTE TO MAINTAINER: I've not seen anything to actually trigger this issue
> but only realized this problem exist while looking into the other issues
> related to bus reset/restore. The fix regardless seems to make sense so
> sending it out.
> 
>  drivers/pci/pci.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e3a49f66982d..98c7b732998a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5671,8 +5671,10 @@ static void pci_bus_restore_locked(struct pci_bus *bus)
>  
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>  		pci_dev_restore(dev);
> -		if (dev->subordinate)
> +		if (dev->subordinate) {
> +			pci_bridge_wait_for_secondary_bus(dev, "bus reset");
>  			pci_bus_restore_locked(dev->subordinate);
> +		}
>  	}
>  }
>  
> @@ -5706,8 +5708,10 @@ static void pci_slot_restore_locked(struct pci_slot *slot)
>  		if (!dev->slot || dev->slot != slot)
>  			continue;
>  		pci_dev_restore(dev);
> -		if (dev->subordinate)
> +		if (dev->subordinate) {
> +			pci_bridge_wait_for_secondary_bus(dev, "slot reset");
>  			pci_bus_restore_locked(dev->subordinate);
> +		}
>  	}
>  }
>  
> -- 
> 2.39.2
>