Messages in this thread | | | Date | Thu, 17 Aug 2023 14:30:02 -0500 | Subject | Re: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices | From | "Limonciello, Mario" <> |
| |
On 8/17/2023 2:25 PM, Bjorn Helgaas wrote: > 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.
Based on this, kernel robot sparse complaints and your comments on v11's last patch I am going to split off to another function that returns the pci_power_t state based upon the situation and better comment the reason for the D0 when not enabled.
> >> + (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. >
Great feedback, thanks. I'm thinking to instead change it to:
lpi_enforce_constraints()
>> +} >> + >> 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 >>
| |