linux-kernel - Re: [PATCH v2] ata: ahci: Do not check ACPI_FADT_LOW_POWER

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <54e439c8-7390-be01-66b2-5692af571d1b@amd.com>
Date:   Thu, 25 Aug 2022 13:29:13 -0500
From:   "Limonciello, Mario" <mario.limonciello@....com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
        "open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers)" 
        <linux-ide@...r.kernel.org>,
        Linux ACPI <linux-acpi@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] ata: ahci: Do not check ACPI_FADT_LOW_POWER_S0

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