Re: [PATCH v2 1/2] alarmtimer: Create alarmtimer sysfs to make duration of kernel suspend check configurable

From: John Stultz
Date: Fri Feb 09 2024 - 15:01:42 EST

On Thu, Feb 8, 2024 at 11:56 AM Pranav Prasad <pranavpp@xxxxxxxxxx> wrote:
> Currently, the alarmtimer_suspend does not allow the kernel
> to suspend if the next alarm is within 2 seconds.
> Create alarmtimer sysfs to make the value of 2 seconds configurable.
> This allows flexibility to provide a different value based on the
> type of device running the Linux kernel. As a data point, about 40% of
> kernel suspend failures in a subset of Android devices were due to
> this check. A differently configured value can avoid these suspend
> failures which performs a lot of additional work affecting the
> power consumption of these Android devices.
> Signed-off-by: Pranav Prasad <pranavpp@xxxxxxxxxx>

I might suggest flipping the order of these two patches, as I'm more
wary of UABI changes, so I don't want to hold up the second patch on
interface bike shedding.

> ---
> kernel/time/alarmtimer.c | 61 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 58 insertions(+), 3 deletions(-)
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index 4657cb8e8b1f..e4b88c8dc0e1 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -33,6 +33,8 @@
> #include <trace/events/alarmtimer.h>
> +static const char alarmtimer_group_name[] = "alarmtimer";
> +
> /**
> * struct alarm_base - Alarm timer bases
> * @lock: Lock for syncrhonized access to the base
> @@ -63,6 +65,56 @@ static struct rtc_timer rtctimer;
> static struct rtc_device *rtcdev;
> static DEFINE_SPINLOCK(rtcdev_lock);
> +/* Duration to check for soonest alarm during kernel suspend */
> +static unsigned long suspend_check_duration_ms = 2 * MSEC_PER_SEC;

Naming is hard, but "suspend_check_duration" feels particularly opaque
for a tunable knob.
I can't say I've got a better suggestion off the top of my head, but
this might be something worth thinking a bit more on.

"imminent_alarm_window" maybe? Though that's not obvious it is
connected to suspend, and maybe sounds more urgent than it should.

It might also be nice to provide some more details in the commit
message about why this should be configurable, and how a user of the
interface might choose a proper value to use (including the downsides
of going too far in either direction?).