| Message ID | 4426478.LvFx2qVVIh@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | ATA: ACPI: Do not check ACPI_FADT_LOW_POWER_S0 | expand |
On 2022/08/24 10:29, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> The patch title should be "ata: ahci: ..." > > 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(). > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/ata/ahci.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > Index: linux-pm/drivers/ata/ahci.c > =================================================================== > --- linux-pm.orig/drivers/ata/ahci.c > +++ linux-pm/drivers/ata/ahci.c > @@ -1610,8 +1610,7 @@ static void ahci_update_initial_lpm_poli > } > > #ifdef CONFIG_ACPI > - if (policy > ATA_LPM_MED_POWER && > - (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) { > + if (policy > ATA_LPM_MED_POWER) { > if (hpriv->cap & HOST_CAP_PART) > policy = ATA_LPM_MIN_POWER_WITH_PARTIAL; > else if (hpriv->cap & HOST_CAP_SSC) > > >
On 8/24/22 18:54, Damien Le Moal wrote: > On 2022/08/24 10:29, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > The patch title should be "ata: ahci: ..." > >> >> 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(). >> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> --- >> drivers/ata/ahci.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> Index: linux-pm/drivers/ata/ahci.c >> =================================================================== >> --- linux-pm.orig/drivers/ata/ahci.c >> +++ linux-pm/drivers/ata/ahci.c >> @@ -1610,8 +1610,7 @@ static void ahci_update_initial_lpm_poli >> } >> >> #ifdef CONFIG_ACPI >> - if (policy > ATA_LPM_MED_POWER && >> - (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) { >> + if (policy > ATA_LPM_MED_POWER) { If making this change, perhaps the #ifdef CONFIG_ACPI can drop too. >> if (hpriv->cap & HOST_CAP_PART) >> policy = ATA_LPM_MIN_POWER_WITH_PARTIAL; >> else if (hpriv->cap & HOST_CAP_SSC) >> >> >> > >
On 2022/08/24 16:55, Mario Limonciello wrote: > On 8/24/22 18:54, Damien Le Moal wrote: >> On 2022/08/24 10:29, Rafael J. Wysocki wrote: >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> The patch title should be "ata: ahci: ..." >> >>> >>> 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(). >>> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> --- >>> drivers/ata/ahci.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> Index: linux-pm/drivers/ata/ahci.c >>> =================================================================== >>> --- linux-pm.orig/drivers/ata/ahci.c >>> +++ linux-pm/drivers/ata/ahci.c >>> @@ -1610,8 +1610,7 @@ static void ahci_update_initial_lpm_poli >>> } >>> >>> #ifdef CONFIG_ACPI >>> - if (policy > ATA_LPM_MED_POWER && >>> - (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) { >>> + if (policy > ATA_LPM_MED_POWER) { > > If making this change, perhaps the #ifdef CONFIG_ACPI can drop too. Indeed. > >>> if (hpriv->cap & HOST_CAP_PART) >>> policy = ATA_LPM_MIN_POWER_WITH_PARTIAL; >>> else if (hpriv->cap & HOST_CAP_SSC) >>> >>> >>> >> >> >
Index: linux-pm/drivers/ata/ahci.c =================================================================== --- linux-pm.orig/drivers/ata/ahci.c +++ linux-pm/drivers/ata/ahci.c @@ -1610,8 +1610,7 @@ static void ahci_update_initial_lpm_poli } #ifdef CONFIG_ACPI - if (policy > ATA_LPM_MED_POWER && - (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) { + if (policy > ATA_LPM_MED_POWER) { if (hpriv->cap & HOST_CAP_PART) policy = ATA_LPM_MIN_POWER_WITH_PARTIAL; else if (hpriv->cap & HOST_CAP_SSC)