Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <3ed302e4-4287-49be-838b-932ecbdc8d12@amd.com>
Date:   Thu, 14 Dec 2023 10:44:43 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/2] PCI/portdrv: Place PCIe port hierarchy into D3cold at
 shutdown
Content-Language: en-US
To:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc:     Bjorn Helgaas <bhelgaas@google.com>,
        "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        linux-pci@vger.kernel.org, linux-acpi@vger.kernel.org,
        linux-kernel@vger.kernel.org, mpearson-lenovo@squebb.ca
References: <20231213182656.6165-1-mario.limonciello@amd.com>
 <20231213182656.6165-3-mario.limonciello@amd.com>
 <CAJZ5v0gDjwEpx-WNSY0=qchoSGxizsD3XM7Bgq=i0xufBm=Cag@mail.gmail.com>
 <766d621c-695d-4ae7-87cf-690cb8d066df@amd.com>
 <CAJZ5v0i95EeS40pzkBH=jgB1wbMP6SNO_s=pNZ8FPOtcMywgAA@mail.gmail.com>
 <CAAd53p6XynUJimepnXDzcVa4Dps4-F0BNEXxGZh_O38LvSdkkg@mail.gmail.com>
 <CAJZ5v0ijf9-faD-bDaGi9U0JR4iQ68DECyPM8c-AeECOfhQ=Bg@mail.gmail.com>
From:   Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <CAJZ5v0ijf9-faD-bDaGi9U0JR4iQ68DECyPM8c-AeECOfhQ=Bg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YitRZnk4bDF5RHVRTWxabkNRM282UjlEOTQ3ZWlaRWVWaC9WTDRQQWhtTExq?=
 =?utf-8?B?ZVR2YklZbWF6UUlEVkZKUmJ4REVldlNQNmRQRkE5OGgyVXg5OVJyb1VCMnlV?=
 =?utf-8?B?ekc1Tnp1UWxrS0l6WVREUXhadUFFUzFCRmo2Z1MrM0gyUUJVK2RvQnRzU3Bq?=
 =?utf-8?B?N1Y0aStkZ1ovb1NXSG9aUDQxMmN3MTdaWnQreTJEY1NNZy8xSkV0aG8zMWZa?=
 =?utf-8?B?NVlnTmd0dHBzWndrOGpyd2pKSDkzb0doUkI0Vy9tZWVjdkFNbjRCRFNOMFYr?=
 =?utf-8?B?MXFBR1RtbDdLT3IrYnB2dS9SZDZXN1F4bzBmbW9qMGNGU0NpbC94Y2s0SFFo?=
 =?utf-8?B?N0lYZm9mUnMzWkplZWNDU0Iyakc5QnFDQnJUM1htNDFRbWRrVEZ4b2Z4MlRy?=
 =?utf-8?B?d3JlOVVuWG83VGFhZU5hS3d6ZnpHOVh3bE5qMW12VU8rT216dWNpMUFpQms3?=
 =?utf-8?B?SmpmUi9KaGw2S2ZKVTNXNU5WVVd4MzF2c3NSWmZWM3hKQ2lDN1FqNHF5cjk4?=
 =?utf-8?B?b2oyd2wxS1BHakQ3cTRyMzJIem4zdkRLZ1lDZEpHdElSeGYzZHorUWwzOWc1?=
 =?utf-8?B?UlRMN3lNTmxtRXJFcHdvekVPYmtBRktDVk4wYWhTZ2pnSW1rd3VvOGx5cGJL?=
 =?utf-8?B?ejFZMzJxVUdoeHBkZ09JQzgwcGU1WWN1aTBXNEJtcWN5N2JzdVhMYkloRnBQ?=
 =?utf-8?B?aTJmWlZjZmxwZnZiVmt0aW9TY1gwREt3U3JJRGtwOEk0enZMRGR1R1NscjVm?=
 =?utf-8?B?WERsQmpBTzFIZnp1Y05Rdmx4K2E5T09MaFN0cWdiZVQyb3JqOWtBUUZCUUZ3?=
 =?utf-8?B?M3hQNzloK085S1k2WnJ4L2xERjBZT3pJc0piUnlJbEc1M0d1UHJBb2ZObXRy?=
 =?utf-8?B?bElDeEhQaDVuRndyUFRqR25MR1FiTzlWcSttKzRsM2g5dWxvbGlLOWZ1YnJx?=
 =?utf-8?B?QWoxN1pZTlBCK2JKTTJLWHNzZk1jZlZwUXNlSkFrZVJCSGRVMTB1RVJuUWQz?=
 =?utf-8?B?cmpqRU9qakZDQStRYjBXZVN2RDBobyt2S3VvSmpsQmhFd1daN25WVjYwVXdT?=
 =?utf-8?B?Z0ZVK3Q5K3lycmpIOXZuWTkxYWZRRjVDSVlHM1VtNFZOdmVvdWhhRDdXV1VM?=
 =?utf-8?B?TWs3dVJSTk1ESUhyQ1MyYXFWYlVRLzJqTkxpOWxTd3ZTa05NMXJ4MlRzUmxY?=
 =?utf-8?B?dTAwaG50YjFyUVBsVnpIL3QvZTdZMHpBSXI5Wm5SMFA1RlM5WU9iNUFoc1Bh?=
 =?utf-8?B?em0vZk5NV0RqbFlxUW5iMlJKQWhTYmxpZzJEbDhBMXVCcExwYmpTY1Z3RXFS?=
 =?utf-8?B?TC82dlhRUUtiTDlLWTdJdkFDSEZRN1VOWXMwV2YwbFNxVmRFQVpCNzBlLytu?=
 =?utf-8?B?czZWUDBzSlVoeHEybDJzdkc1bVJBVkxqb1A5QVpaV1FoVzV5clVndmo3SnZS?=
 =?utf-8?B?V09UUDRXQUloYTVlVVE1MmoyaEdXdWNuY3d5dzZVc3ZRMG56dzQvVWlmRUFW?=
 =?utf-8?B?MmJoTUZjTXRuellpUWlMQjY2M2UvaWdNdWlkMU5IcjJFa09FNWdLV0FjODdL?=
 =?utf-8?B?MVNmSXNlUkw2TGVvcE1ZWVZmWE52Z0hTR1VmenFXT0lNRjVUd24wT0RtLy9R?=
 =?utf-8?B?b1VoRVY3Y1RiRExFSUt0NTFoVjA2R042N3hVZlJIekNTL1B0NUo3dE1mRHFW?=
 =?utf-8?B?SS92NWpYR0ZBYldQSE55Z3VXWGthT0NGeUMzcU9tMWNFZlZGZDhKNzd2N0dn?=
 =?utf-8?B?bUZ0NFZTMlVlbXZzaHk2OVJlbWw4YlJ0ZVZPLy9wWndMSE1pTXVqeHdCMlRr?=
 =?utf-8?B?TEdhNWdEYmgwVjE0Q3VvMVE5dzlhWjhxUmwwa2dYZTdCZStmYURvajMzVVFl?=
 =?utf-8?B?aEp6bFR3LzlGVm1WZ0FCLzFwODFxeGRkZlN0TEpYMFI5WUVJMnozZHphMmtu?=
 =?utf-8?B?WVk1R09Gblp4V2RBdDJMMkFjck1TQ2FOSGR2YThYTThWUnlyWStjMnVzOVJP?=
 =?utf-8?B?RlZxUkRKYmtUQ0hBbzQwOW1USmg5dSswQU00dFV5V2NzN29oUkZiWmJERmUy?=
 =?utf-8?B?U0ZpazRVcEtmZC9YdnlndGVVd3l5Q1YyTEZPcDdlNjJvdWRJMjl4cVRpeFV6?=
 =?utf-8?Q?yf0ldTHVKJWxMwHWIoSLC7PfL?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7c4597be-ed62-4ba3-f643-08dbfcc3fa67
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Dec 2023 16:44:45.4305
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: A6UP5RKxHa7QTyuTo+u7/FiK9EYMrJM+ZVqEeINUkajrekJruFLVsDkzXYQiiI5NyVoDEX3zCCa4eCbn6bbMjQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB9250
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Thu, 14 Dec 2023 08:44:58 -0800 (PST)

On 12/14/2023 03:00, Rafael J. Wysocki wrote:
> On Thu, Dec 14, 2023 at 4:46 AM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
>>
>> Hi Mario and Rafael,
>>
>> On Thu, Dec 14, 2023 at 2:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>
>>> On Wed, Dec 13, 2023 at 7:42 PM Mario Limonciello
>>> <mario.limonciello@amd.com> wrote:
>>>>
>>>> On 12/13/2023 12:38, Rafael J. Wysocki wrote:
>>>>> On Wed, Dec 13, 2023 at 7:27 PM Mario Limonciello
>>>>> <mario.limonciello@amd.com> wrote:
>>>>>>
>>>>>> When a system is being powered off it's important that PCIe ports
>>>>>> have been put into D3cold as there is no other software to turn
>>>>>> off the devices at S5.
>>>>>>
>>>>>> If PCIe ports are left in D0 then any GPIOs toggled by the ACPI
>>>>>> power resources may be left enabled and devices may consume excess
>>>>>> power.
>>>>>
>>>>> Isn't that a platform firmware issue?
>>>>>
>>>>> It is the responsibility of the platform firmware to properly put the
>>>>> platform into S5, including power removal from devices that are not
>>>>> armed for power-on.
>>>>
>>>> The specific issues that triggered this series were tied to the PCIe
>>>> ports for dGPUs.  There is a GPIO that is toggled by _ON or _OFF.
>>>>
>>>> Windows calls _OFF as part of S5..
>>>
>>> I see.
>>>
>>>>>
>>>>>> Cc: mpearson-lenovo@squebb.ca
>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>> ---
>>>>>>    drivers/pci/pcie/portdrv.c | 11 ++++++++---
>>>>>>    1 file changed, 8 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
>>>>>> index 14a4b89a3b83..08238680c481 100644
>>>>>> --- a/drivers/pci/pcie/portdrv.c
>>>>>> +++ b/drivers/pci/pcie/portdrv.c
>>>>>> @@ -734,9 +734,14 @@ static void pcie_portdrv_remove(struct pci_dev *dev)
>>>>>>    static void pcie_portdrv_shutdown(struct pci_dev *dev)
>>>>>>    {
>>>>>>           if (pci_bridge_d3_possible(dev)) {
>>>>>> -               pm_runtime_forbid(&dev->dev);
>>>>>> -               pm_runtime_get_noresume(&dev->dev);
>>>>>> -               pm_runtime_dont_use_autosuspend(&dev->dev);
>>>>>> +               /* whole hierarchy goes into a low power state for S5 */
>>>>>> +               if (system_state == SYSTEM_POWER_OFF) {
>>>>>> +                       pci_set_power_state(dev, PCI_D3cold);
>>>>>> +               } else {
>>>>>> +                       pm_runtime_forbid(&dev->dev);
>>>>>> +                       pm_runtime_get_noresume(&dev->dev);
>>>>>> +                       pm_runtime_dont_use_autosuspend(&dev->dev);
>>>>>> +               }
>>>>>>           }
>>>>>
>>>>> Wouldn't it be better to remove power from the port after running the
>>>>> code below?
>>>>>
>>>>
>>>> Yes; I think you're right.  I'll do some more testing with this.
>>>>
>>>>>>           pcie_port_device_remove(dev);
>>>>>> --
>>>
>>> IIRC, to do this all properly, you'd need to rework the shutdown path
>>> to look like the hibernation power-off one.  Or even use the latter
>>> for shutdown?
>>>
>>> There was no reason to do that till now, so it has not been done, but
>>> it looks like you have one.
>>>
>>
>> I am working on exactly same thing but with a different approach.
>> Because this is needed for more than just PCI devices.
>> I haven't written a proper commit message yet, but the implementation
>> is quite simple:
> 
> As I said, doing this properly requires something like the hibernation
> power-off transition to be carried out for S5.
> 
> I think that the existing hibernation power-off code can be used as-is
> for this purpose even.
> 

I feel Rafael is right here that unifying the hibernation and shutdown 
paths is the right direction.  Our team just double checked the 
"unpatched" Linux S4 measurements on a system that otherwise had 
problems with S5 and they show the same decreases in power my patch 
series showed.

KH,

I'm going to be OOO for a while with the holidays around the corner and 
some personal time.  If you end up working on some patches to unify the 
S4/S5 codepaths CC me on them and I'll look when I'm back from my leave.

Thanks,