From: Rafael J. Wysocki <[email protected]>
If ACPI_FADT_LOW_POWER_S0 is not set, this doesn't mean that low-power
S0 idle is not usable. It merely means that using S3 on the given
system is more beneficial from the energy saving perspective than using
low-power S0 idle, as long as S3 is supported.
Suspend-to-idle is still a valid suspend mode if ACPI_FADT_LOW_POWER_S0
is not set and the pm_suspend_via_firmware() check in pch_wpt_suspend()
is sufficient to distinguish suspend-to-idle from S3, so drop the
confusing ACPI_FADT_LOW_POWER_S0 check.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/thermal/intel/intel_pch_thermal.c | 8 --------
1 file changed, 8 deletions(-)
Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
===================================================================
--- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
+++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
@@ -207,14 +207,6 @@ static int pch_wpt_suspend(struct pch_th
return 0;
}
- /* Do not check temperature if it is not a S0ix capable platform */
-#ifdef CONFIG_ACPI
- if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
- return 0;
-#else
- return 0;
-#endif
-
/* Do not check temperature if it is not s2idle */
if (pm_suspend_via_firmware())
return 0;
On Thu, 2022-07-14 at 21:11 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> If ACPI_FADT_LOW_POWER_S0 is not set, this doesn't mean that low-
> power
> S0 idle is not usable. It merely means that using S3 on the given
> system is more beneficial from the energy saving perspective than
> using
> low-power S0 idle, as long as S3 is supported.
Agreed.
>
> Suspend-to-idle is still a valid suspend mode if
> ACPI_FADT_LOW_POWER_S0
> is not set and the pm_suspend_via_firmware() check in
> pch_wpt_suspend()
> is sufficient to distinguish suspend-to-idle from S3, so drop the
> confusing ACPI_FADT_LOW_POWER_S0 check.
the cooling delay in the suspend callback is to make sure PCH
temperature won't block S0ix during s2idle. So if S0ix is not
supported, it is meaningless to invoke the cooling delay during s2idle.
so the problem is that we don't have an indicator for S0ix capability.
And this also applies to drivers/rtc/rtc-cmos.c, where we use ACPI SCI
for runtime RTC wakeup instead of HPET interrupt on "S0ix capable"
platforms because the HPET timer may block S0ix.
thanks,
rui
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/thermal/intel/intel_pch_thermal.c | 8 --------
> 1 file changed, 8 deletions(-)
>
> Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
> +++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
> @@ -207,14 +207,6 @@ static int pch_wpt_suspend(struct pch_th
> return 0;
> }
>
> - /* Do not check temperature if it is not a S0ix capable
> platform */
> -#ifdef CONFIG_ACPI
> - if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> - return 0;
> -#else
> - return 0;
> -#endif
> -
> /* Do not check temperature if it is not s2idle */
> if (pm_suspend_via_firmware())
> return 0;
>
>
>
On Sun, Jul 17, 2022 at 8:14 AM Zhang Rui <[email protected]> wrote:
>
> On Thu, 2022-07-14 at 21:11 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > If ACPI_FADT_LOW_POWER_S0 is not set, this doesn't mean that low-
> > power
> > S0 idle is not usable. It merely means that using S3 on the given
> > system is more beneficial from the energy saving perspective than
> > using
> > low-power S0 idle, as long as S3 is supported.
>
> Agreed.
>
> >
> > Suspend-to-idle is still a valid suspend mode if
> > ACPI_FADT_LOW_POWER_S0
> > is not set and the pm_suspend_via_firmware() check in
> > pch_wpt_suspend()
> > is sufficient to distinguish suspend-to-idle from S3, so drop the
> > confusing ACPI_FADT_LOW_POWER_S0 check.
>
> the cooling delay in the suspend callback is to make sure PCH
> temperature won't block S0ix during s2idle. So if S0ix is not
> supported, it is meaningless to invoke the cooling delay during s2idle.
But there is no way to determine whether or not S0ix is supported. In
particular, ACPI_FADT_LOW_POWER_S0 is not one.
> so the problem is that we don't have an indicator for S0ix capability.
> And this also applies to drivers/rtc/rtc-cmos.c, where we use ACPI SCI
> for runtime RTC wakeup instead of HPET interrupt on "S0ix capable"
> platforms because the HPET timer may block S0ix.
"S0ix capable" doesn't matter. What matters is whether or not the
current transition under way is into S0 or into suspend-to-idle. In
the latter case there is no reason to avoid doing whatever is done in
the expectation that S0ix may be entered going forward.
> -----Original Message-----
> From: Rafael J. Wysocki <[email protected]>
> Sent: Monday, July 18, 2022 3:39 AM
> To: Zhang, Rui <[email protected]>
> Cc: Rafael J. Wysocki <[email protected]>; Linux PM <linux-
> [email protected]>; Linux ACPI <[email protected]>; LKML <linux-
> [email protected]>; Mario Limonciello <[email protected]>;
> Srinivas Pandruvada <[email protected]>
> Subject: Re: [PATCH] intel: thermal: PCH: Drop ACPI_FADT_LOW_POWER_S0
> check
> Importance: High
>
> On Sun, Jul 17, 2022 at 8:14 AM Zhang Rui <[email protected]> wrote:
> >
> > On Thu, 2022-07-14 at 21:11 +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > If ACPI_FADT_LOW_POWER_S0 is not set, this doesn't mean that low-
> > > power
> > > S0 idle is not usable. It merely means that using S3 on the given
> > > system is more beneficial from the energy saving perspective than
> > > using low-power S0 idle, as long as S3 is supported.
> >
> > Agreed.
> >
> > >
> > > Suspend-to-idle is still a valid suspend mode if
> > > ACPI_FADT_LOW_POWER_S0
> > > is not set and the pm_suspend_via_firmware() check in
> > > pch_wpt_suspend()
> > > is sufficient to distinguish suspend-to-idle from S3, so drop the
> > > confusing ACPI_FADT_LOW_POWER_S0 check.
> >
> > the cooling delay in the suspend callback is to make sure PCH
> > temperature won't block S0ix during s2idle. So if S0ix is not
> > supported, it is meaningless to invoke the cooling delay during s2idle.
>
> But there is no way to determine whether or not S0ix is supported. In
> particular, ACPI_FADT_LOW_POWER_S0 is not one.
>
> > so the problem is that we don't have an indicator for S0ix capability.
> > And this also applies to drivers/rtc/rtc-cmos.c, where we use ACPI SCI
> > for runtime RTC wakeup instead of HPET interrupt on "S0ix capable"
> > platforms because the HPET timer may block S0ix.
>
> "S0ix capable" doesn't matter. What matters is whether or not the current
> transition under way is into S0 or into suspend-to-idle. In the latter case
> there is no reason to avoid doing whatever is done in the expectation that
> S0ix may be entered going forward.
Okay. It is not perfect but we have to live with this.
Acked-by: Zhang Rui <[email protected]>