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

["Limonciello, Mario" <mario.limonciello@amd.com>]

On 8/25/2022 13:26, Rafael J. Wysocki wrote:
> On Thu, Aug 25, 2022 at 8:17 PM Limonciello, Mario
> <mario.limonciello@amd.com> wrote:
>>
>> On 8/25/2022 13:01, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> 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.