From: Bjorn Helgaas <helgaas@kernel.org>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: linux-pci@vger.kernel.org, "Limonciello,
Mario" <mario.limonciello@amd.com>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Len Brown <lenb@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
Mika Westerberg <mika.westerberg@linux.intel.com>,
Mehta Sanju <Sanju.Mehta@amd.com>, Lukas Wunner <lukas@wunner.de>,
"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
Linux PM <linux-pm@vger.kernel.org>
Subject: Re: [PATCH v4] PCI / ACPI: PM: Take _S0W of the target bridge into account in acpi_pci_bridge_d3(()
Date: Thu, 12 Jan 2023 16:01:57 -0600 [thread overview]
Message-ID: <20230112220157.GA1795768@bhelgaas> (raw)
In-Reply-To: <12155458.O9o76ZdvQC@kreacher>
On Thu, Jan 12, 2023 at 09:51:24PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> It is generally questionable to allow a PCI bridge to go into D3 if
> it has _S0W returning D2 or a shallower power state, so modify
> acpi_pci_bridge_d3(() to always take the return value of _S0W for the
> target bridge into accout. That is, make it return 'false' if _S0W
> returns D2 or a shallower power state for the target bridge regardless
> of its ancestor PCIe Root Port properties. Of course, this also causes
> 'false' to be returned if the PCIe Root Port itself is the target and
> its _S0W returns D2 or a shallower power state.
>
> However, still allow bridges without _S0W that are power-manageable via
> ACPI to enter D3 to retain the current code behavior in that case.
>
> Link: https://lore.kernel.org/linux-pci/20221031223356.32570-1-mario.limonciello@amd.com/
> Reported-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Applied to pci/pm for v6.3, thanks!
It'd be great if we could include a short description of the problems
users might see. I think the original problem was that on some AMD
systems we put a USB4 router in D3 when it should remain in D0. And I
assume this means something doesn't wake up when it should? Or maybe
we miss a hotplug event?
If somebody has an example or some text, I'll add it to the commit
log.
> ---
>
> v3 -> v4:
> * Use ACPI_STATE_D2 in the checks in acpi_pci_bridge_d3().
>
> v2 -> v3:
> * Use rpadev for the ACPI companion of the Root Port in acpi_pci_bridge_d3(()
> to avoid confusion.
> * Make the function evaluating _S0W return the value produced by it or "unknown
> state" on errors and let its caller deal with that value.
>
> ---
> drivers/acpi/device_pm.c | 19 +++++++++++++++++++
> drivers/pci/pci-acpi.c | 45 +++++++++++++++++++++++++++++++--------------
> include/acpi/acpi_bus.h | 1 +
> 3 files changed, 51 insertions(+), 14 deletions(-)
>
> Index: linux-pm/drivers/pci/pci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-acpi.c
> +++ linux-pm/drivers/pci/pci-acpi.c
> @@ -976,24 +976,41 @@ bool acpi_pci_power_manageable(struct pc
> bool acpi_pci_bridge_d3(struct pci_dev *dev)
> {
> struct pci_dev *rpdev;
> - struct acpi_device *adev;
> - acpi_status status;
> - unsigned long long state;
> + struct acpi_device *adev, *rpadev;
> const union acpi_object *obj;
>
> if (acpi_pci_disabled || !dev->is_hotplug_bridge)
> return false;
>
> - /* Assume D3 support if the bridge is power-manageable by ACPI. */
> - if (acpi_pci_power_manageable(dev))
> - return true;
> + adev = ACPI_COMPANION(&dev->dev);
> + if (adev) {
> + /*
> + * If the bridge has _S0W, whether or not it can go into D3
> + * depends on what is returned by that object. In particular,
> + * if the power state returned by _S0W is D2 or shallower,
> + * entering D3 should not be allowed.
> + */
> + if (acpi_dev_power_state_for_wake(adev) <= ACPI_STATE_D2)
> + return false;
> +
> + /*
> + * Otherwise, assume that the bridge can enter D3 so long as it
> + * is power-manageable via ACPI.
> + */
> + if (acpi_device_power_manageable(adev))
> + return true;
> + }
>
> rpdev = pcie_find_root_port(dev);
> if (!rpdev)
> return false;
>
> - adev = ACPI_COMPANION(&rpdev->dev);
> - if (!adev)
> + if (rpdev == dev)
> + rpadev = adev;
> + else
> + rpadev = ACPI_COMPANION(&rpdev->dev);
> +
> + if (!rpadev)
> return false;
>
> /*
> @@ -1001,15 +1018,15 @@ bool acpi_pci_bridge_d3(struct pci_dev *
> * doesn't supply a wakeup GPE via _PRW, it cannot signal hotplug
> * events from low-power states including D3hot and D3cold.
> */
> - if (!adev->wakeup.flags.valid)
> + if (!rpadev->wakeup.flags.valid)
> return false;
>
> /*
> - * If the Root Port cannot wake itself from D3hot or D3cold, we
> - * can't use D3.
> + * In the bridge-below-a-Root-Port case, evaluate _S0W for the Root Port
> + * to verify whether or not it can signal wakeup from D3.
> */
> - status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
> - if (ACPI_SUCCESS(status) && state < ACPI_STATE_D3_HOT)
> + if (rpadev != adev &&
> + acpi_dev_power_state_for_wake(rpadev) <= ACPI_STATE_D2)
> return false;
>
> /*
> @@ -1018,7 +1035,7 @@ bool acpi_pci_bridge_d3(struct pci_dev *
> * bridges *below* that Root Port can also signal hotplug events
> * while in D3.
> */
> - if (!acpi_dev_get_property(adev, "HotPlugSupportInD3",
> + if (!acpi_dev_get_property(rpadev, "HotPlugSupportInD3",
> ACPI_TYPE_INTEGER, &obj) &&
> obj->integer.value == 1)
> return true;
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -484,6 +484,25 @@ void acpi_dev_power_up_children_with_adr
> acpi_dev_for_each_child(adev, acpi_power_up_if_adr_present, NULL);
> }
>
> +/**
> + * acpi_dev_power_state_for_wake - Deepest power state for wakeup signaling
> + * @adev: ACPI companion of the target device.
> + *
> + * Evaluate _S0W for @adev and return the value produced by it or return
> + * ACPI_STATE_UNKNOWN on errors (including _S0W not present).
> + */
> +u8 acpi_dev_power_state_for_wake(struct acpi_device *adev)
> +{
> + unsigned long long state;
> + acpi_status status;
> +
> + status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
> + if (ACPI_FAILURE(status))
> + return ACPI_STATE_UNKNOWN;
> +
> + return state;
> +}
> +
> #ifdef CONFIG_PM
> static DEFINE_MUTEX(acpi_pm_notifier_lock);
> static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
> Index: linux-pm/include/acpi/acpi_bus.h
> ===================================================================
> --- linux-pm.orig/include/acpi/acpi_bus.h
> +++ linux-pm/include/acpi/acpi_bus.h
> @@ -533,6 +533,7 @@ int acpi_bus_update_power(acpi_handle ha
> int acpi_device_update_power(struct acpi_device *device, int *state_p);
> bool acpi_bus_power_manageable(acpi_handle handle);
> void acpi_dev_power_up_children_with_adr(struct acpi_device *adev);
> +u8 acpi_dev_power_state_for_wake(struct acpi_device *adev);
> int acpi_device_power_add_dependent(struct acpi_device *adev,
> struct device *dev);
> void acpi_device_power_remove_dependent(struct acpi_device *adev,
>
>
>
next prev parent reply other threads:[~2023-01-12 22:11 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-31 22:33 [PATCH v5] PCI/ACPI: PCI/ACPI: Validate devices with power resources support D3 Mario Limonciello
2022-11-11 17:41 ` Bjorn Helgaas
2022-11-11 18:58 ` Limonciello, Mario
2022-11-11 21:42 ` Bjorn Helgaas
2022-11-14 15:33 ` Rafael J. Wysocki
2022-11-14 15:37 ` Limonciello, Mario
2022-11-14 16:54 ` Bjorn Helgaas
2022-11-16 0:37 ` Bjorn Helgaas
2022-11-16 2:31 ` Limonciello, Mario
2022-11-16 12:00 ` Rafael J. Wysocki
2022-11-16 23:28 ` Bjorn Helgaas
2022-11-17 17:01 ` Rafael J. Wysocki
2022-11-17 22:16 ` Bjorn Helgaas
2022-11-18 13:16 ` Rafael J. Wysocki
2022-11-18 20:23 ` Bjorn Helgaas
2022-11-18 21:13 ` Rafael J. Wysocki
2022-11-21 14:33 ` Rafael J. Wysocki
2022-11-21 22:17 ` Bjorn Helgaas
2023-01-02 16:34 ` Rafael J. Wysocki
2023-01-02 16:59 ` Rafael J. Wysocki
2023-01-03 22:44 ` Limonciello, Mario
2023-01-10 18:02 ` Rafael J. Wysocki
2023-01-10 20:55 ` Bjorn Helgaas
2023-01-11 10:56 ` Rafael J. Wysocki
2023-01-12 20:13 ` Bjorn Helgaas
2023-01-11 10:38 ` [PATCH v3] PCI / ACPI: PM: Take _S0W of the target bridge into account in acpi_pci_bridge_d3(() Rafael J. Wysocki
2023-01-12 20:21 ` Bjorn Helgaas
2023-01-12 20:31 ` Rafael J. Wysocki
2023-01-12 20:51 ` [PATCH v4] " Rafael J. Wysocki
2023-01-12 22:01 ` Bjorn Helgaas [this message]
2023-01-12 22:09 ` Limonciello, Mario
2023-01-12 22:40 ` Bjorn Helgaas
2023-01-12 22:45 ` Limonciello, Mario
2023-01-13 17:51 ` Bjorn Helgaas
2023-01-13 17:53 ` Limonciello, Mario
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230112220157.GA1795768@bhelgaas \
--to=helgaas@kernel.org \
--cc=Sanju.Mehta@amd.com \
--cc=bhelgaas@google.com \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=mario.limonciello@amd.com \
--cc=mika.westerberg@linux.intel.com \
--cc=rafael.j.wysocki@intel.com \
--cc=rafael@kernel.org \
--cc=rjw@rjwysocki.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).