Re: [PATCH 2/2] PCI/portdrv: Place PCIe port hierarchy into D3cold at shutdown

[Mario Limonciello <mario.limonciello@amd.com>]

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,