On Mon, Mar 15, 2021 at 7:28 PM Maximilian Luz [off-list ref] wrote:

On 3/15/21 4:34 PM, Rafael J. Wysocki wrote:

quoted

On Sun, Mar 14, 2021 at 1:06 AM Maximilian Luz [off-list ref] wrote:

quoted

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").

quoted

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!