On Wed, Aug 16, 2023 at 03:41:43PM -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.
> ...
> +static void lpi_check_pci_dev(struct lpi_constraints *entry, struct pci_dev *pdev)
> +{
> + pci_power_t target = entry->enabled ? entry->min_dstate : PCI_D0;
> +
> + if (pdev->current_state == target)
> + return;
> +
> + /* constraint of ACPI D3hot means PCI D3hot _or_ D3cold */
> + if (target == ACPI_STATE_D3_HOT &&
ACPI_STATE_D3_HOT is not a valid pci_power_t value.
> + (pdev->current_state == PCI_D3hot ||
> + pdev->current_state == PCI_D3cold))
> + return;
> +
> + if (pm_debug_messages_on)
> + acpi_handle_info(entry->handle,
> + "LPI: PCI device in %s, not in %s\n",
> + acpi_power_state_string(pdev->current_state),
> + acpi_power_state_string(target));
> +
> + /* don't try with things that PCI core hasn't touched */
> + if (pdev->current_state == PCI_UNKNOWN) {
> + entry->handle = NULL;
> + return;
> + }
> +
> + pci_set_power_state(pdev, target);
It doesn't seem logical for a "check_constraints()" function that
takes no parameters and returns nothing to actively set the PCI power
state.
lpi_check_constraints() returns nothing, and from the fact that it was
previously only called when "pm_debug_messages_on", I infer that it
should have no side effects.
IMHO "lpi_check_constraints" is not a great name because "check"
doesn't suggest anything specific about what it does.
"dump_constraints()" -- fine. "log_unmet_constraints()" -- fine
(seems like the original intention of 726fb6b4f2a8 ("ACPI / PM: Check
low power idle constraints for debug only"), which added it.
> +}
> +
> static void lpi_check_constraints(void)
> {
> struct lpi_constraints *entry;
>
> for_each_lpi_constraint(entry) {
> struct acpi_device *adev = acpi_fetch_acpi_dev(entry->handle);
> + struct device *dev;
>
> if (!adev)
> continue;
>
> + /* Check and adjust PCI devices explicitly */
> + dev = acpi_get_first_physical_node(adev);
> + if (dev && dev_is_pci(dev)) {
> + lpi_check_pci_dev(entry, to_pci_dev(dev));
> + continue;
> + }
> + if (!entry->enabled)
> + continue;
> acpi_handle_debug(entry->handle,
> "LPI: required min power state:%s current power state:%s\n",
> acpi_power_state_string(entry->min_dstate),
> acpi_power_state_string(adev->power.state));
>
> - if (!adev->flags.power_manageable) {
> - acpi_handle_info(entry->handle, "LPI: Device not power manageable\n");
> - entry->handle = NULL;
> - continue;
> - }
> -
> - if (adev->power.state < entry->min_dstate)
> + if (pm_debug_messages_on &&
> + adev->flags.power_manageable &&
> + adev->power.state < entry->min_dstate)
> acpi_handle_info(entry->handle,
> "LPI: Constraint not met; min power state:%s current power state:%s\n",
> acpi_power_state_string(entry->min_dstate),
> @@ -512,8 +546,7 @@ int acpi_s2idle_prepare_late(void)
> if (!lps0_device_handle || sleep_no_lps0)
> return 0;
>
> - if (pm_debug_messages_on)
> - lpi_check_constraints();
> + lpi_check_constraints();
>
> /* Screen off */
> if (lps0_dsm_func_mask > 0)
> --
> 2.34.1
>
