[PATCH] PCI/PM: Skip the suspend_noirq config save if runtime-suspended

[PATCH] PCI/PM: Skip the suspend_noirq config save if runtime-suspended

[Navon John Lukose]

pci_pm_suspend_noirq() saves config space unconditionally when the driver
of the device has no PM callbacks, which includes unbound devices.  If
such a device is runtime-suspended, the bridge above it may be in a
low-power state with the link down.  Depending on the platform, the
config reads then either hang the CPU or return all ones, which overwrite
the snapshot taken at runtime suspend and are written back to the device
on resume.

Skip the save if the device is runtime-suspended, as pci_pm_freeze()
does, since pci_pm_runtime_suspend() has already saved the config space.
Use pm_runtime_status_suspended(), because runtime PM is disabled by the
noirq phase and pm_runtime_suspended() would always be false.

Fixes: 931ff68a5a53 ("PCI PM: Restore config spaces of all devices during early resume")
Cc: stable@vger.kernel.org # v6.19+
Signed-off-by: Navon John Lukose <navonjohnlukose@gmail.com>
---
Found on a Lenovo Yoga 83KF (Arrow Lake-H).  A driverless O2 Micro SD
reader at 57:00.0 with power/control=auto lets its root port 00:1c.0
runtime-suspend to D3hot.  The port then swallows the ECAM reads in
pci_save_state(), and the forward-progress watchdog raises a fatal machine
check at the ECAM load in pci_mmcfg_read().

The same port returns all ones for CF8/CFC reads.  A local quirk hid the
reader's extended config space so the save did only those reads, and it
was tried once on each kernel.  The unpatched kernel still died, and the
patched one resumed with no "restore config" writes for the reader.

!pci_dev->state_saved would only work for one cycle per boot, because
pci_restore_state() clears it and pci_pm_runtime_suspend() does not run
again for a device left in RPM_SUSPENDED.

Stable starts at v6.19 because the fix relies on a2f1e22390ac2 ("PCI/ERR:
Ensure error recoverability at all times").  Older trees still return
early from pci_restore_state() when state_saved is false, so a backport
there also needs "pci_dev->state_saved = true;" on the skip path.

Tested on 7.2.5 with the root port in D3hot, three suspend/resume cycles
in one boot.  The unpatched kernel dies on the first.  W=1 and sparse
clean.

 drivers/pci/pci-driver.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index e16aa59dd..bdc8bad57 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -915,7 +915,15 @@ static int pci_pm_suspend_noirq(struct device *dev)
 		return pci_legacy_suspend_late(dev);
 
 	if (!pm) {
-		pci_save_state(pci_dev);
+		/*
+		 * The bridge above a runtime-suspended device may be in a
+		 * low-power state with the link down, which makes the device's
+		 * config space inaccessible.  pci_pm_runtime_suspend() has
+		 * saved it already.
+		 */
+		if (!pm_runtime_status_suspended(dev))
+			pci_save_state(pci_dev);
+
 		goto set_unknown;
 	}
 
-- 
2.55.0

Re: [PATCH] PCI/PM: Skip the suspend_noirq config save if runtime-suspended

[Lukas Wunner]

On Wed, Sep 23, 2026 at 07:55:11AM +0530, Navon John Lukose wrote:
> pci_pm_suspend_noirq() saves config space unconditionally when the driver
> of the device has no PM callbacks, which includes unbound devices.  If
> such a device is runtime-suspended, the bridge above it may be in a
> low-power state with the link down.  Depending on the platform, the
> config reads then either hang the CPU or return all ones, which overwrite
> the snapshot taken at runtime suspend and are written back to the device
> on resume.
> 
> Skip the save if the device is runtime-suspended, as pci_pm_freeze()
> does, since pci_pm_runtime_suspend() has already saved the config space.
> Use pm_runtime_status_suspended(), because runtime PM is disabled by the
> noirq phase and pm_runtime_suspended() would always be false.
> 
> Fixes: 931ff68a5a53 ("PCI PM: Restore config spaces of all devices during early resume")
> Cc: stable@vger.kernel.org # v6.19+
> Signed-off-by: Navon John Lukose <navonjohnlukose@gmail.com>
> ---
> Found on a Lenovo Yoga 83KF (Arrow Lake-H).  A driverless O2 Micro SD
> reader at 57:00.0 with power/control=auto lets its root port 00:1c.0
> runtime-suspend to D3hot.  The port then swallows the ECAM reads in
> pci_save_state(), and the forward-progress watchdog raises a fatal machine
> check at the ECAM load in pci_mmcfg_read().

Hm, pci_pm_suspend_noirq() should bail out earlier because of the
dev_pm_skip_suspend() check.  That check resolves to:

  dev_pm_smart_suspend(dev) && pm_runtime_status_suspended(dev)

The code comment in device_prepare_smart_suspend() explains that
smart_suspend is true for devices without PM callbacks.  But only
if the parent has smart suspend enabled as well.  I guess that's
the sticking point?  That the port above the card reader doesn't
have smart suspend enabled?

Thanks,

Lukas

Re: [PATCH] PCI/PM: Skip the suspend_noirq config save if runtime-suspended

[Navon John Lukose]

On Wed, Sep 23, 2026 at 12:27:27PM +0200, Lukas Wunner wrote:
> Hm, pci_pm_suspend_noirq() should bail out earlier because of the
> dev_pm_skip_suspend() check.  That check resolves to:
>
>   dev_pm_smart_suspend(dev) && pm_runtime_status_suspended(dev)
>
> The code comment in device_prepare_smart_suspend() explains that
> smart_suspend is true for devices without PM callbacks.  But only
> if the parent has smart suspend enabled as well.  I guess that's
> the sticking point?  That the port above the card reader doesn't
> have smart suspend enabled?

It is that check, but it fails on the card reader itself, before the
parent is looked at.  no_pm_callbacks is only set when the bus has no
PM ops either, and pci_bus_type always has pci_dev_pm_ops, so no PCI
device ever counts as having no callbacks.  With no driver to set
DPM_FLAG_SMART_SUSPEND, the reader never gets smart_suspend.

pcieport does set the flag for the port, and on this machine the port
stays in D3hot through suspend.

Thanks,
Navon

Re: [PATCH] PCI/PM: Skip the suspend_noirq config save if runtime-suspended

[Navon John Lukose]

Hi Bjorn,

Gentle ping. The patch still applies to pci/pm on top of Mario's
pci_pm_suspend_noirq_common() split, since the !pm branch it touches is
unchanged there.

Thanks,
Navon

Re: [PATCH] PCI/PM: Skip the suspend_noirq config save if runtime-suspended

[Bjorn Helgaas]

[+cc Francisco, tangentially related to your series at
https://lore.kernel.org/linux-pci/20260930141914.6678-1-fbeltranmillalen@gmail.com/t/#u]

On Wed, Sep 23, 2026 at 07:55:11AM +0530, Navon John Lukose wrote:
> pci_pm_suspend_noirq() saves config space unconditionally when the driver
> of the device has no PM callbacks, which includes unbound devices.  If
> such a device is runtime-suspended, the bridge above it may be in a
> low-power state with the link down.  Depending on the platform, the
> config reads then either hang the CPU or return all ones, which overwrite
> the snapshot taken at runtime suspend and are written back to the device
> on resume.

Right, bridges in non-D0 states don't forward requests downstream, so
they end with Unsupported Request (PCIe r7.0, sec 5.3.1) or master
abort (PCI PM r1.2, sec 6).  It's up to the host bridge to choose how
to report those.

> Skip the save if the device is runtime-suspended, as pci_pm_freeze()
> does, since pci_pm_runtime_suspend() has already saved the config space.

pci_pm_freeze() doesn't call pci_save_state() itself, so I guess it's
the combination of pci_pm_freeze() and pci_pm_freeze_noirq(), where if
not runtime-suspended, pci_pm_freeze() clears dev->state_saved so
pci_pm_freeze_noirq() will call pci_save_state()?

> Use pm_runtime_status_suspended(), because runtime PM is disabled by the
> noirq phase and pm_runtime_suspended() would always be false.
> 
> Fixes: 931ff68a5a53 ("PCI PM: Restore config spaces of all devices during early resume")
> Cc: stable@vger.kernel.org # v6.19+
> Signed-off-by: Navon John Lukose <navonjohnlukose@gmail.com>

Seems ok to me, but would be good to get Rafael's ack.

> ---
> Found on a Lenovo Yoga 83KF (Arrow Lake-H).  A driverless O2 Micro SD
> reader at 57:00.0 with power/control=auto lets its root port 00:1c.0
> runtime-suspend to D3hot.  The port then swallows the ECAM reads in
> pci_save_state(), and the forward-progress watchdog raises a fatal machine
> check at the ECAM load in pci_mmcfg_read().
> 
> The same port returns all ones for CF8/CFC reads.  A local quirk hid the
> reader's extended config space so the save did only those reads, and it
> was tried once on each kernel.  The unpatched kernel still died, and the
> patched one resumed with no "restore config" writes for the reader.
> 
> !pci_dev->state_saved would only work for one cycle per boot, because
> pci_restore_state() clears it and pci_pm_runtime_suspend() does not run
> again for a device left in RPM_SUSPENDED.
> 
> Stable starts at v6.19 because the fix relies on a2f1e22390ac2 ("PCI/ERR:
> Ensure error recoverability at all times").  Older trees still return
> early from pci_restore_state() when state_saved is false, so a backport
> there also needs "pci_dev->state_saved = true;" on the skip path.
> 
> Tested on 7.2.5 with the root port in D3hot, three suspend/resume cycles
> in one boot.  The unpatched kernel dies on the first.  W=1 and sparse
> clean.
> 
>  drivers/pci/pci-driver.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index e16aa59dd..bdc8bad57 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -915,7 +915,15 @@ static int pci_pm_suspend_noirq(struct device *dev)
>  		return pci_legacy_suspend_late(dev);
>  
>  	if (!pm) {
> -		pci_save_state(pci_dev);
> +		/*
> +		 * The bridge above a runtime-suspended device may be in a
> +		 * low-power state with the link down, which makes the device's
> +		 * config space inaccessible.  pci_pm_runtime_suspend() has
> +		 * saved it already.
> +		 */
> +		if (!pm_runtime_status_suspended(dev))
> +			pci_save_state(pci_dev);
> +
>  		goto set_unknown;
>  	}
>  
> -- 
> 2.55.0
> 

Re: [PATCH] PCI/PM: Skip the suspend_noirq config save if runtime-suspended

[Navon John Lukose]

On Fri, Oct 16, 2026 at 04:41:23PM -0500, Bjorn Helgaas wrote:
> pci_pm_freeze() doesn't call pci_save_state() itself, so I guess it's
> the combination of pci_pm_freeze() and pci_pm_freeze_noirq(), where if
> not runtime-suspended, pci_pm_freeze() clears dev->state_saved so
> pci_pm_freeze_noirq() will call pci_save_state()?

Yes, that's what I meant.

Thanks,
Navon