Re: [PATCH v2] ata: ahci: Do not check ACPI_FADT_LOW_POWER_S0

[Rafael J. Wysocki]

On Thu, Aug 25, 2022 at 8:17 PM Limonciello, Mario
<mario.limonciello@xxxxxxx> wrote:
>
> On 8/25/2022 13:01, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >
> > The ACPI_FADT_LOW_POWER_S0 flag merely means that it is better to
> > use low-power S0 idle on the given platform than S3 (provided that
> > the latter is supported) and it doesn't preclude using either of
> > them (which of them will be used depends on the choices made by user
> > space).
> >
> > For this reason, there is no benefit from checking that flag in
> > ahci_update_initial_lpm_policy().
> >
> > First off, it cannot be a bug to do S3 with policy set to either
> > ATA_LPM_MIN_POWER_WITH_PARTIAL or ATA_LPM_MIN_POWER, because S3 can be
> > used on systems with ACPI_FADT_LOW_POWER_S0 set and it must work if
> > really supported, so the ACPI_FADT_LOW_POWER_S0 check is not needed to
> > protect the S3-capable systems from failing.
> >
> > Second, suspend-to-idle can be carried out on a system with
> > ACPI_FADT_LOW_POWER_S0 unset and it is expected to work, so if setting
> > policy to either ATA_LPM_MIN_POWER_WITH_PARTIAL or ATA_LPM_MIN_POWER is
> > needed to handle that case correctly, it should be done regardless of
> > the ACPI_FADT_LOW_POWER_S0 value.
> >
> > Accordingly, drop the ACPI_FADT_LOW_POWER_S0 check from
> > ahci_update_initial_lpm_policy() along with the CONFIG_ACPI #ifdef
> > around it that is not necessary any more.
>
> Looking at the source commit for this behavior:
>
> b1a9585cc396 ("ata: ahci: Enable DEVSLP by default on x86 with SLP_S0")
>
> It was trying to set a policy tied to when the system is defaulting to
> suspend to idle.
>
> To try to match the spirit of the original request but not tying it to
> the FADT, how about using pm_suspend_default_s2idle()?

The user can switch to "default S3" later anyway, so this wouldn't
help more than the check being dropped.