[PATCH 0/2] PCI: Universal error recoverability of devices

[PATCH 0/2] PCI: Universal error recoverability of devices

[Lukas Wunner]

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(-)

-- 
2.51.0

[PATCH 1/2] PCI: Ensure error recoverability at all times

[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 <riana.tauro@intel.com> # off-list
Tested-by: Riana Tauro <riana.tauro@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
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(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index f26aec6..4318568 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -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
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b14dd06..2f0da5d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index c83e75a..c7c7a3d 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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);
 
 	/*
-- 
2.51.0

[PATCH 2/2] treewide: Drop pci_save_state() after pci_restore_state()

[Lukas Wunner]

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 <lukas@wunner.de>
---
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(-)

diff --git a/drivers/crypto/intel/qat/qat_common/adf_aer.c b/drivers/crypto/intel/qat/qat_common/adf_aer.c
index 35679b2..9a5a4b3 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_aer.c
+++ b/drivers/crypto/intel/qat/qat_common/adf_aer.c
@@ -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;
diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
index 02f68b3..2273986 100644
--- a/drivers/dma/ioat/init.c
+++ b/drivers/dma/ioat/init.c
@@ -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);
 	}
 
diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index cb1011f..805daae 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -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);
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index f0f05d7..8e6eec8 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -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);
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 7f00ec7..ecc1220 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -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;
diff --git a/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c b/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c
index f92a355..3b1321c 100644
--- a/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c
@@ -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);
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 392723e..1ce2091 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -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;
diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c
index 83cf75b..2eb1e3d 100644
--- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c
@@ -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;
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 201322d..7589660 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -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);
 
diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
index ae5fe34..d75b8a5 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
@@ -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;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 50be0a6..d8192aa 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -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);
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 86f5859..6c7dcca 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -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 */
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 85f9589..dbea372 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -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);
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 728d7ca..7aafa60b 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -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);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 90d4e57..d65d691 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -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);
 
diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index 03d2fc7..d1fbf37 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -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;
 }
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index df93625..08f7778 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -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) {
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
index a7a6b4d..0fa90ba 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
@@ -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,
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 9d70b51..e4c542f 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -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)
diff --git a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
index e611ff7..7be30a8 100644
--- a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
+++ b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
@@ -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
diff --git a/drivers/net/ethernet/neterion/s2io.c b/drivers/net/ethernet/neterion/s2io.c
index 5026b02..1e55ccb 100644
--- a/drivers/net/ethernet/neterion/s2io.c
+++ b/drivers/net/ethernet/neterion/s2io.c
@@ -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;
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index d1b68c18..38a41cc 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -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;
 }
 
diff --git a/drivers/scsi/bfa/bfad.c b/drivers/scsi/bfa/bfad.c
index ff9adfc..bdfd065 100644
--- a/drivers/scsi/bfa/bfad.c
+++ b/drivers/scsi/bfa/bfad.c
@@ -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));
diff --git a/drivers/scsi/csiostor/csio_init.c b/drivers/scsi/csiostor/csio_init.c
index 79c8daf..db0c217 100644
--- a/drivers/scsi/csiostor/csio_init.c
+++ b/drivers/scsi/csiostor/csio_init.c
@@ -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.
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 4421488..9512368 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -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)) {
diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index f206267..065eb91 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -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);
 
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 5ffd945..9007533e 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -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
diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index a761c0a..1f52379 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -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) {
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 152f914..65bd370 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -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;
 }
diff --git a/drivers/tty/serial/jsm/jsm_driver.c b/drivers/tty/serial/jsm/jsm_driver.c
index 417a5b6..8d21373 100644
--- a/drivers/tty/serial/jsm/jsm_driver.c
+++ b/drivers/tty/serial/jsm/jsm_driver.c
@@ -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);
 }
-- 
2.51.0

Re: [PATCH 2/2] treewide: Drop pci_save_state() after pci_restore_state()

[Dave Jiang]

On 10/12/25 6:25 AM, Lukas Wunner wrote:
> 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 <lukas@wunner.de>

For ioatdma changes:
Acked-by: Dave Jiang <dave.jiang@intel.com>

> ---
> 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(-)
> 
> diff --git a/drivers/crypto/intel/qat/qat_common/adf_aer.c b/drivers/crypto/intel/qat/qat_common/adf_aer.c
> index 35679b2..9a5a4b3 100644
> --- a/drivers/crypto/intel/qat/qat_common/adf_aer.c
> +++ b/drivers/crypto/intel/qat/qat_common/adf_aer.c
> @@ -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;
> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> index 02f68b3..2273986 100644
> --- a/drivers/dma/ioat/init.c
> +++ b/drivers/dma/ioat/init.c
> @@ -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);
>  	}
>  
> diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
> index cb1011f..805daae 100644
> --- a/drivers/net/ethernet/broadcom/bnx2.c
> +++ b/drivers/net/ethernet/broadcom/bnx2.c
> @@ -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);
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> index f0f05d7..8e6eec8 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -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);
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 7f00ec7..ecc1220 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -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;
> diff --git a/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c b/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c
> index f92a355..3b1321c 100644
> --- a/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c
> +++ b/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c
> @@ -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);
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> index 392723e..1ce2091 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> @@ -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;
> diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c
> index 83cf75b..2eb1e3d 100644
> --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c
> +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c
> @@ -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;
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 201322d..7589660 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -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);
>  
> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
> index ae5fe34..d75b8a5 100644
> --- a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
> @@ -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;
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 50be0a6..d8192aa 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -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);
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 86f5859..6c7dcca 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -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 */
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 85f9589..dbea372 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -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);
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 728d7ca..7aafa60b 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -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);
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 90d4e57..d65d691 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -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);
>  
> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
> index 03d2fc7..d1fbf37 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
> @@ -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;
>  }
>  
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> index df93625..08f7778 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> @@ -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) {
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> index a7a6b4d..0fa90ba 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> @@ -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,
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index 9d70b51..e4c542f 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -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)
> diff --git a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
> index e611ff7..7be30a8 100644
> --- a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
> +++ b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
> @@ -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
> diff --git a/drivers/net/ethernet/neterion/s2io.c b/drivers/net/ethernet/neterion/s2io.c
> index 5026b02..1e55ccb 100644
> --- a/drivers/net/ethernet/neterion/s2io.c
> +++ b/drivers/net/ethernet/neterion/s2io.c
> @@ -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;
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index d1b68c18..38a41cc 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -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;
>  }
>  
> diff --git a/drivers/scsi/bfa/bfad.c b/drivers/scsi/bfa/bfad.c
> index ff9adfc..bdfd065 100644
> --- a/drivers/scsi/bfa/bfad.c
> +++ b/drivers/scsi/bfa/bfad.c
> @@ -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));
> diff --git a/drivers/scsi/csiostor/csio_init.c b/drivers/scsi/csiostor/csio_init.c
> index 79c8daf..db0c217 100644
> --- a/drivers/scsi/csiostor/csio_init.c
> +++ b/drivers/scsi/csiostor/csio_init.c
> @@ -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.
> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
> index 4421488..9512368 100644
> --- a/drivers/scsi/ipr.c
> +++ b/drivers/scsi/ipr.c
> @@ -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)) {
> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index f206267..065eb91 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -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);
>  
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 5ffd945..9007533e 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -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
> diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
> index a761c0a..1f52379 100644
> --- a/drivers/scsi/qla4xxx/ql4_os.c
> +++ b/drivers/scsi/qla4xxx/ql4_os.c
> @@ -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) {
> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index 152f914..65bd370 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -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;
>  }
> diff --git a/drivers/tty/serial/jsm/jsm_driver.c b/drivers/tty/serial/jsm/jsm_driver.c
> index 417a5b6..8d21373 100644
> --- a/drivers/tty/serial/jsm/jsm_driver.c
> +++ b/drivers/tty/serial/jsm/jsm_driver.c
> @@ -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);
>  }

Re: [PATCH 2/2] treewide: Drop pci_save_state() after pci_restore_state()

[Giovanni Cabiddu]

On Sun, Oct 12, 2025 at 02:25:02PM +0100, Lukas Wunner wrote:
> 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 <lukas@wunner.de>

For the changes in the QAT driver:
Acked-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com> [qat]

Regards,

-- 
Giovanni

Re: [PATCH 1/2] PCI: Ensure error recoverability at all times

[Bjorn Helgaas]

On Sun, Oct 12, 2025 at 03:25:01PM +0200, Lukas Wunner wrote:
> 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.  

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

> 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 <riana.tauro@intel.com> # off-list
> Tested-by: Riana Tauro <riana.tauro@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
> 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(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index f26aec6..4318568 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -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, and 
Documentation/PCI/pci-error-recovery.rst contains this:

  It is important for the platform to restore the PCI config space
  to the "fresh poweron" state, rather than the "last state". After
  a slot reset, the device driver will almost always use its standard
  device initialization routines, and an unusual config space setup
  may result in hung devices, kernel panics, or silent data corruption.

The PCI core doesn't call pci_restore_state() in the error recovery
path, so maybe that matches the platform (PCI core) restoring "fresh
poweron" state.  And maybe the driver using "its standard device init
routines" is an oblique reference to things like pci_restore_state(),
which is called by several .slot_reset() and .reset_done() callbacks?

pci.rst mentions the PCI core saving and restoring the "standard
configuration registers if the driver hasn't done that", which I guess
refers to pci_save_state().  This seems like it could be more explicit
and maybe could mention the default of saving/restoring the state at
suspend, and the option that if the driver itself calls
pci_save_state(), *that* state will be restored.

This all looks good to me, and I'm sorry I took so long to get to it.

But I surely would like to have Rafael's ack for this because power
management is a mystery to me.

> +	 */
> +	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
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b14dd06..2f0da5d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index c83e75a..c7c7a3d 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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);
>  
>  	/*
> -- 
> 2.51.0
> 

Re: [PATCH 1/2] PCI: Ensure error recoverability at all times

[Lukas Wunner]

On Wed, Nov 12, 2025 at 04:38:31PM -0600, Bjorn Helgaas wrote:
> On Sun, Oct 12, 2025 at 03:25:01PM +0200, Lukas Wunner wrote:
> > 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.  
> 
> 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.

> > +++ b/drivers/pci/bus.c
> > @@ -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."

> 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.

> 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

Right.

> 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. :)

The "state_saved" flag in struct pci_dev is an internal flag used by
the PCI core to keep track of whether a driver called pci_save_state()
on suspend.

The logic to update the flag is not modified by the patch, deliberately so
to avoid any breakage.  The flag is currently initialized to false in
pci_device_add() (even though it already is false due to kzalloc() zeroing
the memory).  I'm now later calling pci_save_state() in pci_bus_add_device(),
which sets the flag to true.  To preserve the existing logic, I am resetting
the flag to false again.

The only change made by the patch is to not invalidate the saved state
upon pci_restore_state() and thus allow re-using it for error recovery.
The patch seeks to avoid changing the behavior of suspend/resume.
I wanted to keep this minimal, non-intrusive and as low risk as possible.

Thanks,

Lukas

Re: [PATCH 1/2] PCI: Ensure error recoverability at all times

[Bjorn Helgaas]

On Thu, Nov 13, 2025 at 10:38:09AM +0100, Lukas Wunner wrote:
> On Wed, Nov 12, 2025 at 04:38:31PM -0600, Bjorn Helgaas wrote:
> > On Sun, Oct 12, 2025 at 03:25:01PM +0200, Lukas Wunner wrote:
> > > 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.  
> > 
> > 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.

Agreed.

> > > +++ b/drivers/pci/bus.c
> > > @@ -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."

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 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.

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)?

> > 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
> 
> Right.
> 
> > 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. :)

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.  I guess that's obvious now that I write it
down, but I'm sure learning a lot from this conversation :)

Bjorn

Re: [PATCH 1/2] PCI: Ensure error recoverability at all times

[Rafael J. Wysocki]

On Sun, Oct 12, 2025 at 3:30 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> 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 <riana.tauro@intel.com> # off-list
> Tested-by: Riana Tauro <riana.tauro@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
> 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(-)
>
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index f26aec6..4318568 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -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
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b14dd06..2f0da5d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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;
> -

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?

>         pci_restore_pcie_state(dev);
>         pci_restore_pasid_state(dev);
>         pci_restore_pri_state(dev);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index c83e75a..c7c7a3d 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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);
>
>         /*
> --

Re: [PATCH 1/2] PCI: Ensure error recoverability at all times

[Rafael J. Wysocki]

On Thu, Nov 13, 2025 at 9:49 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sun, Oct 12, 2025 at 3:30 PM Lukas Wunner <lukas@wunner.de> wrote:
> >
> > 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 <riana.tauro@intel.com> # off-list
> > Tested-by: Riana Tauro <riana.tauro@intel.com>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > ---
> > 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(-)
> >
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index f26aec6..4318568 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -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
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index b14dd06..2f0da5d 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -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;
> > -
>
> 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?

Never mind, this hasn't changed.

So I agree with Bjorn that it would be good to expand the new comment
in pci_bus_add_device() because it doesn't really explain much in its
current form.  Or maybe even split it to say "Save config space for
error recoverability" before pci_save_state(dev) and then explain why
state_saved is cleared after it?

Apart from this and modulo possible changelog adjustments

Reviewed-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>

Re: [PATCH 1/2] PCI: Ensure error recoverability at all times

[Lukas Wunner]

On Thu, Nov 13, 2025 at 10:15:56AM -0600, Bjorn Helgaas wrote:
> 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.

> > > > +++ b/drivers/pci/bus.c
> > > > @@ -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
[...]
> > > 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 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.

> 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 :)

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. :)

Thanks,

Lukas

Re: [PATCH 1/2] PCI: Ensure error recoverability at all times

[Bjorn Helgaas]

On Fri, Nov 14, 2025 at 07:58:19PM +0100, Lukas Wunner wrote:
> On Thu, Nov 13, 2025 at 10:15:56AM -0600, Bjorn Helgaas wrote:
> > 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.

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.  I think it's the sequence of
"pci_save_state(); dev->state_saved = false" that seems
counter-intuitive: we just saved the state; why would we immediately
turn around and say we didn't?  Maybe "state_saved" isn't the most
appropriate name.

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?

> 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.
> 
> > > > > +++ b/drivers/pci/bus.c
> > > > > @@ -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
> [...]
> > > > 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 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.
> 
> > 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 :)
> 
> 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. :)

Absolutely.  Would you mind posting an update with the tweaks above?
I'm not at all confident about doing it myself.

Bjorn

Re: [PATCH 0/2] PCI: Universal error recoverability of devices

[Bjorn Helgaas]

On Sun, Oct 12, 2025 at 03:25:00PM +0200, Lukas Wunner wrote:
> 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(-)

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

It touches a lot of drivers, so it'd be nice to have more time in
-next, but it is mostly in error recovery paths that aren't going to
be exercised much anyway.

I'll watch for a minor update of comments and update if I see it.

Thanks a lot for your work and description of this.  It's a big step
in my understanding of PM and error recovery.  Which still leaves me
mostly ignorant, just slightly less so.

Bjorn

Re: [PATCH 1/2] PCI: Ensure error recoverability at all times

[Lukas Wunner]

On Fri, Nov 14, 2025 at 05:39:27PM -0600, Bjorn Helgaas wrote:
> On Fri, Nov 14, 2025 at 07:58:19PM +0100, Lukas Wunner wrote:
> > On Thu, Nov 13, 2025 at 10:15:56AM -0600, Bjorn Helgaas wrote:
> > > 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.
> 
> 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.

I agree it defies common sense.  So I've just submitted a series
which adds the missing "state_saved = false" in the legacy suspend
and !pm codepaths:

https://lore.kernel.org/r/094f2aad64418710daf0940112abe5a0afdc6bce.1763483367.git.lukas@wunner.de/

After this patch, the flag is always cleared before commencing the
suspend sequence and hence there is no longer a need for drivers to
clear state_saved after they call pci_save_state().  They can just
call pci_save_state() if they've modified Config Space in their
probe hook and be done with 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?

Correct.  The expectation is generally that they're identical.

E.g. I've just double-checked that we're enabling wakeup *after*
pci_save_state() in pci_pm_suspend_noirq().  So when the saved
state is restored on resume and later re-used for error recovery,
we're restoring the device with wakeup disabled, which is the
right thing to do because the device is in D0 after error recovery
issues a reset.

(pci_pm_suspend_noirq() first calls pci_save_state() and then calls
pci_prepare_to_sleep(), which enables wakeup.)

Thanks,

Lukas

Re: [PATCH 1/2] PCI: Ensure error recoverability at all times

[Lukas Wunner]

On Thu, Nov 13, 2025 at 10:15:56AM -0600, Bjorn Helgaas wrote:
> On Thu, Nov 13, 2025 at 10:38:09AM +0100, Lukas Wunner wrote:
> > On Wed, Nov 12, 2025 at 04:38:31PM -0600, Bjorn Helgaas wrote:
> > > 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
> > 
> > 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.

> 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/

Thanks,

Lukas

Re: [PATCH 1/2] PCI: Ensure error recoverability at all times

[Bjorn Helgaas]

On Fri, Nov 21, 2025 at 06:40:23PM +0100, Lukas Wunner wrote:
> On Thu, Nov 13, 2025 at 10:15:56AM -0600, Bjorn Helgaas wrote:
> > On Thu, Nov 13, 2025 at 10:38:09AM +0100, Lukas Wunner wrote:
> > > On Wed, Nov 12, 2025 at 04:38:31PM -0600, Bjorn Helgaas wrote:
> > > > 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
> > > 
> > > 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.

The part that seems confusing to me is that pci_save_state() is the
switch that turns off PCI core power management.  The state save part
is not obviously connected with the power management part, at least
from the function name.

But that paragraph goes on to say:

  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.

so the doc actually *does* mention the connection, and although
pci_prepare_to_sleep() and pci_set_power_state() sound sort of like
internal functions, I guess that makes sense because drivers doing
their own power management would be using things like that.

I do raise my eyebrows a bit at them though; there are only seven
drivers that use pci_prepare_to_sleep(), which makes me a little
suspicious -- what is so unique about those drivers?

A bunch of drivers use pci_set_power_state().  Many seem to be rolling
their own PM resets.  Several others use it in suspend/resume for
reasons that aren't obvious to me but are likely legit.

> > 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/

Thanks!

Bjorn

Re: [PATCH 2/2] treewide: Drop pci_save_state() after pci_restore_state()

[Bjorn Helgaas]

On Sun, Oct 12, 2025 at 03:25:02PM +0200, Lukas Wunner wrote:
> 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.

Heads-up network and scsi folks: this touches some of your drivers,
mostly in error recovery paths and a few suspend/resume paths.

This change has been in linux-next since Nov 17, and I hope to merge
it for v6.19.  Acks would be great if you have a chance.

Bjorn

> 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 <lukas@wunner.de>
> ---
> 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(-)
> 
> diff --git a/drivers/crypto/intel/qat/qat_common/adf_aer.c b/drivers/crypto/intel/qat/qat_common/adf_aer.c
> index 35679b2..9a5a4b3 100644
> --- a/drivers/crypto/intel/qat/qat_common/adf_aer.c
> +++ b/drivers/crypto/intel/qat/qat_common/adf_aer.c
> @@ -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;
> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> index 02f68b3..2273986 100644
> --- a/drivers/dma/ioat/init.c
> +++ b/drivers/dma/ioat/init.c
> @@ -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);
>  	}
>  
> diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
> index cb1011f..805daae 100644
> --- a/drivers/net/ethernet/broadcom/bnx2.c
> +++ b/drivers/net/ethernet/broadcom/bnx2.c
> @@ -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);
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> index f0f05d7..8e6eec8 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -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);
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 7f00ec7..ecc1220 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -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;
> diff --git a/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c b/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c
> index f92a355..3b1321c 100644
> --- a/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c
> +++ b/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c
> @@ -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);
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> index 392723e..1ce2091 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> @@ -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;
> diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c
> index 83cf75b..2eb1e3d 100644
> --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c
> +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_err.c
> @@ -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;
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 201322d..7589660 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -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);
>  
> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
> index ae5fe34..d75b8a5 100644
> --- a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
> @@ -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;
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 50be0a6..d8192aa 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -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);
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 86f5859..6c7dcca 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -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 */
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 85f9589..dbea372 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -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);
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 728d7ca..7aafa60b 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -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);
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 90d4e57..d65d691 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -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);
>  
> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
> index 03d2fc7..d1fbf37 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
> @@ -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;
>  }
>  
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> index df93625..08f7778 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> @@ -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) {
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> index a7a6b4d..0fa90ba 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> @@ -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,
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index 9d70b51..e4c542f 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -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)
> diff --git a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
> index e611ff7..7be30a8 100644
> --- a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
> +++ b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
> @@ -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
> diff --git a/drivers/net/ethernet/neterion/s2io.c b/drivers/net/ethernet/neterion/s2io.c
> index 5026b02..1e55ccb 100644
> --- a/drivers/net/ethernet/neterion/s2io.c
> +++ b/drivers/net/ethernet/neterion/s2io.c
> @@ -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;
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index d1b68c18..38a41cc 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -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;
>  }
>  
> diff --git a/drivers/scsi/bfa/bfad.c b/drivers/scsi/bfa/bfad.c
> index ff9adfc..bdfd065 100644
> --- a/drivers/scsi/bfa/bfad.c
> +++ b/drivers/scsi/bfa/bfad.c
> @@ -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));
> diff --git a/drivers/scsi/csiostor/csio_init.c b/drivers/scsi/csiostor/csio_init.c
> index 79c8daf..db0c217 100644
> --- a/drivers/scsi/csiostor/csio_init.c
> +++ b/drivers/scsi/csiostor/csio_init.c
> @@ -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.
> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
> index 4421488..9512368 100644
> --- a/drivers/scsi/ipr.c
> +++ b/drivers/scsi/ipr.c
> @@ -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)) {
> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index f206267..065eb91 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -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);
>  
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 5ffd945..9007533e 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -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
> diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
> index a761c0a..1f52379 100644
> --- a/drivers/scsi/qla4xxx/ql4_os.c
> +++ b/drivers/scsi/qla4xxx/ql4_os.c
> @@ -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) {
> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index 152f914..65bd370 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -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;
>  }
> diff --git a/drivers/tty/serial/jsm/jsm_driver.c b/drivers/tty/serial/jsm/jsm_driver.c
> index 417a5b6..8d21373 100644
> --- a/drivers/tty/serial/jsm/jsm_driver.c
> +++ b/drivers/tty/serial/jsm/jsm_driver.c
> @@ -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);
>  }
> -- 
> 2.51.0
>