Re: [PATCH AUTOSEL 6.18-6.1] PCI: Avoid saving config space state if inaccessible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci: skipping pci_save_state() in pci_dev_save_and_disable() breaks restore
--

commit 4874b436f59a1ea615f5afac75821d16d1d67918
Author: Farhan Ali <alifm@linux.ibm.com>

PCI: Avoid saving config space state if inaccessible

This commit adds a helper to check whether PCI config space is accessible
before saving state during device reset, skipping pci_save_state() when
reads return an error response.

Link: https://patch.msgid.link/20260805165518.794-4-alifm@linux.ibm.com

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b14dd064006cc..78048bce859d4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c

[ ... ]

> @@ -5127,6 +5149,9 @@ 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"))
> +		return;
> +
>  	pci_save_state(dev);
>  	/*
>  	 * Disable the device by clearing the Command register, except for

[Severity: High]
Does skipping pci_save_state() in pci_dev_save_and_disable() prevent
pci_dev_restore() -> pci_restore_state() from restoring config space after
the reset completes?

Upstream commit e18d1abc3bff0a24a5299f3c150ab9546e9439aa relies on
prerequisite commit a2f1e22390ac ("PCI/ERR: Ensure error recoverability at
all times"), which added pci_save_state(dev) to pci_bus_add_device() during
enumeration and removed the dev->state_saved check from
pci_restore_state().

In this tree, commit a2f1e22390ac is not present.  In drivers/pci/probe.c,
pci_device_add() initializes dev->state_saved = false, and in
drivers/pci/bus.c, pci_bus_add_device() does not call pci_save_state(dev):

drivers/pci/probe.c:pci_device_add() {
	...
	dev->state_saved = false;

	pci_init_capabilities(dev);
	...
}

Furthermore, in drivers/pci/pci.c, pci_restore_state() still checks
dev->state_saved at entry and clears it on every restore:

drivers/pci/pci.c:pci_restore_state() {
	if (!dev->state_saved)
		return;
	...
	dev->state_saved = false;
}

When config space is inaccessible prior to a reset (such as during DPC
containment or link-down error recovery via pci_reset_function(),
pci_reset_function_locked(), pci_try_reset_function(), __pci_reset_bus(),
or __pci_reset_slot()) and dev->state_saved is false — either because no
driver saved state at probe or because a prior reset or PM transition
(such as vfio_pci_ioctl_reset(), pci_reset_bus(), vfio_pci_set_power_state(),
or pci_pm_runtime_resume()) already called pci_restore_state() and cleared
dev->state_saved = false — returning early in pci_dev_save_and_disable()
leaves dev->state_saved as false.

When the reset finishes and pci_dev_restore() calls pci_restore_state(dev):

drivers/pci/pci.c:pci_dev_restore() {
	const struct pci_error_handlers *err_handler =
			dev->driver ? dev->driver->err_handler : NULL;

	pci_restore_state(dev);
	...
}

won't pci_restore_state() bail out immediately without restoring BARs,
PCI_COMMAND, MSI/MSI-X, or PCIe capabilities, leaving the device at its
unconfigured post-reset register defaults?

Should prerequisite commit a2f1e22390ac ("PCI/ERR: Ensure error
recoverability at all times") be backported along with this change?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-191-sashal@kernel.org?part=1