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

[Maximilian Luz <luzmaximilian@gmail.com>]

On 3/16/21 2:36 PM, Rafael J. Wysocki wrote:
> On Mon, Mar 15, 2021 at 7:28 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>
>> 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?
> 
> I'll post it, thanks!

Thank you!

Feel free to add my tested-by tag.

Regards,
Max