Date:   Fri, 24 Feb 2023 11:32:11 +0100
From:   Michael Nazzareno Trimarchi <michael@...rulasolutions.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     John Stultz <jstultz@...gle.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

Hi

On Fri, Feb 24, 2023 at 11:02 AM Michael Nazzareno Trimarchi
<michael@...rulasolutions.com> wrote:
>
> Hi Thomas
>
> On Tue, Feb 21, 2023 at 8:10 AM Michael Nazzareno Trimarchi
> <michael@...rulasolutions.com> wrote:
> >
> > Hi
> >
> > On Tue, Feb 21, 2023 at 1:12 AM Thomas Gleixner <tglx@...utronix.de> wrote:
> > >
> > > Michael!
> > >
> > > On Mon, Feb 20 2023 at 22:32, Michael Nazzareno Trimarchi wrote:
> > > > On Mon, Feb 20, 2023 at 10:18 PM Thomas Gleixner <tglx@...utronix.de> wrote:
> > > >>   * alarmtimer_fired - Handles alarm hrtimer being fired.
> > > >> @@ -194,6 +196,8 @@ static enum hrtimer_restart alarmtimer_f
> > > >>         int ret = HRTIMER_NORESTART;
> > > >>         int restart = ALARMTIMER_NORESTART;
> > > >>
> > > >> +       atomic_inc(&alarmtimer_wakeup);
> > > >> +
> > > >
> > > >        ptr->it_active = 0;
> > > >         if (ptr->it_interval) {
> > > >                 atomic_inc(&alarmtimer_wakeup);
> > > >                 si_private = ++ptr->it_requeue_pending;
> > > >         }
> > > >
> > > > Should I not go to the alarm_handle_timer? and only if it's a periodic
> > > > one?
> > >
> > > Why?
> > >
> >
> > You are right. I will pay more attention to my reply.
> >
>
> I get time to test it and if the system suspend to ram we need to catch:
>
> case PM_SUSPEND_PREPARE:
> case PM_POST_SUSPEND:
>
> Michael
>
> > Michael
> >
> > > Any alarmtimer which hits that window has exactly the same problem.
> > >
> > > It's not restricted to periodic timers. Why would a dropped one-shot
> > > wakeup be acceptable?
> > >
> > > It's neither restricted to posix timers. If a clock_nanosleep(ALARM)
> > > expires in that window then the task wake up will just end up in the
> > > /dev/null bucket for the very same reason. Why would this be correct?
> > >
> > > Hmm?
> > >
> > > <GRMBL>
> > > > Michael
> > > >
> > > >>         spin_lock_irqsave(&base->lock, flags);
> > >
[snip]

I have something like this

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index b68cb7f02a6b..b5f15e7f76cb 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -26,6 +26,7 @@
 #include <linux/freezer.h>
 #include <linux/compat.h>
 #include <linux/module.h>
+#include <linux/suspend.h>
 #include <linux/time_namespace.h>

 #include "posix-timers.h"
@@ -176,6 +177,7 @@ static void alarmtimer_dequeue(struct alarm_base
*base, struct alarm *alarm)
     alarm->state &= ~ALARMTIMER_STATE_ENQUEUED;
 }

+static atomic_t alarmtimer_wakeup;

 /**
  * alarmtimer_fired - Handles alarm hrtimer being fired.
@@ -194,6 +196,8 @@ static enum hrtimer_restart
alarmtimer_fired(struct hrtimer *timer)
     int ret = HRTIMER_NORESTART;
     int restart = ALARMTIMER_NORESTART;

+    atomic_inc(&alarmtimer_wakeup);
+
     spin_lock_irqsave(&base->lock, flags);
     alarmtimer_dequeue(base, alarm);
     spin_unlock_irqrestore(&base->lock, flags);
@@ -263,8 +267,18 @@ static int alarmtimer_suspend(struct device *dev)
         }
     }
     /* No timers to expire */
-    if (min == KTIME_MAX)
+    if (min == KTIME_MAX) {
+        /*
+         * Handle wakeups which happened between the start of suspend and
+         * now as those wakeups might have tried to wake up a frozen task
+         * which means they are no longer in the alarm timer list.
+         */
+        if (atomic_read(&alarmtimer_wakeup)) {
+            pm_wakeup_event(dev, 0);
+            return -EBUSY;
+        }
         return 0;
+    }

     if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
         pm_wakeup_event(dev, 2 * MSEC_PER_SEC);
@@ -296,6 +310,30 @@ static int alarmtimer_resume(struct device *dev)
     return 0;
 }

+static int alarmtimer_pm_notifier_fn(struct notifier_block *bl,
unsigned long state,
+                                    void *unused)
+{
+    switch (state) {
+    case PM_POST_SUSPEND:
+        atomic_set(&alarmtimer_wakeup, 0);
+        break;
+    }
+    return NOTIFY_DONE;
+}
+
+static struct notifier_block alarmtimer_pm_notifier = {
+       .notifier_call = alarmtimer_pm_notifier_fn,
+};
+
+static inline int alarmtimer_register_pm_notifier(void)
+{
+       return register_pm_notifier(&alarmtimer_pm_notifier);
+}
+
+static inline void alarmtimer_unregister_pm_notifier(void)
+{
+       unregister_pm_notifier(&alarmtimer_pm_notifier);
+}
 #else
 static int alarmtimer_suspend(struct device *dev)
 {
@@ -306,6 +344,15 @@ static int alarmtimer_resume(struct device *dev)
 {
     return 0;
 }
+
+static inline int alarmtimer_register_pm_notifier(void)
+{
+       return 0;
+}
+
+static inline void alarmtimer_unregister_pm_notifier(void)
+{
+}
 #endif

 static void
@@ -904,11 +951,18 @@ static int __init alarmtimer_init(void)
     if (error)
         return error;

-    error = platform_driver_register(&alarmtimer_driver);
+    error = alarmtimer_register_pm_notifier();
     if (error)
         goto out_if;

+    error = platform_driver_register(&alarmtimer_driver);
+    if (error)
+        goto out_pm;
+
     return 0;
+
+out_pm:
+    alarmtimer_unregister_pm_notifier();
 out_if:
     alarmtimer_rtc_interface_remove();
     return error;

Powered by blists - more mailing lists