MIME-Version: 1.0
References: <20230211064527.3481754-1-jstultz@google.com>
In-Reply-To: <20230211064527.3481754-1-jstultz@google.com>
From:   Michael Nazzareno Trimarchi <michael@amarulasolutions.com>
Date:   Sat, 18 Feb 2023 15:51:40 +0100
Message-ID: <CAOf5uwncu5Otb+aQJiOL=twv6tPhNxb9-_pKQEkwe1TZX3gNNQ@mail.gmail.com>
Subject: Re: [RFC][PATCH 1/2] time: alarmtimer: Fix erroneous case of using 0
 as an "invalid" initialization value
To:     John Stultz <jstultz@google.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Stephen Boyd <sboyd@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
        Michael <michael@mipisi.de>, kernel-team@android.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com