Messages in this thread | | | Subject | Re: [PATCH v2] PCI: Run platform power transition on initial D0 entry | From | Maximilian Luz <> | Date | Mon, 15 Mar 2021 19:28:16 +0100 |
| |
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
| |