Re: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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




[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux