Re: [PATCH v2 2/3] PCI/PM: Do not save the config space of an inaccessible device

[Francisco Beltrán Millalén]

Hi Bjorn,

Thanks, Farhan, for the pointer to the earlier discussion.  I hadn't
seen it.

On Fri, Oct 09, 2026 at 10:08:15AM -0700, Farhan Ali wrote:
> When I had initially looked into this, I had proposed adding the
> accessibility check in pci_save_state() as well [1]. Though Lukas had
> some valid concerns around the additional overhead from adding it in
> pci_save_state(). But maybe its worth re-visiting again?

After reading that thread I went through the callers of
pci_save_state().  When a device is already inaccessible while its
driver's suspend callback runs, for example because it dropped off the
bus and hasn't been removed yet, returning -EIO from pci_save_state()
changes what some drivers do:

- Some return the error from their suspend callback, so system suspend
  is aborted: stmmac_pci_plat_suspend() (used by stmmac_pci,
  dwmac-loongson and dwmac-motorcomm), intel_eth_pci_suspend(),
  atl1e_suspend(), atl2_suspend() and amd_mp2_pci_suspend().  Today
  pci_save_state() returns 0 in that case, and these drivers go on.

- Many more ignore the return value and then put the device into D3hot,
  for example ata_pci_device_do_suspend(), which most libata PCI drivers
  use.  On an inaccessible device that marks it D3cold, and since
  state_saved is now false, pci_legacy_suspend() or pci_pm_suspend()
  warns once that the state was not saved.  This is the same warning
  that patch 1 avoids for the USB HCD in suspend_noirq.

I found all of this by reading the code; I can't test these drivers.
I think this is what Lukas meant about callers that don't need the
check.

The underlying problem is that pci_save_state() doesn't check that the
device answered: for an inaccessible device it fills the saved header
with all ones, marks the state saved and returns 0, and
pci_restore_state() later writes those values back.  What I have
prepared for v3 fixes it in pci_save_state() itself, with the changes
you suggested: it checks once that the device is accessible and returns
-EIO if it isn't, and pci_dev_save_and_disable() uses the return value.
v2 2/3 returns -EIO in the same way.  The drivers above are the side
effect of that: they now see a failure where they never saw one before.

I found this only recently, so I'd rather not send v3, and I'd ask you
not to apply v2 2/3 as it is, until you've had a chance to look at
this.  Patch 3 doesn't depend on it.  How would you like these drivers
to be handled?

Thanks again,
Francisco

[1] https://lore.kernel.org/all/aOQX6ZTMvekd6gWy@xxxxxxxxx/