Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Wed, 31 Jan 2024 21:10:34 +0100 | Subject | Re: [PATCH] alarmtimer, PM: suspend: Expose a function from |
| |
On Wed, Jan 31, 2024 at 8:13 PM 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 suspend > 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 the > patch of interest is below this message. Any feedback or insights would be > 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, since > the system is expected to wake up soon anyway. The entire suspend process > 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 average > leading to unnecessary power wastage. > > To facilitate this, expose > function time_check_suspend_fail() from alarmtimer to be used by the power > 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!
| |