| Message ID | 20231213182656.6165-3-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Improvements to system power consumption at S5 | expand |
On Fri, Dec 15, 2023 at 12:44 AM Mario Limonciello <mario.limonciello@amd.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@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. 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, >
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); + } } pcie_port_device_remove(dev);
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. 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(-)