diff mbox series

PM: s2idle: Don't allow s2idle when cpuidle isn't supported

Message ID 20230214215003.70683-1-kazukih0205@gmail.com
State New
Headers
Series PM: s2idle: Don't allow s2idle when cpuidle isn't supported |

Commit Message

Kazuki Hashimoto Feb. 14, 2023, 9:50 p.m. UTC 
  s2idle isn't supported on platforms that don't support cpuidle as of
31a3409065d1 ("cpuidle / sleep: Do sanity checks in cpuidle_enter_freeze()
too"), so update the suspend framework to reflect this in order to avoid
breakages.

Link: https://lore.kernel.org/all/20230204152747.drte4uitljzngdt6@kazuki-mac
Fixes: 31a3409065d1 ("cpuidle / sleep: Do sanity checks in cpuidle_enter_freeze() too")
Signed-off-by: Kazuki Hashimoto <kazukih0205@gmail.com>
---
 kernel/power/suspend.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Hector Martin Feb. 21, 2023, 3:44 a.m. UTC | #1 
On 15/02/2023 06.50, Kazuki Hashimoto wrote:
> s2idle isn't supported on platforms that don't support cpuidle as of
> 31a3409065d1 ("cpuidle / sleep: Do sanity checks in cpuidle_enter_freeze()
> too"), so update the suspend framework to reflect this in order to avoid
> breakages.
> 
> Link: https://lore.kernel.org/all/20230204152747.drte4uitljzngdt6@kazuki-mac
> Fixes: 31a3409065d1 ("cpuidle / sleep: Do sanity checks in cpuidle_enter_freeze() too")
> Signed-off-by: Kazuki Hashimoto <kazukih0205@gmail.com>
> ---
>  kernel/power/suspend.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 3f436282547c..27507ae7c9c9 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -556,6 +556,12 @@ static int enter_state(suspend_state_t state)
>  
>  	trace_suspend_resume(TPS("suspend_enter"), state, true);
>  	if (state == PM_SUSPEND_TO_IDLE) {
> +		struct cpuidle_device *dev = cpuidle_get_device();
> +		struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
> +
> +		if (cpuidle_not_available(drv, dev))
> +			return -EINVAL;
> +
>  #ifdef CONFIG_PM_DEBUG
>  		if (pm_test_level != TEST_NONE && pm_test_level <= TEST_CPUS) {
>  			pr_warn("Unsupported test mode for suspend to idle, please choose none/freezer/devices/platform.\n");

Well... this turns s2idle from "slighly broken" to "fully broken". I'm
not sure that's a good idea.

If this (or something equivalent) goes in, we'll have to carry a revert
in the Asahi tree until the PSCI-replacement story goes in, because lots
of people are using the current somewhat broken s2idle. It's a lot
better than having no sleep modes at all.

Also, if you intend to disable s2idle on these platforms, you have to
actually stop advertising it. Advertising it and then refusing to enter
s2idle is not acceptable, it means people will get "sleep" buttons and
expect sleep to work and then it won't. But then that could also
introduce race conditions with userspace checking for sleep support
before the cpuidle driver loads. So there is definitely a can of worms here.

- Hector
Rafael J. Wysocki Feb. 21, 2023, 3:40 p.m. UTC | #2 
On Tue, Feb 14, 2023 at 11:04 PM Kazuki Hashimoto <kazukih0205@gmail.com> wrote:
>
> s2idle isn't supported on platforms that don't support cpuidle as of
> 31a3409065d1 ("cpuidle / sleep: Do sanity checks in cpuidle_enter_freeze()
> too"), so update the suspend framework to reflect this in order to avoid
> breakages.

Hmm.  Doesn't the cpuidle_not_available() check in cpuidle_idle_call()
trigger then?

The commit mentioned above hasn't changed that AFAICS.
Kazuki Hashimoto March 15, 2023, 10:07 p.m. UTC | #3 
On Tue, Feb 21, 2023 at 04:40:01PM +0100, Rafael J. Wysocki wrote:
> On Tue, Feb 14, 2023 at 11:04 PM Kazuki Hashimoto <kazukih0205@gmail.com> wrote:
> >
> > s2idle isn't supported on platforms that don't support cpuidle as of
> > 31a3409065d1 ("cpuidle / sleep: Do sanity checks in cpuidle_enter_freeze()
> > too"), so update the suspend framework to reflect this in order to avoid
> > breakages.
> 
Hello, apologies for the late reply.
> Hmm.  Doesn't the cpuidle_not_available() check in cpuidle_idle_call()
> trigger then?
> 
> The commit mentioned above hasn't changed that AFAICS.
Yes it does, but there's nothing in enter_state() that's preventing the
system from entering s2idle. Both the suspend and cpuidle subsystems
need to be aware that the system is entering s2idle, otherwise it'll
result in breakges.

Thanks,
Kazuki
diff mbox series

Patch

diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 3f436282547c..27507ae7c9c9 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -556,6 +556,12 @@  static int enter_state(suspend_state_t state)
 
 	trace_suspend_resume(TPS("suspend_enter"), state, true);
 	if (state == PM_SUSPEND_TO_IDLE) {
+		struct cpuidle_device *dev = cpuidle_get_device();
+		struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+
+		if (cpuidle_not_available(drv, dev))
+			return -EINVAL;
+
 #ifdef CONFIG_PM_DEBUG
 		if (pm_test_level != TEST_NONE && pm_test_level <= TEST_CPUS) {
 			pr_warn("Unsupported test mode for suspend to idle, please choose none/freezer/devices/platform.\n");