Re: [PATCH v3 1/2] PCI: Add flr_delay parameter to pci_dev struct

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Wed, 11 Jun 2025, grwhyte@linux.microsoft.com wrote:

> From: Graham Whyte <grwhyte@linux.microsoft.com>
> 
> Add a new flr_delay member of the pci_dev struct to allow customization of
> the delay after FLR for devices that do not support immediate readiness.
> 
> Signed-off-by: Graham Whyte <grwhyte@linux.microsoft.com>
> ---
>  drivers/pci/pci.c   | 8 ++++++--
>  drivers/pci/pci.h   | 2 ++
>  include/linux/pci.h | 1 +
>  3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e9448d55113b..04f2660df7c4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3233,6 +3233,8 @@ void pci_pm_init(struct pci_dev *dev)
>  	dev->bridge_d3 = pci_bridge_d3_possible(dev);
>  	dev->d3cold_allowed = true;
>  
> +	dev->flr_delay = PCI_FLR_DELAY;
> +
>  	dev->d1_support = false;
>  	dev->d2_support = false;
>  	if (!pci_no_d1d2(dev)) {
> @@ -4529,9 +4531,11 @@ int pcie_flr(struct pci_dev *dev)
>  	/*
>  	 * Per PCIe r4.0, sec 6.6.2, a device must complete an FLR within
>  	 * 100ms, but may silently discard requests while the FLR is in
> -	 * progress.  Wait 100ms before trying to access the device.
> +	 * progress.  Wait 100ms before trying to access the device, unless
> +	 * otherwise modified if the device supports a faster reset.
> +	 * Use usleep_range to support delays under 20ms.
>  	 */
> -	msleep(100);
> +	usleep_range(dev->flr_delay, dev->flr_delay+1);

Missing spaces around +.

Are you sure + 1us is really useful as the range? Usually much bigger 
numbers are used.

There's also fsleep() which would autoselect the sleep mechanism.

>  	return pci_dev_wait(dev, "FLR", PCIE_RESET_READY_POLL_MS);
>  }
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 12215ee72afb..abc1cf6e6d9b 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -135,6 +135,8 @@ struct pci_cap_saved_state *pci_find_saved_ext_cap(struct pci_dev *dev,
>  #define PCI_PM_D3HOT_WAIT       10	/* msec */
>  #define PCI_PM_D3COLD_WAIT      100	/* msec */
>  
> +#define PCI_FLR_DELAY           100000 /* usec */

Please put the unit into the define name (_US).

> +
>  void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
>  void pci_refresh_power_state(struct pci_dev *dev);
>  int pci_power_up(struct pci_dev *dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 05e68f35f392..4c9989037ed1 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -402,6 +402,7 @@ struct pci_dev {
>  						   bit manually */
>  	unsigned int	d3hot_delay;	/* D3hot->D0 transition time in ms */
>  	unsigned int	d3cold_delay;	/* D3cold->D0 transition time in ms */
> +	unsigned int    flr_delay;      /* pci post flr sleep time in us */

Please follow how the spec writes things in capitalization of letters.

>  
>  	u16		l1ss;		/* L1SS Capability pointer */
>  #ifdef CONFIG_PCIEASPM
> 

-- 
 i.