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

On Tue, May 12, 2026 at 2:01 AM Bjorn Helgaas <[email protected]> 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 <[email protected]> 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 <[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:
> > ...
>
> > > > >     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.