Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2] PCI: Run platform power transition on initial D0 entry
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     Bjorn Helgaas <bhelgaas@google.com>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux PCI <linux-pci@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20210314000439.3138941-1-luzmaximilian@gmail.com>
 <CAJZ5v0hY=NgKAU+N_kaya=q3Vk6SnkRTfXuiiP0ttoxHq+pRTA@mail.gmail.com>
From:   Maximilian Luz <luzmaximilian@gmail.com>
Message-ID: <781f0963-4ce6-74c9-e884-1e57f1ff9673@gmail.com>
Date:   Mon, 15 Mar 2021 19:28:16 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <CAJZ5v0hY=NgKAU+N_kaya=q3Vk6SnkRTfXuiiP0ttoxHq+pRTA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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