Re: [RFC] acpi/x86: s2idle: Use constraints to enforce behavior

["Rafael J. Wysocki" <rafael@kernel.org>]

On Fri, Oct 1, 2021 at 6:19 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> Currently s2idle constraints are only verified when pm_debug_messages
> is set.

Very much intentionally.

> Although useful for debugging, it does have a tendency to paper
> over some underlying issues.

What do you mean "paper over"?  It is not expected to do anything
except for providing information.

> Of particular note, I found a case that a system that has two physical
> SATA controllers that share the same ACPI Power Resource.  When a drive is
> connected to one of the controllers then it will bind with PCI devices with
> the ahci driver and form a relationship with the firmware node and physical
> node.  During s2idle I see that the constraints are met for this device as
> it's transitioned into the appropriate state. However the second ACPI node
> doesn't have any relationship with a physical node and so it stays in "D0":

So the debug information is actually useful.

>
> ```
> ACPI: \_SB_.PCI0.GP18.SATA: ACPI: PM: Power state change: D0 -> D3cold
> ACPI: PM: Power resource [P0SA] still in use
> acpi device:2a: Power state changed to D3cold
> ```
>
> Due to the refcounting used on the shared power resource putting the
> device with a physical node into D3 doesn't result in the _OFF method
> being called.
>
> To help this problem, make the following changes to the constraint
> checking:
> 1) Make constraint checking compulsory

That would break things AFAICS due to false-positives resulting from it.

The rule of thumb is to check the constraints only if you don't get
the expected state.

> but gate the output on pm_debug_messages
> 2) As part of constraint checking verify if the ACPI device has a physical
> node or not.
> 3) If a device doesn't have a physical node, but does have a requirement
> set the power state for the device to allow shared resources to transition.

This fixes your particular issue, but is it generally safe?

Also, I think that this needs to be taken care of during system
initialization rather than here.  Device objects without physical
nodes that prevent power resources from being turned off are generally
not useful at all and they prevent the SoC from getting into
lower-power states through active-state power management as well.

> After making this change, here is what the flow looks like:
> ```
> ACPI: \_SB_.PCI0.GP18.SATA: ACPI: PM: Power state change: D0 -> D3cold
> ACPI: PM: Power resource [P0SA] still in use
> acpi device:2a: Power state changed to Dcold
> <snip>
> ACPI: \_SB_.PCI0.GP18.SAT1: LPI: Device is not physically present - forcing transition from D0 to D3cold
> ACPI: \_SB_.PCI0.GP18.SAT1: ACPI: PM: Power state change: D0 -> D3cold
> ACPI: PM: Power resource [P0SA] turned off
> acpi device:2c: Power state changed to D3cold
> ```
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214091
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/x86/s2idle.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index bd92b549fd5a..fbfac40733eb 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -304,14 +304,25 @@ static void lpi_check_constraints(void)
>                         acpi_power_state_string(adev->power.state));
>
>                 if (!adev->flags.power_manageable) {
> -                       acpi_handle_info(handle, "LPI: Device not power manageable\n");
> +                       if (pm_debug_messages_on)
> +                               acpi_handle_info(handle, "LPI: Device not power manageable\n");
>                         lpi_constraints_table[i].handle = NULL;
>                         continue;
>                 }
>
> -               if (adev->power.state < lpi_constraints_table[i].min_dstate)
> -                       acpi_handle_info(handle,
> -                               "LPI: Constraint not met; min power state:%s current power state:%s\n",
> +               if (adev->power.state >= lpi_constraints_table[i].min_dstate)
> +                       continue;
> +               /* make sure that anything with shared resources isn't blocked */
> +               if (!acpi_get_first_physical_node(adev)) {
> +                       if (pm_debug_messages_on)
> +                               acpi_handle_info(handle, "LPI: Device is not physically present - forcing transition from %s to %s\n",
> +                                               acpi_power_state_string(adev->power.state),
> +                                               acpi_power_state_string(ACPI_STATE_D3_COLD));
> +                       acpi_device_set_power(adev, ACPI_STATE_D3_COLD);
> +                       continue;
> +               }
> +               if (pm_debug_messages_on)
> +                       acpi_handle_info(handle, "LPI: Constraint not met; min power state:%s current power state:%s\n",
>                                 acpi_power_state_string(lpi_constraints_table[i].min_dstate),
>                                 acpi_power_state_string(adev->power.state));
>         }
> @@ -446,8 +457,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.25.1
>