MIME-Version: 1.0
References: <20230211064527.3481754-1-jstultz@google.com> <20230211064527.3481754-2-jstultz@google.com>
 <CAOf5uwnW1u=nfFnj3C8kCVmhgwRaVh6sHZR1RGnXdbrCNpkGVg@mail.gmail.com>
 <87o7porea9.ffs@tglx> <CAOf5uwmhtQ8GXhMiE-Y3-wgL5=xfjOv0Tpq1vqPB8p=LyZHBmw@mail.gmail.com>
 <CAOf5uwmpayJwpAFzUS6qsCgdpyek1f-2t2t9YNr76vnRjSC8=w@mail.gmail.com>
 <87a618qlcp.ffs@tglx> <CAOf5uw=a2RYYFj+4_WOX+KaF_FCXSsUgfM+T2m2XjVuqKMdooA@mail.gmail.com>
 <87sff0ox1a.ffs@tglx> <CANDhNCrfoiz1WdTs+5B8y+TVv8cn4_J-770=bsPqC9Xe=j14hA@mail.gmail.com>
 <CANDhNCq8_Ly9SOwwxrsRCtATotnxpcmkS+5GCnkFVWOWtXfwKQ@mail.gmail.com> <87h6v4ktp5.ffs@tglx>
In-Reply-To: <87h6v4ktp5.ffs@tglx>
From:   John Stultz <jstultz@google.com>
Date:   Wed, 1 Mar 2023 16:47:48 -0800
Message-ID: <CANDhNCrFWhOBLFWbm+qFFO4H3+VmEjtNLZAxA57EHMuV7YmUdg@mail.gmail.com>
Subject: Re: [RFC][PATCH 2/2] time: alarmtimer: Use TASK_FREEZABLE to cleanup
 freezer handling
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Michael Nazzareno Trimarchi <michael@amarulasolutions.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Stephen Boyd <sboyd@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
        Michael <michael@mipisi.de>, kernel-team@android.com,
        Peter Zijlstra <peterz@infradead.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Mar 1, 2023 at 2:11=E2=80=AFPM Thomas Gleixner <tglx@linutronix.de>=
 wrote:
>
> On Mon, Feb 27 2023 at 20:06, John Stultz wrote:
> > On Mon, Feb 27, 2023 at 4:03=E2=80=AFPM John Stultz <jstultz@google.com=
> wrote:
> >> > On Mon, Feb 20 2023 at 19:11, Michael Nazzareno Trimarchi wrote:
> >> > +static int alarmtimer_pm_notifier_fn(struct notifier_block *bl, uns=
igned long state,
> >> > +                                    void *unused)
> >> > +{
> >> > +       switch (state) {
> >> > +       case PM_HIBERNATION_PREPARE:
> >> > +       case PM_POST_HIBERNATION:
> >> > +               atomic_set(&alarmtimer_wakeup, 0);
> >> > +               break;
> >> > +       }
> >> > +       return NOTIFY_DONE;
> >>
> >> But here, we're setting the alarmtimer_wakeup count to zero if we get
> >> PM_HIBERNATION_PREPARE or  PM_POST_HIBERNATION notifications?
> >> And Michael noted we need to add  PM_SUSPEND_PREPARE and
> >> PM_POST_SUSPEND there for this to seemingly work.
>
> Yup. I missed those when sending out that hack.
>
> > So Thomas's notifier method of zeroing at the begining of suspend and
> > tracking any wakeups after that point makes more sense now. It still
> > feels a bit messy, but I'm not sure there's something better.
>
> I'm not enthused about it either.

That said, it does work. :) In my testing, your approach has been
reliable, so it has that going for it.

> > My only thought is this feels a little bit like its mirroring what the
> > pm_wakeup_event() logic is supposed to do. Should we be adding a
> > pm_wakeup_event() to alarmtimer_fired() to try to prevent suspend from
> > occuring for 500ms or so after an alarmtimer has fired so there is
> > enough time for it to be re-armed if needed?
>
> The question is whether this can be called unconditionally and how that
> interacts with the suspend logic. Rafael?

I took a brief stab at this, and one thing is the test needs to use
the /sys/power/wakeup_count dance before suspending.
However, I still had some cases where the recurring alarmtimer got
lost, so I need to dig a bit more to understand what was going wrong
there.

In the meantime, I'm ok with Thomas' approach, but we probably need
some comment documentation that suggests it might be reworked in a
cleaner way?

thanks
-john