Re: [PATCH v9 3/3] PCI/ACPI: Use device constraints to decide PCI target state fallback policy
From: Mario Limonciello
Date: Fri Aug 04 2023 - 00:37:33 EST
On 8/3/23 23:30, Andy Shevchenko wrote:
On Thu, Aug 03, 2023 at 08:02:29PM -0500, Mario Limonciello wrote:
Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
PCIe ports from modern machines (>=2015) are allowed to be put into D3 by
storing a value to the `bridge_d3` variable in the `struct pci_dev`
structure.
pci_power_manageable() uses this variable to indicate a PCIe port can
enter D3.
pci_pm_suspend_noirq() uses the return from pci_power_manageable() to
decide whether to try to put a device into its target state for a sleep
cycle via pci_prepare_to_sleep().
For devices that support D3, the target state is selected by this policy:
1. If platform_pci_power_manageable():
Use platform_pci_choose_state()
2. If the device is armed for wakeup:
Select the deepest D-state that supports a PME.
3. Else:
Use D3hot.
Devices are considered power manageable by the platform when they have
one or more objects described in the table in section 7.3 of the ACPI 6.5
specification.
When devices are not considered power manageable; specs are ambiguous as
to what should happen. In this situation Windows 11 leaves PCIe
ports in D0 while Linux puts them into D3 due to the above mentioned
commit.
In Windows systems that support Modern Standby specify hardware
pre-conditions for the SoC to achieve the lowest power state by device
constraints in a SOC specific "Power Engine Plugin" (PEP) [2] [3].
They can be marked as disabled or enabled and when enabled can specify
the minimum power state required for an ACPI device.
When it is ambiguous what should happen, adjust the logic for
pci_target_state() to check whether a device constraint is present
and enabled.
* If power manageable by ACPI use this to get to select target state
* If a device constraint is present but disabled then choose D0
* If a device constraint is present and enabled then use it
* If a device constraint is not present, then continue to existing
logic (if marked for wakeup use deepest state that PME works)
* If not marked for wakeup choose D3hot
...
+/**
+ * acpi_get_lps0_constraint - get any LPS0 constraint for a device
+ * @dev: device to get constraint for
+ *
+ * If a constraint has been specified in the _DSM method for the device,
+ * and the constraint is enabled return it. If the constraint is disabled,
+ * return 0. Otherwise, return -ENODEV.
+ */
I believe you get a kernel-doc warning. Always test kernel doc with
scripts/kernel-doc -v -none -Wall ...your file...
Thanks, will double check these.
...
+/**
+ * acpi_pci_device_constraint - determine if the platform has a contraint for the device
+ * @dev: PCI device to check
+ * @result (out): the constraint specified by the platform
+ *
+ * If the platform has specified a constraint for a device, this function will
+ * return 0 and set @result to the constraint.
+ * Otherwise, it will return an error code.
+ */
Ditto.
...
+int acpi_pci_device_constraint(struct pci_dev *dev, int *result)
+{
+ int constraint;
+
+ constraint = acpi_get_lps0_constraint(&dev->dev);
+ pci_dbg(dev, "ACPI device constraint: %d\n", constraint);
Does it make sense before the below check? Why can we be interested in the
_exact_ negative values? (Note that non-printing is already a sign that either
we don't call this or have negative constraint.)
There are two different negative values that can come up:
-ENODEV or -EINVAL. Both were interesting while coming up with this
series because they mean something different about why a constraint
wasn't selected.
-ENODEV means the constraint wasn't found.
-EINVAL means the constraint was found but something is wrong with the
table parser or the table itself. I found the table parser wasn't
working correctly originaly thanks to this.
Maybe now that I've got it all working you're right and this should go
after the error checking.
+ if (constraint < 0)
+ return constraint;
+ *result = constraint;
+
+ return 0;
+}