Messages in this thread | | | Date | Thu, 25 Aug 2022 13:29:13 -0500 | Subject | Re: [PATCH v2] ata: ahci: Do not check ACPI_FADT_LOW_POWER_S0 | From | "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@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.
| |