Re: [PATCH v5 2/4] PCI: Use standard wait times for PCIe link monitoring

[Thierry Reding <thierry.reding@gmail.com>]

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

On Tue, May 26, 2026 at 01:10:39PM +0200, Lukas Wunner wrote:
> On Tue, May 26, 2026 at 10:53:11AM +0200, Thierry Reding wrote:
> > Instead of defining the wait values for each driver, use common values
> > defined in the core pci.h header file. Note that most drivers don't use
> > the millisecond waits, but rather usleep_range(), so add these commonly
> > used values to the header so that all drivers can use them.
> 
> Hm, why not just replace usleep_range() with msleep() and use the existing
> macro instead of defining new ones?

I'm trying to somewhat unify the implementations across drivers without
changing behaviour. I don't have a way of testing any of these drivers,
so keeping the existing implementation and just switching out the symbol
seemed like a good compromise.

> > +++ b/drivers/pci/pci.h
> > @@ -63,6 +63,8 @@ struct pcie_tlp_log;
> >  /* Parameters for the waiting for link up routine */
> >  #define PCIE_LINK_WAIT_MAX_RETRIES	10
> >  #define PCIE_LINK_WAIT_SLEEP_MS		90
> > +#define PCIE_LINK_WAIT_US_MIN		90000
> > +#define PCIE_LINK_WAIT_US_MAX		100000
> 
> If you absolutely positively want to add a new macro, consider
> 90 * USEC_PER_MSEC instead of 90000.

That seems rather useless to me. We have a _US infix in those symbolic
names, so it's obvious what the unit is, so nobody should be confused as
to the purpose of these. I suppose if you want to make sure it
correlates to the SLEEP_MS variant we could maybe do:

  #define PCIE_LINK_WAIT_US_MIN (PCIE_LINK_WAIT_SLEEP_MS * USEC_PER_MSEC)

Then again, I count 2 drivers (in linux-next) that use the existing
PCIE_LINK_WAIT_SLEEP_MS, one of which multiplies by MILLI to get at the
US version (so it could easily be converted to the US_MIN version). Only
pcie-designware.c uses msleep() with PCIE_LINK_WAIT_SLEEP_MS, so it is
clearly the outlier.

Maybe I should just go all the way and drop the SLEEP_MS symbol and
replace usage by the US versions?

> Moreover, consider using fsleep() instead of adding an additional MAX
> macro.

This would then be changing behaviour again for all the drivers.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]