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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250825203051.GA781401@bhelgaas>
Date: Mon, 25 Aug 2025 15:30:51 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Hans Zhang <18255117159@....com>
Cc: bhelgaas@...gle.com, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/7] PCI: Replace short msleep() calls with more
 precise delay functions

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