Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8f89131c-41a2-02e4-1427-8a5b1704e0ea@nvidia.com>
Date:   Tue, 8 Feb 2022 16:00:35 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.1
Subject: Re: [PATCH] PCI: Fix the ACPI power state during runtime resume
Content-Language: en-US
To:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Bjorn Helgaas <helgaas@kernel.org>
Cc:     Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, Linux PM <linux-pm@vger.kernel.org>,
        Mika Westerberg <mika.westerberg@linux.intel.com>
References: <20220204233219.GA228585@bhelgaas> <2249802.ElGaqSPkdT@kreacher>
From:   Abhishek Sahu <abhsahu@nvidia.com>
In-Reply-To: <2249802.ElGaqSPkdT@kreacher>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TVpORWsxMWlpNTJ0eFhEZVJPZjlTbzJLZlRXQVFkOFlFa0hWQzVLMTkrM1lw?=
 =?utf-8?B?cjJ4K0RhR085YXhUMUVWdXBhRVl0SUh5T3NmY2IvVzIyejJuRmJkUE1ERk5V?=
 =?utf-8?B?REs3TEkyNDU3MDl4anBYUUI3NjdZUWxGSXJPenFKcStBMS8wTmZ0YnQwNWtS?=
 =?utf-8?B?UjFCUWl2bVMveGk1NEpiMHFlb1psQWVtQjZEenFnTlVlQ3lMWlltb21lUGxM?=
 =?utf-8?B?NkFvLzNTTVY4dGE4RHcxTkNhVG9CL0xmczFmM0FYYWMwRmtlMjVtODdjb2xi?=
 =?utf-8?B?RmJFcmliRnV5OXIrenpqakpGVU1uSHdMUlhYdlFIVlROS1hCSnB5MU1ra3pl?=
 =?utf-8?B?TFFiUnB2bjRKK3l4eGRiT2NtQWxaLzlFWmR6VE9BY2I1WG1WZGdsV1JiR2ZF?=
 =?utf-8?B?NmpCdFJQRGcrOUJ3clpnMHE5dnVLWE9sQWJRZUNyWVcyVDVYWFMvUVNqeGlJ?=
 =?utf-8?B?STJsRi95NUgveUcrR1FlY1lWZEtKdXR0SSs5Rk1LenNrUjFQdmNzZE43UGlX?=
 =?utf-8?B?Zk51NSs0TnhzTjhlTWdzWEJFeFBPZ2hOZmh2L2lRQThoM3NqbUdsQUM3OFlK?=
 =?utf-8?B?bWo3MW9ERThsVHEyMEY0N1R0akQxZWNjSWFob3pNdlp0bjI0eGh4cVJXUlov?=
 =?utf-8?B?S0I2WDZXM09IaGNnc1oycFJDaHN6Z0lqcnl4ZHhBTnZUZm96Tk5DaEZzMVcz?=
 =?utf-8?B?emQrQWZSVFpQM0c3bm1ZNVZjTERZMGNxSmxWb3hlUDFOSzFUMGU2Wm90aXdk?=
 =?utf-8?B?dUFyQmpwWHdxdWhJMzM4ZXJ0UkUwaVRoOHo0ZzJvNyt1WlNVaUJNUlRRbWta?=
 =?utf-8?B?Q3JlL1VWekJrZXdMem1qTDVEakpaSFBLd3dwWWZlVXZkdU8rUlhYYnU5K0Zp?=
 =?utf-8?B?WFRrZkd3cWdINTNpZlZVb3hLempPcTcwQTYvbmF0ZmtLYjQ3b3NnZTAvcGJN?=
 =?utf-8?B?eG5kdllKS1E1eXliQm9sQjIzcVpxOVQ5dUgyNml4ck95UmN1S01zQkhHNXVY?=
 =?utf-8?B?c3o2QVN5Z1RCOHd2TXNabUZrZ2xoSnRsT2tiRkwzZmlGYkdBMkVvazB2QUF1?=
 =?utf-8?B?aTRJQm11UDdvQWhlQUVuMWNOeVFFQ251OTh5RnpsV2RwL0gzMmk4ZktaQjJx?=
 =?utf-8?B?cW9TNGNMRFZZd0FmOHcyeTJJdy9NL2ROUHNydWRxd1crYzNNZGJjS2xMYyto?=
 =?utf-8?B?VHhuYnlldFBxUVFxZ3VyL1BEUlM2NFltMmUwRXFoTkwwNW5MY3l5b2Z4Y0ZG?=
 =?utf-8?B?b2VJL0xuZ1k2cEJYb2JmVHBlcG5leklpUkNOMGcxTGhNNWh5N2hlUEZmQjRS?=
 =?utf-8?B?WTlQOFhrdWJ1eFFIM1BXTWJrWWkySEVKMUtFc09oQk1EVXBOTjBzVVVLREVD?=
 =?utf-8?B?MVpDMXlVbmNiMzVxaml4SFJjUVhjSko0U1B2ZjFzQUQyYjNXUHBFUy9Sb0l3?=
 =?utf-8?B?LzF6eXRBdmdDUXdqVjduYXptMUNZaExETjZaQkxjZVZmVFRPZWNoMVkwR3BI?=
 =?utf-8?B?MXBzL1FSbTh2TGMwRzVsaUxQVkc4MmtrdndNcmg5b0JuVkt6a0JoYTF0aDV1?=
 =?utf-8?B?aDRCZFFoc2hBT2FXV1F0RUxUdjNUU0RBRVVSZldUNStUZVczbjNEZVJsYVJ6?=
 =?utf-8?B?VGFIbU11Vjc5ZTJ3QmtYRzczSFZ2THJQSDFtZ2FIeFJ4QzhmUlcrMFdDM1Yx?=
 =?utf-8?B?K1FhS2pJSFhkWm9qU0lyR1F4YVEwU0ptdGpJTnRwYmYwanZCYkdhM0ppd2tj?=
 =?utf-8?B?VFpoSUtuQ2tuRjRnT21MZ1hLNlc1YXhhRlhNRm9HeGJCWHZnN3JINTFZLzlE?=
 =?utf-8?B?NHlJUmtBa3JCTDE2LzZ3dDRFTzl0RkErRXFCdDBRQmZ4RWtZckVtd0ZGY3Ny?=
 =?utf-8?B?alhqRVJkZXJoRitxYzUyYk8zNWR3MTlrbnhNTFhCZ081cVM5NkdaeXlJbis5?=
 =?utf-8?B?RjdXbll4VWtoUmQyelFwSzkwa1ovMzN1UGRXWThmU3FQb2NkbitBSjFFRGFO?=
 =?utf-8?B?b09CdGtYUVFFTFEybXdRdi80UHlOVlFkR0lnMEkrYkdEWDFEWlMwRnEvSm1s?=
 =?utf-8?B?WU5ZQkhsTk1TSWM1UEQ5MmNYb3lPaEZNMVl0dGhDbVFnd2U4ZlA1THNCNzEx?=
 =?utf-8?B?Y2dLNCtiN04rT0tSWkNWMzVtN1pyWTJHYm10bnpVUGs3blZaUllhSGlsb3pB?=
 =?utf-8?Q?+G7w9lfePT9Zd8uh04HGlYs=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4cee501c-4b0a-473b-ffd6-08d9eaee11c0
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5304.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Feb 2022 10:30:47.4205
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: KjlPOvIlBV+m1gQgVSO8i4xpHuByDSztOgmJXRj1I+QPucSjQSoNHn99tVPwqVNxHtqxouy/pzP27VpAF32idg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB2665
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2/8/2022 12:28 AM, Rafael J. Wysocki wrote:
> On Saturday, February 5, 2022 12:32:19 AM CET Bjorn Helgaas wrote:
>> [+cc Rafael, hoping for your review :)
> 
> +Mika
> 
>> Wonder if we should add something like this to MAINTAINERS so you get
>> cc'd on power-related things:
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ea3e6c914384..3d9a211cad5d 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15422,6 +15422,7 @@ F:    include/linux/pm.h
>>  F:   include/linux/pm_*
>>  F:   include/linux/powercap.h
>>  F:   kernel/configs/nopm.config
>> +K:   pci_[a-z_]*power[a-z_]*\(
> 
> It seems so, but generally PM patches should be CCed to linux-pm anyway.
> 
>>
>>  DYNAMIC THERMAL POWER MANAGEMENT (DTPM)
>>  M:   Daniel Lezcano <daniel.lezcano@kernel.org>
>> ]
>>
>> On Mon, Jan 24, 2022 at 05:51:07PM +0530, Abhishek Sahu wrote:
>>> Consider the following sequence during PCI device runtime
>>> suspend/resume:
>>>
>>> 1. PCI device goes into runtime suspended state. The PCI state
>>>    will be changed to PCI_D0 and then pci_platform_power_transition()
>>>    will be called which changes the ACPI state to ACPI_STATE_D3_HOT.
> 
> You mean PCI_D3hot I suppose?
>

 Yes. It should be PCI_D3hot here. 

>>> 2. Parent bridge goes into runtime suspended state. If parent
>>>    bridge supports D3cold, then it will change the power state of all its
>>>    children to D3cold state and the power will be removed.
>>>
>>> 3. During wake-up time, the bridge will be runtime resumed first
>>>    and pci_power_up() will be called for the bridge. Now, the power
>>>    supply will be resumed.
>>>
>>> 4. pci_resume_bus() will be called which will internally invoke
>>>    pci_restore_standard_config(). pci_update_current_state()
>>>    will read PCI_PM_CTRL register and the current_state will be
>>>    updated to D0.
>>>
>>> In the above process, at step 4, the ACPI device state will still be
>>> ACPI_STATE_D3_HOT since pci_platform_power_transition() is not being
>>> invoked.
> 
> I'm not quite following.
> 
> I'm assuming that this description applies to the endpoint device that was
> previously put into D3_hot.
> 

 Yes. This is applicable for endpoint devices which was previously put
 into D3hot.

> Since its current state is D3_hot, it is not D0 (in particular) and the
> pci_set_power_state() in pci_restore_standard_config() should put int into
> D0 proper, including the platform firmware part.
> 

 The pci_restore_standard_config() for endpoint devices are being called
 internally during wake-up of upstream bridge. 

 pci_power_up(struct pci_dev *dev)
 {
      ...
      if (dev->runtime_d3cold) {
        /*
         * When powering on a bridge from D3cold, the whole hierarchy
         * may be powered on into D0uninitialized state, resume them to
         * give them a chance to suspend again
         */
        pci_resume_bus(dev->subordinate);
    }
    ...
 }

 For the upstream bridge, the above code will trigger the wake-up of
 endpoint devices and then following code will be executed for the
 endpoint devices:

 pci_update_current_state(struct pci_dev *dev, pci_power_t state)
 {
    if (platform_pci_get_power_state(dev) == PCI_D3cold ||
        !pci_device_is_present(dev)) {
        dev->current_state = PCI_D3cold;
    } else if (dev->pm_cap) {
        u16 pmcsr;

        pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
        dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
    } else {
        dev->current_state = state;
    }
 }

 In the above code, the current_state will be set to D0 for the
 endpoint devices since it will go into second block where
 it will read the PM_CTRL register.
 
>>> We need call the pci_platform_power_transition() with state
>>> D0 to change the ACPI state to ACPI_STATE_D0.
>>>
>>> This patch calls pci_power_up() if current power state is D0 inside
>>> pci_restore_standard_config(). This pci_power_up() will change the
>>> ACPI state to ACPI_STATE_D0.
>>>
>>> Following are the steps to confirm:
>>>
>>> Enable the debug prints in acpi_pci_set_power_state()
>>>
>>> 0000:01:00.0 is PCI device and 0000:00:01.0 is parent bridge device
>>>
>>> Before:
>>>
>>> 0000:01:00.0: power state changed by ACPI to D3hot
>>> 0000:00:01.0: power state changed by ACPI to D3cold
>>> 0000:00:01.0: power state changed by ACPI to D0
>>>
>>> After:
>>>
>>> 0000:01:00.0: power state changed by ACPI to D3hot
>>> 0000:00:01.0: power state changed by ACPI to D3cold
>>> 0000:00:01.0: power state changed by ACPI to D0
>>> 0000:01:00.0: power state changed by ACPI to D0
>>>
>>> So with this patch, the PCI device ACPI state is also being
>>> changed to D0.
>>>
>>> Signed-off-by: Abhishek Sahu <abhsahu@nvidia.com>
>>> ---
>>>  drivers/pci/pci-driver.c | 14 +++++++++++---
>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>>> index 588588cfda48..64e0cca12f16 100644
>>> --- a/drivers/pci/pci-driver.c
>>> +++ b/drivers/pci/pci-driver.c
>>> @@ -521,14 +521,22 @@ static void pci_device_shutdown(struct device *dev)
>>>   */
>>>  static int pci_restore_standard_config(struct pci_dev *pci_dev)
>>>  {
>>> +   int error = 0;
>>>     pci_update_current_state(pci_dev, PCI_UNKNOWN);
>>>
>>>     if (pci_dev->current_state != PCI_D0) {
>>> -           int error = pci_set_power_state(pci_dev, PCI_D0);
>>> -           if (error)
>>> -                   return error;
>>> +           error = pci_set_power_state(pci_dev, PCI_D0);
>>> +   } else {
>>> +           /*
>>> +            * The platform power state can still be non-D0, so this is
>>> +            * required to change the platform power state to D0.
>>> +            */
> 
> This really isn't expected to happen.
> 
> If the device's power state has been changed to D3hot by ACPI, it is not in D0.
> 
> It looks like the state tracking is not working here.
> 

 The state setting to D0 is happening due to the current logic present in
 pci_update_current_state(). If we can fix the logic in
 pci_update_current_state() to detect this condition and return state D3hot,
 then it should also fix the issue. 

 Thanks,
 Abhishek

>>> +           error = pci_power_up(pci_dev);
>>>     }
>>>
>>> +   if (error)
>>> +           return error;
>>> +
>>>     pci_restore_state(pci_dev);
>>>     pci_pme_restore(pci_dev);
>>>     return 0;
>>
> 
> 
> 
>