On Thu, Aug 17, 2023 at 9:30 PM Limonciello, Mario <mario.limonciello@xxxxxxx> wrote: > > > > 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() Don't even try to go this way, please. Originally, the LPI constraints are there to indicate to Windows whether or not it should attempt to enter Connected/Modern Standby. Because Linux doesn't do Modern Standby, it doesn't use the LPI constraints the way Windows does and it really shouldn't do that. I think that the exercise here is to use the information from the constraints list as an indication whether or not a given PCI Root Port is supposed to be put into D3hot/cold on suspend-to-idle and this has nothing to do with enforcement.