lkml.org 
[lkml]   [2023]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices
From


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
>>

\
 
 \ /
  Last update: 2023-08-17 21:31    [W:0.080 / U:0.728 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site