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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0hY=NgKAU+N_kaya=q3Vk6SnkRTfXuiiP0ttoxHq+pRTA@mail.gmail.com>
Date:   Mon, 15 Mar 2021 16:34:02 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Maximilian Luz <luzmaximilian@...il.com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Linux PCI <linux-pci@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] PCI: Run platform power transition on initial D0 entry

On Sun, Mar 14, 2021 at 1:06 AM Maximilian Luz <luzmaximilian@...il.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.]

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.