| Message ID | 20230816204143.66281-10-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix wakeup problems on some AMD platforms | expand |
Hi Mario, kernel test robot noticed the following build warnings: [auto build test WARNING on 2ccdd1b13c591d306f0401d98dedc4bdcd02b421] url: https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/ACPI-Add-comments-to-clarify-some-ifdef-statements/20230817-061259 base: 2ccdd1b13c591d306f0401d98dedc4bdcd02b421 patch link: https://lore.kernel.org/r/20230816204143.66281-10-mario.limonciello%40amd.com patch subject: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices config: i386-randconfig-i063-20230817 (https://download.01.org/0day-ci/archive/20230817/202308171239.xQFwhccA-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce: (https://download.01.org/0day-ci/archive/20230817/202308171239.xQFwhccA-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308171239.xQFwhccA-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/acpi/x86/s2idle.c:306:67: sparse: sparse: restricted pci_power_t degrades to integer >> drivers/acpi/x86/s2idle.c:306:45: sparse: sparse: incorrect type in initializer (different base types) @@ expected restricted pci_power_t [usertype] target @@ got int @@ drivers/acpi/x86/s2idle.c:306:45: sparse: expected restricted pci_power_t [usertype] target drivers/acpi/x86/s2idle.c:306:45: sparse: got int drivers/acpi/x86/s2idle.c:312:13: sparse: sparse: restricted pci_power_t degrades to integer >> drivers/acpi/x86/s2idle.c:318:17: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected int state @@ got restricted pci_power_t [usertype] current_state @@ drivers/acpi/x86/s2idle.c:318:17: sparse: expected int state drivers/acpi/x86/s2idle.c:318:17: sparse: got restricted pci_power_t [usertype] current_state >> drivers/acpi/x86/s2idle.c:318:17: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected int state @@ got restricted pci_power_t [usertype] target @@ drivers/acpi/x86/s2idle.c:318:17: sparse: expected int state drivers/acpi/x86/s2idle.c:318:17: sparse: got restricted pci_power_t [usertype] target drivers/acpi/x86/s2idle.c:522:13: sparse: sparse: restricted suspend_state_t degrades to integer drivers/acpi/x86/s2idle.c:522:33: sparse: sparse: restricted suspend_state_t degrades to integer vim +306 drivers/acpi/x86/s2idle.c 303 304 static void lpi_check_pci_dev(struct lpi_constraints *entry, struct pci_dev *pdev) 305 { > 306 pci_power_t target = entry->enabled ? entry->min_dstate : PCI_D0; 307 308 if (pdev->current_state == target) 309 return; 310 311 /* constraint of ACPI D3hot means PCI D3hot _or_ D3cold */ 312 if (target == ACPI_STATE_D3_HOT && 313 (pdev->current_state == PCI_D3hot || 314 pdev->current_state == PCI_D3cold)) 315 return; 316 317 if (pm_debug_messages_on) > 318 acpi_handle_info(entry->handle, 319 "LPI: PCI device in %s, not in %s\n", 320 acpi_power_state_string(pdev->current_state), 321 acpi_power_state_string(target)); 322 323 /* don't try with things that PCI core hasn't touched */ 324 if (pdev->current_state == PCI_UNKNOWN) { 325 entry->handle = NULL; 326 return; 327 } 328 329 pci_set_power_state(pdev, target); 330 } 331
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 >
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 >>
On Thu, Aug 17, 2023 at 9:25 PM Bjorn Helgaas <helgaas@kernel.org> 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. > > > + (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. That's correct, it is not expected to have 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. It seems that we are entering bikeshedding territory here ...
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.
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);
On Thu, Aug 17, 2023 at 9:40 PM Limonciello, Mario <mario.limonciello@amd.com> wrote: > > > > 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); I think that this is going to regress things in the field. I agree with replacing and/or amending the dmi_get_bios_year() check in pci_bridge_d3_possible() with the information from the constraints list, but I don't agree with using it for pretty much anything else that may affect functionality.
diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c index 8bda45579d44a..5192a7147655d 100644 --- a/drivers/acpi/x86/s2idle.c +++ b/drivers/acpi/x86/s2idle.c @@ -16,6 +16,7 @@ */ #include <linux/acpi.h> +#include <linux/pci.h> #include <linux/device.h> #include <linux/dmi.h> #include <linux/suspend.h> @@ -300,28 +301,61 @@ static void lpi_device_get_constraints(void) ACPI_FREE(out_obj); } +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 && + (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); +} + 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)
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 [1]. When devices are not considered power manageable; specs are ambiguous as to what should happen. Linux puts PCIe ports into D3 due to the above described policy. Windows 11 uses the PoFX framework to let the an SOC specific Power Engine Plugin (PEP) [2] [3] [4] to decide what to do with those devices. Specifically Microsoft documentation says: ``` Devices that are integrated into the SoC can be power-managed through the Windows Power Framework (PoFx). These framework-integrated devices are power-managed by PoFx through a SoC-specific power engine plug-in (microPEP) that knows the specifics of the SoC's power and clock controls. ``` Effectively this causes PCIe root ports on a variety of AMD systems to be put into D0 on Windows 11 but D3 on Linux. Instead of only using constraints for debugging messages use them to enforce that PCI devices have been put into the expected state when the system is being put into s2idle. * If a device constraint is present but disabled then choose D0. * If a device constraint is present and enabled then use it. Link: https://uefi.org/specs/ACPI/6.5/07_Power_and_Performance_Mgmt.html#device-power-management-objects [1] Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/platform-design-for-modern-standby#low-power-core-silicon-cpu-soc-dram [2] Link: https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf [3] Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/device-power-management#device-power-management-in-windows [4] Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend") Reported-by: Iain Lane <iain@orangesquash.org.uk> Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121 Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- drivers/acpi/x86/s2idle.c | 51 ++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 9 deletions(-)