On Thu, Nov 17, 2022 at 06:01:26PM +0100, Rafael J. Wysocki wrote:
> On Thu, Nov 17, 2022 at 12:28 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > On Wed, Nov 16, 2022 at 01:00:36PM +0100, Rafael J. Wysocki wrote:
> > > On Wed, Nov 16, 2022 at 1:37 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > > On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
> > > > > On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > > > > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > > > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > > > > > devices can support D3 by a combination of the value returned by
> > > > > > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > > > > > >
> > > > > > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > > > > > device it's associated with can support D3. This was introduced
> > > > > > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > > > > > D3 if power managed by ACPI").
> > > > > > > > >
> > > > > > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > > > > > runtime PM in the kernel [2] because of the logic within
> > > > > > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > > > > > device contains power resources.
> > > > > >
> > > > > > Wait. Is this as simple as just recognizing that:
> > > > > >
> > > > > > _PS0 means the OS has a knob to put the device in D0, but it doesn't
> > > > > > mean the device can wake itself from a low-power state. The OS has
> > > > > > to use _S0W to learn the device's ability to wake itself.
> > > > >
> > > > > It is.
> > > >
> > > > Now I'm confused again about what "HotPlugSupportInD3" means. The MS
> > > > web page [1] says it identifies Root Ports capable of handling hot
> > > > plug events while in D3. That sounds kind of related to _S0W: If _S0W
> > > > says "I can wake myself from D3hot and D3cold", how is that different
> > > > from "I can handle hotplug events in D3"?
> > >
> > > For native PME/hot-plug signaling there is no difference. This is the
> > > same interrupt by the spec after all IIRC.
> > >
> > > For GPE-based signaling, though, there is a difference, because GPEs
> > > can only be used directly for wake signaling (this is related to
> > > _PRW). In particular, the only provision in the ACPI spec for device
> > > hot-add are the Bus Check and Device Check notification values (0 and
> > > 1) which require AML to run and evaluate Notify() on specific AML
> > > objects.
> > >
> > > Hence, there is no spec-defined way to tell the OS that "something can
> > > be hot-added under this device while in D3 and you will get notified
> > > about that".
> >
> > So I guess acpi_pci_bridge_d3() looks for:
> >
> > - "wake signaling while in D3" (_S0W) and
> > - "notification of hotplug while in D3" ("HotPlugSupportInD3")
> >
> > For Root Ports with both those abilities (or bridges below such Root
> > Ports), we allow D3, and this patch doesn't change that.
> >
> > What this patch *does* change is that all bridges with _PS0 or _PR0
> > previously could use D3, but now will only be able to use D3 if they
> > are also (or are below) a Root Port that can signal wakeup
> > (wakeup.flags.valid) and can wakeup from D3hot or D3cold (_S0W).
> >
> > And this fixes the Pink Sardine because it has Root Ports that do
> > Thunderbolt tunneling, and they have _PS0 or _PR0 but their _S0W says
> > they cannot wake from D3. Previously we put those in D3, but they
> > couldn't wake up. Now we won't put them in D3.
> >
> > I guess there's a possibility that this could break or cause higher
> > power consumption on systems that were fixed by c6e331312ebf
> > ("PCI/ACPI: Whitelist hotplug ports for D3 if power managed by ACPI").
> > I don't know enough about that scenario. Maybe Lukas will chime in.
>
> Well, it is possible that some of these systems will be affected.
>
> One of such cases is when the port in question has _S0W which says
> that wakeup from D3 is not supported. In that case I think the kernel
> should honor the _S0W input, because there may be a good reason known
> to the platform integrator for it.
>
> The other case is when wakeup.flags.valid is unset for the port's ACPI
> companion which means that the port cannot signal wakeup through
> ACPI-related means at all and this may be problematic, especially in
> the system-wide suspend case in which the wakeup capability is not too
> relevant unless there is a system wakeup device under the port.
>
> I don't think that the adev->wakeup.flags.valid check has any bearing
> on the _S0W check - if there is _S0W and it says "no wakeup from D3",
> it should still be taken into account - so that check can be moved
> past the _S0W check.
So if _S0W says it can wake from D3, but wakeup.flags is not valid,
it's still OK to use D3? I guess in this case we assume wakeup would
be via native PME/hotplug signaling?
> Now, for compatibility with systems where ports have neither _S0W nor
> the HotPlugSupportInD3 property, the acpi_pci_power_manageable()
> return value should determine the outcome regardless of the
> adev->wakeup.flags.valid value, so the latter should only determine
> whether or not the HotPlugSupportInD3 property will be inspected
> (which may cause true to be returned before the "power manageable"
> check).
>
> IOW, something like this (after checking _S0W):
>
> if (adev->wakeup.flags.valid &&
> !acpi_dev_get_property(adev, "HotPlugSupportInD3",
> ACPI_TYPE_INTEGER, &obj) &&
> obj->integer.value == 1)
> return true;
>
> return acpi_pci_power_manageable(dev);
>
> Where the if () condition basically means that wakeup signaling is
> supported (and there is no indication that it cannot be done from D3
> as per the previous _S0W check) and hotplug signaling from D3 is
> supported.
>
> > > > This patch says that if dev's Root Port has "HotPlugSupportInD3", we
> > > > don't need _PS0 or _PR0 for dev. I guess that must be true, because
> > > > previously the fact that we checked for "HotPlugSupportInD3" meant the
> > > > device did NOT have _PS0 or _PR0.
> > > >
> > > > [1] https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-pcie-root-ports-supporting-hot-plug-in-d3
I think you're suggesting the patch below, which will make
acpi_pci_bridge_d3(dev) return "true" if:
- Root Port can wake from D3hot or D3cold, has "HotPlugSupportInD3",
and has wakeup.flags.valid, OR
- Root Port can wake from D3hot or D3cold, and "dev" has _PR0 or
_PS0
Previously, all bridges with _PR0 or _PS0 could use D3; now we also
require that the Root Port's _S0W says it can wake from at least
D3hot.
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index a46fec776ad7..66c9ae1dc5da 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -984,10 +984,6 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
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;
-
rpdev = pcie_find_root_port(dev);
if (!rpdev)
return false;
@@ -996,14 +992,6 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
if (!adev)
return false;
- /*
- * If the Root Port cannot signal wakeup signals at all, i.e., it
- * 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)
- return false;
-
/*
* If the Root Port cannot wake itself from D3hot or D3cold, we
* can't use D3.
@@ -1014,16 +1002,21 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
/*
* The "HotPlugSupportInD3" property in a Root Port _DSD indicates
- * the Port can signal hotplug events while in D3. We assume any
- * bridges *below* that Root Port can also signal hotplug events
- * while in D3.
+ * the Port can signal hotplug events while in D3. This differs
+ * from _S0W because _S0W may rely on GPEs, which can only be used
+ * directly for wake signaling, not hotplug events.
+ *
+ * We assume any bridges *below* that Root Port can also signal
+ * hotplug events while in D3.
*/
- if (!acpi_dev_get_property(adev, "HotPlugSupportInD3",
+ if (adev->wakeup.flags.valid &&
+ !acpi_dev_get_property(adev, "HotPlugSupportInD3",
ACPI_TYPE_INTEGER, &obj) &&
obj->integer.value == 1)
return true;
- return false;
+ /* Assume D3 support if the bridge is power-manageable by ACPI. */
+ return acpi_pci_power_manageable(dev);
}
int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
