Messages in this thread | | | Date | Thu, 17 Aug 2023 14:40:48 -0500 | Subject | Re: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices | From | "Limonciello, Mario" <> |
| |
On 8/17/2023 2:37 PM, Rafael J. Wysocki wrote: > On Thu, Aug 17, 2023 at 9:30 PM Limonciello, Mario > <mario.limonciello@amd.com> 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.
What do you think about me making the changes to pci_prepare_to_sleep()?
Something like this:
@@ -2733,11 +2742,17 @@ int pci_prepare_to_sleep(struct pci_dev *dev) { bool wakeup = device_may_wakeup(&dev->dev); pci_power_t target_state = pci_target_state(dev, wakeup); + pci_power_t constraint; int error;
if (target_state == PCI_POWER_ERROR) return -EIO;
+ /* if platform indicates device constraint for suspend, use it */ + constraint = platform_check_constraint(dev, target_state); + if (constraint != PCI_POWER_ERROR) + target_state = constraint; + pci_enable_wake(dev, target_state, wakeup);
error = pci_set_power_state(dev, target_state);
| |