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

[Limonciello, Mario]

On 8/25/2022 13:26, Rafael J. Wysocki wrote:
> 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.

Right, they could also change LPM policy to different policy later too 
if they want.

This is just for setting up default policy.  I think if you matched to 
only when pm_suspend_default_s2idle() it would be the least likelihood 
to change this default policy on unsuspecting people upgrading.