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>
 <781f0963-4ce6-74c9-e884-1e57f1ff9673@gmail.com>
 <CAJZ5v0g+wkyzrD120yiyyBFjVO=LYS3j0WK1Fi-j+LS5fwgqZg@mail.gmail.com>
From:   Maximilian Luz <luzmaximilian@gmail.com>
Message-ID: <821c10e8-ef19-4d2e-5ea2-a1964ef58d67@gmail.com>
Date:   Tue, 16 Mar 2021 15:10:04 +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: <CAJZ5v0g+wkyzrD120yiyyBFjVO=LYS3j0WK1Fi-j+LS5fwgqZg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/16/21 2:36 PM, Rafael J. Wysocki wrote:
> On Mon, Mar 15, 2021 at 7:28 PM Maximilian Luz <luzmaximilian@gmail.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@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?
> 
> I'll post it, thanks!

Thank you!

Feel free to add my tested-by tag.

Regards,
Max