[PATCH v2 1/3] PCI/PM: Reinstate clearing state_saved in legacy and !pm codepaths

[Bjorn Helgaas]

On Wed, Nov 19, 2025 at 09:50:01AM +0100, Lukas Wunner wrote:
> When a PCI device is suspended, it is normally the PCI core's job to save
> Config Space and put the device into a low power state.  However drivers
> are allowed to assume these responsibilities.  When they do, the PCI core
> can tell by looking at the state_saved flag in struct pci_dev:  The flag
> is cleared before commencing the suspend sequence and it is set when
> pci_save_state() is called.  If the PCI core finds the flag set late in
> the suspend sequence, it refrains from calling pci_save_state() itself.

This has been applied already, no issue there.  I have a few questions
to help come up with a short higher-level merge commit log.

> But there are two corner cases where the PCI core neglects to clear the
> flag before commencing the suspend sequence:
> 
> * If a driver has legacy PCI PM callbacks, pci_legacy_suspend() neglects
>   to clear the flag.  The (stale) flag is subsequently queried by
>   pci_legacy_suspend() itself and pci_legacy_suspend_late().
> 
> * If a device has no driver or its driver has no PCI PM callbacks,
>   pci_pm_freeze() neglects to clear the flag.  The (stale) flag is
>   subsequently queried by pci_pm_freeze_noirq().
> 
> The flag may be set prior to suspend if the device went through error
> recovery:  Drivers commonly invoke pci_restore_state() + pci_save_state()
> to restore Config Space after reset.

I guess the only point of pci_save_state() in this case is to set
state_saved again so a future pci_restore_state() will work, right?

The actual state being *saved* is pointless, assuming pci_save_state()
saves exactly the same data that pci_restore_state() restored.

And these are the pci_save_state() calls you removed with "treewide:
Drop pci_save_state() after pci_restore_state()".  Too bad we have to
document the behavior we're about to change, but that's what we need
to do.  It's just a little clutter to keep in mind for this release.

> The flag may also be set if drivers call pci_save_state() on probe to
> allow for recovery from subsequent errors.
> 
> The result is that pci_legacy_suspend_late() and pci_pm_freeze_noirq()
> don't call pci_save_state() and so the state that will be restored on
> resume is the one recorded on last error recovery or on probe, not the one
> that the device had on suspend.  If the two states happen to be identical,
> there's no problem.

So IIUC the effect is that after this change and the "treewide"
change,

  - If the driver uses legacy PM, the state restored on resume will be
    the state from suspend instead of the state on probe.

  - For devices with no driver or a driver without PM, if the device
    has already been runtime-suspended, we avoid a pointless
    pci_save_state(), so it's an optimization and not logically
    related to the legacy PM case.

Right?

I'm thinking of something like this for the merge commit and eventual
pull request; please correct me if this isn't right:

  Restore the suspend config state, not the state from probe or error
  recovery, for drivers using legacy PCI suspend.

  Avoid saving config state again for devices without driver PM if
  their state was already saved by runtime suspend.

> Reinstate clearing the flag in pci_legacy_suspend() and pci_pm_freeze().
> The two functions used to do that until commit 4b77b0a2ba27 ("PCI: Clear
> saved_state after the state has been restored") deemed it unnecessary
> because it assumed that it's sufficient to clear the flag on resume in
> pci_restore_state().  The commit seemingly did not take into account that
> pci_save_state() and pci_restore_state() are not only used by power
> management code, but also for error recovery.
> 
> Devices without driver or whose driver has no PCI PM callbacks may be in
> runtime suspend when pci_pm_freeze() is called.  Their state has already
> been saved, so don't clear the flag to skip a pointless pci_save_state()
> in pci_pm_freeze_noirq().
> 
> None of the drivers with legacy PCI PM callbacks seem to use runtime PM,
> so clear the flag unconditionally in their case.
> 
> Fixes: 4b77b0a2ba27 ("PCI: Clear saved_state after the state has been restored")
> Signed-off-by: Lukas Wunner <lukas at wunner.de>
> Cc: stable at vger.kernel.org # v2.6.32+
> ---
>  drivers/pci/pci-driver.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 302d61783f6c..327b21c48614 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -629,6 +629,8 @@ static int pci_legacy_suspend(struct device *dev, pm_message_t state)
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	struct pci_driver *drv = pci_dev->driver;
>  
> +	pci_dev->state_saved = false;
> +
>  	if (drv && drv->suspend) {
>  		pci_power_t prev = pci_dev->current_state;
>  		int error;
> @@ -1036,6 +1038,8 @@ static int pci_pm_freeze(struct device *dev)
>  
>  	if (!pm) {
>  		pci_pm_default_suspend(pci_dev);
> +		if (!pm_runtime_suspended(dev))
> +			pci_dev->state_saved = false;
>  		return 0;
>  	}
>  
> -- 
> 2.51.0
>