PCI: Universal error recoverability of devices

When the PCI core gained power management support in 2002, it introduced pci_save_state() and pci_restore_state() helpers to restore Config Space after a D3hot or D3cold transition, which implies a Soft or Fundamental Reset (PCIe r7.0 sec 5.8):

https://git.kernel.org/tglx/history/c/a5287abe398b

In 2006, EEH and AER were introduced to recover from errors by performing a reset. Because errors can occur at any time, drivers began calling pci_save_state() on probe to ensure recoverability.

In 2009, recoverability was foiled by commit c82f63e411f1 ("PCI: check saved state before restore"): It amended pci_restore_state() to bail out if the "state_saved" flag has been cleared. The flag is cleared by pci_restore_state() itself, hence a saved state is now allowed to be restored only once and is then invalidated. That doesn't seem to make sense because the saved state should be good enough to be reused.

Soon after, drivers began to work around this behavior by calling pci_save_state() immediately after pci_restore_state(), see e.g. commit b94f2d775a71 ("igb: call pci_save_state after pci_restore_state"). Hilariously, two drivers even set the "saved_state" flag to true before invoking pci_restore_state(), see ipr_reset_restore_cfg_space() and e1000_io_slot_reset().

Despite these workarounds, recoverability at all times is not guaranteed:
E.g. when a PCIe port goes through a runtime suspend and resume cycle, the "saved_state" flag is cleared by:

pci_pm_runtime_resume()
pci_pm_default_resume_early()
pci_restore_state()

... and hence on a subsequent AER event, the port's Config Space cannot be restored.

Riana reports a recovery failure of a GPU-integrated PCIe switch and has root-caused it to the behavior of pci_restore_state(). Another workaround would be necessary, namely calling pci_save_state() in pcie_port_device_runtime_resume().

The motivation of commit c82f63e411f1 was to prevent restoring state if pci_save_state() hasn't been called before. But that can be achieved by saving state already on device addition, after Config Space has been initialized. A desirable side effect is that devices become recoverable even if no driver gets bound. This renders the commit unnecessary, so revert it.

Reported-by Riana Tauro <[email protected]> # off-list

Tested-by Riana Tauro <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
Proof that removing the check in pci_restore_state() makes no difference for the PCI core:

* The only relevant invocations of pci_restore_state() are in drivers/pci/pci-driver.c
* One invocation is in pci_restore_standard_config(), which is always called conditionally on "if (pci_dev->state_saved)". So the check at the beginning of pci_restore_state() which this patch removes is an unnecessary duplication.
* Another invocation is in pci_pm_default_resume_early(), which is called from pci_pm_resume_noirq(), pci_pm_restore_noirq() and pci_pm_runtime_resume(). Those functions are only called after prior calls to pci_pm_suspend_noirq(), pci_pm_freeze_noirq(), and pci_pm_runtime_suspend(), respectively. And all of them call pci_save_state(). So the "if (!dev->state_saved)" check in pci_restore_state() never evaluates to true.
* A third invocation is in pci_pm_thaw_noirq(). It is only called after a prior call to pci_pm_freeze_noirq(), which invokes pci_save_state(). So likewise the "if (!dev->state_saved)" check in pci_restore_state() never evaluates to true.

 drivers/pci/bus.c   | 7 +++++++
 drivers/pci/pci.c   | 3 ---
 drivers/pci/probe.c | 2 --
 3 files changed, 7 insertions(+), 5 deletions(-)

drivers/pci/bus.c +0 -0

index f26aec6..4318568 100644

drivers/pci/bus.c +5 -0

+	 */
+	pci_save_state(dev);
+	dev->state_saved = false;
+
+	/*
 	 * If the PCI device is associated with a pwrctrl device with a
 	 * power supply, create a device link between the PCI device and
 	 * pwrctrl device.  This ensures that pwrctrl drivers are probed

drivers/pci/pci.c +0 -3

index b14dd06..2f0da5d 100644
@@ -1855,9 +1855,6 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
  */
 void pci_restore_state(struct pci_dev *dev)
 {
-	if (!dev->state_saved)
-		return;
-
 	pci_restore_pcie_state(dev);
 	pci_restore_pasid_state(dev);
 	pci_restore_pri_state(dev);

drivers/pci/probe.c +0 -2

index c83e75a..c7c7a3d 100644
@@ -2747,8 +2747,6 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 
 	pci_reassigndev_resource_alignment(dev);
 
-	dev->state_saved = false;
-
 	pci_init_capabilities(dev);
 
 	/*

Signature · 1 line

2.51.0

Quoted 8 lines · Lukas Wunner

Despite these workarounds, recoverability at all times is not guaranteed:
E.g. when a PCIe port goes through a runtime suspend and resume cycle, the "saved_state" flag is cleared by:

pci_pm_runtime_resume()
pci_pm_default_resume_early()
pci_restore_state()

Quoted 2 lines

... and hence on a subsequent AER event, the port's Config Space cannot be restored.

I guess this restore would be done by a driver's pci_error_handlers.slot_reset() or .reset_done() calling pci_restore_state()?

drivers/pci/bus.c void pci_bus_add_device(struct pci_dev *dev) · line 362

Quoted 7 lines

+++ b/drivers/pci/bus.c

drivers/pci/bus.c +2 -0

@@ -358,6 +358,13 @@ void pci_bus_add_device(struct pci_dev *dev)
 	pci_bridge_d3_update(dev);
 
 	/*
+	 * Save config space for error recoverability.  Clear state_saved
+	 * to detect whether drivers invoked pci_save_state() on suspend.

Can we expand this a little to explain how this is detected and what drivers *should* be doing?

I think the reason is that the PCI core
can invoke pci_save_state() on suspend if the driver did not.

I assume:

- PCI core always calls pci_save_state() and clears state_saved when device is enumerated (below)

- When it has configured the device to the state it wants restore, the driver may call pci_save_state() again, which will set state_saved

- If driver has not called pci_save_state(), i.e., state_saved is still clear, we want the PCI core to call pci_save_state() during suspend

This sounds sensible to me. It would be nice if there were a few more words about pci_save_state() and pci_restore_state() in Documentation/.

pci_save_state() isn't mentioned at all in Documentation/PCI

Quoted 9 lines · Bjorn Helgaas

Quoted 8 lines · Lukas Wunner

Despite these workarounds, recoverability at all times is not guaranteed:
E.g. when a PCIe port goes through a runtime suspend and resume cycle, the "saved_state" flag is cleared by:

pci_pm_runtime_resume()
pci_pm_default_resume_early()
pci_restore_state()

Quoted 6 lines

Quoted 2 lines

... and hence on a subsequent AER event, the port's Config Space cannot be restored.

I guess this restore would be done by a driver's pci_error_handlers.slot_reset() or .reset_done() calling pci_restore_state()?

Yes. Restoration of config space after an error-recovery-induced reset is currently always the job of the device driver.

E.g. in the case of portdrv, it happens in pcie_portdrv_slot_reset().

We could revisit this design decision and change the behavior to have pcie_do_recovery() call pci_restore_state(), thus reducing boilerplate in the drivers. But that would be a separate effort, orthogonal to the present patch.

Quoted 10 lines

drivers/pci/bus.c void pci_bus_add_device(struct pci_dev *dev) · line 362

Quoted 7 lines

+++ b/drivers/pci/bus.c

drivers/pci/bus.c +2 -0

@@ -358,6 +358,13 @@ void pci_bus_add_device(struct pci_dev *dev)
 	pci_bridge_d3_update(dev);
 
 	/*
+	 * Save config space for error recoverability.  Clear state_saved
+	 * to detect whether drivers invoked pci_save_state() on suspend.

Can we expand this a little to explain how this is detected and what drivers *should* be doing?

That is documented in Documentation/power/pci.rst, "3.1.2. suspend()":

"This callback is expected to quiesce the device and prepare it to be
put into a low-power state by the PCI subsystem. It is not required (in fact it even is not recommended) that a PCI driver's suspend() callback save the standard configuration registers of the device [...]

However, in some rare case it is convenient to carry out these operations in a PCI driver. Then, pci_save_state() [...] should be used to save the device's standard configuration registers [...]. Moreover, if the driver calls pci_save_state(), the PCI subsystem will not execute either pci_prepare_to_sleep(), or pci_set_power_state() for its device, so the driver is then responsible for handling the device as appropriate."

Quoted 2 lines

I think the reason is that the PCI core
can invoke pci_save_state() on suspend if the driver did not.

Right. By calling pci_save_state(), a driver signals to the PCI core that it assumes responsibility for putting the device into a low power state. If a driver wants to keep a device in D0, it could call pci_save_state() and thus prevent the PCI core from putting it e.g. into D3.

Quoted 5 lines

I assume:

- PCI core always calls pci_save_state() and clears state_saved when device is enumerated (below)

Quoted 7 lines

- When it has configured the device to the state it wants restore, the driver may call pci_save_state() again, which will set state_saved

- If driver has not called pci_save_state(), i.e., state_saved is still clear, we want the PCI core to call pci_save_state() during suspend

Right.

Quoted 5 lines

This sounds sensible to me. It would be nice if there were a few more words about pci_save_state() and pci_restore_state() in Documentation/.

pci_save_state() isn't mentioned at all in Documentation/PCI

Right, it's documented in the Documentation/power directory. :)

It seems like there are two things going on here, and I'm not sure they're completely compatible:

1) Driver calls pci_save_state() to take over device power management and prevent the PCI core from doing it.

2) Driver calls pci_save_state() to capture the device state it wants to restore when recovering from an error.

Shouldn't a driver be able to do 2) without also getting 1)?

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

+++ b/drivers/pci/bus.c

drivers/pci/bus.c +2 -0

@@ -358,6 +358,13 @@ void pci_bus_add_device(struct pci_dev *dev)
 	pci_bridge_d3_update(dev);
 
 	/*
+	 * Save config space for error recoverability.  Clear state_saved
+	 * to detect whether drivers invoked pci_save_state() on suspen

Quoted 2 lines

Quoted 2 lines

Can we expand this a little to explain how this is detected and what drivers *should* be doing?

Yes. I should have proposed some text for the comment, e.g.,

Save config space for error recoverability. Clear state_saved. If driver calls pci_save_state() again, state_saved will be set and we'll know that on suspend, the PCI core shouldn't call pci_save_state() or change the device power state.

I'm just wishing for a more concrete mention of "pci_save_state()", since that's where the critical "state_saved" flag is updated.

And I'm not sure Documentation/ includes anything about the idea of a driver using pci_save_state() to capture the state it wants to restore after an error. I guess that's obvious now that I write it down, but I'm sure learning a lot from this conversation :)

Quoted 10 lines · Bjorn Helgaas

It seems like there are two things going on here, and I'm not sure they're completely compatible:

1) Driver calls pci_save_state() to take over device power management and prevent the PCI core from doing it.

2) Driver calls pci_save_state() to capture the device state it wants to restore when recovering from an error.

Shouldn't a driver be able to do 2) without also getting 1)?

In general, it can:

A number of drivers already call pci_save_state() on probe to capture the state for subsequent error recovery. If the driver has modified config space in its probe hook, then calling pci_save_state() continues to make sense. If the driver has *not* modified config space, then the call becomes obsolete once this patch is accepted.

The reason I'm using the "in general" qualifier:

I've identified two corner cases where the PCI core neglects to set state_saved = false before commencing the suspend sequence:

* If a driver has legacy PCI PM callbacks, pci_legacy_suspend() neglects to set state_saved = false. Yet both pci_legacy_suspend() and pci_legacy_suspend_late() subsequently query the state_saved flag.

* If a device is unbound or its driver has no PM callbacks (driver->pm == NULL), pci_pm_freeze() neglects to set state_saved = false. Yet pci_pm_freeze_noirq() subsequently queries the state_saved flag.

In these corner cases, pci_legacy_suspend() and pci_pm_freeze() depend on some other part of the PCI core to set state_saved = false. For a freshly enumerated device, the flag is initialized to false by kzalloc() and pci_device_add() also explicitly sets it to false for good measure. On resume (or thaw or restore), the flag is set to false by pci_restore_state(). The latter is preserved as is by my patch and the former is moved to pci_bus_add_device() to retain the current behavior.

Clearly, the two corner cases should be fixed and then setting state_saved = false in pci_bus_add_device() becomes unnecessary.

I'd prefer doing that in a separate step though.

So drivers which use legacy PCI PM callbacks or have no PM callbacks should currently not call pci_save_state() on probe without manually setting state_saved = false afterwards. If they neglect that, then pci_legacy_suspend_late() and pci_pm_freeze_noirq() will not call pci_save_state() on the next suspend cycle and so the state that will be restored on resume is the one recorded on probe, not the one that the device had on suspend. If these two states happen to be identical, there's no problem.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

+++ b/drivers/pci/bus.c

drivers/pci/bus.c +2 -0

@@ -358,6 +358,13 @@ void pci_bus_add_device(struct pci_dev *dev)
 	pci_bridge_d3_update(dev);
 
 	/*
+	 * Save config space for error recoverability.  Clear state_saved
+	 * to detect whether drivers invoked pci_save_state() on suspen

[...]

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Can we expand this a little to explain how this is detected and what drivers *should* be doing?

[...]

Quoted 6 lines

Yes. I should have proposed some text for the comment, e.g.,

Save config space for error recoverability. Clear state_saved. If driver calls pci_save_state() again, state_saved will be set and we'll know that on suspend, the PCI core shouldn't call pci_save_state() or change the device power state.

I'm fine with rewording the code comment like this, as well as splitting the code comment as suggested by Rafael. Would you prefer amending the code comment when applying or should I respin with a reworded comment?

Again, clearing state_saved in pci_bus_add_device() is just a temporary measure to retain the existing behavior. It (and an accompanying code comment) can be dropped once pci_legacy_suspend() and pci_pm_freeze() are fixed.

Quoted 3 lines

I'm just wishing for a more concrete mention of "pci_save_state()", since that's where the critical "state_saved" flag is updated.

Quoted 4 lines

And I'm not sure Documentation/ includes anything about the idea of a driver using pci_save_state() to capture the state it wants to restore after an error. I guess that's obvious now that I write it down, but I'm sure learning a lot from this conversation :)

Okay, noted that the documentation could be improved. I'd be glad if this could be postponed to a separate step as well though. I can only address problems one at a time. :)

Quoted 12 lines · Lukas Wunner

Quoted 10 lines · Bjorn Helgaas

It seems like there are two things going on here, and I'm not sure they're completely compatible:

1) Driver calls pci_save_state() to take over device power management and prevent the PCI core from doing it.

2) Driver calls pci_save_state() to capture the device state it wants to restore when recovering from an error.

Shouldn't a driver be able to do 2) without also getting 1)?

Quoted 7 lines

In general, it can:

A number of drivers already call pci_save_state() on probe to capture the state for subsequent error recovery. If the driver has modified config space in its probe hook, then calling pci_save_state() continues to make sense. If the driver has *not* modified config space, then the call becomes obsolete once this patch is accepted.

So I guess "state_saved == true" means "driver does its own power management and PCI core shouldn't do it", and drivers that want 2) but not 1) just need to set state_saved = false after they call pci_save_state()?

That makes sense in sort of a weird way that makes my head hurt every time I try to understand it.

After error recovery, those drivers will see the state the driver identified when it called pci_save_state(). But after resume, they will see the state the PCI core saved at suspend time. Right?

Quoted 8 lines · Lukas Wunner

Quoted 6 lines · Bjorn Helgaas

On Sun, Oct 12, 2025 at 03:25:01PM +0200, Lukas Wunner wrote:
It would be nice if there were a few more
words about pci_save_state() and pci_restore_state() in Documentation/.

pci_save_state() isn't mentioned at all in Documentation/PCI

Quoted 1 line

Right, it's documented in the Documentation/power directory. :)

Yes, in the pci.rst I mentioned, but it mostly uses the "saves the device's standard configuration registers" wording.

I'm just wishing for a more concrete mention of "pci_save_state()", since that's where the critical "state_saved" flag is updated.

And I'm not sure Documentation/ includes anything about the idea of a driver using pci_save_state() to capture the state it wants to restore after an error.

Quoted 9 lines · Bjorn Helgaas

Quoted 8 lines · Lukas Wunner

Quoted 6 lines · Bjorn Helgaas

On Sun, Oct 12, 2025 at 03:25:01PM +0200, Lukas Wunner wrote:
It would be nice if there were a few more
words about pci_save_state() and pci_restore_state() in Documentation/.

pci_save_state() isn't mentioned at all in Documentation/PCI

Quoted 7 lines

Quoted 1 line

Right, it's documented in the Documentation/power directory. :)

Yes, in the pci.rst I mentioned, but it mostly uses the "saves the device's standard configuration registers" wording.

I'm just wishing for a more concrete mention of "pci_save_state()", since that's where the critical "state_saved" flag is updated.

Hm, Documentation/power/pci.rst does contain this:

"Then, pci_save_state(), pci_prepare_to_sleep(), and
pci_set_power_state() should be used to save the device’s standard configuration registers, to prepare it for system wakeup (if necessary), and to put it into a low-power state, respectively."

I'm struggling to find a better way to phrase it.

Quoted 3 lines

And I'm not sure Documentation/ includes anything about the idea of a driver using pci_save_state() to capture the state it wants to restore after an error.

Right, while pci_save_state() usage is mentioned in the PCI power management documentation, it's not mentioned at all in the error recovery context. So I'm proposing this amendment:

https://lore.kernel.org/r/077596ba70202be0e43fdad3bb9b93d356cbe4ec.1763746079.git.lukas@wunner.de/

When the PCI core gained power management support in 2002, it introduced pci_save_state() and pci_restore_state() helpers to restore Config Space after a D3hot or D3cold transition, which implies a Soft or Fundamental Reset (PCIe r7.0 sec 5.8):

https://git.kernel.org/tglx/history/c/a5287abe398b

In 2006, EEH and AER were introduced to recover from errors by performing a reset. Because errors can occur at any time, drivers began calling pci_save_state() on probe to ensure recoverability.

In 2009, recoverability was foiled by commit c82f63e411f1 ("PCI: check saved state before restore"): It amended pci_restore_state() to bail out if the "state_saved" flag has been cleared. The flag is cleared by pci_restore_state() itself, hence a saved state is now allowed to be restored only once and is then invalidated. That doesn't seem to make sense because the saved state should be good enough to be reused.

Soon after, drivers began to work around this behavior by calling pci_save_state() immediately after pci_restore_state(), see e.g. commit b94f2d775a71 ("igb: call pci_save_state after pci_restore_state"). Hilariously, two drivers even set the "saved_state" flag to true before invoking pci_restore_state(), see ipr_reset_restore_cfg_space() and e1000_io_slot_reset().

Despite these workarounds, recoverability at all times is not guaranteed:
E.g. when a PCIe port goes through a runtime suspend and resume cycle, the "saved_state" flag is cleared by:

pci_pm_runtime_resume()
pci_pm_default_resume_early()
pci_restore_state()

... and hence on a subsequent AER event, the port's Config Space cannot be restored. Riana reports a recovery failure of a GPU-integrated PCIe switch and has root-caused it to the behavior of pci_restore_state(). Another workaround would be necessary, namely calling pci_save_state() in pcie_port_device_runtime_resume().

The motivation of commit c82f63e411f1 was to prevent restoring state if pci_save_state() hasn't been called before. But that can be achieved by saving state already on device addition, after Config Space has been initialized. A desirable side effect is that devices become recoverable even if no driver gets bound. This renders the commit unnecessary, so revert it.

Reported-by Riana Tauro <[email protected]> # off-list

Tested-by Riana Tauro <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
Proof that removing the check in pci_restore_state() makes no difference for the PCI core:

* The only relevant invocations of pci_restore_state() are in drivers/pci/pci-driver.c
* One invocation is in pci_restore_standard_config(), which is always called conditionally on "if (pci_dev->state_saved)". So the check at the beginning of pci_restore_state() which this patch removes is an unnecessary duplication.
* Another invocation is in pci_pm_default_resume_early(), which is called from pci_pm_resume_noirq(), pci_pm_restore_noirq() and pci_pm_runtime_resume(). Those functions are only called after prior calls to pci_pm_suspend_noirq(), pci_pm_freeze_noirq(), and pci_pm_runtime_suspend(), respectively. And all of them call pci_save_state(). So the "if (!dev->state_saved)" check in pci_restore_state() never evaluates to true.
* A third invocation is in pci_pm_thaw_noirq(). It is only called after a prior call to pci_pm_freeze_noirq(), which invokes pci_save_state(). So likewise the "if (!dev->state_saved)" check in pci_restore_state() never evaluates to true.

 drivers/pci/bus.c   | 7 +++++++
 drivers/pci/pci.c   | 3 ---
 drivers/pci/probe.c | 2 --
 3 files changed, 7 insertions(+), 5 deletions(-)

drivers/pci/bus.c +7 -0

index f26aec6..4318568 100644
@@ -358,6 +358,13 @@ void pci_bus_add_device(struct pci_dev *dev)
        pci_bridge_d3_update(dev);
 
        /*
+        * Save config space for error recoverability.  Clear state_saved
+        * to detect whether drivers invoked pci_save_state() on suspend.
+        */
+       pci_save_state(dev);
+       dev->state_saved = false;
+
+       /*
         * If the PCI device is associated with a pwrctrl device with a
         * power supply, create a device link between the PCI device and
         * pwrctrl device.  This ensures that pwrctrl drivers are probed

drivers/pci/pci.c +0 -0

index b14dd06..2f0da5d 100644

drivers/pci/pci.c +0 -0

        pci_restore_pcie_state(dev);
        pci_restore_pasid_state(dev);
        pci_restore_pri_state(dev);

drivers/pci/probe.c +0 -3

index c83e75a..c7c7a3d 100644
@@ -2747,8 +2747,6 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 
        pci_reassigndev_resource_alignment(dev);
 
-       dev->state_saved = false;
-
        pci_init_capabilities(dev);
 
        /*
--

Quoted 102 lines · Lukas Wunner

When the PCI core gained power management support in 2002, it introduced pci_save_state() and pci_restore_state() helpers to restore Config Space after a D3hot or D3cold transition, which implies a Soft or Fundamental Reset (PCIe r7.0 sec 5.8):

https://git.kernel.org/tglx/history/c/a5287abe398b

In 2006, EEH and AER were introduced to recover from errors by performing a reset. Because errors can occur at any time, drivers began calling pci_save_state() on probe to ensure recoverability.

In 2009, recoverability was foiled by commit c82f63e411f1 ("PCI: check saved state before restore"): It amended pci_restore_state() to bail out if the "state_saved" flag has been cleared. The flag is cleared by pci_restore_state() itself, hence a saved state is now allowed to be restored only once and is then invalidated. That doesn't seem to make sense because the saved state should be good enough to be reused.

Soon after, drivers began to work around this behavior by calling pci_save_state() immediately after pci_restore_state(), see e.g. commit b94f2d775a71 ("igb: call pci_save_state after pci_restore_state"). Hilariously, two drivers even set the "saved_state" flag to true before invoking pci_restore_state(), see ipr_reset_restore_cfg_space() and e1000_io_slot_reset().

Despite these workarounds, recoverability at all times is not guaranteed:
E.g. when a PCIe port goes through a runtime suspend and resume cycle, the "saved_state" flag is cleared by:

pci_pm_runtime_resume()
pci_pm_default_resume_early()
pci_restore_state()

... and hence on a subsequent AER event, the port's Config Space cannot be restored. Riana reports a recovery failure of a GPU-integrated PCIe switch and has root-caused it to the behavior of pci_restore_state(). Another workaround would be necessary, namely calling pci_save_state() in pcie_port_device_runtime_resume().

The motivation of commit c82f63e411f1 was to prevent restoring state if pci_save_state() hasn't been called before. But that can be achieved by saving state already on device addition, after Config Space has been initialized. A desirable side effect is that devices become recoverable even if no driver gets bound. This renders the commit unnecessary, so revert it.

Reported-by Riana Tauro <[email protected]> # off-list

Tested-by Riana Tauro <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
Proof that removing the check in pci_restore_state() makes no difference for the PCI core:

* The only relevant invocations of pci_restore_state() are in drivers/pci/pci-driver.c
* One invocation is in pci_restore_standard_config(), which is always called conditionally on "if (pci_dev->state_saved)". So the check at the beginning of pci_restore_state() which this patch removes is an unnecessary duplication.
* Another invocation is in pci_pm_default_resume_early(), which is called from pci_pm_resume_noirq(), pci_pm_restore_noirq() and pci_pm_runtime_resume(). Those functions are only called after prior calls to pci_pm_suspend_noirq(), pci_pm_freeze_noirq(), and pci_pm_runtime_suspend(), respectively. And all of them call pci_save_state(). So the "if (!dev->state_saved)" check in pci_restore_state() never evaluates to true.
* A third invocation is in pci_pm_thaw_noirq(). It is only called after a prior call to pci_pm_freeze_noirq(), which invokes pci_save_state(). So likewise the "if (!dev->state_saved)" check in pci_restore_state() never evaluates to true.

 drivers/pci/bus.c   | 7 +++++++
 drivers/pci/pci.c   | 3 ---
 drivers/pci/probe.c | 2 --
 3 files changed, 7 insertions(+), 5 deletions(-)

drivers/pci/bus.c +7 -0

index f26aec6..4318568 100644
@@ -358,6 +358,13 @@ void pci_bus_add_device(struct pci_dev *dev)
        pci_bridge_d3_update(dev);
 
        /*
+        * Save config space for error recoverability.  Clear state_saved
+        * to detect whether drivers invoked pci_save_state() on suspend.
+        */
+       pci_save_state(dev);
+       dev->state_saved = false;
+
+       /*
         * If the PCI device is associated with a pwrctrl device with a
         * power supply, create a device link between the PCI device and
         * pwrctrl device.  This ensures that pwrctrl drivers are probed

drivers/pci/pci.c +0 -0

index b14dd06..2f0da5d 100644
@@ -1855,9 +1855,6 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
  */
 void pci_restore_state(struct pci_dev *dev)

drivers/pci/pci.c static void pci_restore_rebar_state(struct pci_dev *pdev) · line 1860

Quoted 4 lines

drivers/pci/pci.c +0 -3

 {
-       if (!dev->state_saved)
-               return;
-

So after this change, is there any mechanism to clear state_saved after a suspend-resume cycle so that the next cycle is not confused by seeing it set?

In 2009, commit c82f63e411f1 ("PCI: check saved state before restore") changed the behavior of pci_restore_state() such that it became necessary to call pci_save_state() afterwards, lest recovery from subsequent PCI errors fails.

The commit has just been reverted and so all the pci_save_state() after pci_restore_state() calls that have accumulated in the tree are now superfluous. Drop them.

Two drivers chose a different approach to achieve the same result:
drivers/scsi/ipr.c and drivers/net/ethernet/intel/e1000e/netdev.c set the pci_dev's "state_saved" flag to true before calling pci_restore_state(). Drop this as well.

Signed-off-by Lukas Wunner <[email protected]>

---
Some of the pci_save_state() calls in drivers' probe hooks may now likewise be superfluous if the probe hook doesn't touch Config Space. These calls will be identified and dealt with individually.

 drivers/crypto/intel/qat/qat_common/adf_aer.c    | 2 --
 drivers/dma/ioat/init.c                          | 1 -
 drivers/net/ethernet/broadcom/bnx2.c             | 2 --
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 1 -
 drivers/net/ethernet/broadcom/tg3.c              | 1 -
 drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c  | 1 -
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c  | 2 --
 drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c | 1 -
 drivers/net/ethernet/intel/e1000e/netdev.c       | 1 -
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c     | 6 ------
 drivers/net/ethernet/intel/i40e/i40e_main.c      | 1 -
 drivers/net/ethernet/intel/ice/ice_main.c        | 2 --
 drivers/net/ethernet/intel/igb/igb_main.c        | 2 --
 drivers/net/ethernet/intel/igc/igc_main.c        | 2 --
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    | 1 -
 drivers/net/ethernet/mellanox/mlx4/main.c        | 1 -
 drivers/net/ethernet/mellanox/mlx5/core/main.c   | 1 -
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c      | 1 -
 drivers/net/ethernet/microchip/lan743x_main.c    | 1 -
 drivers/net/ethernet/myricom/myri10ge/myri10ge.c | 4 ----
 drivers/net/ethernet/neterion/s2io.c             | 1 -
 drivers/pci/pcie/portdrv.c                       | 1 -
 drivers/scsi/bfa/bfad.c                          | 1 -
 drivers/scsi/csiostor/csio_init.c                | 1 -
 drivers/scsi/ipr.c                               | 1 -
 drivers/scsi/lpfc/lpfc_init.c                    | 6 ------
 drivers/scsi/qla2xxx/qla_os.c                    | 5 -----
 drivers/scsi/qla4xxx/ql4_os.c                    | 5 -----
 drivers/tty/serial/8250/8250_pci.c               | 1 -
 drivers/tty/serial/jsm/jsm_driver.c              | 1 -
 30 files changed, 57 deletions(-)

drivers/crypto/intel/qat/qat_common/adf_aer.c +0 -2

index 35679b2..9a5a4b3 100644
@@ -105,7 +105,6 @@ void adf_dev_restore(struct adf_accel_dev *accel_dev)
 			 accel_dev->accel_id);
 		hw_device->reset_device(accel_dev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 	}
 }
 

@@ -204,7 +203,6 @@ static pci_ers_result_t adf_slot_reset(struct pci_dev *pdev)
 	if (!pdev->is_busmaster)
 		pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 	res = adf_dev_up(accel_dev, false);
 	if (res && res != -EALREADY)
 		return PCI_ERS_RESULT_DISCONNECT;

drivers/dma/ioat/init.c +0 -1

index 02f68b3..2273986 100644
@@ -1286,7 +1286,6 @@ static pci_ers_result_t ioat_pcie_error_slot_reset(struct pci_dev *pdev)
 	} else {
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 		pci_wake_from_d3(pdev, false);
 	}
 

drivers/net/ethernet/broadcom/bnx2.c +0 -2

index cb1011f..805daae 100644
@@ -6444,7 +6444,6 @@ static u32 bnx2_find_max_ring(u32 ring_size, u32 max_size)
 	if (!(pcicmd & PCI_COMMAND_MEMORY)) {
 		/* in case PCI block has reset */
 		pci_restore_state(bp->pdev);
-		pci_save_state(bp->pdev);
 	}
 	rc = bnx2_init_nic(bp, 1);
 	if (rc) {

@@ -8718,7 +8717,6 @@ static pci_ers_result_t bnx2_io_slot_reset(struct pci_dev *pdev)
 	} else {
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 
 		if (netif_running(dev))
 			err = bnx2_init_nic(bp, 1);

drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +0 -1

index f0f05d7..8e6eec8 100644
@@ -14216,7 +14216,6 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev)
 
 	pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	if (netif_running(dev))
 		bnx2x_set_power_state(bp, PCI_D0);

drivers/net/ethernet/broadcom/tg3.c +0 -1

index 7f00ec7..ecc1220 100644
@@ -18352,7 +18352,6 @@ static pci_ers_result_t tg3_io_slot_reset(struct pci_dev *pdev)
 
 	pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	if (!netdev || !netif_running(netdev)) {
 		rc = PCI_ERS_RESULT_RECOVERED;

drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c +0 -1

index f92a355..3b1321c 100644
@@ -2933,7 +2933,6 @@ static int t3_reenable_adapter(struct adapter *adapter)
 	}
 	pci_set_master(adapter->pdev);
 	pci_restore_state(adapter->pdev);
-	pci_save_state(adapter->pdev);
 
 	/* Free sge resources */
 	t3_free_sge_resources(adapter);

drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c +0 -2

index 392723e..1ce2091 100644
@@ -5456,7 +5456,6 @@ static pci_ers_result_t eeh_slot_reset(struct pci_dev *pdev)
 
 	if (!adap) {
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 		return PCI_ERS_RESULT_RECOVERED;
 	}
 

@@ -5471,7 +5470,6 @@ static pci_ers_result_t eeh_slot_reset(struct pci_dev *pdev)
 
 	pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	if (t4_wait_dev_ready(adap->regs) < 0)
 		return PCI_ERS_RESULT_DISCONNECT;

drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c +0 -1

index 83cf75b..2eb1e3d 100644
@@ -158,7 +158,6 @@ static pci_ers_result_t hbg_pci_err_slot_reset(struct pci_dev *pdev)
 
 	pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	hbg_err_reset(priv);
 	return PCI_ERS_RESULT_RECOVERED;

drivers/net/ethernet/intel/e1000e/netdev.c +0 -1

index 201322d..7589660 100644
@@ -7195,7 +7195,6 @@ static pci_ers_result_t e1000_io_slot_reset(struct pci_dev *pdev)
 			"Cannot re-enable PCI device after reset.\n");
 		result = PCI_ERS_RESULT_DISCONNECT;
 	} else {
-		pdev->state_saved = true;
 		pci_restore_state(pdev);
 		pci_set_master(pdev);
 

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +0 -6

index ae5fe34..d75b8a5 100644
@@ -2423,12 +2423,6 @@ static pci_ers_result_t fm10k_io_slot_reset(struct pci_dev *pdev)
 	} else {
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-
-		/* After second error pci->state_saved is false, this
-		 * resets it so EEH doesn't break.
-		 */
-		pci_save_state(pdev);
-
 		pci_wake_from_d3(pdev, false);
 
 		result = PCI_ERS_RESULT_RECOVERED;

drivers/net/ethernet/intel/i40e/i40e_main.c +0 -1

index 50be0a6..d8192aa 100644
@@ -16455,7 +16455,6 @@ static pci_ers_result_t i40e_pci_error_slot_reset(struct pci_dev *pdev)
 	} else {
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 		pci_wake_from_d3(pdev, false);
 
 		reg = rd32(&pf->hw, I40E_GLGEN_RTRIG);

drivers/net/ethernet/intel/ice/ice_main.c +0 -2

index 86f5859..6c7dcca 100644
@@ -5663,7 +5663,6 @@ static int ice_resume(struct device *dev)
 
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	if (!pci_device_is_present(pdev))
 		return -ENODEV;

@@ -5763,7 +5762,6 @@ static pci_ers_result_t ice_pci_err_slot_reset(struct pci_dev *pdev)
 	} else {
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 		pci_wake_from_d3(pdev, false);
 
 		/* Check for life */

drivers/net/ethernet/intel/igb/igb_main.c +0 -2

index 85f9589..dbea372 100644
@@ -9599,7 +9599,6 @@ static int __igb_resume(struct device *dev, bool rpm)
 
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	if (!pci_device_is_present(pdev))
 		return -ENODEV;

@@ -9754,7 +9753,6 @@ static pci_ers_result_t igb_io_slot_reset(struct pci_dev *pdev)
 	} else {
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 
 		pci_enable_wake(pdev, PCI_D3hot, 0);
 		pci_enable_wake(pdev, PCI_D3cold, 0);

drivers/net/ethernet/intel/igc/igc_main.c +0 -2

index 728d7ca..7aafa60b 100644
@@ -7530,7 +7530,6 @@ static int __igc_resume(struct device *dev, bool rpm)
 
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	if (!pci_device_is_present(pdev))
 		return -ENODEV;

@@ -7667,7 +7666,6 @@ static pci_ers_result_t igc_io_slot_reset(struct pci_dev *pdev)
 	} else {
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 
 		pci_enable_wake(pdev, PCI_D3hot, 0);
 		pci_enable_wake(pdev, PCI_D3cold, 0);

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0 -1

index 90d4e57..d65d691 100644
@@ -12297,7 +12297,6 @@ static pci_ers_result_t ixgbe_io_slot_reset(struct pci_dev *pdev)
 		adapter->hw.hw_addr = adapter->io_addr;
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 
 		pci_wake_from_d3(pdev, false);
 

drivers/net/ethernet/mellanox/mlx4/main.c +0 -1

index 03d2fc7..d1fbf37 100644
@@ -4366,7 +4366,6 @@ static pci_ers_result_t mlx4_pci_slot_reset(struct pci_dev *pdev)
 
 	pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 	return PCI_ERS_RESULT_RECOVERED;
 }
 

drivers/net/ethernet/mellanox/mlx5/core/main.c +0 -1

index df93625..08f7778 100644
@@ -2095,7 +2095,6 @@ static pci_ers_result_t mlx5_pci_slot_reset(struct pci_dev *pdev)
 
 	pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	err = wait_vital(pdev);
 	if (err) {

drivers/net/ethernet/meta/fbnic/fbnic_pci.c +0 -1

index a7a6b4d..0fa90ba 100644
@@ -574,7 +574,6 @@ static pci_ers_result_t fbnic_err_slot_reset(struct pci_dev *pdev)
 
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	if (pci_enable_device_mem(pdev)) {
 		dev_err(&pdev->dev,

drivers/net/ethernet/microchip/lan743x_main.c +0 -1

index 9d70b51..e4c542f 100644
@@ -3915,7 +3915,6 @@ static int lan743x_pm_resume(struct device *dev)
 
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	/* Restore HW_CFG that was saved during pm suspend */
 	if (adapter->is_pci11x1x)

drivers/net/ethernet/myricom/myri10ge/myri10ge.c +0 -4

index e611ff7..7be30a8 100644
@@ -3416,10 +3416,6 @@ static void myri10ge_watchdog(struct work_struct *work)
 		 * nic was resumed from power saving mode.
 		 */
 		pci_restore_state(mgp->pdev);
-
-		/* save state again for accounting reasons */
-		pci_save_state(mgp->pdev);
-
 	} else {
 		/* if we get back -1's from our slot, perhaps somebody
 		 * powered off our card.  Don't try to reset it in

drivers/net/ethernet/neterion/s2io.c +0 -1

index 5026b02..1e55ccb 100644
@@ -3425,7 +3425,6 @@ static void s2io_reset(struct s2io_nic *sp)
 
 		/* Restore the PCI state saved during initialization. */
 		pci_restore_state(sp->pdev);
-		pci_save_state(sp->pdev);
 		pci_read_config_word(sp->pdev, 0x2, &val16);
 		if (check_pci_device_id(val16) != (u16)PCI_ANY_ID)
 			break;

drivers/pci/pcie/portdrv.c +0 -1

index d1b68c18..38a41cc 100644
@@ -760,7 +760,6 @@ static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
 	device_for_each_child(&dev->dev, &off, pcie_port_device_iter);
 
 	pci_restore_state(dev);
-	pci_save_state(dev);
 	return PCI_ERS_RESULT_RECOVERED;
 }
 

drivers/scsi/bfa/bfad.c +0 -1

index ff9adfc..bdfd065 100644
@@ -1528,7 +1528,6 @@ static int restart_bfa(struct bfad_s *bfad)
 		goto out_disable_device;
 	}
 
-	pci_save_state(pdev);
 	pci_set_master(pdev);
 
 	rc = dma_set_mask_and_coherent(&bfad->pcidev->dev, DMA_BIT_MASK(64));

drivers/scsi/csiostor/csio_init.c +0 -1

index 79c8daf..db0c217 100644
@@ -1093,7 +1093,6 @@ static void csio_remove_one(struct pci_dev *pdev)
 
 	pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	/* Bring HW s/m to ready state.
 	 * but don't resume IOs.

drivers/scsi/ipr.c +0 -1

index 4421488..9512368 100644
@@ -7859,7 +7859,6 @@ static int ipr_reset_restore_cfg_space(struct ipr_cmnd *ipr_cmd)
 	struct ipr_ioa_cfg *ioa_cfg = ipr_cmd->ioa_cfg;
 
 	ENTER;
-	ioa_cfg->pdev->state_saved = true;
 	pci_restore_state(ioa_cfg->pdev);
 
 	if (ipr_set_pcix_cmd_reg(ioa_cfg)) {

drivers/scsi/lpfc/lpfc_init.c +0 -6

index f206267..065eb91 100644
@@ -14434,12 +14434,6 @@ static int lpfc_cpu_online(unsigned int cpu, struct hlist_node *node)
 
 	pci_restore_state(pdev);
 
-	/*
-	 * As the new kernel behavior of pci_restore_state() API call clears
-	 * device saved_state flag, need to save the restored state again.
-	 */
-	pci_save_state(pdev);
-
 	if (pdev->is_busmaster)
 		pci_set_master(pdev);
 

drivers/scsi/qla2xxx/qla_os.c +0 -5

index 5ffd945..9007533e 100644
@@ -7886,11 +7886,6 @@ static void qla_pci_error_cleanup(scsi_qla_host_t *vha)
 
 	pci_restore_state(pdev);
 
-	/* pci_restore_state() clears the saved_state flag of the device
-	 * save restored state which resets saved_state flag
-	 */
-	pci_save_state(pdev);
-
 	if (ha->mem_only)
 		rc = pci_enable_device_mem(pdev);
 	else

drivers/scsi/qla4xxx/ql4_os.c +0 -5

index a761c0a..1f52379 100644
@@ -9796,11 +9796,6 @@ static uint32_t qla4_8xxx_error_recovery(struct scsi_qla_host *ha)
 	 */
 	pci_restore_state(pdev);
 
-	/* pci_restore_state() clears the saved_state flag of the device
-	 * save restored state which resets saved_state flag
-	 */
-	pci_save_state(pdev);
-
 	/* Initialize device or resume if in suspended state */
 	rc = pci_enable_device(pdev);
 	if (rc) {

drivers/tty/serial/8250/8250_pci.c +0 -1

index 152f914..65bd370 100644
@@ -6178,7 +6178,6 @@ static pci_ers_result_t serial8250_io_slot_reset(struct pci_dev *dev)
 		return PCI_ERS_RESULT_DISCONNECT;
 
 	pci_restore_state(dev);
-	pci_save_state(dev);
 
 	return PCI_ERS_RESULT_RECOVERED;
 }

drivers/tty/serial/jsm/jsm_driver.c +0 -1

index 417a5b6..8d21373 100644
@@ -355,7 +355,6 @@ static void jsm_io_resume(struct pci_dev *pdev)
 	struct jsm_board *brd = pci_get_drvdata(pdev);
 
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	jsm_uart_port_init(brd);
 }

In 2009, commit c82f63e411f1 ("PCI: check saved state before restore") changed the behavior of pci_restore_state() such that it became necessary to call pci_save_state() afterwards, lest recovery from subsequent PCI errors fails.

The commit has just been reverted and so all the pci_save_state() after pci_restore_state() calls that have accumulated in the tree are now superfluous. Drop them.

Two drivers chose a different approach to achieve the same result:
drivers/scsi/ipr.c and drivers/net/ethernet/intel/e1000e/netdev.c set the pci_dev's "state_saved" flag to true before calling pci_restore_state(). Drop this as well.

Signed-off-by Lukas Wunner <[email protected]>

In 2009, commit c82f63e411f1 ("PCI: check saved state before restore") changed the behavior of pci_restore_state() such that it became necessary to call pci_save_state() afterwards, lest recovery from subsequent PCI errors fails.

The commit has just been reverted and so all the pci_save_state() after pci_restore_state() calls that have accumulated in the tree are now superfluous. Drop them.

Two drivers chose a different approach to achieve the same result:
drivers/scsi/ipr.c and drivers/net/ethernet/intel/e1000e/netdev.c set the pci_dev's "state_saved" flag to true before calling pci_restore_state(). Drop this as well.

Signed-off-by Lukas Wunner <[email protected]>

---
Some of the pci_save_state() calls in drivers' probe hooks may now likewise be superfluous if the probe hook doesn't touch Config Space. These calls will be identified and dealt with individually.

 drivers/crypto/intel/qat/qat_common/adf_aer.c    | 2 --
 drivers/dma/ioat/init.c                          | 1 -
 drivers/net/ethernet/broadcom/bnx2.c             | 2 --
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 1 -
 drivers/net/ethernet/broadcom/tg3.c              | 1 -
 drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c  | 1 -
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c  | 2 --
 drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c | 1 -
 drivers/net/ethernet/intel/e1000e/netdev.c       | 1 -
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c     | 6 ------
 drivers/net/ethernet/intel/i40e/i40e_main.c      | 1 -
 drivers/net/ethernet/intel/ice/ice_main.c        | 2 --
 drivers/net/ethernet/intel/igb/igb_main.c        | 2 --
 drivers/net/ethernet/intel/igc/igc_main.c        | 2 --
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    | 1 -
 drivers/net/ethernet/mellanox/mlx4/main.c        | 1 -
 drivers/net/ethernet/mellanox/mlx5/core/main.c   | 1 -
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c      | 1 -
 drivers/net/ethernet/microchip/lan743x_main.c    | 1 -
 drivers/net/ethernet/myricom/myri10ge/myri10ge.c | 4 ----
 drivers/net/ethernet/neterion/s2io.c             | 1 -
 drivers/pci/pcie/portdrv.c                       | 1 -
 drivers/scsi/bfa/bfad.c                          | 1 -
 drivers/scsi/csiostor/csio_init.c                | 1 -
 drivers/scsi/ipr.c                               | 1 -
 drivers/scsi/lpfc/lpfc_init.c                    | 6 ------
 drivers/scsi/qla2xxx/qla_os.c                    | 5 -----
 drivers/scsi/qla4xxx/ql4_os.c                    | 5 -----
 drivers/tty/serial/8250/8250_pci.c               | 1 -
 drivers/tty/serial/jsm/jsm_driver.c              | 1 -
 30 files changed, 57 deletions(-)

drivers/crypto/intel/qat/qat_common/adf_aer.c +0 -2

index 35679b2..9a5a4b3 100644
@@ -105,7 +105,6 @@ void adf_dev_restore(struct adf_accel_dev *accel_dev)
 			 accel_dev->accel_id);
 		hw_device->reset_device(accel_dev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 	}
 }
 

@@ -204,7 +203,6 @@ static pci_ers_result_t adf_slot_reset(struct pci_dev *pdev)
 	if (!pdev->is_busmaster)
 		pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 	res = adf_dev_up(accel_dev, false);
 	if (res && res != -EALREADY)
 		return PCI_ERS_RESULT_DISCONNECT;

drivers/dma/ioat/init.c +0 -1

index 02f68b3..2273986 100644
@@ -1286,7 +1286,6 @@ static pci_ers_result_t ioat_pcie_error_slot_reset(struct pci_dev *pdev)
 	} else {
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 		pci_wake_from_d3(pdev, false);
 	}
 

drivers/net/ethernet/broadcom/bnx2.c +0 -2

index cb1011f..805daae 100644
@@ -6444,7 +6444,6 @@ static u32 bnx2_find_max_ring(u32 ring_size, u32 max_size)
 	if (!(pcicmd & PCI_COMMAND_MEMORY)) {
 		/* in case PCI block has reset */
 		pci_restore_state(bp->pdev);
-		pci_save_state(bp->pdev);
 	}
 	rc = bnx2_init_nic(bp, 1);
 	if (rc) {

@@ -8718,7 +8717,6 @@ static pci_ers_result_t bnx2_io_slot_reset(struct pci_dev *pdev)
 	} else {
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 
 		if (netif_running(dev))
 			err = bnx2_init_nic(bp, 1);

drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +0 -1

index f0f05d7..8e6eec8 100644
@@ -14216,7 +14216,6 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev)
 
 	pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	if (netif_running(dev))
 		bnx2x_set_power_state(bp, PCI_D0);

drivers/net/ethernet/broadcom/tg3.c +0 -1

index 7f00ec7..ecc1220 100644
@@ -18352,7 +18352,6 @@ static pci_ers_result_t tg3_io_slot_reset(struct pci_dev *pdev)
 
 	pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	if (!netdev || !netif_running(netdev)) {
 		rc = PCI_ERS_RESULT_RECOVERED;

drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c +0 -1

index f92a355..3b1321c 100644
@@ -2933,7 +2933,6 @@ static int t3_reenable_adapter(struct adapter *adapter)
 	}
 	pci_set_master(adapter->pdev);
 	pci_restore_state(adapter->pdev);
-	pci_save_state(adapter->pdev);
 
 	/* Free sge resources */
 	t3_free_sge_resources(adapter);

drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c +0 -2

index 392723e..1ce2091 100644
@@ -5456,7 +5456,6 @@ static pci_ers_result_t eeh_slot_reset(struct pci_dev *pdev)
 
 	if (!adap) {
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 		return PCI_ERS_RESULT_RECOVERED;
 	}
 

@@ -5471,7 +5470,6 @@ static pci_ers_result_t eeh_slot_reset(struct pci_dev *pdev)
 
 	pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	if (t4_wait_dev_ready(adap->regs) < 0)
 		return PCI_ERS_RESULT_DISCONNECT;

drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c +0 -1

index 83cf75b..2eb1e3d 100644
@@ -158,7 +158,6 @@ static pci_ers_result_t hbg_pci_err_slot_reset(struct pci_dev *pdev)
 
 	pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	hbg_err_reset(priv);
 	return PCI_ERS_RESULT_RECOVERED;

drivers/net/ethernet/intel/e1000e/netdev.c +0 -1

index 201322d..7589660 100644
@@ -7195,7 +7195,6 @@ static pci_ers_result_t e1000_io_slot_reset(struct pci_dev *pdev)
 			"Cannot re-enable PCI device after reset.\n");
 		result = PCI_ERS_RESULT_DISCONNECT;
 	} else {
-		pdev->state_saved = true;
 		pci_restore_state(pdev);
 		pci_set_master(pdev);
 

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +0 -6

index ae5fe34..d75b8a5 100644
@@ -2423,12 +2423,6 @@ static pci_ers_result_t fm10k_io_slot_reset(struct pci_dev *pdev)
 	} else {
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-
-		/* After second error pci->state_saved is false, this
-		 * resets it so EEH doesn't break.
-		 */
-		pci_save_state(pdev);
-
 		pci_wake_from_d3(pdev, false);
 
 		result = PCI_ERS_RESULT_RECOVERED;

drivers/net/ethernet/intel/i40e/i40e_main.c +0 -1

index 50be0a6..d8192aa 100644
@@ -16455,7 +16455,6 @@ static pci_ers_result_t i40e_pci_error_slot_reset(struct pci_dev *pdev)
 	} else {
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 		pci_wake_from_d3(pdev, false);
 
 		reg = rd32(&pf->hw, I40E_GLGEN_RTRIG);

drivers/net/ethernet/intel/ice/ice_main.c +0 -2

index 86f5859..6c7dcca 100644
@@ -5663,7 +5663,6 @@ static int ice_resume(struct device *dev)
 
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	if (!pci_device_is_present(pdev))
 		return -ENODEV;

@@ -5763,7 +5762,6 @@ static pci_ers_result_t ice_pci_err_slot_reset(struct pci_dev *pdev)
 	} else {
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 		pci_wake_from_d3(pdev, false);
 
 		/* Check for life */

drivers/net/ethernet/intel/igb/igb_main.c +0 -2

index 85f9589..dbea372 100644
@@ -9599,7 +9599,6 @@ static int __igb_resume(struct device *dev, bool rpm)
 
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	if (!pci_device_is_present(pdev))
 		return -ENODEV;

@@ -9754,7 +9753,6 @@ static pci_ers_result_t igb_io_slot_reset(struct pci_dev *pdev)
 	} else {
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 
 		pci_enable_wake(pdev, PCI_D3hot, 0);
 		pci_enable_wake(pdev, PCI_D3cold, 0);

drivers/net/ethernet/intel/igc/igc_main.c +0 -2

index 728d7ca..7aafa60b 100644
@@ -7530,7 +7530,6 @@ static int __igc_resume(struct device *dev, bool rpm)
 
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	if (!pci_device_is_present(pdev))
 		return -ENODEV;

@@ -7667,7 +7666,6 @@ static pci_ers_result_t igc_io_slot_reset(struct pci_dev *pdev)
 	} else {
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 
 		pci_enable_wake(pdev, PCI_D3hot, 0);
 		pci_enable_wake(pdev, PCI_D3cold, 0);

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0 -1

index 90d4e57..d65d691 100644
@@ -12297,7 +12297,6 @@ static pci_ers_result_t ixgbe_io_slot_reset(struct pci_dev *pdev)
 		adapter->hw.hw_addr = adapter->io_addr;
 		pci_set_master(pdev);
 		pci_restore_state(pdev);
-		pci_save_state(pdev);
 
 		pci_wake_from_d3(pdev, false);
 

drivers/net/ethernet/mellanox/mlx4/main.c +0 -1

index 03d2fc7..d1fbf37 100644
@@ -4366,7 +4366,6 @@ static pci_ers_result_t mlx4_pci_slot_reset(struct pci_dev *pdev)
 
 	pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 	return PCI_ERS_RESULT_RECOVERED;
 }
 

drivers/net/ethernet/mellanox/mlx5/core/main.c +0 -1

index df93625..08f7778 100644
@@ -2095,7 +2095,6 @@ static pci_ers_result_t mlx5_pci_slot_reset(struct pci_dev *pdev)
 
 	pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	err = wait_vital(pdev);
 	if (err) {

drivers/net/ethernet/meta/fbnic/fbnic_pci.c +0 -1

index a7a6b4d..0fa90ba 100644
@@ -574,7 +574,6 @@ static pci_ers_result_t fbnic_err_slot_reset(struct pci_dev *pdev)
 
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	if (pci_enable_device_mem(pdev)) {
 		dev_err(&pdev->dev,

drivers/net/ethernet/microchip/lan743x_main.c +0 -1

index 9d70b51..e4c542f 100644
@@ -3915,7 +3915,6 @@ static int lan743x_pm_resume(struct device *dev)
 
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	/* Restore HW_CFG that was saved during pm suspend */
 	if (adapter->is_pci11x1x)

drivers/net/ethernet/myricom/myri10ge/myri10ge.c +0 -4

index e611ff7..7be30a8 100644
@@ -3416,10 +3416,6 @@ static void myri10ge_watchdog(struct work_struct *work)
 		 * nic was resumed from power saving mode.
 		 */
 		pci_restore_state(mgp->pdev);
-
-		/* save state again for accounting reasons */
-		pci_save_state(mgp->pdev);
-
 	} else {
 		/* if we get back -1's from our slot, perhaps somebody
 		 * powered off our card.  Don't try to reset it in

drivers/net/ethernet/neterion/s2io.c +0 -1

index 5026b02..1e55ccb 100644
@@ -3425,7 +3425,6 @@ static void s2io_reset(struct s2io_nic *sp)
 
 		/* Restore the PCI state saved during initialization. */
 		pci_restore_state(sp->pdev);
-		pci_save_state(sp->pdev);
 		pci_read_config_word(sp->pdev, 0x2, &val16);
 		if (check_pci_device_id(val16) != (u16)PCI_ANY_ID)
 			break;

drivers/pci/pcie/portdrv.c +0 -1

index d1b68c18..38a41cc 100644
@@ -760,7 +760,6 @@ static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
 	device_for_each_child(&dev->dev, &off, pcie_port_device_iter);
 
 	pci_restore_state(dev);
-	pci_save_state(dev);
 	return PCI_ERS_RESULT_RECOVERED;
 }
 

drivers/scsi/bfa/bfad.c +0 -1

index ff9adfc..bdfd065 100644
@@ -1528,7 +1528,6 @@ static int restart_bfa(struct bfad_s *bfad)
 		goto out_disable_device;
 	}
 
-	pci_save_state(pdev);
 	pci_set_master(pdev);
 
 	rc = dma_set_mask_and_coherent(&bfad->pcidev->dev, DMA_BIT_MASK(64));

drivers/scsi/csiostor/csio_init.c +0 -1

index 79c8daf..db0c217 100644
@@ -1093,7 +1093,6 @@ static void csio_remove_one(struct pci_dev *pdev)
 
 	pci_set_master(pdev);
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	/* Bring HW s/m to ready state.
 	 * but don't resume IOs.

drivers/scsi/ipr.c +0 -1

index 4421488..9512368 100644
@@ -7859,7 +7859,6 @@ static int ipr_reset_restore_cfg_space(struct ipr_cmnd *ipr_cmd)
 	struct ipr_ioa_cfg *ioa_cfg = ipr_cmd->ioa_cfg;
 
 	ENTER;
-	ioa_cfg->pdev->state_saved = true;
 	pci_restore_state(ioa_cfg->pdev);
 
 	if (ipr_set_pcix_cmd_reg(ioa_cfg)) {

drivers/scsi/lpfc/lpfc_init.c +0 -6

index f206267..065eb91 100644
@@ -14434,12 +14434,6 @@ static int lpfc_cpu_online(unsigned int cpu, struct hlist_node *node)
 
 	pci_restore_state(pdev);
 
-	/*
-	 * As the new kernel behavior of pci_restore_state() API call clears
-	 * device saved_state flag, need to save the restored state again.
-	 */
-	pci_save_state(pdev);
-
 	if (pdev->is_busmaster)
 		pci_set_master(pdev);
 

drivers/scsi/qla2xxx/qla_os.c +0 -5

index 5ffd945..9007533e 100644
@@ -7886,11 +7886,6 @@ static void qla_pci_error_cleanup(scsi_qla_host_t *vha)
 
 	pci_restore_state(pdev);
 
-	/* pci_restore_state() clears the saved_state flag of the device
-	 * save restored state which resets saved_state flag
-	 */
-	pci_save_state(pdev);
-
 	if (ha->mem_only)
 		rc = pci_enable_device_mem(pdev);
 	else

drivers/scsi/qla4xxx/ql4_os.c +0 -5

index a761c0a..1f52379 100644
@@ -9796,11 +9796,6 @@ static uint32_t qla4_8xxx_error_recovery(struct scsi_qla_host *ha)
 	 */
 	pci_restore_state(pdev);
 
-	/* pci_restore_state() clears the saved_state flag of the device
-	 * save restored state which resets saved_state flag
-	 */
-	pci_save_state(pdev);
-
 	/* Initialize device or resume if in suspended state */
 	rc = pci_enable_device(pdev);
 	if (rc) {

drivers/tty/serial/8250/8250_pci.c +0 -1

index 152f914..65bd370 100644
@@ -6178,7 +6178,6 @@ static pci_ers_result_t serial8250_io_slot_reset(struct pci_dev *dev)
 		return PCI_ERS_RESULT_DISCONNECT;
 
 	pci_restore_state(dev);
-	pci_save_state(dev);
 
 	return PCI_ERS_RESULT_RECOVERED;
 }

drivers/tty/serial/jsm/jsm_driver.c +0 -1

index 417a5b6..8d21373 100644
@@ -355,7 +355,6 @@ static void jsm_io_resume(struct pci_dev *pdev)
 	struct jsm_board *brd = pci_get_drvdata(pdev);
 
 	pci_restore_state(pdev);
-	pci_save_state(pdev);
 
 	jsm_uart_port_init(brd);
 }

Signature · 1 line

2.51.0

When PCI devices are reset -- either to recover from an error or after a D3hot/D3cold transition -- their Config Space needs to be restored.

D3hot/D3cold transitions happen under the control of the kernel, hence it is able to save Config Space before and restore it afterwards.

However errors may occur unexpectedly and it may then be impossible to save Config Space because the device may be inaccessible (e.g. DPC) or Config Space may be corrupted. So it must be saved ahead of time.

This isn't done consistently because the PCI core doesn't take care of it and only a subset of drivers do. The situation is aggravated by the behavior of pci_restore_state(), which only allows restoring Config Space once and invalidates the saved copy afterwards.

Solve all these problems by saving an initial copy of Config Space on device addition which drivers may update if they change registers. Modify pci_restore_state() to allow using the saved copy indefinitely and drop all the workarounds for its previous behavior that have accumulated in the tree.

Lukas Wunner (2):
PCI: Ensure error recoverability at all times
treewide: Drop pci_save_state() after pci_restore_state()

 drivers/crypto/intel/qat/qat_common/adf_aer.c    | 2 --
 drivers/dma/ioat/init.c                          | 1 -
 drivers/net/ethernet/broadcom/bnx2.c             | 2 --
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 1 -
 drivers/net/ethernet/broadcom/tg3.c              | 1 -
 drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c  | 1 -
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c  | 2 --
 drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c | 1 -
 drivers/net/ethernet/intel/e1000e/netdev.c       | 1 -
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c     | 6 ------
 drivers/net/ethernet/intel/i40e/i40e_main.c      | 1 -
 drivers/net/ethernet/intel/ice/ice_main.c        | 2 --
 drivers/net/ethernet/intel/igb/igb_main.c        | 2 --
 drivers/net/ethernet/intel/igc/igc_main.c        | 2 --
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    | 1 -
 drivers/net/ethernet/mellanox/mlx4/main.c        | 1 -
 drivers/net/ethernet/mellanox/mlx5/core/main.c   | 1 -
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c      | 1 -
 drivers/net/ethernet/microchip/lan743x_main.c    | 1 -
 drivers/net/ethernet/myricom/myri10ge/myri10ge.c | 4 ----
 drivers/net/ethernet/neterion/s2io.c             | 1 -
 drivers/pci/bus.c                                | 7 +++++++
 drivers/pci/pci.c                                | 3 ---
 drivers/pci/pcie/portdrv.c                       | 1 -
 drivers/pci/probe.c                              | 2 --
 drivers/scsi/bfa/bfad.c                          | 1 -

 drivers/scsi/csiostor/csio_init.c                | 1 -
 drivers/scsi/ipr.c                               | 1 -
 drivers/scsi/lpfc/lpfc_init.c                    | 6 ------
 drivers/scsi/qla2xxx/qla_os.c                    | 5 -----
 drivers/scsi/qla4xxx/ql4_os.c                    | 5 -----
 drivers/tty/serial/8250/8250_pci.c               | 1 -
 drivers/tty/serial/jsm/jsm_driver.c              | 1 -
 33 files changed, 7 insertions(+), 62 deletions(-)

Quoted 3 lines · Lukas Wunner

Lukas Wunner (2):
PCI: Ensure error recoverability at all times
treewide: Drop pci_save_state() after pci_restore_state()

Applied to pci/err, maybe for v6.19?

[trim recipients]

Quoted 6 lines · Bjorn Helgaas

Quoted 3 lines · Lukas Wunner

Lukas Wunner (2):
PCI: Ensure error recoverability at all times
treewide: Drop pci_save_state() after pci_restore_state()

Applied to pci/err, maybe for v6.19?

Thank you! Please note that the second patch (the treewide one) was ack'ed by Giovanni Cabiddu:

https://lore.kernel.org/all/[email protected]/

... so you might want to add that tag to commit 73f1f9b0a2c9 ("treewide: Drop pci_save_state() after pci_restore_state()") on the pci/err topic branch.