diff mbox series

[RFC,1/2] time: alarmtimer: Fix erroneous case of using 0 as an "invalid" initialization value

Message ID 20230211064527.3481754-1-jstultz@google.com
State New
Headers
Series [RFC,1/2] time: alarmtimer: Fix erroneous case of using 0 as an "invalid" initialization value |

Commit Message

John Stultz Feb. 11, 2023, 6:45 a.m. UTC 
  Michael reported seeing an error where alarmtimers would
occasionally not wake the system up.

It was found that in alarmtimer_suspend() it was exiting via
the:
    if (min == 0)
        return 0;
check. This logic was from one of the early versions of the
original alarmtimer patch, where we initialized min to 0, and
then this check would exit early if we found no timers to expire
(leaving min still at 0).

However, its possible for an alarmtimer to expire as we are
checking it, leaving the calculated delta to be zero, and thus
setting min to zero.

This is the result of my using 0 as an invalid time value which
is clearly erroneous. Instead KTIME_MAX should have been used.

This patch, split out from a change originally suggested by
Thomas Gleixner, changes the logic to instead use KTIME_MAX.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Michael <michael@mipisi.de>
Cc: Michael Trimarchi <michael@amarulasolutions.com>
Cc: kernel-team@android.com
Reported-by: Michael <michael@mipisi.de>
Reported-by: Michael Trimarchi <michael@amarulasolutions.com>
Fixes: ff3ead96d17f ("timers: Introduce in-kernel alarm-timer interface")
Originally-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/lkml/alpine.DEB.2.21.1909021247250.3955@nanos.tec.linutronix.de/
[jstultz: Forward ported to 6.2-rc, and split out just the
          KTIME_MAX change]
Signed-off-by: John Stultz <jstultz@google.com>
---
 kernel/time/alarmtimer.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Michael Nazzareno Trimarchi Feb. 18, 2023, 2:51 p.m. UTC | #1 
On Sat, Feb 11, 2023 at 7:45 AM John Stultz <jstultz@google.com> wrote:
>
> Michael reported seeing an error where alarmtimers would
> occasionally not wake the system up.
>
> It was found that in alarmtimer_suspend() it was exiting via
> the:
>     if (min == 0)
>         return 0;
> check. This logic was from one of the early versions of the
> original alarmtimer patch, where we initialized min to 0, and
> then this check would exit early if we found no timers to expire
> (leaving min still at 0).
>
> However, its possible for an alarmtimer to expire as we are
> checking it, leaving the calculated delta to be zero, and thus
> setting min to zero.
>
> This is the result of my using 0 as an invalid time value which
> is clearly erroneous. Instead KTIME_MAX should have been used.
>
> This patch, split out from a change originally suggested by
> Thomas Gleixner, changes the logic to instead use KTIME_MAX.
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Michael <michael@mipisi.de>
> Cc: Michael Trimarchi <michael@amarulasolutions.com>
> Cc: kernel-team@android.com
> Reported-by: Michael <michael@mipisi.de>
> Reported-by: Michael Trimarchi <michael@amarulasolutions.com>
> Fixes: ff3ead96d17f ("timers: Introduce in-kernel alarm-timer interface")
> Originally-by: Thomas Gleixner <tglx@linutronix.de>
> Link: https://lore.kernel.org/lkml/alpine.DEB.2.21.1909021247250.3955@nanos.tec.linutronix.de/
> [jstultz: Forward ported to 6.2-rc, and split out just the
>           KTIME_MAX change]
> Signed-off-by: John Stultz <jstultz@google.com>
> ---
>  kernel/time/alarmtimer.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index 5897828b9d7e..f7b2128f64e2 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -251,7 +251,7 @@ static int alarmtimer_suspend(struct device *dev)
>         min = freezer_delta;
>         expires = freezer_expires;
>         type = freezer_alarmtype;
> -       freezer_delta = 0;
> +       freezer_delta = KTIME_MAX;
>         spin_unlock_irqrestore(&freezer_delta_lock, flags);
>
>         rtc = alarmtimer_get_rtcdev();
> @@ -271,13 +271,14 @@ static int alarmtimer_suspend(struct device *dev)
>                 if (!next)
>                         continue;
>                 delta = ktime_sub(next->expires, base->get_ktime());
> -               if (!min || (delta < min)) {
> +               if (delta < min) {
>                         expires = next->expires;
>                         min = delta;
>                         type = i;
>                 }
>         }
> -       if (min == 0)
> +       /* No timers to expire */
> +       if (min == KTIME_MAX)
>                 return 0;
>
>         if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
> @@ -503,7 +504,7 @@ static void alarmtimer_freezerset(ktime_t absexp, enum alarmtimer_type type)
>         delta = ktime_sub(absexp, base->get_ktime());
>
>         spin_lock_irqsave(&freezer_delta_lock, flags);
> -       if (!freezer_delta || (delta < freezer_delta)) {
> +       if (delta < freezer_delta) {
>                 freezer_delta = delta;
>                 freezer_expires = absexp;
>                 freezer_alarmtype = type;
> --
> 2.39.1.581.gbfd45094c4-goog
>

Tested-by: Michael Trimarchi <michael@amarulasolutions.com>

I don't find regression on this

Michael
diff mbox series

Patch

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 5897828b9d7e..f7b2128f64e2 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -251,7 +251,7 @@  static int alarmtimer_suspend(struct device *dev)
 	min = freezer_delta;
 	expires = freezer_expires;
 	type = freezer_alarmtype;
-	freezer_delta = 0;
+	freezer_delta = KTIME_MAX;
 	spin_unlock_irqrestore(&freezer_delta_lock, flags);
 
 	rtc = alarmtimer_get_rtcdev();
@@ -271,13 +271,14 @@  static int alarmtimer_suspend(struct device *dev)
 		if (!next)
 			continue;
 		delta = ktime_sub(next->expires, base->get_ktime());
-		if (!min || (delta < min)) {
+		if (delta < min) {
 			expires = next->expires;
 			min = delta;
 			type = i;
 		}
 	}
-	if (min == 0)
+	/* No timers to expire */
+	if (min == KTIME_MAX)
 		return 0;
 
 	if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
@@ -503,7 +504,7 @@  static void alarmtimer_freezerset(ktime_t absexp, enum alarmtimer_type type)
 	delta = ktime_sub(absexp, base->get_ktime());
 
 	spin_lock_irqsave(&freezer_delta_lock, flags);
-	if (!freezer_delta || (delta < freezer_delta)) {
+	if (delta < freezer_delta) {
 		freezer_delta = delta;
 		freezer_expires = absexp;
 		freezer_alarmtype = type;