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

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

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

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.
>