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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing the read-back and retry loop for BAR restoration in `pci_restore_state()` introduces a risk of silent regressions for hardware resuming from non-FLR resets (such as D3hot to D0 transitions or custom driver resets). The commit incorrectly assumes the 60s delay from `pci_dev_wait()` covers all usages, but standard PM resume paths only delay for 10ms (`PCI_PM_D3_WAIT`) before calling `pci_restore_state()`. Historically, hardware that needed slightly longer to accept configuration writes relied on the 10x 1ms retry loop to successfully restore BARs. By removing both the retry and the read-back verification, BAR writes to slow devices will be silently dropped, leaving hardware unconfigured and causing MMIO accesses to result in IOMMU faults or kernel crashes.
--

commit 28d12557c0e570f2b2d21da1dcbb7889df2f4672
Author: Lukas Wunner <lukas@wunner.de>

PCI: Drop unnecessary retries when restoring BARs

This commit removes the retry loop when restoring PCI configuration space.
It relies on the lengthened FLR delays now present in the kernel, concluding
that retries for BAR restoration are no longer needed.

Closes: https://lore.kernel.org/r/20260416225745.GA41850@bhelgaas/

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8f7cfcc00090..1e386dfaa669 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1764,7 +1764,7 @@ int pci_save_state(struct pci_dev *dev)

[ ... ]

>  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);
>  }

Does removing the read-back and retry loop introduce a risk for hardware
resuming from non-FLR resets?

While FLR paths might now have a 60 second delay, pci_restore_state() is
used globally across the kernel. For example, during a PM resume from
D3hot to D0, the kernel typically only delays for 10ms (PCI_PM_D3_WAIT)
before restoring state.

If a device is slow to exit reset, will these configuration writes be
silently dropped without the fallback retry loop, potentially leaving BARs
unconfigured and causing MMIO accesses to fault?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/785c98b50a7a00d0698848c75d51b8f5669ad18f.1777814679.git.lukas@wunner.de?part=1