Re: [PATCH] PCI: Only restore state that was successfully saved around a reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] PCI: pci_dev_restore() skips restoring preserved state after
  error reset
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] PCI: capability buffer corruption on disconnect in
  pci_save_state() [1]
--

commit 101ae1e1e66a05607699c91780a5db7e8edbbdc0
Author: Li RongQing <lirongqing@baidu.com>

PCI: Only restore state that was successfully saved around a reset

This commit clears dev->state_saved when pci_dev_save_and_disable() skips
saving inaccessible config space and checks dev->state_saved before calling
pci_restore_state() in pci_dev_restore().

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d6590805faeb2..2327f47f3fda5 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5309,8 +5309,10 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
>  	 */
>  	pci_set_power_state(dev, PCI_D0);
>  
> -	if (!pci_dev_config_accessible(dev, "save state"))
> +	if (!pci_dev_config_accessible(dev, "save state")) {
> +		dev->state_saved = false;
>  		return;
> +	}
>  
>  	pci_save_state(dev);

[Severity: High]
This problem wasn't introduced by this patch, but can pci_save_state()
corrupt the saved PCIe capability state [1] if the device becomes
inaccessible mid-save?

pci_save_state() sets dev->state_saved to true and calls
pci_save_pcie_state(), which reads capability registers into
save_state->cap.data without checking for errors:

drivers/pci/pci.c:pci_save_pcie_state() {
    ...
	cap = (u16 *)&save_state->cap.data[0];
	pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &cap[i++]);
	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[i++]);
	pcie_capability_read_word(dev, PCI_EXP_SLTCTL, &cap[i++]);
    ...
}

If the device disconnects during this window, these reads store ~0 (or 0 on
explicit read failure) into the persistent save buffer, and
pci_restore_state() later writes those corrupted values back to registers
such as PCI_EXP_DEVCTL.

[ ... ]

> @@ -5328,7 +5330,8 @@ static void pci_dev_restore(struct pci_dev *dev)
>  	const struct pci_error_handlers *err_handler =
>  			dev->driver ? dev->driver->err_handler : NULL;
>  
> -	pci_restore_state(dev);
> +	if (dev->state_saved)
> +		pci_restore_state(dev);

[Severity: High]
Does skipping pci_restore_state() here introduce a regression that leaves
the device unconfigured after a reset recovers it from an error?

Commit a2f1e22390ac ("PCI/ERR: Ensure error recoverability at all times")
saves the initial config space in pci_bus_add_device() and removed the
dev->state_saved check from pci_restore_state() so a valid baseline state
is always available even after a prior restore cleared dev->state_saved.

Commit e18d1abc3bff ("PCI: Avoid saving config space state if inaccessible")
then skipped pci_save_state() in pci_dev_save_and_disable() when config
space is inaccessible (such as during DPC) to avoid overwriting that
preserved state with ~0 before the reset.

Since the hardware reset clears the device's registers back to power-on
defaults, clearing dev->state_saved and skipping pci_restore_state() leaves
BARs, PCI_COMMAND, capabilities, and bridge bus numbers unprogrammed once
the reset makes config space accessible again.

[1] https://sashiko.dev/#/bug/linux-60bae679-5c85-4c49-8042-6817f9cb5c8f

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110053348.2419-1-lirongqing@baidu.com?part=1