| Message ID | 12054041.O9o76ZdvQC@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] ata: ahci: Do not check ACPI_FADT_LOW_POWER_S0 | expand |
On 8/25/2022 13:47, 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, replace the ACPI_FADT_LOW_POWER_S0 check in > ahci_update_initial_lpm_policy() with pm_suspend_default_s2idle() > which is more general and also takes the user's preference into > account and drop the CONFIG_ACPI #ifdef around it that is not necessary > any more. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> > --- > > v2 -> v3: > * Use pm_suspend_default_s2idle() instead of the check being dropped. > * Update the changelog. > > v1 -> v2: > * Adjust subject (Damien). > * Drop #ifdef CONFIG_ACPI that is not necessary any more (Mario). > * Update the changelog. > > --- > drivers/ata/ahci.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > Index: linux-pm/drivers/ata/ahci.c > =================================================================== > --- linux-pm.orig/drivers/ata/ahci.c > +++ linux-pm/drivers/ata/ahci.c > @@ -1609,15 +1609,12 @@ static void ahci_update_initial_lpm_poli > goto update_policy; > } > > -#ifdef CONFIG_ACPI > - if (policy > ATA_LPM_MED_POWER && > - (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) { > + if (policy > ATA_LPM_MED_POWER && pm_suspend_default_s2idle()) { > if (hpriv->cap & HOST_CAP_PART) > policy = ATA_LPM_MIN_POWER_WITH_PARTIAL; > else if (hpriv->cap & HOST_CAP_SSC) > policy = ATA_LPM_MIN_POWER; > } > -#endif > > update_policy: > if (policy >= ATA_LPM_UNKNOWN && policy <= ATA_LPM_MIN_POWER) > > >
On 8/26/22 03:47, 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, replace the ACPI_FADT_LOW_POWER_S0 check in > ahci_update_initial_lpm_policy() with pm_suspend_default_s2idle() > which is more general and also takes the user's preference into > account and drop the CONFIG_ACPI #ifdef around it that is not necessary > any more. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Applied to for-6.1. Thanks ! > --- > > v2 -> v3: > * Use pm_suspend_default_s2idle() instead of the check being dropped. > * Update the changelog. > > v1 -> v2: > * Adjust subject (Damien). > * Drop #ifdef CONFIG_ACPI that is not necessary any more (Mario). > * Update the changelog. > > --- > drivers/ata/ahci.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > Index: linux-pm/drivers/ata/ahci.c > =================================================================== > --- linux-pm.orig/drivers/ata/ahci.c > +++ linux-pm/drivers/ata/ahci.c > @@ -1609,15 +1609,12 @@ static void ahci_update_initial_lpm_poli > goto update_policy; > } > > -#ifdef CONFIG_ACPI > - if (policy > ATA_LPM_MED_POWER && > - (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) { > + if (policy > ATA_LPM_MED_POWER && pm_suspend_default_s2idle()) { > if (hpriv->cap & HOST_CAP_PART) > policy = ATA_LPM_MIN_POWER_WITH_PARTIAL; > else if (hpriv->cap & HOST_CAP_SSC) > policy = ATA_LPM_MIN_POWER; > } > -#endif > > update_policy: > if (policy >= ATA_LPM_UNKNOWN && policy <= ATA_LPM_MIN_POWER) > > >
Index: linux-pm/drivers/ata/ahci.c =================================================================== --- linux-pm.orig/drivers/ata/ahci.c +++ linux-pm/drivers/ata/ahci.c @@ -1609,15 +1609,12 @@ static void ahci_update_initial_lpm_poli goto update_policy; } -#ifdef CONFIG_ACPI - if (policy > ATA_LPM_MED_POWER && - (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) { + if (policy > ATA_LPM_MED_POWER && pm_suspend_default_s2idle()) { if (hpriv->cap & HOST_CAP_PART) policy = ATA_LPM_MIN_POWER_WITH_PARTIAL; else if (hpriv->cap & HOST_CAP_SSC) policy = ATA_LPM_MIN_POWER; } -#endif update_policy: if (policy >= ATA_LPM_UNKNOWN && policy <= ATA_LPM_MIN_POWER)