Re: [PATCH] PCI: Fix PCIe SBR dev/link wait error

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 11084 bytes --]

On Wed, 26 Nov 2025, guanghui.fgh wrote:

> Refer to the previous email discussion replies：
> 
> 1. When a multifunction device is connected to a PCIe slot that supports hotplug, 
>    during the passthrogh device to guest process based on VFIO, some devices
>    need to perform a hot reset to initialize the device:
>    vfio_pci_dev_set_hot_reset ---> __pci_reset_slot/__pci_reset_bus --->
>    pci_bridge_secondary_bus_reset ---> pci_bridge_wait_for_secondary_bus.
>
>    After __pci_reset_slot/__pci_reset_bus calls pci_bridge_wait_for_secondary_bus,
>    the device will be restored via pci_dev_restore. 

...and then they call to pci_slot_restore_locked()/pci_bus_restore_locked() 
that do recursively wait + restore config spaces since the commit 
3e40aa29d47e ("PCI: Wait for Link before restoring Downstream Buses").

>    However, when a       ======      multifunction PCIe device      =========
>    is connected, executing pci_bridge_wait_for_secondary_bus only guarantees the restoration
>    of a random device. For other devices that are still restoring, executing pci_dev_restore cannot
>    restore the device state normally, resulting in errors or even device offline.

Addressing this doesn't require recursion AFAICT.

> 2. In the PCIe dpc_handler process, do pcie_do_recovery(pdev, pci_channel_io_frozen, dpc_reset_link)
>    to restores the PCIe link. 
>    But dpc_reset_link waits for the link to recover via pci_bridge_wait_for_secondary_bus before returning.
>    Similarly, pcie_do_recovery restores devices via pci_walk_bridge(bridge, report_resume, &status),
>
>    ======      which also requires pci_bridge_wait_for_secondary_bus to wait for all devices to recover completely.
>    For other devices that are still restoring, executing report_resume cannot restore the device state normally,
>    resulting in errors or even device offline.

This might lack wait for subordinate buses and ordering with restore, but 
I don't think the right place to add that is 
pci_bridge_wait_for_secondary_bus() that is used by others.

> 3. The PCIe specification only constrains the minimum wait time under different resets. In the historical kernel implementation,
>    SBR wait also did not need to restore the PCIe device configuration space before waiting for the coordinate.



I'm not saying there isn't anything to fix/change but I'm not convinced by 
the approach taken by your patch.

--
 i.

> ------------------------------------------------------------------
> On Wed, 26 Nov 2025, guanghui.fgh wrote:
> 
> > 1. Does this add recursion without restoring config space on each level 
> > before starting the child wait?
> > 
> > Yes
> > The current implementation does not require restoring the PCIe device 
> > configuration space. The status is determined during the waiting 
> > process, either based on software-recorded status or on the device's RO 
> > and HWINIT type registers.
> 
> What guarantees a link even come up without restoring the config space
> first (it may work in your case but is that true universally)?
> 
> The commit 3e40aa29d47e ("PCI: Wait for Link before restoring Downstream 
> Buses") tried to address this sub-hierarchy wait issue within the 
> recursive bus restore (without me ever encountering this problem for 
> real). So I don't understand why you had to add the recursion to the wait 
> side as well?
> 
> -- 
>  i.
> 
> > > When executing a PCIe secondary bus reset, all downstream switches and
> > > endpoints will generate reset events. Simultaneously, all PCIe links
> > > will undergo retraining, and each link will independently re-execute the
> > > LTSSM state machine training. Therefore, after executing the SBR, it is
> > > necessary to wait for all downstream links and devices to complete
> > > recovery. Otherwise, after the SBR returns, accessing devices with some
> > > links or endpoints not yet fully recovered may result in driver errors,
> > > or even trigger device offline issues.
> > > 
> > > Signed-off-by: Guanghui Feng <guanghuifeng@linux.alibaba.com>
> > > Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> > > ---
> > >  drivers/pci/pci.c | 112 ++++++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 94 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index b14dd064006c..9cf13fe69d94 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -4788,9 +4788,74 @@ static int pci_bus_max_d3cold_delay(const struct pci_bus *bus)
> > >   return max(min_delay, max_delay);
> > >  }
> > >  
> > > +int __pci_bridge_wait_for_secondary_bus(struct pci_dev *, unsigned long, char *);
> > > +
> > > +static int pci_dev_wait_child(struct pci_dev *pdev, unsigned long start_t, int timeout,
> > > +    char *reset_type)
> > > +{
> > > + struct pci_dev *child, **dev = NULL;
> > > + int ct = 0, i = 0, ret = 0, left = 1;
> > > + unsigned long dev_start_t;
> > > +
> > > + down_read(&pci_bus_sem);
> > > +
> > > + list_for_each_entry(child, &pdev->subordinate->devices, bus_list)
> > > +  ct++;
> > > +
> > > + if (ct) {
> > > +  dev = kzalloc(sizeof(struct pci_dev *) * ct, GFP_KERNEL);
> > > +
> > > +  if(!dev) {
> > > +   pci_err(pdev, "dev mem alloc err\n");
> > > +   up_read(&pci_bus_sem);
> > > +   return -ENOMEM;
> > > +  }
> > > +
> > > +  list_for_each_entry(child, &pdev->subordinate->devices, bus_list)
> > > +   dev[i++] = pci_dev_get(child);
> > > + }
> > > +
> > > + up_read(&pci_bus_sem);
> > > +
> > > + for (i = 0; i < ct; ++i) {
> > > +  left = 1;
> > > +
> > > +dev_wait:
> > > +  dev_start_t = jiffies;
> > > +  ret = pci_dev_wait(dev[i], reset_type, left);
> > > +  timeout -= jiffies_to_msecs(jiffies - dev_start_t);
> > > +
> > > +  if (ret) {
> > > +   if (pci_dev_is_disconnected(dev[i]))
> > > +    continue;
> > > +
> > > +   if (timeout <= 0)
> > > +    goto end;
> > > +
> > > +   left <<= 1;
> > > +   left = timeout > left ? left : timeout;
> > > +   goto dev_wait;
> > > +  }
> > > + }
> > > +
> > > + for (i = 0; i < ct; ++i) {
> > > +  ret = __pci_bridge_wait_for_secondary_bus(dev[i], start_t, reset_type);
> > 
> > Does this add recursion without restoring config space on each level 
> > before starting the child wait?
> > 
> > > +  if (ret)
> > > +   break;
> > > + }
> > > +
> > > +end:
> > > + for (i = 0; i < ct; ++i)
> > > +  pci_dev_put(dev[i]);
> > > +
> > > + kfree(dev);
> > > + return ret;
> > > +}
> > > +
> > >  /**
> > > - * pci_bridge_wait_for_secondary_bus - Wait for secondary bus to be accessible
> > > + * __pci_bridge_wait_for_secondary_bus - Wait for secondary bus to be accessible
> > >   * @dev: PCI bridge
> > > + * @start_t: wait start jiffies time
> > >   * @reset_type: reset type in human-readable form
> > >   *
> > >   * Handle necessary delays before access to the devices on the secondary
> > > @@ -4804,10 +4869,9 @@ static int pci_bus_max_d3cold_delay(const struct pci_bus *bus)
> > >   * Return 0 on success or -ENOTTY if the first device on the secondary bus
> > >   * failed to become accessible.
> > >   */
> > > -int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
> > > +int __pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, unsigned long start_t, char *reset_type)
> > >  {
> > > - struct pci_dev *child __free(pci_dev_put) = NULL;
> > > - int delay;
> > > + int delay, left;
> > >  
> > >   if (pci_dev_is_disconnected(dev))
> > >    return 0;
> > > @@ -4835,8 +4899,6 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
> > >    return 0;
> > >   }
> > >  
> > > - child = pci_dev_get(list_first_entry(&dev->subordinate->devices,
> > > -          struct pci_dev, bus_list));
> > >   up_read(&pci_bus_sem);
> > >  
> > >   /*
> > > @@ -4844,8 +4906,12 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
> > >    * accessing the device after reset (that is 1000 ms + 100 ms).
> > >    */
> > >   if (!pci_is_pcie(dev)) {
> > > -  pci_dbg(dev, "waiting %d ms for secondary bus\n", 1000 + delay);
> > > -  msleep(1000 + delay);
> > > +  left = 1000 + delay - jiffies_to_msecs(jiffies - start_t);
> > > +  pci_dbg(dev, "waiting %d ms for secondary bus\n", left > 0 ? left : 0);
> > > +
> > > +  if (left > 0)
> > > +   msleep(left);
> > > +
> > >    return 0;
> > >   }
> > >  
> > > @@ -4870,10 +4936,14 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
> > >   if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT) {
> > >    u16 status;
> > >  
> > > -  pci_dbg(dev, "waiting %d ms for downstream link\n", delay);
> > > -  msleep(delay);
> > > +  left = delay - jiffies_to_msecs(jiffies - start_t);
> > > +  pci_dbg(dev, "waiting %d ms for downstream link\n", left > 0 ? left : 0);
> > > +
> > > +  if (left > 0)
> > > +   msleep(left);
> > >  
> > > -  if (!pci_dev_wait(child, reset_type, PCI_RESET_WAIT - delay))
> > > +  left = PCI_RESET_WAIT - jiffies_to_msecs(jiffies - start_t);
> > > +  if(!pci_dev_wait_child(dev, start_t, left > 0 ? left : 0, reset_type))
> > >     return 0;
> > >  
> > >    /*
> > > @@ -4888,20 +4958,26 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
> > >    if (!(status & PCI_EXP_LNKSTA_DLLLA))
> > >     return -ENOTTY;
> > >  
> > > -  return pci_dev_wait(child, reset_type,
> > > -        PCIE_RESET_READY_POLL_MS - PCI_RESET_WAIT);
> > > +  left = PCIE_RESET_READY_POLL_MS - jiffies_to_msecs(jiffies - start_t);
> > > +  return pci_dev_wait_child(dev, start_t, left > 0 ? left : 0, reset_type);
> > >   }
> > >  
> > > - pci_dbg(dev, "waiting %d ms for downstream link, after activation\n",
> > > -  delay);
> > > - if (!pcie_wait_for_link_delay(dev, true, delay)) {
> > > + left = delay - jiffies_to_msecs(jiffies - start_t);
> > > + pci_dbg(dev, "waiting %d ms for downstream link, after activation\n", left > 0 ? left : 0);
> > > +
> > > + if (!pcie_wait_for_link_delay(dev, true, left > 0 ? left : 0)) {
> > >    /* Did not train, no need to wait any further */
> > >    pci_info(dev, "Data Link Layer Link Active not set in %d msec\n", delay);
> > >    return -ENOTTY;
> > >   }
> > >  
> > > - return pci_dev_wait(child, reset_type,
> > > -       PCIE_RESET_READY_POLL_MS - delay);
> > > + left = PCIE_RESET_READY_POLL_MS - jiffies_to_msecs(jiffies - start_t);
> > > + return pci_dev_wait_child(dev, start_t, left > 0 ? left : 0, reset_type);
> > > +}
> > > +
> > > +int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
> > > +{
> > > + return __pci_bridge_wait_for_secondary_bus(dev, jiffies, reset_type);
> > >  }
> > >  
> > >  void pci_reset_secondary_bus(struct pci_dev *dev)
> > > 
> > 
> > -- 
> >  i.
> > 
> 

-- 
 i.