lkml.org 
[lkml]   [2021]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] PCI: Run platform power transition on initial D0 entry
From
Date
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

\
 
 \ /
  Last update: 2021-03-15 19:29    [W:0.089 / U:0.356 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site