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

On Fri, May 8, 2026 at 2:17 AM Bjorn Helgaas <[email protected]> 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 <[email protected]> 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.