On Fri, Dec 15, 2023 at 12:44 AM Mario Limonciello
<mario.limonciello@xxxxxxx> 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@xxxxxxxxxxxxx> wrote:
> >>
> >> Hi Mario and Rafael,
> >>
> >> On Thu, Dec 14, 2023 at 2:46 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> >>>
> >>> On Wed, Dec 13, 2023 at 7:42 PM Mario Limonciello
> >>> <mario.limonciello@xxxxxxx> wrote:
> >>>>
> >>>> On 12/13/2023 12:38, Rafael J. Wysocki wrote:
> >>>>> On Wed, Dec 13, 2023 at 7:27 PM Mario Limonciello
> >>>>> <mario.limonciello@xxxxxxx> 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@xxxxxxxxx
> >>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> >>>>>> ---
> >>>>>> 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,
>
