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

[Maximilian Luz <luzmaximilian@gmail.com>]

On 3/15/21 4:34 PM, Rafael J. Wysocki wrote:
> On Sun, Mar 14, 2021 at 1:06 AM Maximilian Luz <luzmaximilian@gmail.com> 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?

> Either it should be possible to do that and all should work, or there
> is a good reason to make current_state reflect the real current power
> state of the device upfront, but then that should be done by putting
> it into D0 diractly at that point rather than later.
> 
> Calling pci_power_up(dev) instead of pci_set_power_state(dev, PCI_D0)
> when current_state is already 0 only pokes at the power resources,
> because pci_raw_set_power_state() will do nothing then, but that is a
> rather less-than-straightforward way of doing this.  Moreover, the
> ordering of actions mandated by the spec is to set power resources to
> "on" first and then write to the PMCSR, not the other way around.

I don't know much about the PCI core (let alone spec), so that seemed
like the least intrusive way to fix this for me.

Thanks!
Max