drivers/pci/controller/pcie-brcmstb.c | 5 ++++- drivers/pci/controller/pcie-rcar-host.c | 4 +++- drivers/pci/controller/pcie-rcar.c | 4 +++- drivers/pci/hotplug/pciehp_hpc.c | 6 +++++- drivers/pci/pci.c | 9 +++------ drivers/pci/pci.h | 7 +++++++ drivers/pci/pcie/dpc.c | 5 ++++- 7 files changed, 29 insertions(+), 11 deletions(-)
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.
---
Changes for v2:
https://patchwork.kernel.org/project/linux-pci/patch/20250820160944.489061-1-18255117159@163.com/
- According to the Maintainer's suggestion, it was modified to fsleep,
usleep_range, and macro definitions were used instead of hard code. (Bjorn)
---
Hans Zhang (7):
PCI: Replace msleep(2) with fsleep() for precise delay
PCI: Replace msleep(1) with fsleep() for precise link status checking
PCI: rcar-host: Replace msleep(1) with fsleep() for precise speed
change monitoring
PCI: brcmstb: Replace msleep(5) with usleep_range() for precise link
up checking
PCI: rcar: Replace msleep(5) with usleep_range() for precise PHY ready
checking
PCI: pciehp: Replace msleep(10) with usleep_range() for precise delays
PCI/DPC: Replace msleep(10) with usleep_range() for precise RP busy
checking
drivers/pci/controller/pcie-brcmstb.c | 5 ++++-
drivers/pci/controller/pcie-rcar-host.c | 4 +++-
drivers/pci/controller/pcie-rcar.c | 4 +++-
drivers/pci/hotplug/pciehp_hpc.c | 6 +++++-
drivers/pci/pci.c | 9 +++------
drivers/pci/pci.h | 7 +++++++
drivers/pci/pcie/dpc.c | 5 ++++-
7 files changed, 29 insertions(+), 11 deletions(-)
base-commit: b19a97d57c15643494ac8bfaaa35e3ee472d41da
--
2.25.1
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.
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.,
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.
- 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.
Bjorn
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. > > 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., > 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. > > - 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. > Dear Bjorn, Thank you very much for your reply. I have revised version v3 based on your review comments. Please continue your review. Thank you very much. Best regards, Hans > Bjorn
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
On Mon, Aug 25, 2025 at 12:05:26AM +0800, Hans Zhang wrote: > 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. > > 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., > > 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. > 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); This mixes 3 changes: 1) Add #define PCI_T_RST_SEC_BUS_DELAY_US 2) Reduce overall delay from 2ms to 1ms 3) Convert msleep() to fsleep() There's no issue at all with 1), and I don't know if it's really worth doing 2), so I would do this: - msleep(2); + msleep(2 * PCI_T_RST_SEC_BUS_DELAY_MS); Then we can consider the question of whether "msleep(2)" is misleading to the reader because the actual delay is always > 20ms. If that's the case, I would consider a separate patch like this: - msleep(2 * PCI_T_RST_SEC_BUS_DELAY_MS); + fsleep(2 * PCI_T_RST_SEC_BUS_DELAY_US); to make the stated intent of the code closer to the actual behavior. If we do this, the commit log should include concrete details about why short msleep() doesn't work as advertised. > > 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? The #defines are useful when (1) the value comes from a spec or (2) we want to use the same value several places. Otherwise, the value is minimal. For rcar PHY ready, brcmstb link up, hotplug command completion, DPC recover, I don't think either applies, so personally I would probably leave them alone (or, if we think short msleep() is just misleading in principle, convert them to fsleep()). Bjorn
© 2016 - 2025 Red Hat, Inc.