[PATCH] PCI/PM: Ensure power-up succeeded before restoring MMIO state

[PATCH] PCI/PM: Ensure power-up succeeded before restoring MMIO state

[Brian Norris]

From: Brian Norris <briannorris@google.com>

As the comments in pci_pm_thaw_noirq() suggest, pci_restore_state() may
need to restore MSI-X state in MMIO space. This is only possible if we
reach D0; if we failed to power up, this might produce a fatal error
when touching memory space.

Check for errors (as the "verify" in "pci_pm_power_up_and_verify_state"
implies), and skip restoring if it fails.

This mitigates errors seen during resume_noirq, for example, when the
platform did not resume the link properly.

Cc: stable@vger.kernel.org
Signed-off-by: Brian Norris <briannorris@google.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

 drivers/pci/pci-driver.c | 12 +++++++++---
 drivers/pci/pci.c        | 13 +++++++++++--
 drivers/pci/pci.h        |  2 +-
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 302d61783f6c..d66d95bd0ca2 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -557,7 +557,13 @@ static void pci_pm_default_resume(struct pci_dev *pci_dev)
 
 static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
 {
-	pci_pm_power_up_and_verify_state(pci_dev);
+	/*
+	 * If we failed to reach D0, we'd better not touch MSI-X state in MMIO
+	 * space.
+	 */
+	if (pci_pm_power_up_and_verify_state(pci_dev))
+		return;
+
 	pci_restore_state(pci_dev);
 	pci_pme_restore(pci_dev);
 }
@@ -1101,8 +1107,8 @@ static int pci_pm_thaw_noirq(struct device *dev)
 	 * in case the driver's "freeze" callbacks put it into a low-power
 	 * state.
 	 */
-	pci_pm_power_up_and_verify_state(pci_dev);
-	pci_restore_state(pci_dev);
+	if (!pci_pm_power_up_and_verify_state(pci_dev))
+		pci_restore_state(pci_dev);
 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return 0;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e698278229f2..c75fec3b094f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3144,10 +3144,19 @@ void pci_d3cold_disable(struct pci_dev *dev)
 }
 EXPORT_SYMBOL_GPL(pci_d3cold_disable);
 
-void pci_pm_power_up_and_verify_state(struct pci_dev *pci_dev)
+int pci_pm_power_up_and_verify_state(struct pci_dev *pci_dev)
 {
-	pci_power_up(pci_dev);
+	int ret;
+
+	ret = pci_power_up(pci_dev);
 	pci_update_current_state(pci_dev, PCI_D0);
+
+	if (ret < 0 && pci_dev->current_state == PCI_D3cold) {
+		dev_err(&pci_dev->dev, "Failed to power up device: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
 }
 
 /**
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 1c48bc447f58..87ad201417d5 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -233,7 +233,7 @@ void pci_dev_adjust_pme(struct pci_dev *dev);
 void pci_dev_complete_resume(struct pci_dev *pci_dev);
 void pci_config_pm_runtime_get(struct pci_dev *dev);
 void pci_config_pm_runtime_put(struct pci_dev *dev);
-void pci_pm_power_up_and_verify_state(struct pci_dev *pci_dev);
+int pci_pm_power_up_and_verify_state(struct pci_dev *pci_dev);
 void pci_pm_init(struct pci_dev *dev);
 void pci_ea_init(struct pci_dev *dev);
 void pci_msi_init(struct pci_dev *dev);
-- 
2.51.0.rc1.193.gad69d77794-goog

Re: [PATCH] PCI/PM: Ensure power-up succeeded before restoring MMIO state

[Brian Norris]

On Thu, Aug 21, 2025 at 07:58:12AM -0700, Brian Norris wrote:
> From: Brian Norris <briannorris@google.com>
> 
> As the comments in pci_pm_thaw_noirq() suggest, pci_restore_state() may
> need to restore MSI-X state in MMIO space. This is only possible if we
> reach D0; if we failed to power up, this might produce a fatal error
> when touching memory space.
> 
> Check for errors (as the "verify" in "pci_pm_power_up_and_verify_state"
> implies), and skip restoring if it fails.
> 
> This mitigates errors seen during resume_noirq, for example, when the
> platform did not resume the link properly.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Brian Norris <briannorris@google.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Friendly ping on this one.

This bug causes quite a bit of problem for some Pixel devices, although
that's admittedly because of our own failure to resume properly in some
cases. (And we can recover later, if we don't crash here.)

I believe the patch is still a good addition for everyone, in case other
systems fall into similar error conditions.

Brian

Re: [PATCH] PCI/PM: Ensure power-up succeeded before restoring MMIO state

[Bjorn Helgaas]

[+cc Mario, Rafael]

On Thu, Aug 21, 2025 at 07:58:12AM -0700, Brian Norris wrote:
> From: Brian Norris <briannorris@google.com>
> 
> As the comments in pci_pm_thaw_noirq() suggest, pci_restore_state() may
> need to restore MSI-X state in MMIO space. This is only possible if we
> reach D0; if we failed to power up, this might produce a fatal error
> when touching memory space.
> 
> Check for errors (as the "verify" in "pci_pm_power_up_and_verify_state"
> implies), and skip restoring if it fails.
> 
> This mitigates errors seen during resume_noirq, for example, when the
> platform did not resume the link properly.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Brian Norris <briannorris@google.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> 
>  drivers/pci/pci-driver.c | 12 +++++++++---
>  drivers/pci/pci.c        | 13 +++++++++++--
>  drivers/pci/pci.h        |  2 +-
>  3 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 302d61783f6c..d66d95bd0ca2 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -557,7 +557,13 @@ static void pci_pm_default_resume(struct pci_dev *pci_dev)
>  
>  static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
>  {
> -	pci_pm_power_up_and_verify_state(pci_dev);
> +	/*
> +	 * If we failed to reach D0, we'd better not touch MSI-X state in MMIO
> +	 * space.
> +	 */
> +	if (pci_pm_power_up_and_verify_state(pci_dev))
> +		return;

The MSI-X comment here seems oddly specific.

On most platforms, config/mem/io accesses to a device not in D0 result
in an error being logged, writes being dropped, and reads returning ~0
data.

I don't know the details, but I assume the fatal error is a problem
specific to arm64.

If the device is not in D0, we can avoid the problem here, but it
seems like we're just leaving a landmine for somebody else to hit
later.  The driver will surely access the device after resume, won't
it?  Is it better to wait for a fatal error there?

Even if we avoid errors here, aren't we effectively claiming to have
restored the device state, which is now a lie?

Even on other platforms, if the writes that are supposed to restore
the state are dropped because the device isn't in D0, the result is
also not what we expect, and something is probably broken.

Bjorn

Re: [PATCH] PCI/PM: Ensure power-up succeeded before restoring MMIO state

[Brian Norris]

Hi Bjorn,

On Thu, Oct 23, 2025 at 12:25:47PM -0500, Bjorn Helgaas wrote:
> [+cc Mario, Rafael]
> 
> On Thu, Aug 21, 2025 at 07:58:12AM -0700, Brian Norris wrote:
> > From: Brian Norris <briannorris@google.com>
> > 
> > As the comments in pci_pm_thaw_noirq() suggest, pci_restore_state() may
> > need to restore MSI-X state in MMIO space. This is only possible if we
> > reach D0; if we failed to power up, this might produce a fatal error
> > when touching memory space.
> > 
> > Check for errors (as the "verify" in "pci_pm_power_up_and_verify_state"
> > implies), and skip restoring if it fails.
> > 
> > This mitigates errors seen during resume_noirq, for example, when the
> > platform did not resume the link properly.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Brian Norris <briannorris@google.com>
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> > 
> >  drivers/pci/pci-driver.c | 12 +++++++++---
> >  drivers/pci/pci.c        | 13 +++++++++++--
> >  drivers/pci/pci.h        |  2 +-
> >  3 files changed, 21 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 302d61783f6c..d66d95bd0ca2 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -557,7 +557,13 @@ static void pci_pm_default_resume(struct pci_dev *pci_dev)
> >  
> >  static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
> >  {
> > -	pci_pm_power_up_and_verify_state(pci_dev);
> > +	/*
> > +	 * If we failed to reach D0, we'd better not touch MSI-X state in MMIO
> > +	 * space.
> > +	 */
> > +	if (pci_pm_power_up_and_verify_state(pci_dev))
> > +		return;
> 
> The MSI-X comment here seems oddly specific.

It's just as "oddly specific" as the existing comment in
pci_pm_thaw_noirq(), as mentioned in the commit message :)

The key point for MSI-X is that unlike the rest of pci_restore_state(),
it requires touching memory space. While config registers are OK to
touch in D3, memory space is not.

> On most platforms, config/mem/io accesses to a device not in D0 result
> in an error being logged, writes being dropped, and reads returning ~0
> data.

On my arm64 / pcie-designware-based platforms, that is mostly similar,
but there are some cases that are different. See below:

> I don't know the details, but I assume the fatal error is a problem
> specific to arm64.

Maybe. See my response here also:

  Re: [PATCH] PCI/sysfs: Ensure devices are powered for config reads
  https://lore.kernel.org/all/aNMoMY17CTR2_jQz@google.com/

In particular, when resuming the system in a case where the link was in
L2 and failed to resume properly, the PCIe controller may not be alive
enough even to emit completion timeouts. So it might hit case (a):

  "PCIe HW is not powered [...] and this tends to be SError, and a
  crash."

Memory space is unique, because while config accesses can be
intercepted/avoided by driver software, memory accesses cannot.

> If the device is not in D0, we can avoid the problem here, but it
> seems like we're just leaving a landmine for somebody else to hit
> later.  The driver will surely access the device after resume, won't
> it?

It's a possible landmine, yes. Although in my case, the link can go
through error recovery and restore itself later in the resume process.

> Is it better to wait for a fatal error there?
> 
> Even if we avoid errors here, aren't we effectively claiming to have
> restored the device state, which is now a lie?

I'm not sure we claim that. The device will stay in PCI_D3cold, and
pdev->state_saved will remain true.

But yes, it's a tricky situation to decide what to do next. My basic
assertion is that it's not OK to continue to restore state though.

Alternatives: pci_dev_set_disconnected()? pcie_do_recovery() /
pci_channel_io_frozen?

> Even on other platforms, if the writes that are supposed to restore
> the state are dropped because the device isn't in D0, the result is
> also not what we expect, and something is probably broken.

Sure. IMO, that's even more reason not to run pci_restore_state(),
because that will erroneously drop the state, and we'll have zero chance
of restoring it later.

Brian