Messages in this thread | | | Date | Thu, 3 Aug 2023 23:37:10 -0500 | Subject | Re: [PATCH v9 3/3] PCI/ACPI: Use device constraints to decide PCI target state fallback policy | From | Mario Limonciello <> |
| |
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; >> +} >
| |