Re: [PATCH v2] PCI: Run platform power transition on initial D0 entry

[Maximilian Luz]

On 3/16/21 2:36 PM, Rafael J. Wysocki wrote:
> On Mon, Mar 15, 2021 at 7:28 PM Maximilian Luz <luzmaximilian@xxxxxxxxx> wrote:
> > On 3/15/21 4:34 PM, Rafael J. Wysocki wrote:
> > > On Sun, Mar 14, 2021 at 1:06 AM Maximilian Luz <luzmaximilian@xxxxxxxxx> wrote:
> > > > On some devices and platforms, the initial platform (e.g. ACPI) power
> > > > state is not in sync with the power state of the PCI device.
> > > >
> > > > This seems like it is, for all intents and purposes, an issue with the
> > > > device firmware (e.g. ACPI). On some devices, specifically Microsoft
> > > > Surface Books 2 and 3, we encounter ACPI code akin to the following
> > > > power resource, corresponding to a PCI device:
> > > >
> > > >       PowerResource (PRP5, 0x00, 0x0000)
> > > >       {
> > > >           // Initialized to zero, i.e. off. There is no logic for checking
> > > >           // the actual state dynamically.
> > > >           Name (_STA, Zero)
> > > >
> > > >           Method (_ON, 0, Serialized)
> > > >           {
> > > >               // ... code omitted ...
> > > >               _STA = One
> > > >           }
> > > >
> > > >           Method (_OFF, 0, Serialized)
> > > >           {
> > > >               // ... code omitted ...
> > > >               _STA = Zero
> > > >           }
> > > >       }
> > > >
> > > > This resource is initialized to 'off' and does not have any logic for
> > > > checking its actual state, i.e. the state of the corresponding PCI
> > > > device. The stored state of this resource can only be changed by running
> > > > the (platform/ACPI) power transition functions (i.e. _ON and _OFF).
> > >
> > > Well, there is _STA that returns "off" initially, so the OS should set
> > > the initial state of the device to D3cold and transition it into D0 as
> > > appropriate (i.e. starting with setting all of the power resources
> > > used by it to "on").
> > >
> > > > This means that, at boot, the PCI device power state is out of sync with
> > > > the power state of the corresponding ACPI resource.
> > > >
> > > > During initial bring-up of a PCI device, pci_enable_device_flags()
> > > > updates its PCI core state (from initially 'unknown') by reading from
> > > > its PCI_PM_CTRL register. It does, however, not check if the platform
> > > > (here ACPI) state is in sync with/valid for the actual device state and
> > > > needs updating.
> > >
> > > Well, that's inconsistent.
> > >
> > > Also, it is rather pointless to update the device's power state at
> > > this point, because nothing between this point and the later
> > > do_pci_enable_device() call in this function requires its
> > > current_state to be up to date AFAICS.
> > >
> > > Have you tried to drop the power state update from
> > > pci_enable_device_flags()?  [Note that we're talking about relatively
> > > old code here and it looks like that code is not necessary any more.]
> >
> > I had not tried this before, as I assumed the comment was still
> > relevant. I did test that now and it works! I can't detect any
> > regressions.
> >
> > Do you want to send this in or should I do that?
>
> I'll post it, thanks!

Thank you!

Feel free to add my tested-by tag.

Regards,
Max