Re: [PATCH] PCI: Drop unnecessary retries when restoring BARs

[Bjorn Helgaas <helgaas@kernel.org>]

On Sun, May 03, 2026 at 03:34:46PM +0200, Lukas Wunner wrote:
> In 2012, commit 26f41062f28d ("PCI: check for pci bar restore completion
> and retry") amended pci_restore_state() to attempt BAR restoration up to
> 10 times.  This was necessary because back in the day, only a 100 msec
> delay was observed after pcie_flr() carried out a Function Level Reset.
> The retries ensured that BARs were restored even if devices needed more
> time to come out of reset.
> 
> In 2016, commit 5adecf817dd6 ("PCI: Wait for up to 1000ms after FLR
> reset") extended the delay to 1 sec.  Commit a2758b6b8fdb ("PCI: Rename
> pci_flr_wait() to pci_dev_wait() and make it generic") subsequently
> extended it further to 60 sec.
> 
> The lengthened delay makes it unnecessary to retry BAR restoration, so
> drop it.
> 
> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> Closes: https://lore.kernel.org/r/20260416225745.GA41850@bhelgaas/
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Applied to pci/reset for v7.2, thanks!

> ---
> Jesse did ask "why we don't do this delay/check when we perform the FLR
> in the first place" when he applied the patch, but nobody followed up
> with a patch until Alex did four years later:
> 
> https://lore.kernel.org/r/003AAFE53969E14CB1F09B6FD68C3CD409A8B4F2@ORSMSX105.amr.corp.intel.com/
> 
>  drivers/pci/pci.c | 36 +++++++++++++-----------------------
>  1 file changed, 13 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8f7cfcc..1e386df 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1764,7 +1764,7 @@ int pci_save_state(struct pci_dev *dev)
>  EXPORT_SYMBOL(pci_save_state);
>  
>  static void pci_restore_config_dword(struct pci_dev *pdev, int offset,
> -				     u32 saved_val, int retry, bool force)
> +				     u32 saved_val, bool force)
>  {
>  	u32 val;
>  
> @@ -1772,52 +1772,42 @@ static void pci_restore_config_dword(struct pci_dev *pdev, int offset,
>  	if (!force && val == saved_val)
>  		return;
>  
> -	for (;;) {
> -		pci_dbg(pdev, "restore config %#04x: %#010x -> %#010x\n",
> -			offset, val, saved_val);
> -		pci_write_config_dword(pdev, offset, saved_val);
> -		if (retry-- <= 0)
> -			return;
> +	pci_dbg(pdev, "restore config %#04x: %#010x -> %#010x\n", offset, val,
> +		saved_val);
>  
> -		pci_read_config_dword(pdev, offset, &val);
> -		if (val == saved_val)
> -			return;
> -
> -		mdelay(1);
> -	}
> +	pci_write_config_dword(pdev, offset, saved_val);
>  }
>  
>  static void pci_restore_config_space_range(struct pci_dev *pdev,
> -					   int start, int end, int retry,
> -					   bool force)
> +					   int start, int end, bool force)
>  {
>  	int index;
>  
>  	for (index = end; index >= start; index--)
>  		pci_restore_config_dword(pdev, 4 * index,
>  					 pdev->saved_config_space[index],
> -					 retry, force);
> +					 force);
>  }
>  
>  static void pci_restore_config_space(struct pci_dev *pdev)
>  {
>  	if (pdev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
> -		pci_restore_config_space_range(pdev, 10, 15, 0, false);
> +		pci_restore_config_space_range(pdev, 10, 15, false);
>  		/* Restore BARs before the command register. */
> -		pci_restore_config_space_range(pdev, 4, 9, 10, false);
> -		pci_restore_config_space_range(pdev, 0, 3, 0, false);
> +		pci_restore_config_space_range(pdev, 4, 9, false);
> +		pci_restore_config_space_range(pdev, 0, 3, false);
>  	} else if (pdev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> -		pci_restore_config_space_range(pdev, 12, 15, 0, false);
> +		pci_restore_config_space_range(pdev, 12, 15, false);
>  
>  		/*
>  		 * Force rewriting of prefetch registers to avoid S3 resume
>  		 * issues on Intel PCI bridges that occur when these
>  		 * registers are not explicitly written.
>  		 */
> -		pci_restore_config_space_range(pdev, 9, 11, 0, true);
> -		pci_restore_config_space_range(pdev, 0, 8, 0, false);
> +		pci_restore_config_space_range(pdev, 9, 11, true);
> +		pci_restore_config_space_range(pdev, 0, 8, false);
>  	} else {
> -		pci_restore_config_space_range(pdev, 0, 15, 0, false);
> +		pci_restore_config_space_range(pdev, 0, 15, false);
>  	}
>  }
>  
> -- 
> 2.51.0
>