Re: [PATCH] PCI: Drop unnecessary retries when restoring BARs

Re: [PATCH] PCI: Drop unnecessary retries when restoring BARs

[Lukas Wunner]

On Sun, May 03, 2026 at 01:51:08PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [High] Removing the read-back and retry loop for BAR restoration in
> `pci_restore_state()` introduces a risk of silent regressions for
> hardware resuming from non-FLR resets (such as D3hot to D0 transitions
> or custom driver resets). The commit incorrectly assumes the 60s delay
> from `pci_dev_wait()` covers all usages, but standard PM resume paths
> only delay for 10ms (`PCI_PM_D3_WAIT`) before calling `pci_restore_state()`.
> Historically, hardware that needed slightly longer to accept
> configuration writes relied on the 10x 1ms retry loop to successfully
> restore BARs. By removing both the retry and the read-back verification,
> BAR writes to slow devices will be silently dropped, leaving hardware
> unconfigured and causing MMIO accesses to result in IOMMU faults or
> kernel crashes.

Hallucination alert:

PCI_PM_D3_WAIT does not exist, it was renamed to PCI_PM_D3HOT_WAIT
six years ago by commit 3789af9a13e5, which went into v5.10.

The macro is used in:

pci_pm_resume_noirq()
  pci_pm_default_resume_early()
    pci_pm_power_up_and_verify_state()
      pci_power_up()
        pci_dev_d3_sleep()

However before pci_power_up() calls pci_dev_d3_sleep(), it reads
the PMCSR register and errors out if config space is inaccessible.

Hence when pci_restore_state() is invoked a bit later, config space
can be assumed to be accessible.

sashiko reviews are a mixed bag, I think this one doesn't even reach
junior developer level. :(

Thanks,

Lukas

Re: [PATCH] PCI: Drop unnecessary retries when restoring BARs

[Bjorn Helgaas]

[+cc Mario]

On Mon, May 04, 2026 at 09:49:36AM +0200, Lukas Wunner wrote:
> On Sun, May 03, 2026 at 01:51:08PM +0000, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential
> > issue(s) to consider:
> > - [High] Removing the read-back and retry loop for BAR restoration in
> > `pci_restore_state()` introduces a risk of silent regressions for
> > hardware resuming from non-FLR resets (such as D3hot to D0 transitions
> > or custom driver resets). The commit incorrectly assumes the 60s delay
> > from `pci_dev_wait()` covers all usages, but standard PM resume paths
> > only delay for 10ms (`PCI_PM_D3_WAIT`) before calling `pci_restore_state()`.
> > Historically, hardware that needed slightly longer to accept
> > configuration writes relied on the 10x 1ms retry loop to successfully
> > restore BARs. By removing both the retry and the read-back verification,
> > BAR writes to slow devices will be silently dropped, leaving hardware
> > unconfigured and causing MMIO accesses to result in IOMMU faults or
> > kernel crashes.
> 
> Hallucination alert:
> 
> PCI_PM_D3_WAIT does not exist, it was renamed to PCI_PM_D3HOT_WAIT
> six years ago by commit 3789af9a13e5, which went into v5.10.
> 
> The macro is used in:
> 
> pci_pm_resume_noirq()
>   pci_pm_default_resume_early()
>     pci_pm_power_up_and_verify_state()
>       pci_power_up()
>         pci_dev_d3_sleep()
> 
> However before pci_power_up() calls pci_dev_d3_sleep(), it reads
> the PMCSR register and errors out if config space is inaccessible.
> 
> Hence when pci_restore_state() is invoked a bit later, config space
> can be assumed to be accessible.

I don't quite follow this.  In this path, pci_power_up() changes a
device from some low-power state to D0.  If the device was in D3hot or
D3cold, we must delay at least 10ms before any access to it (PCIe
r7.0, sec 5.9).  pci_power_up() doesn't do any delay before the PMCSR
read.  That part seems like a pre-existing issue even before this
patch.

If the PMCSR read returns PCI_POSSIBLE_ERROR(), pci_power_up() does
complain "Unable to change power state ... to D0" and return -EIO, but
pci_pm_power_up_and_verify_state() doesn't look at it, and
pci_pm_default_resume_early() continues on to pci_restore_state(), so
it looks to me like we could try to restore state to an inaccessible
device.

We do call pci_dev_wait() in pci_pm_reset(), which does a D3hot -> D0
transition; shouldn't we do the same in pci_power_up()?

Re: [PATCH] PCI: Drop unnecessary retries when restoring BARs

[Rafael J. Wysocki]

On Mon, May 4, 2026 at 7:09 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Mario]
>
> On Mon, May 04, 2026 at 09:49:36AM +0200, Lukas Wunner wrote:
> > On Sun, May 03, 2026 at 01:51:08PM +0000, sashiko-bot@kernel.org wrote:
> > > Thank you for your contribution! Sashiko AI review found 1 potential
> > > issue(s) to consider:
> > > - [High] Removing the read-back and retry loop for BAR restoration in
> > > `pci_restore_state()` introduces a risk of silent regressions for
> > > hardware resuming from non-FLR resets (such as D3hot to D0 transitions
> > > or custom driver resets). The commit incorrectly assumes the 60s delay
> > > from `pci_dev_wait()` covers all usages, but standard PM resume paths
> > > only delay for 10ms (`PCI_PM_D3_WAIT`) before calling `pci_restore_state()`.
> > > Historically, hardware that needed slightly longer to accept
> > > configuration writes relied on the 10x 1ms retry loop to successfully
> > > restore BARs. By removing both the retry and the read-back verification,
> > > BAR writes to slow devices will be silently dropped, leaving hardware
> > > unconfigured and causing MMIO accesses to result in IOMMU faults or
> > > kernel crashes.
> >
> > Hallucination alert:
> >
> > PCI_PM_D3_WAIT does not exist, it was renamed to PCI_PM_D3HOT_WAIT
> > six years ago by commit 3789af9a13e5, which went into v5.10.
> >
> > The macro is used in:
> >
> > pci_pm_resume_noirq()
> >   pci_pm_default_resume_early()
> >     pci_pm_power_up_and_verify_state()
> >       pci_power_up()
> >         pci_dev_d3_sleep()
> >
> > However before pci_power_up() calls pci_dev_d3_sleep(), it reads
> > the PMCSR register and errors out if config space is inaccessible.
> >
> > Hence when pci_restore_state() is invoked a bit later, config space
> > can be assumed to be accessible.
>
> I don't quite follow this.  In this path, pci_power_up() changes a
> device from some low-power state to D0.  If the device was in D3hot or
> D3cold, we must delay at least 10ms before any access to it (PCIe
> r7.0, sec 5.9).

D3hot and D3cold are different in this respect, so using "or" here is
inaccurate and confusing.

Accessing the config space of a device in D3hot is entirely correct
and doesn't require any delay.

Accessing the config space of a device in D3cold is questionable
because the device may not be accessible, but then the host bridge
should just fail the access.

The 10 ms delay is after attempting to program the device into D0 from
D3hot (or the other way around) and it is observed as required.

> pci_power_up() doesn't do any delay before the PMCSR read.

It assumes that platform_pci_set_power_state() has run and either it
has succeeded or the config space is not accessible which is when
PCI_POSSIBLE_ERROR() will trigger.  Unfortunately, there is no
canonical way to verify that power has been restored to the device
other than attempting to access it.

> That part seems like a pre-existing issue even before this patch.

I beg to differ.

> If the PMCSR read returns PCI_POSSIBLE_ERROR(), pci_power_up() does
> complain "Unable to change power state ... to D0" and return -EIO, but
> pci_pm_power_up_and_verify_state() doesn't look at it,

In fact, pci_update_current_state() changes the power state to D3cold
if the config space is not accessible.

> and pci_pm_default_resume_early() continues on to pci_restore_state(), so
> it looks to me like we could try to restore state to an inaccessible
> device.

So the pci_restore_state() call could be avoided if the power state of
the device was D3cold, but then the question is how much of a problem
that is in practice.

> We do call pci_dev_wait() in pci_pm_reset(), which does a D3hot -> D0
> transition; shouldn't we do the same in pci_power_up()?

It would be good to make them consistent, but maybe the other way around?

Re: [PATCH] PCI: Drop unnecessary retries when restoring BARs

[Bjorn Helgaas]

On Mon, May 04, 2026 at 09:31:51PM +0200, Rafael J. Wysocki wrote:
> On Mon, May 4, 2026 at 7:09 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, May 04, 2026 at 09:49:36AM +0200, Lukas Wunner wrote:
> > > On Sun, May 03, 2026 at 01:51:08PM +0000, sashiko-bot@kernel.org wrote:
> > > > Thank you for your contribution! Sashiko AI review found 1 potential
> > > > issue(s) to consider:
> > > > - [High] Removing the read-back and retry loop for BAR restoration in
> > > > `pci_restore_state()` introduces a risk of silent regressions for
> > > > hardware resuming from non-FLR resets (such as D3hot to D0 transitions
> > > > or custom driver resets). The commit incorrectly assumes the 60s delay
> > > > from `pci_dev_wait()` covers all usages, but standard PM resume paths
> > > > only delay for 10ms (`PCI_PM_D3_WAIT`) before calling `pci_restore_state()`.
> > > > Historically, hardware that needed slightly longer to accept
> > > > configuration writes relied on the 10x 1ms retry loop to successfully
> > > > restore BARs. By removing both the retry and the read-back verification,
> > > > BAR writes to slow devices will be silently dropped, leaving hardware
> > > > unconfigured and causing MMIO accesses to result in IOMMU faults or
> > > > kernel crashes.
> > >
> > > Hallucination alert:
> > >
> > > PCI_PM_D3_WAIT does not exist, it was renamed to PCI_PM_D3HOT_WAIT
> > > six years ago by commit 3789af9a13e5, which went into v5.10.
> > >
> > > The macro is used in:
> > >
> > > pci_pm_resume_noirq()
> > >   pci_pm_default_resume_early()
> > >     pci_pm_power_up_and_verify_state()
> > >       pci_power_up()
> > >         pci_dev_d3_sleep()
> > >
> > > However before pci_power_up() calls pci_dev_d3_sleep(), it reads
> > > the PMCSR register and errors out if config space is inaccessible.
> > >
> > > Hence when pci_restore_state() is invoked a bit later, config space
> > > can be assumed to be accessible.
> >
> > I don't quite follow this.  In this path, pci_power_up() changes a
> > device from some low-power state to D0.  If the device was in D3hot or
> > D3cold, we must delay at least 10ms before any access to it (PCIe
> > r7.0, sec 5.9).
> 
> D3hot and D3cold are different in this respect, so using "or" here is
> inaccurate and confusing.
> 
> Accessing the config space of a device in D3hot is entirely correct
> and doesn't require any delay.
> 
> Accessing the config space of a device in D3cold is questionable
> because the device may not be accessible, but then the host bridge
> should just fail the access.
> 
> The 10 ms delay is after attempting to program the device into D0 from
> D3hot (or the other way around) and it is observed as required.

Thanks for taking a look at this!

If pci_power_up() is doing D3cold -> D0, main power is initially off,
so platform_pci_set_power_state(PCI_D0) would turn on main power,
which is a Fundamental Reset leaving the device in D0uninitialized.
In general the device needs at least 100 ms after that reset before
any access, e.g., before the PMCSR read.

If pci_power_up() is doing D3hot -> D0, the device already has main
power, so I suppose platform_pci_set_power_state(PCI_D0) doesn't do
anything.  The device remains in D3hot, the PMCSR read should be fine,
and the PMCSR write to transition to D0 is immediately followed by the
10 ms delay.  When No_Soft_Reset == 1, this should be enough.

But when No_Soft_Reset == 0, the transition is to D0uninitialized,
which sec 2.3.1 includes as a reset condition after which the device
is permitted to return RRS.

So to me it looks like we need these delays:

  - For D3cold -> D0, msleep(100) and pci_dev_wait() between
    platform_pci_set_power_state(PCI_D0) and the PMCSR read, similar
    to what pcie_flr() does.

    Maybe platform_pci_set_power_state() takes care of some or all of
    this internally?

  - For D3hot -> D0 with No_Soft_Reset == 0, pci_dev_wait() after the
    PMCSR write to transition to D0.

    abbcf0e2a99d ("PCI: Wait for device to become ready after a power
    management reset") added the similar wait in pci_pm_reset().

> > pci_power_up() doesn't do any delay before the PMCSR read.
> 
> It assumes that platform_pci_set_power_state() has run and either it
> has succeeded or the config space is not accessible which is when
> PCI_POSSIBLE_ERROR() will trigger.  Unfortunately, there is no
> canonical way to verify that power has been restored to the device
> other than attempting to access it.
> 
> > That part seems like a pre-existing issue even before this patch.
> 
> I beg to differ.
> 
> > If the PMCSR read returns PCI_POSSIBLE_ERROR(), pci_power_up() does
> > complain "Unable to change power state ... to D0" and return -EIO, but
> > pci_pm_power_up_and_verify_state() doesn't look at it,
> 
> In fact, pci_update_current_state() changes the power state to D3cold
> if the config space is not accessible.
> 
> > and pci_pm_default_resume_early() continues on to pci_restore_state(), so
> > it looks to me like we could try to restore state to an inaccessible
> > device.
> 
> So the pci_restore_state() call could be avoided if the power state of
> the device was D3cold, but then the question is how much of a problem
> that is in practice.
> 
> > We do call pci_dev_wait() in pci_pm_reset(), which does a D3hot -> D0
> > transition; shouldn't we do the same in pci_power_up()?
> 
> It would be good to make them consistent, but maybe the other way around?

Re: [PATCH] PCI: Drop unnecessary retries when restoring BARs

[Rafael J. Wysocki]

On Mon, May 4, 2026 at 11:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Mon, May 04, 2026 at 09:31:51PM +0200, Rafael J. Wysocki wrote:
> > On Mon, May 4, 2026 at 7:09 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Mon, May 04, 2026 at 09:49:36AM +0200, Lukas Wunner wrote:
> > > > On Sun, May 03, 2026 at 01:51:08PM +0000, sashiko-bot@kernel.org wrote:
> > > > > Thank you for your contribution! Sashiko AI review found 1 potential
> > > > > issue(s) to consider:
> > > > > - [High] Removing the read-back and retry loop for BAR restoration in
> > > > > `pci_restore_state()` introduces a risk of silent regressions for
> > > > > hardware resuming from non-FLR resets (such as D3hot to D0 transitions
> > > > > or custom driver resets). The commit incorrectly assumes the 60s delay
> > > > > from `pci_dev_wait()` covers all usages, but standard PM resume paths
> > > > > only delay for 10ms (`PCI_PM_D3_WAIT`) before calling `pci_restore_state()`.
> > > > > Historically, hardware that needed slightly longer to accept
> > > > > configuration writes relied on the 10x 1ms retry loop to successfully
> > > > > restore BARs. By removing both the retry and the read-back verification,
> > > > > BAR writes to slow devices will be silently dropped, leaving hardware
> > > > > unconfigured and causing MMIO accesses to result in IOMMU faults or
> > > > > kernel crashes.
> > > >
> > > > Hallucination alert:
> > > >
> > > > PCI_PM_D3_WAIT does not exist, it was renamed to PCI_PM_D3HOT_WAIT
> > > > six years ago by commit 3789af9a13e5, which went into v5.10.
> > > >
> > > > The macro is used in:
> > > >
> > > > pci_pm_resume_noirq()
> > > >   pci_pm_default_resume_early()
> > > >     pci_pm_power_up_and_verify_state()
> > > >       pci_power_up()
> > > >         pci_dev_d3_sleep()
> > > >
> > > > However before pci_power_up() calls pci_dev_d3_sleep(), it reads
> > > > the PMCSR register and errors out if config space is inaccessible.
> > > >
> > > > Hence when pci_restore_state() is invoked a bit later, config space
> > > > can be assumed to be accessible.
> > >
> > > I don't quite follow this.  In this path, pci_power_up() changes a
> > > device from some low-power state to D0.  If the device was in D3hot or
> > > D3cold, we must delay at least 10ms before any access to it (PCIe
> > > r7.0, sec 5.9).
> >
> > D3hot and D3cold are different in this respect, so using "or" here is
> > inaccurate and confusing.
> >
> > Accessing the config space of a device in D3hot is entirely correct
> > and doesn't require any delay.
> >
> > Accessing the config space of a device in D3cold is questionable
> > because the device may not be accessible, but then the host bridge
> > should just fail the access.
> >
> > The 10 ms delay is after attempting to program the device into D0 from
> > D3hot (or the other way around) and it is observed as required.
>
> Thanks for taking a look at this!

No prob.

> If pci_power_up() is doing D3cold -> D0, main power is initially off,
> so platform_pci_set_power_state(PCI_D0) would turn on main power,
> which is a Fundamental Reset leaving the device in D0uninitialized.
> In general the device needs at least 100 ms after that reset before
> any access, e.g., before the PMCSR read.

Yes, it does in general, but that should be covered by
platform_pci_set_power_state() because on some platforms the delay can
be shorter.

> If pci_power_up() is doing D3hot -> D0, the device already has main
> power, so I suppose platform_pci_set_power_state(PCI_D0) doesn't do
> anything.

It may or may not.  Some platforms supply AML to run during D3hot ->
D0 transitions (and the ACPI spec allows that).

> The device remains in D3hot, the PMCSR read should be fine,
> and the PMCSR write to transition to D0 is immediately followed by the
> 10 ms delay.  When No_Soft_Reset == 1, this should be enough.
>
> But when No_Soft_Reset == 0, the transition is to D0uninitialized,
> which sec 2.3.1 includes as a reset condition after which the device
> is permitted to return RRS.
>
> So to me it looks like we need these delays:
>
>   - For D3cold -> D0, msleep(100) and pci_dev_wait() between
>     platform_pci_set_power_state(PCI_D0) and the PMCSR read, similar
>     to what pcie_flr() does.

Adding those delays directly to the PCI PM code would ruin the timing
of system resume on many systems where devices go into D3cold during
suspend, or are powered off by the platform firmware later in the case
of S3 transitions.  For instance, during resume from S3, everything is
typically powered up by the platform firmware which also takes care of
the delays and the kernel effectively gets all devices in D0uninit.

Also, as stated above, IMO it is better if
platform_pci_set_power_state() takes care of the delay.

>     Maybe platform_pci_set_power_state() takes care of some or all of
>     this internally?

At least on some platforms, yes it does.

Moreover, the power-on may not actually happen during
platform_pci_set_power_state() for the device in question, but for its
parent.  It is quite common to provide an ACPI power resource for a
PCIe root port for bringing up the downstream link, which doesn't
really change the power state of the root port itself, but it affects
the subordinate device (and possibly everything below it).

Of course, a good question is what Alternative OSes do when
transitioning PCIe devices from D3cold to D0, but I think I can find
that out.

>   - For D3hot -> D0 with No_Soft_Reset == 0, pci_dev_wait() after the
>     PMCSR write to transition to D0.
>
>     abbcf0e2a99d ("PCI: Wait for device to become ready after a power
>     management reset") added the similar wait in pci_pm_reset().
>
> > > pci_power_up() doesn't do any delay before the PMCSR read.
> >
> > It assumes that platform_pci_set_power_state() has run and either it
> > has succeeded or the config space is not accessible which is when
> > PCI_POSSIBLE_ERROR() will trigger.  Unfortunately, there is no
> > canonical way to verify that power has been restored to the device
> > other than attempting to access it.
> >
> > > That part seems like a pre-existing issue even before this patch.
> >
> > I beg to differ.
> >
> > > If the PMCSR read returns PCI_POSSIBLE_ERROR(), pci_power_up() does
> > > complain "Unable to change power state ... to D0" and return -EIO, but
> > > pci_pm_power_up_and_verify_state() doesn't look at it,
> >
> > In fact, pci_update_current_state() changes the power state to D3cold
> > if the config space is not accessible.
> >
> > > and pci_pm_default_resume_early() continues on to pci_restore_state(), so
> > > it looks to me like we could try to restore state to an inaccessible
> > > device.
> >
> > So the pci_restore_state() call could be avoided if the power state of
> > the device was D3cold, but then the question is how much of a problem
> > that is in practice.
> >
> > > We do call pci_dev_wait() in pci_pm_reset(), which does a D3hot -> D0
> > > transition; shouldn't we do the same in pci_power_up()?
> >
> > It would be good to make them consistent, but maybe the other way around?

Re: [PATCH] PCI: Drop unnecessary retries when restoring BARs

[Bjorn Helgaas]

On Tue, May 05, 2026 at 12:43:17PM +0200, Rafael J. Wysocki wrote:
> On Mon, May 4, 2026 at 11:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> ...

> > If pci_power_up() is doing D3cold -> D0, main power is initially
> > off, so platform_pci_set_power_state(PCI_D0) would turn on main
> > power, which is a Fundamental Reset leaving the device in
> > D0uninitialized.  In general the device needs at least 100 ms
> > after that reset before any access, e.g., before the PMCSR read.
> 
> Yes, it does in general, but that should be covered by
> platform_pci_set_power_state() because on some platforms the delay
> can be shorter.

It's surprising to me that platform_pci_set_power_state() takes
care of that delay because there's so much PCIe infrastructure
(dependencies on link speed, RRS polling, Immediate Readiness,
Readiness Notifications, etc), much of which sounds more like
OS-level support than firmware support.

But if platform_pci_set_power_state() does guarantee that the device
is Configuration-Ready, we should document that somewhere.

> > If pci_power_up() is doing D3hot -> D0, the device already has
> > main power, so I suppose platform_pci_set_power_state(PCI_D0)
> > doesn't do anything.
> 
> It may or may not.  Some platforms supply AML to run during D3hot ->
> D0 transitions (and the ACPI spec allows that).

If the device started in D3hot, it never lost main power, and I
assumed platform_pci_set_power_state(PCI_D0) might just leave it in
D3hot.  It sounds like it takes it all the way to D0, although the
subsequent code that checks the PMCSR state does suggest that the
device might *not* be in D0:

    platform_pci_set_power_state(dev, PCI_D0);

    pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
    state = pmcsr & PCI_PM_CTRL_STATE_MASK;
    if (state == PCI_D0)
      goto end;

    pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);
    if (state == PCI_D3hot)
      pci_dev_d3_sleep(dev);
    ...

  end:

If platform_pci_set_power_state(PCI_D0) puts the device in D0 and
takes care of all the delays, "state" should always be PCI_D0, and we
shouldn't need the D3hot and D2 delays here.

Bjorn

Re: [PATCH] PCI: Drop unnecessary retries when restoring BARs

[Rafael J. Wysocki]

On Fri, May 8, 2026 at 2:17 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, May 05, 2026 at 12:43:17PM +0200, Rafael J. Wysocki wrote:
> > On Mon, May 4, 2026 at 11:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > ...
>
> > > If pci_power_up() is doing D3cold -> D0, main power is initially
> > > off, so platform_pci_set_power_state(PCI_D0) would turn on main
> > > power, which is a Fundamental Reset leaving the device in
> > > D0uninitialized.  In general the device needs at least 100 ms
> > > after that reset before any access, e.g., before the PMCSR read.
> >
> > Yes, it does in general, but that should be covered by
> > platform_pci_set_power_state() because on some platforms the delay
> > can be shorter.
>
> It's surprising to me that platform_pci_set_power_state() takes
> care of that delay because there's so much PCIe infrastructure
> (dependencies on link speed, RRS polling, Immediate Readiness,
> Readiness Notifications, etc), much of which sounds more like
> OS-level support than firmware support.

It is kind of a mixed bag because it involves platform-specific
operations (the PCIe spec doesn't define a standard method of
transitioning devices in D3cold into D0).

The consensus in the industry appears to be that if AML is used for
carrying out D3cold->D0 transitions, it is expected to take the
requisite delays into account.

> But if platform_pci_set_power_state() does guarantee that the device
> is Configuration-Ready, we should document that somewhere.

It is more along the lines of what I said before: After
platform_pci_set_power_state() the device is either
Configuration-Ready, or inaccessible (in which case it can be assumed
to have dropped off the bus, but this is all platform-specific, so on
some platforms there may be ways to revive such devices).

That said, I'm all for documenting it.

> > > If pci_power_up() is doing D3hot -> D0, the device already has
> > > main power, so I suppose platform_pci_set_power_state(PCI_D0)
> > > doesn't do anything.
> >
> > It may or may not.  Some platforms supply AML to run during D3hot ->
> > D0 transitions (and the ACPI spec allows that).
>
> If the device started in D3hot, it never lost main power, and I
> assumed platform_pci_set_power_state(PCI_D0) might just leave it in
> D3hot.  It sounds like it takes it all the way to D0, although the
> subsequent code that checks the PMCSR state does suggest that the
> device might *not* be in D0:

On platforms using ACPI, platform_pci_set_power_state() carries out
the AML part of the transition which may do nothing for devices in
D3hot (for instance, there may be an ACPI power resource that needs to
be turned "on" in order to restore power to the device, but if the
power resource is "on" already, say because it is shared with another
device that is in D0 ATM, its state may not change).

platform_pci_set_power_state() may also be a no-op for the given
device (for example, if this is an add-on device without any
associated AML).

The rule of thumb is that if there is a programmatic way to remove
power from a PCIe device (so it can go into D3cold), it is generally
platform-specific and there should be a complementary programmatic way
to do the reverse (also generally platform-specific).  Otherwise, the
device can go only as deep as D3hot except when the system as a whole
is powered down or it is on a Thunderbolt link that can be
disconnected at any time etc.

>     platform_pci_set_power_state(dev, PCI_D0);
>
>     pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
>     state = pmcsr & PCI_PM_CTRL_STATE_MASK;
>     if (state == PCI_D0)
>       goto end;
>
>     pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);
>     if (state == PCI_D3hot)
>       pci_dev_d3_sleep(dev);
>     ...
>
>   end:
>
> If platform_pci_set_power_state(PCI_D0) puts the device in D0 and
> takes care of all the delays, "state" should always be PCI_D0, and we
> shouldn't need the D3hot and D2 delays here.

Well, not quite, as per the above.

I guess the name is somewhat confusing, but I'm not sure what other
name would be better.

Re: [PATCH] PCI: Drop unnecessary retries when restoring BARs

[Bjorn Helgaas]

On Fri, May 08, 2026 at 02:51:37PM +0200, Rafael J. Wysocki wrote:
> On Fri, May 8, 2026 at 2:17 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, May 05, 2026 at 12:43:17PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, May 4, 2026 at 11:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > ...
> >
> > > > If pci_power_up() is doing D3cold -> D0, main power is initially
> > > > off, so platform_pci_set_power_state(PCI_D0) would turn on main
> > > > power, which is a Fundamental Reset leaving the device in
> > > > D0uninitialized.  In general the device needs at least 100 ms
> > > > after that reset before any access, e.g., before the PMCSR read.
> > >
> > > Yes, it does in general, but that should be covered by
> > > platform_pci_set_power_state() because on some platforms the delay
> > > can be shorter.
> >
> > It's surprising to me that platform_pci_set_power_state() takes
> > care of that delay because there's so much PCIe infrastructure
> > (dependencies on link speed, RRS polling, Immediate Readiness,
> > Readiness Notifications, etc), much of which sounds more like
> > OS-level support than firmware support.
> 
> It is kind of a mixed bag because it involves platform-specific
> operations (the PCIe spec doesn't define a standard method of
> transitioning devices in D3cold into D0).
> 
> The consensus in the industry appears to be that if AML is used for
> carrying out D3cold->D0 transitions, it is expected to take the
> requisite delays into account.
> 
> > But if platform_pci_set_power_state() does guarantee that the device
> > is Configuration-Ready, we should document that somewhere.
> 
> It is more along the lines of what I said before: After
> platform_pci_set_power_state() the device is either
> Configuration-Ready, or inaccessible (in which case it can be assumed
> to have dropped off the bus, but this is all platform-specific, so on
> some platforms there may be ways to revive such devices).

If the device is Configuration-Ready or inaccessible after
platform_pci_set_power_state(PCI_D0), then I agree we shouldn't add
that delay in pci_power_up().

> That said, I'm all for documenting it.

Strawman below.  "Inaccessible upon return" is bound to lead to
questions, but if the PCI core can't do anything I don't know what
else to say.

  diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
  index ed1b1d7a7b46..2b0513c35387 100644
  --- a/drivers/pci/pci.c
  +++ b/drivers/pci/pci.c
  @@ -1068,6 +1068,12 @@ static inline bool platform_pci_power_manageable(struct pci_dev *dev)
	  return acpi_pci_power_manageable(dev);
   }
   
  +/*
  + * When setting power state to D0, platform_pci_set_power_state() ensures
  + * main power is on and that any required delays after a transition to D0
  + * have been completed.  The device is either Configuration-Ready or
  + * inaccessible upon return.
  + */
   static inline int platform_pci_set_power_state(struct pci_dev *dev,
						 pci_power_t t)
   {

> > > > If pci_power_up() is doing D3hot -> D0, the device already has
> > > > main power, so I suppose platform_pci_set_power_state(PCI_D0)
> > > > doesn't do anything.
> > >
> > > It may or may not.  Some platforms supply AML to run during D3hot ->
> > > D0 transitions (and the ACPI spec allows that).
> >
> > If the device started in D3hot, it never lost main power, and I
> > assumed platform_pci_set_power_state(PCI_D0) might just leave it in
> > D3hot.  It sounds like it takes it all the way to D0, although the
> > subsequent code that checks the PMCSR state does suggest that the
> > device might *not* be in D0:
> 
> On platforms using ACPI, platform_pci_set_power_state() carries out
> the AML part of the transition which may do nothing for devices in
> D3hot (for instance, there may be an ACPI power resource that needs to
> be turned "on" in order to restore power to the device, but if the
> power resource is "on" already, say because it is shared with another
> device that is in D0 ATM, its state may not change).
> 
> platform_pci_set_power_state() may also be a no-op for the given
> device (for example, if this is an add-on device without any
> associated AML).
> 
> The rule of thumb is that if there is a programmatic way to remove
> power from a PCIe device (so it can go into D3cold), it is generally
> platform-specific and there should be a complementary programmatic way
> to do the reverse (also generally platform-specific).  Otherwise, the
> device can go only as deep as D3hot except when the system as a whole
> is powered down or it is on a Thunderbolt link that can be
> disconnected at any time etc.
> 
> >     platform_pci_set_power_state(dev, PCI_D0);
> >
> >     pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> >     state = pmcsr & PCI_PM_CTRL_STATE_MASK;
> >     if (state == PCI_D0)
> >       goto end;
> >
> >     pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);
> >     if (state == PCI_D3hot)
> >       pci_dev_d3_sleep(dev);
> >     ...
> >
> >   end:
> >
> > If platform_pci_set_power_state(PCI_D0) puts the device in D0 and
> > takes care of all the delays, "state" should always be PCI_D0, and we
> > shouldn't need the D3hot and D2 delays here.
> 
> Well, not quite, as per the above.

OK, so after platform_pci_set_power_state(PCI_D0), the device may be
in D3hot in some cases.  Then there's a D3hot -> D0 transition, and we
do pci_dev_d3_sleep(), but not pci_dev_wait().  Sec 2.3.1 says RRS is
permitted after a D3hot to D0uninitialized, and pci_dev_wait() is what
waits and retries for the RRS case.

It looks to me like we need pci_dev_wait() after that transition when
No_Soft_Reset == 0, i.e., something like the below.  Do you think this
is unnecessary?

  diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
  index 8f7cfcc00090..ed1b1d7a7b46 100644
  --- a/drivers/pci/pci.c
  +++ b/drivers/pci/pci.c
  @@ -1341,10 +1341,14 @@ int pci_power_up(struct pci_dev *dev)
	  pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);
   
	  /* Mandatory transition delays; see PCI PM 1.2. */
  -	if (state == PCI_D3hot)
  +	if (state == PCI_D3hot) {
		  pci_dev_d3_sleep(dev);
  -	else if (state == PCI_D2)
  +		if (!(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET))
  +			pci_dev_wait(dev, "power up D3hot->D0",
  +				     PCIE_RESET_READY_POLL_MS);
  +	} else if (state == PCI_D2) {
		  udelay(PCI_PM_D2_DELAY);
  +	}
   
   end:
	  dev->current_state = PCI_D0;

Re: [PATCH] PCI: Drop unnecessary retries when restoring BARs

[Rafael J. Wysocki]

On Fri, May 8, 2026 at 11:43 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, May 08, 2026 at 02:51:37PM +0200, Rafael J. Wysocki wrote:
> > On Fri, May 8, 2026 at 2:17 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Tue, May 05, 2026 at 12:43:17PM +0200, Rafael J. Wysocki wrote:
> > > > On Mon, May 4, 2026 at 11:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > ...
> > >
> > > > > If pci_power_up() is doing D3cold -> D0, main power is initially
> > > > > off, so platform_pci_set_power_state(PCI_D0) would turn on main
> > > > > power, which is a Fundamental Reset leaving the device in
> > > > > D0uninitialized.  In general the device needs at least 100 ms
> > > > > after that reset before any access, e.g., before the PMCSR read.
> > > >
> > > > Yes, it does in general, but that should be covered by
> > > > platform_pci_set_power_state() because on some platforms the delay
> > > > can be shorter.
> > >
> > > It's surprising to me that platform_pci_set_power_state() takes
> > > care of that delay because there's so much PCIe infrastructure
> > > (dependencies on link speed, RRS polling, Immediate Readiness,
> > > Readiness Notifications, etc), much of which sounds more like
> > > OS-level support than firmware support.
> >
> > It is kind of a mixed bag because it involves platform-specific
> > operations (the PCIe spec doesn't define a standard method of
> > transitioning devices in D3cold into D0).
> >
> > The consensus in the industry appears to be that if AML is used for
> > carrying out D3cold->D0 transitions, it is expected to take the
> > requisite delays into account.
> >
> > > But if platform_pci_set_power_state() does guarantee that the device
> > > is Configuration-Ready, we should document that somewhere.
> >
> > It is more along the lines of what I said before: After
> > platform_pci_set_power_state() the device is either
> > Configuration-Ready, or inaccessible (in which case it can be assumed
> > to have dropped off the bus, but this is all platform-specific, so on
> > some platforms there may be ways to revive such devices).
>
> If the device is Configuration-Ready or inaccessible after
> platform_pci_set_power_state(PCI_D0), then I agree we shouldn't add
> that delay in pci_power_up().
>
> > That said, I'm all for documenting it.
>
> Strawman below.  "Inaccessible upon return" is bound to lead to
> questions, but if the PCI core can't do anything I don't know what
> else to say.
>
>   diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>   index ed1b1d7a7b46..2b0513c35387 100644
>   --- a/drivers/pci/pci.c
>   +++ b/drivers/pci/pci.c
>   @@ -1068,6 +1068,12 @@ static inline bool platform_pci_power_manageable(struct pci_dev *dev)
>           return acpi_pci_power_manageable(dev);
>    }
>
>   +/*
>   + * When setting power state to D0, platform_pci_set_power_state() ensures
>   + * main power is on and that any required delays after a transition to D0
>   + * have been completed.  The device is either Configuration-Ready or
>   + * inaccessible upon return.
>   + */

It looks as good as it gets to me, but I'd put it into pci_power_up()
which is where it really matters.

>    static inline int platform_pci_set_power_state(struct pci_dev *dev,
>                                                  pci_power_t t)
>    {
>
> > > > > If pci_power_up() is doing D3hot -> D0, the device already has
> > > > > main power, so I suppose platform_pci_set_power_state(PCI_D0)
> > > > > doesn't do anything.
> > > >
> > > > It may or may not.  Some platforms supply AML to run during D3hot ->
> > > > D0 transitions (and the ACPI spec allows that).
> > >
> > > If the device started in D3hot, it never lost main power, and I
> > > assumed platform_pci_set_power_state(PCI_D0) might just leave it in
> > > D3hot.  It sounds like it takes it all the way to D0, although the
> > > subsequent code that checks the PMCSR state does suggest that the
> > > device might *not* be in D0:
> >
> > On platforms using ACPI, platform_pci_set_power_state() carries out
> > the AML part of the transition which may do nothing for devices in
> > D3hot (for instance, there may be an ACPI power resource that needs to
> > be turned "on" in order to restore power to the device, but if the
> > power resource is "on" already, say because it is shared with another
> > device that is in D0 ATM, its state may not change).
> >
> > platform_pci_set_power_state() may also be a no-op for the given
> > device (for example, if this is an add-on device without any
> > associated AML).
> >
> > The rule of thumb is that if there is a programmatic way to remove
> > power from a PCIe device (so it can go into D3cold), it is generally
> > platform-specific and there should be a complementary programmatic way
> > to do the reverse (also generally platform-specific).  Otherwise, the
> > device can go only as deep as D3hot except when the system as a whole
> > is powered down or it is on a Thunderbolt link that can be
> > disconnected at any time etc.
> >
> > >     platform_pci_set_power_state(dev, PCI_D0);
> > >
> > >     pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> > >     state = pmcsr & PCI_PM_CTRL_STATE_MASK;
> > >     if (state == PCI_D0)
> > >       goto end;
> > >
> > >     pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);
> > >     if (state == PCI_D3hot)
> > >       pci_dev_d3_sleep(dev);
> > >     ...
> > >
> > >   end:
> > >
> > > If platform_pci_set_power_state(PCI_D0) puts the device in D0 and
> > > takes care of all the delays, "state" should always be PCI_D0, and we
> > > shouldn't need the D3hot and D2 delays here.
> >
> > Well, not quite, as per the above.
>
> OK, so after platform_pci_set_power_state(PCI_D0), the device may be
> in D3hot in some cases.  Then there's a D3hot -> D0 transition, and we
> do pci_dev_d3_sleep(), but not pci_dev_wait().  Sec 2.3.1 says RRS is
> permitted after a D3hot to D0uninitialized, and pci_dev_wait() is what
> waits and retries for the RRS case.
>
> It looks to me like we need pci_dev_wait() after that transition when
> No_Soft_Reset == 0, i.e., something like the below.  Do you think this
> is unnecessary?

Well, let me turn this around: Is there any evidence that it is necessary?

I guess if it is not necessary, there will be no harm, but this is a
performance-sensitive code path.

>   diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>   index 8f7cfcc00090..ed1b1d7a7b46 100644
>   --- a/drivers/pci/pci.c
>   +++ b/drivers/pci/pci.c
>   @@ -1341,10 +1341,14 @@ int pci_power_up(struct pci_dev *dev)
>           pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);
>
>           /* Mandatory transition delays; see PCI PM 1.2. */
>   -     if (state == PCI_D3hot)
>   +     if (state == PCI_D3hot) {
>                   pci_dev_d3_sleep(dev);
>   -     else if (state == PCI_D2)
>   +             if (!(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET))
>   +                     pci_dev_wait(dev, "power up D3hot->D0",
>   +                                  PCIE_RESET_READY_POLL_MS);
>   +     } else if (state == PCI_D2) {
>                   udelay(PCI_PM_D2_DELAY);
>   +     }
>
>    end:
>           dev->current_state = PCI_D0;

Re: [PATCH] PCI: Drop unnecessary retries when restoring BARs

[Bjorn Helgaas]

On Mon, May 11, 2026 at 05:24:49PM +0200, Rafael J. Wysocki wrote:
> On Fri, May 8, 2026 at 11:43 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, May 08, 2026 at 02:51:37PM +0200, Rafael J. Wysocki wrote:
> > > On Fri, May 8, 2026 at 2:17 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Tue, May 05, 2026 at 12:43:17PM +0200, Rafael J. Wysocki wrote:
> > > > > On Mon, May 4, 2026 at 11:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> ...

> > > >     platform_pci_set_power_state(dev, PCI_D0);
> > > >
> > > >     pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> > > >     state = pmcsr & PCI_PM_CTRL_STATE_MASK;
> > > >     if (state == PCI_D0)
> > > >       goto end;
> > > >
> > > >     pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);
> > > >     if (state == PCI_D3hot)
> > > >       pci_dev_d3_sleep(dev);
> > > >     ...
> > > >
> > > >   end:
> ...

> > OK, so after platform_pci_set_power_state(PCI_D0), the device may be
> > in D3hot in some cases.  Then there's a D3hot -> D0 transition, and we
> > do pci_dev_d3_sleep(), but not pci_dev_wait().  Sec 2.3.1 says RRS is
> > permitted after a D3hot to D0uninitialized, and pci_dev_wait() is what
> > waits and retries for the RRS case.
> >
> > It looks to me like we need pci_dev_wait() after that transition when
> > No_Soft_Reset == 0, i.e., something like the below.  Do you think this
> > is unnecessary?
> 
> Well, let me turn this around: Is there any evidence that it is necessary?

PCIe r7.0, sec 2.3.1, includes this:

  ◦ For Configuration Requests only, if Device Readiness Status is not
    supported, following reset it is permitted for a Function to
    terminate the request and indicate that it is temporarily unable
    to process the Request, but will be able to process the Request in
    the future - in this case, the Request Retry Status (RRS)
    Completion Status must be used (see § Section 6.6 ). Valid reset
    conditions after which a device/Function is permitted to return
    RRS in response to a Configuration Request are:

    ▪ Cold, Warm, and Hot Resets
    ▪ FLRs
    ▪ A reset initiated in response to a D3Hot to D0uninitialized
      device state transition

When No_Soft_Reset == 0, the D3hot->D0 transition is to
D0uninitialized, so I think the device is allowed to respond with RRS.

For the D3hot->D0 case, pci_power_up() already calls
pci_dev_d3_sleep(), which waits the 10ms required by sec 5.9, but it
does not currently account for an RRS response after that.

My argument is that if the device *does* respond with RRS, subsequent
config reads in callers of pci_power_up() will see invalid ~0 data
even though the device may become ready in the future.

For example, pci_set_full_power_state() reads PCI_PM_CTRL and may
incorrectly complain "Refused to change power state from D3hot to D0".

> I guess if it is not necessary, there will be no harm, but this is a
> performance-sensitive code path.

If the device is already Configuration-Ready, pci_dev_wait() will do
one additional config read, see a valid Vendor/Device ID, and won't
wait at all.

Bjorn

Re: [PATCH] PCI: Drop unnecessary retries when restoring BARs

[Rafael J. Wysocki]

On Tue, May 12, 2026 at 2:01 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Mon, May 11, 2026 at 05:24:49PM +0200, Rafael J. Wysocki wrote:
> > On Fri, May 8, 2026 at 11:43 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Fri, May 08, 2026 at 02:51:37PM +0200, Rafael J. Wysocki wrote:
> > > > On Fri, May 8, 2026 at 2:17 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Tue, May 05, 2026 at 12:43:17PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Mon, May 4, 2026 at 11:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > ...
>
> > > > >     platform_pci_set_power_state(dev, PCI_D0);
> > > > >
> > > > >     pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> > > > >     state = pmcsr & PCI_PM_CTRL_STATE_MASK;
> > > > >     if (state == PCI_D0)
> > > > >       goto end;
> > > > >
> > > > >     pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);
> > > > >     if (state == PCI_D3hot)
> > > > >       pci_dev_d3_sleep(dev);
> > > > >     ...
> > > > >
> > > > >   end:
> > ...
>
> > > OK, so after platform_pci_set_power_state(PCI_D0), the device may be
> > > in D3hot in some cases.  Then there's a D3hot -> D0 transition, and we
> > > do pci_dev_d3_sleep(), but not pci_dev_wait().  Sec 2.3.1 says RRS is
> > > permitted after a D3hot to D0uninitialized, and pci_dev_wait() is what
> > > waits and retries for the RRS case.
> > >
> > > It looks to me like we need pci_dev_wait() after that transition when
> > > No_Soft_Reset == 0, i.e., something like the below.  Do you think this
> > > is unnecessary?
> >
> > Well, let me turn this around: Is there any evidence that it is necessary?
>
> PCIe r7.0, sec 2.3.1, includes this:
>
>   ◦ For Configuration Requests only, if Device Readiness Status is not
>     supported, following reset it is permitted for a Function to
>     terminate the request and indicate that it is temporarily unable
>     to process the Request, but will be able to process the Request in
>     the future - in this case, the Request Retry Status (RRS)
>     Completion Status must be used (see § Section 6.6 ). Valid reset
>     conditions after which a device/Function is permitted to return
>     RRS in response to a Configuration Request are:
>
>     ▪ Cold, Warm, and Hot Resets
>     ▪ FLRs
>     ▪ A reset initiated in response to a D3Hot to D0uninitialized
>       device state transition
>
> When No_Soft_Reset == 0, the D3hot->D0 transition is to
> D0uninitialized, so I think the device is allowed to respond with RRS.
>
> For the D3hot->D0 case, pci_power_up() already calls
> pci_dev_d3_sleep(), which waits the 10ms required by sec 5.9, but it
> does not currently account for an RRS response after that.
>
> My argument is that if the device *does* respond with RRS, subsequent
> config reads in callers of pci_power_up() will see invalid ~0 data
> even though the device may become ready in the future.
>
> For example, pci_set_full_power_state() reads PCI_PM_CTRL and may
> incorrectly complain "Refused to change power state from D3hot to D0".

Fair enough.

> > I guess if it is not necessary, there will be no harm, but this is a
> > performance-sensitive code path.
>
> If the device is already Configuration-Ready, pci_dev_wait() will do
> one additional config read, see a valid Vendor/Device ID, and won't
> wait at all.

One extra config read shouldn't make a measurable difference.