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]

Date:   Tue, 16 Mar 2021 14:36:52 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Maximilian Luz <luzmaximilian@...il.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        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 Mon, Mar 15, 2021 at 7:28 PM Maximilian Luz <luzmaximilian@...il.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@...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.]
>
> 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!