linux-kernel - Re: [RFC][PATCH 2/2] time: alarmtimer: Use TASK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 1 Mar 2023 16:47:48 -0800
From:   John Stultz <jstultz@...gle.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Michael Nazzareno Trimarchi <michael@...rulasolutions.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Stephen Boyd <sboyd@...nel.org>, Arnd Bergmann <arnd@...db.de>,
        Michael <michael@...isi.de>, kernel-team@...roid.com,
        Peter Zijlstra <peterz@...radead.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [RFC][PATCH 2/2] time: alarmtimer: Use TASK_FREEZABLE to cleanup
 freezer handling

On Wed, Mar 1, 2023 at 2:11 PM Thomas Gleixner <tglx@...utronix.de> wrote:
>
> On Mon, Feb 27 2023 at 20:06, John Stultz wrote:
> > On Mon, Feb 27, 2023 at 4:03 PM John Stultz <jstultz@...gle.com> wrote:
> >> > On Mon, Feb 20 2023 at 19:11, Michael Nazzareno Trimarchi wrote:
> >> > +static int alarmtimer_pm_notifier_fn(struct notifier_block *bl, unsigned 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