Re: [PATCH v2 0/7] PCI: Replace short msleep() calls with more precise delay functions

[Hans Zhang]

On 2025/8/23 00:46, Bjorn Helgaas wrote:
> On Fri, Aug 22, 2025 at 11:59:01PM +0800, Hans Zhang wrote:
> > This series replaces short msleep() calls (less than 20ms) with more
> > precise delay functions (fsleep() and usleep_range()) throughout the
> > PCI subsystem.
> >
> > The msleep() function with small values can sleep longer than intended
> > due to timer granularity, which can cause unnecessary delays in PCI
> > operations such as link status checking, reset handling, and hotplug
> > operations.
> >
> > These changes:
> > - Use fsleep() for delays that require precise timing (1-2ms).
> > - Use usleep_range() for delays that can benefit from a small range.
> > - Add #defines for all delay values with references to PCIe specifications.
> > - Update comments to reference the latest PCIe r7.0 specification.
> >
> > This improves the responsiveness of PCI operations while maintaining
> > compliance with PCIe specifications.

Dear Bjron,

Thank you very much for your reply.


> I would split this a little differently:
>
>    - Add #defines for values from PCIe base spec.  Make the #define
>      value match the spec value.  If there's adjustment, e.g.,

Ok. For patch 0001, I will modify it again.

>      doubling, do it at the sleep site.  Adjustment like this seems a
>      little paranoid since the spec should already have some margin
>      built into it.

Can I understand that it's enough to just use fsleep(1000) here?

patch 0001 I intend to modify it as follows:

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b0f4d98036cd..fb4aff520f64 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4963,11 +4963,8 @@ void pci_reset_secondary_bus(struct pci_dev *dev)
        ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
        pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);

-       /*
-        * PCI spec v3.0 7.6.4.2 requires minimum Trst of 1ms.  Double
-        * this to 2ms to ensure that we meet the minimum requirement.
-        */
-       msleep(2);
+       /* Wait for the reset to take effect */
+       fsleep(PCI_T_RST_SEC_BUS_DELAY_US);

        ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
        pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 34f65d69662e..9d38ef26c6a9 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -60,6 +60,9 @@ struct pcie_tlp_log;
 #define PCIE_LINK_WAIT_MAX_RETRIES     10
 #define PCIE_LINK_WAIT_SLEEP_MS                90

+/* PCIe r7.0, sec 7.5.1.3.13, requires minimum Trst of 1ms */
+#define PCI_T_RST_SEC_BUS_DELAY_US     1000
+
 /* Message Routing (r[2:0]); PCIe r6.0, sec 2.2.8 */
 #define PCIE_MSG_TYPE_R_RC     0
 #define PCIE_MSG_TYPE_R_ADDR   1



>    - Change to fsleep() (or usleep_range()) in separate patch.  There
>      might be discussion about these changes, but the #defines are
>      desirable regardless.
>
> I'm personally dubious about the places you used usleep_range().
> These are low-frequency paths (rcar PHY ready, brcmstb link up,
> hotplug command completion, DPC recover) that don't seem critical.  I
> think they're all using made-up delays that don't come from any spec
> or hardware requirement anyway.  I think it's hard to make an argument
> for precision here.

My initial understanding was the same. There was no need for such 
precision here. Then msleep will be retained, but only modified to #defines?

If you think it's unnecessary, I will discard the remaining patches.

Best regards,
Hans