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.
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index f007116a8427..b90c6cf6faf4 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -967,15 +967,17 @@ EXPORT_SYMBOL_GPL(acpi_pm_set_device_wakeup);
> * @adev: ACPI device node corresponding to @dev.
> * @system_state: System state to choose the device state for.
> */
> -static int acpi_dev_pm_low_power(struct device *dev, struct acpi_device *adev,
> - u32 system_state)
> +static int acpi_dev_pm_low_power(struct acpi_device *adev, void* data)
> {
> int ret, state;
> + u32 *system_state = data;
>
> if (!acpi_device_power_manageable(adev))
> return 0;
>
> - ret = acpi_dev_pm_get_state(dev, adev, system_state, NULL, &state);
> + acpi_dev_for_each_child(adev, acpi_dev_pm_low_power, data);
> +
> + ret = acpi_dev_pm_get_state(&adev->dev, adev, *system_state, NULL, &state);
> return ret ? ret : acpi_device_set_power(adev, state);
> }
>
> @@ -1016,7 +1018,7 @@ int acpi_dev_suspend(struct device *dev, bool wakeup)
> wakeup = false;
> }
>
> - error = acpi_dev_pm_low_power(dev, adev, target_state);
> + error = acpi_dev_pm_low_power(adev, &target_state);
> if (error && wakeup)
> acpi_device_wakeup_disable(adev);
>
> @@ -1386,6 +1388,7 @@ static struct dev_pm_domain acpi_general_pm_domain = {
> static void acpi_dev_pm_detach(struct device *dev, bool power_off)
> {
> struct acpi_device *adev = ACPI_COMPANION(dev);
> + u32 state = ACPI_STATE_S0;
>
> if (adev && dev->pm_domain == &acpi_general_pm_domain) {
> dev_pm_domain_set(dev, NULL);
> @@ -1400,7 +1403,7 @@ static void acpi_dev_pm_detach(struct device
> *dev, bool power_off)
> dev_pm_qos_hide_latency_limit(dev);
> dev_pm_qos_hide_flags(dev);
> acpi_device_wakeup_disable(adev);
> - acpi_dev_pm_low_power(dev, adev, ACPI_STATE_S0);
> + acpi_dev_pm_low_power(adev, &state);
> }
> }
> }
> @@ -1514,4 +1517,16 @@ bool acpi_dev_state_d0(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(acpi_dev_state_d0);
>
> +void acpi_dev_shutdown(struct device *dev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> + u32 state = ACPI_STATE_S5;
> +
> + if (!adev)
> + return;
> +
> + acpi_device_wakeup_disable(adev);
> + acpi_dev_pm_low_power(adev, &state);
> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_shutdown);
> #endif /* CONFIG_PM */
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 6ceaf50f5a67..7e7c99eade63 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -45,6 +45,15 @@ static void __fw_devlink_link_to_consumers(struct
> device *dev);
> static bool fw_devlink_drv_reg_done;
> static bool fw_devlink_best_effort;
>
> +#ifdef CONFIG_ACPI
> +static inline void fw_dev_shutdown(struct device *dev)
> +{
> + acpi_dev_shutdown(dev);
> +}
> +#else
> +static inline void fw_dev_shutdown(struct device *dev) { }
> +#endif
> +
> /**
> * __fwnode_link_add - Create a link between two fwnode_handles.
> * @con: Consumer end of the link.
> @@ -4780,6 +4789,8 @@ void device_shutdown(void)
> dev->driver->shutdown(dev);
> }
>
> + fw_dev_shutdown(dev);
> +
> device_unlock(dev);
> if (parent)
> device_unlock(parent);
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 641dc4843987..374f9eb75c22 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1130,6 +1130,7 @@ int acpi_subsys_runtime_resume(struct device *dev);
> int acpi_dev_pm_attach(struct device *dev, bool power_on);
> bool acpi_storage_d3(struct device *dev);
> bool acpi_dev_state_d0(struct device *dev);
> +void acpi_dev_shutdown(struct device *dev);
> #else
> static inline int acpi_subsys_runtime_suspend(struct device *dev) { return 0; }
> static inline int acpi_subsys_runtime_resume(struct device *dev) { return 0; }
> @@ -1145,6 +1146,7 @@ static inline bool acpi_dev_state_d0(struct device *dev)
> {
> return true;
> }
> +static inline void acpi_dev_shutdown(struct device *dev) { }
> #endif
>
> #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP)
>
