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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAAd53p6gHy6geX9PZHOmkuN_cxzAv1RiqoGfg0-kyo5wJEHomA@mail.gmail.com>
Date: Mon, 18 Dec 2023 11:41:32 +0800
From: Kai-Heng Feng <kai.heng.feng@...onical.com>
To: Mario Limonciello <mario.limonciello@....com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	"Rafael J . Wysocki" <rjw@...ysocki.net>, linux-pci@...r.kernel.org, linux-acpi@...r.kernel.org, 
	linux-kernel@...r.kernel.org, mpearson-lenovo@...ebb.ca
Subject: Re: [PATCH 2/2] PCI/portdrv: Place PCIe port hierarchy into D3cold at shutdown

On Fri, Dec 15, 2023 at 12:44 AM Mario Limonciello
<mario.limonciello@....com> wrote:
>
> 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@...onical.com> wrote:
> >>
> >> Hi Mario and Rafael,
> >>
> >> On Thu, Dec 14, 2023 at 2:46 AM Rafael J. Wysocki <rafael@...nel.org> wrote:
> >>>
> >>> On Wed, Dec 13, 2023 at 7:42 PM Mario Limonciello
> >>> <mario.limonciello@....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@....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@...ebb.ca
> >>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@....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.

I agree this is the right approach because Windows is using S4 for its
"Fast Startup" feature.

Is there any historical reason that .power_off and .shutdown are separated?
And, should we take a step further and also unify the .remove
callback? I recall there was a discussion on consolidating .shutdown
and .remove but somehow it didn't take off.

>
> 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.

Sure thing. This can take a while.

Kai-Heng

>
> Thanks,
>