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

[Hans Zhang <18255117159@163.com>]

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