Received-SPF: pass (google.com: domain of linux-kernel+bounces-47049-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240131191317.2191421-1-pranavpp@google.com>
In-Reply-To: <20240131191317.2191421-1-pranavpp@google.com>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Wed, 31 Jan 2024 21:10:34 +0100
Message-ID: <CAJZ5v0gXsCuVvqynDeuf_NZtyAJ07umx1NUqfFZb25vjNABAfw@mail.gmail.com>
Subject: Re: [PATCH] alarmtimer, PM: suspend: Expose a function from
To: Pranav Prasad <pranavpp@google.com>
Cc: rafael@kernel.org, pavel@ucw.cz, linux-pm@vger.kernel.org, 
	linux-kernel@vger.kernel.org, krossmo@google.com, jstultz@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 31, 2024 at 8:13=E2=80=AFPM Pranav Prasad <pranavpp@google.com>=
 wrote:
>
> Hi!
>
> I am proposing a patch in which I want to return the errno code ETIME
> instead of EBUSY in enter_state() in the kernel suspend flow. Currently,
> EBUSY is returned  when an imminent alarm is pending which is checked in
> alarmtimer_suspend() in alarmtimer.c. The proposed patch series moves the
> check to enter_state() in suspend.c to catch a potential suspend failure
> early in the suspend flow. I want to replace EBUSY with ETIME to make it
> more diagnosable in userspace, and may be more appropriate considering a
> timer is about to expire.
>
> I am reaching out to get an opinion from the
> suspend maintainers if this would act as any potential risk in the suspen=
d
> flow which only has EBUSY, EAGAIN, and EINVAL as return error codes
> currently. This has been developed as part of a patch series, and only th=
e
> patch of interest is below this message. Any feedback or insights would b=
e
> greatly appreciated.
>
> Thank you,
> Pranav Prasad
>
> The alarmtimer driver currently fails suspend attempts when there is an
> alarm pending within the next suspend_check_duration_ns nanoseconds, sinc=
e
> the   system is expected to wake up soon anyway. The entire suspend proce=
ss
> is initiated even though the system will immediately awaken. This process
> includes substantial work before the suspend fails and additional work
> afterwards to undo the failed suspend that was attempted. Therefore on
> battery-powered devices that initiate suspend attempts from userspace, it
> may be advantageous to be able to fail the suspend earlier in the suspend
> flow to avoid power consumption instead of unnecessarily doing extra work=
.
> As one data point, an analysis of a subset of Android devices showed that
> imminent alarms account for roughly 40% of all suspend failures on averag=
e
> leading to unnecessary power wastage.
>
> To facilitate this, expose
> function time_check_suspend_fail() from alarmtimer to be used by the powe=
r
> subsystem to perform the check earlier in the suspend flow. Perform the
> check in enter_state() and return early if an alarm is to be fired in the
> next suspend_check_duration_ns nanoseconds, failing suspend.
>
> Signed-off-by: Pranav Prasad <pranavpp@google.com>
> Signed-off-by: Kelly Rossmoyer <krossmo@google.com>
> ---
>  include/linux/time.h     |   1 +
>  kernel/power/suspend.c   |   3 ++
>  kernel/time/alarmtimer.c | 113 ++++++++++++++++++++++++++++-----------
>  3 files changed, 87 insertions(+), 30 deletions(-)
>
> diff --git a/include/linux/time.h b/include/linux/time.h
> index 16cf4522d6f3..aab7c4e51e11 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -56,6 +56,7 @@ struct tm {
>  };
>
>  void time64_to_tm(time64_t totalsecs, int offset, struct tm *result);
> +int time_check_suspend_fail(void);
>
>  # include <linux/time32.h>
>
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index fa3bf161d13f..7a0175dae0d9 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -26,6 +26,7 @@
>  #include <linux/suspend.h>
>  #include <linux/syscore_ops.h>
>  #include <linux/swait.h>
> +#include <linux/time.h>
>  #include <linux/ftrace.h>
>  #include <trace/events/power.h>
>  #include <linux/compiler.h>
> @@ -564,6 +565,8 @@ static int enter_state(suspend_state_t state)
>  #endif
>         } else if (!valid_state(state)) {
>                 return -EINVAL;
> +       } else if (time_check_suspend_fail()) {
> +               return -ETIME;

This causes a function defined in modular code to be called from
non-modular code which is an obvious mistake.

It also makes the generic suspend code call a function defined in a
random driver, which is a total no-go as far as I am concerned.

Why don't you instead define a PM notifier in the alarmtimer driver
and check if it is going to trigger shortly from there?  PM notifiers
run before the tasks freezer, so there would be a little difference
timing-wise and you can return whatever error code you like from
there.  As an additional benefit, you'd be able to handle hibernation
in the same way.

Thanks!