Re: [PATCH v5] PCI/ACPI: PCI/ACPI: Validate devices with power resources support D3

From: Bjorn Helgaas <helgaas@kernel.org>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: "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-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5] PCI/ACPI: PCI/ACPI: Validate devices with power resources support D3
Date: Tue, 10 Jan 2023 14:55:13 -0600	[thread overview]
Message-ID: <20230110205513.GA1608269@bhelgaas> (raw)
In-Reply-To: <1945994.PYKUYFuaPT@kreacher>

On Mon, Jan 02, 2023 at 05:59:36PM +0100, Rafael J. Wysocki wrote:
> On Monday, January 2, 2023 5:34:19 PM CET Rafael J. Wysocki wrote:
> ...

> > I don't really think that Root Port support is required for a bridge below
> > a Root Port if that bridge itself is power-manageable via ACPI.  Moreover,
> > I don't think that the _S0W of a Root Port has any bearing on devices below
> > it that have their own _S0W.
> > 
> > So what we really want appears to be to evaluate _S0W for the target bridge,
> > regardless of whether or not it is a Root Port, and return 'false' if that
> > produces D2 or a shallower power state.  Otherwise, we can do what we've
> > done so far.

> +bool acpi_dev_no_wakeup_from_d3(struct acpi_device *adev)
> +{
> +	unsigned long long state;
> +	acpi_status status;
> +
> +	status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
> +	return ACPI_SUCCESS(status) && state < ACPI_STATE_D3_HOT;

This returns "false" (i.e., "yes, device can signal wakeup from D3")
if _S0W doesn't exist.  Is that right?

I think this might be less confusing as:

  bool acpi_dev_can_wake_from_d3(struct acpi_device *adev)
  {
    status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
    return ACPI_SUCCESS(status) && state >= ACPI_STATE_D3_HOT;
  }

That would look like this (including minor change to add "rp_adev" to
make it more obviously that it's different from "adev"):

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 97450f4003cc..789a717d4819 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -484,6 +484,22 @@ void acpi_dev_power_up_children_with_adr(struct acpi_device *adev)
 	acpi_dev_for_each_child(adev, acpi_power_up_if_adr_present, NULL);
 }
 
+/**
+ * acpi_dev_can_wake_from_d3 - Check if wakeup signaling from D3 is supported
+ * @adev: ACPI companion of the target device.
+ *
+ * Evaluate _S0W for @adev and return 'true' if it is successful and the power
+ * state returned by it is D3hot or deeper.
+ */
+bool acpi_dev_can_wake_from_d3(struct acpi_device *adev)
+{
+	unsigned long long state;
+	acpi_status status;
+
+	status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
+	return ACPI_SUCCESS(status) && state >= ACPI_STATE_D3_HOT;
+}
+
 #ifdef CONFIG_PM
 static DEFINE_MUTEX(acpi_pm_notifier_lock);
 static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 068d6745bf98..03dbb57047be 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -976,24 +976,37 @@ bool acpi_pci_power_manageable(struct pci_dev *dev)
 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, *rp_adev;
 	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_can_wake_from_d3(adev))
+			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)
+	rp_adev = (rpdev == dev) ? adev : ACPI_COMPANION(&rpdev->dev);
+	if (!rp_adev)
 		return false;
 
 	/*
@@ -1001,15 +1014,14 @@ bool acpi_pci_bridge_d3(struct pci_dev *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 (!rp_adev->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 (rp_adev != adev && !acpi_dev_can_wake_from_d3(rp_adev))
 		return false;
 
 	/*
@@ -1018,7 +1030,7 @@ bool acpi_pci_bridge_d3(struct pci_dev *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(rp_adev, "HotPlugSupportInD3",
 				   ACPI_TYPE_INTEGER, &obj) &&
 	    obj->integer.value == 1)
 		return true;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index cd3b75e08ec3..0d0c6aa367e0 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -533,6 +533,7 @@ int acpi_bus_update_power(acpi_handle handle, int *state_p);
 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);
+bool acpi_dev_can_wake_from_d3(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,
