linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: Problem when function alarmtimer_suspend returns 0 if time delta is zero
       [not found] ` <08fbdf25-faa1-aa13-4f13-d30acbf27dda@mipisi.de>
@ 2019-09-02  7:49   ` Alexandre Belloni
  2019-09-02 10:57     ` Thomas Gleixner
  0 siblings, 1 reply; 11+ messages in thread
From: Alexandre Belloni @ 2019-09-02  7:49 UTC (permalink / raw)
  To: Michael
  Cc: linux-rtc, Thomas Gleixner, John Stultz, Stephen Boyd, linux-kernel

Hello Michael,

This code is maintained by the timekeeping maintainers, now in Cc and I
think John will be able to answer.

On 31/08/2019 20:32:06+0200, Michael wrote:
> Dear members of the linux-rtc list,
> 
> currently I have a problem with the alarmtimer i'm using to cyclically wake
> up my i.MX6 ULL board from suspend to RAM.
> 
> The problem is that in principle the timer wake ups work fine but seem to be
> not 100% stable. In about 1 percent the wake up alarm from suspend is
> missing.
> 
> When I look at the code of alarmtimer in function alarmtimer_suspend
> (kernel/time/alarmtimer.c)
> I find the following:
> 
> ....
> 
> /* Find the soonest timer to expire*/
> 
>     for (i = 0; i < ALARM_NUMTYPE; i++) {
>         struct alarm_base *base = &alarm_bases[i];
>         struct timerqueue_node *next;
>         ktime_t delta;
> 
>         spin_lock_irqsave(&base->lock, flags);
>         next = timerqueue_getnext(&base->timerqueue);
>         spin_unlock_irqrestore(&base->lock, flags);
>         if (!next)
>             continue;
>         delta = ktime_sub(next->expires, base->gettime());
>         if (!min || (delta < min)) {
>             expires = next->expires;
>             min = delta;
>             type = i;
>         }
>     }
>     if (min == 0)
>         return 0;
> 
>     if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
>         __pm_wakeup_event(ws, 2 * MSEC_PER_SEC);
>         return -EBUSY;
>     }
> 
> In my error case the alarm wake up always fails if the path "if(min==0)" is
> entered. If I understand this code correctly that means that
> when ever one of the timers in the list has a remaining tick time of zero,
> the function just returns 0 and continues the suspend process until
> it reaches suspend mode.
> 
> If I implement a hack here "if(min == 0) {min = 1;}" and do not return, my
> system runs 100% ok, as the following -EBUSY path is hit.
> 
> So my question to you is: Why is there a check if min < 2 seconds and do a
> return -EBUSY here, but handle (min==0) differently?
> Could there be some race condition here, where the function
> alarmtimer_suspend just returns 0 and shortly after this the alarmtimer
> expires
> right before the RTC driver was able to allow the wake up interrupt?
> 
> If I look through the kernel versions I found the alarmtimer_suspend to be a
> very stable function, so I don't think there is anything wrong here.
> 
> But do you have a hint for me where else I could have a look to encircle the
> error?
> 
> Thank you very much!
> 
> Br,
> Michael
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Problem when function alarmtimer_suspend returns 0 if time delta is zero
  2019-09-02  7:49   ` Problem when function alarmtimer_suspend returns 0 if time delta is zero Alexandre Belloni
@ 2019-09-02 10:57     ` Thomas Gleixner
  2019-09-03 18:48       ` Michael
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Gleixner @ 2019-09-02 10:57 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Michael, linux-rtc, John Stultz, Stephen Boyd, linux-kernel

Michael,

On Mon, 2 Sep 2019, Alexandre Belloni wrote:
> On 31/08/2019 20:32:06+0200, Michael wrote:
> > currently I have a problem with the alarmtimer i'm using to cyclically wake
> > up my i.MX6 ULL board from suspend to RAM.
> > 
> > The problem is that in principle the timer wake ups work fine but seem to be
> > not 100% stable. In about 1 percent the wake up alarm from suspend is
> > missing.

> > In my error case the alarm wake up always fails if the path "if(min==0)" is
> > entered. If I understand this code correctly that means that
> > when ever one of the timers in the list has a remaining tick time of zero,
> > the function just returns 0 and continues the suspend process until
> > it reaches suspend mode.

No. That code is simply broken because it tries to handle the case where a
alarmtimer nanosleep got woken up by the freezer. That's broken because it
makes the delta = 0 assumption which leads to the issue you discovered.

That whole cruft can be removed by switching alarmtimer nanosleep to use
freezable_schedule(). That keeps the timer queued and avoids all the issues.

Completely untested patch below.

Thanks,

	tglx

8<----------------------

kernel/time/alarmtimer.c |   57 +++--------------------------------------------
 1 file changed, 4 insertions(+), 53 deletions(-)

--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -46,14 +46,6 @@ static struct alarm_base {
 	clockid_t		base_clockid;
 } alarm_bases[ALARM_NUMTYPE];
 
-#if defined(CONFIG_POSIX_TIMERS) || defined(CONFIG_RTC_CLASS)
-/* freezer information to handle clock_nanosleep triggered wakeups */
-static enum alarmtimer_type freezer_alarmtype;
-static ktime_t freezer_expires;
-static ktime_t freezer_delta;
-static DEFINE_SPINLOCK(freezer_delta_lock);
-#endif
-
 #ifdef CONFIG_RTC_CLASS
 static struct wakeup_source *ws;
 
@@ -241,19 +233,12 @@ EXPORT_SYMBOL_GPL(alarm_expires_remainin
  */
 static int alarmtimer_suspend(struct device *dev)
 {
-	ktime_t min, now, expires;
+	ktime_t now, expires, min = KTIME_MAX;
 	int i, ret, type;
 	struct rtc_device *rtc;
 	unsigned long flags;
 	struct rtc_time tm;
 
-	spin_lock_irqsave(&freezer_delta_lock, flags);
-	min = freezer_delta;
-	expires = freezer_expires;
-	type = freezer_alarmtype;
-	freezer_delta = 0;
-	spin_unlock_irqrestore(&freezer_delta_lock, flags);
-
 	rtc = alarmtimer_get_rtcdev();
 	/* If we have no rtcdev, just return */
 	if (!rtc)
@@ -271,13 +256,13 @@ static int alarmtimer_suspend(struct dev
 		if (!next)
 			continue;
 		delta = ktime_sub(next->expires, base->gettime());
-		if (!min || (delta < min)) {
+		if (delta < min) {
 			expires = next->expires;
 			min = delta;
 			type = i;
 		}
 	}
-	if (min == 0)
+	if (min == KTIME_MAX)
 		return 0;
 
 	if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
@@ -479,38 +464,6 @@ u64 alarm_forward_now(struct alarm *alar
 EXPORT_SYMBOL_GPL(alarm_forward_now);
 
 #ifdef CONFIG_POSIX_TIMERS
-
-static void alarmtimer_freezerset(ktime_t absexp, enum alarmtimer_type type)
-{
-	struct alarm_base *base;
-	unsigned long flags;
-	ktime_t delta;
-
-	switch(type) {
-	case ALARM_REALTIME:
-		base = &alarm_bases[ALARM_REALTIME];
-		type = ALARM_REALTIME_FREEZER;
-		break;
-	case ALARM_BOOTTIME:
-		base = &alarm_bases[ALARM_BOOTTIME];
-		type = ALARM_BOOTTIME_FREEZER;
-		break;
-	default:
-		WARN_ONCE(1, "Invalid alarm type: %d\n", type);
-		return;
-	}
-
-	delta = ktime_sub(absexp, base->gettime());
-
-	spin_lock_irqsave(&freezer_delta_lock, flags);
-	if (!freezer_delta || (delta < freezer_delta)) {
-		freezer_delta = delta;
-		freezer_expires = absexp;
-		freezer_alarmtype = type;
-	}
-	spin_unlock_irqrestore(&freezer_delta_lock, flags);
-}
-
 /**
  * clock2alarm - helper that converts from clockid to alarmtypes
  * @clockid: clockid.
@@ -715,7 +668,7 @@ static int alarmtimer_do_nsleep(struct a
 		set_current_state(TASK_INTERRUPTIBLE);
 		alarm_start(alarm, absexp);
 		if (likely(alarm->data))
-			schedule();
+			freezable_schedule();
 
 		alarm_cancel(alarm);
 	} while (alarm->data && !signal_pending(current));
@@ -727,8 +680,6 @@ static int alarmtimer_do_nsleep(struct a
 	if (!alarm->data)
 		return 0;
 
-	if (freezing(current))
-		alarmtimer_freezerset(absexp, type);
 	restart = &current->restart_block;
 	if (restart->nanosleep.type != TT_NONE) {
 		struct timespec64 rmt;

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Problem when function alarmtimer_suspend returns 0 if time delta is zero
  2019-09-02 10:57     ` Thomas Gleixner
@ 2019-09-03 18:48       ` Michael
  2019-09-03 22:49         ` Thomas Gleixner
  0 siblings, 1 reply; 11+ messages in thread
From: Michael @ 2019-09-03 18:48 UTC (permalink / raw)
  To: Thomas Gleixner, Alexandre Belloni
  Cc: linux-rtc, John Stultz, Stephen Boyd, linux-kernel

Thomas,

thank you very much for your patch. Unfortunately currently I can only 
test it with a kernel 4.1.52 but i've tried to patch
your new logic into my older kernel version.

There seem to be rare cases where the "delta" value becomes negative. 
Therefore I added

if(unlikely(delta < 0)) {
     delta = 0;
}
before min-check.

Currently I still get returns here in the new code

+	if (min == KTIME_MAX)
  		return 0;

where the board afterwards is not woken up.So I think there is still 
something missing.

I'm doing further tests and keep you informed.

Again Thanks!
Michael



On 02.09.2019 12:57, Thomas Gleixner wrote:
> Michael,
>
> On Mon, 2 Sep 2019, Alexandre Belloni wrote:
>> On 31/08/2019 20:32:06+0200, Michael wrote:
>>> currently I have a problem with the alarmtimer i'm using to cyclically wake
>>> up my i.MX6 ULL board from suspend to RAM.
>>>
>>> The problem is that in principle the timer wake ups work fine but seem to be
>>> not 100% stable. In about 1 percent the wake up alarm from suspend is
>>> missing.
>>> In my error case the alarm wake up always fails if the path "if(min==0)" is
>>> entered. If I understand this code correctly that means that
>>> when ever one of the timers in the list has a remaining tick time of zero,
>>> the function just returns 0 and continues the suspend process until
>>> it reaches suspend mode.
> No. That code is simply broken because it tries to handle the case where a
> alarmtimer nanosleep got woken up by the freezer. That's broken because it
> makes the delta = 0 assumption which leads to the issue you discovered.
>
> That whole cruft can be removed by switching alarmtimer nanosleep to use
> freezable_schedule(). That keeps the timer queued and avoids all the issues.
>
> Completely untested patch below.
>
> Thanks,
>
> 	tglx
>
> 8<----------------------
>
> kernel/time/alarmtimer.c |   57 +++--------------------------------------------
>   1 file changed, 4 insertions(+), 53 deletions(-)
>
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -46,14 +46,6 @@ static struct alarm_base {
>   	clockid_t		base_clockid;
>   } alarm_bases[ALARM_NUMTYPE];
>   
> -#if defined(CONFIG_POSIX_TIMERS) || defined(CONFIG_RTC_CLASS)
> -/* freezer information to handle clock_nanosleep triggered wakeups */
> -static enum alarmtimer_type freezer_alarmtype;
> -static ktime_t freezer_expires;
> -static ktime_t freezer_delta;
> -static DEFINE_SPINLOCK(freezer_delta_lock);
> -#endif
> -
>   #ifdef CONFIG_RTC_CLASS
>   static struct wakeup_source *ws;
>   
> @@ -241,19 +233,12 @@ EXPORT_SYMBOL_GPL(alarm_expires_remainin
>    */
>   static int alarmtimer_suspend(struct device *dev)
>   {
> -	ktime_t min, now, expires;
> +	ktime_t now, expires, min = KTIME_MAX;
>   	int i, ret, type;
>   	struct rtc_device *rtc;
>   	unsigned long flags;
>   	struct rtc_time tm;
>   
> -	spin_lock_irqsave(&freezer_delta_lock, flags);
> -	min = freezer_delta;
> -	expires = freezer_expires;
> -	type = freezer_alarmtype;
> -	freezer_delta = 0;
> -	spin_unlock_irqrestore(&freezer_delta_lock, flags);
> -
>   	rtc = alarmtimer_get_rtcdev();
>   	/* If we have no rtcdev, just return */
>   	if (!rtc)
> @@ -271,13 +256,13 @@ static int alarmtimer_suspend(struct dev
>   		if (!next)
>   			continue;
>   		delta = ktime_sub(next->expires, base->gettime());
> -		if (!min || (delta < min)) {
> +		if (delta < min) {
>   			expires = next->expires;
>   			min = delta;
>   			type = i;
>   		}
>   	}
> -	if (min == 0)
> +	if (min == KTIME_MAX)
>   		return 0;
>   
>   	if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
> @@ -479,38 +464,6 @@ u64 alarm_forward_now(struct alarm *alar
>   EXPORT_SYMBOL_GPL(alarm_forward_now);
>   
>   #ifdef CONFIG_POSIX_TIMERS
> -
> -static void alarmtimer_freezerset(ktime_t absexp, enum alarmtimer_type type)
> -{
> -	struct alarm_base *base;
> -	unsigned long flags;
> -	ktime_t delta;
> -
> -	switch(type) {
> -	case ALARM_REALTIME:
> -		base = &alarm_bases[ALARM_REALTIME];
> -		type = ALARM_REALTIME_FREEZER;
> -		break;
> -	case ALARM_BOOTTIME:
> -		base = &alarm_bases[ALARM_BOOTTIME];
> -		type = ALARM_BOOTTIME_FREEZER;
> -		break;
> -	default:
> -		WARN_ONCE(1, "Invalid alarm type: %d\n", type);
> -		return;
> -	}
> -
> -	delta = ktime_sub(absexp, base->gettime());
> -
> -	spin_lock_irqsave(&freezer_delta_lock, flags);
> -	if (!freezer_delta || (delta < freezer_delta)) {
> -		freezer_delta = delta;
> -		freezer_expires = absexp;
> -		freezer_alarmtype = type;
> -	}
> -	spin_unlock_irqrestore(&freezer_delta_lock, flags);
> -}
> -
>   /**
>    * clock2alarm - helper that converts from clockid to alarmtypes
>    * @clockid: clockid.
> @@ -715,7 +668,7 @@ static int alarmtimer_do_nsleep(struct a
>   		set_current_state(TASK_INTERRUPTIBLE);
>   		alarm_start(alarm, absexp);
>   		if (likely(alarm->data))
> -			schedule();
> +			freezable_schedule();
>   
>   		alarm_cancel(alarm);
>   	} while (alarm->data && !signal_pending(current));
> @@ -727,8 +680,6 @@ static int alarmtimer_do_nsleep(struct a
>   	if (!alarm->data)
>   		return 0;
>   
> -	if (freezing(current))
> -		alarmtimer_freezerset(absexp, type);
>   	restart = &current->restart_block;
>   	if (restart->nanosleep.type != TT_NONE) {
>   		struct timespec64 rmt;



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Problem when function alarmtimer_suspend returns 0 if time delta is zero
  2019-09-03 18:48       ` Michael
@ 2019-09-03 22:49         ` Thomas Gleixner
  2023-02-08 15:23           ` Michael Trimarchi
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Gleixner @ 2019-09-03 22:49 UTC (permalink / raw)
  To: Michael
  Cc: Alexandre Belloni, linux-rtc, John Stultz, Stephen Boyd, linux-kernel

On Tue, 3 Sep 2019, Michael wrote:

> Thomas,
> 
> thank you very much for your patch. Unfortunately currently I can only test it
> with a kernel 4.1.52 but i've tried to patch
> your new logic into my older kernel version.

<formletter>

 https://people.kernel.org/tglx/notes-about-netiquette

</formletter>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Problem when function alarmtimer_suspend returns 0 if time delta is zero
  2019-09-03 22:49         ` Thomas Gleixner
@ 2023-02-08 15:23           ` Michael Trimarchi
  2023-02-08 18:06             ` Thomas Gleixner
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Trimarchi @ 2023-02-08 15:23 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Michael, Alexandre Belloni, linux-rtc, John Stultz, Stephen Boyd,
	linux-kernel

Hi Thomas

On Wed, Sep 04, 2019 at 12:49:21AM +0200, Thomas Gleixner wrote:
> On Tue, 3 Sep 2019, Michael wrote:
> 
> > Thomas,
> > 
> > thank you very much for your patch. Unfortunately currently I can only test it
> > with a kernel 4.1.52 but i've tried to patch
> > your new logic into my older kernel version.
>

Is this patch valid on mainline too? because apply it was let rtc
working 100% of the time

Michael

> <formletter>
> 
>  https://people.kernel.org/tglx/notes-about-netiquette
> 
> </formletter>
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Problem when function alarmtimer_suspend returns 0 if time delta is zero
  2023-02-08 15:23           ` Michael Trimarchi
@ 2023-02-08 18:06             ` Thomas Gleixner
  2023-02-09 11:19               ` Michael Nazzareno Trimarchi
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Gleixner @ 2023-02-08 18:06 UTC (permalink / raw)
  To: Michael Trimarchi
  Cc: Michael, Alexandre Belloni, linux-rtc, John Stultz, Stephen Boyd,
	linux-kernel

Michael!

On Wed, Feb 08 2023 at 16:23, Michael Trimarchi wrote:
> On Wed, Sep 04, 2019 at 12:49:21AM +0200, Thomas Gleixner wrote:
>> On Tue, 3 Sep 2019, Michael wrote:
>> > 
>> > thank you very much for your patch. Unfortunately currently I can only test it
>> > with a kernel 4.1.52 but i've tried to patch
>> > your new logic into my older kernel version.
>>
> Is this patch valid on mainline too? because apply it was let rtc
> working 100% of the time

I wrote that patch against the back then mainline code. No idea if it's
still applying, but the underlying issue is still the same AFAICT.

It needs some polishing and a proper changelog.

Thanks,

        tglx



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Problem when function alarmtimer_suspend returns 0 if time delta is zero
  2023-02-08 18:06             ` Thomas Gleixner
@ 2023-02-09 11:19               ` Michael Nazzareno Trimarchi
  2023-02-09 15:40                 ` Thomas Gleixner
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Nazzareno Trimarchi @ 2023-02-09 11:19 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Michael, Alexandre Belloni, linux-rtc, John Stultz, Stephen Boyd,
	linux-kernel

Hi Thomas

On Wed, Feb 8, 2023 at 7:06 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Michael!
>
> On Wed, Feb 08 2023 at 16:23, Michael Trimarchi wrote:
> > On Wed, Sep 04, 2019 at 12:49:21AM +0200, Thomas Gleixner wrote:
> >> On Tue, 3 Sep 2019, Michael wrote:
> >> >
> >> > thank you very much for your patch. Unfortunately currently I can only test it
> >> > with a kernel 4.1.52 but i've tried to patch
> >> > your new logic into my older kernel version.
> >>
> > Is this patch valid on mainline too? because apply it was let rtc
> > working 100% of the time
>
> I wrote that patch against the back then mainline code. No idea if it's
> still applying, but the underlying issue is still the same AFAICT.
>
> It needs some polishing and a proper changelog.
>

Ok, I will try to update it on some mainline kernel in my environment
and test it back. I need
a little information if it's possible. Consider that I have no
experience in this area. I understand how
code was designed in general but the part around the freezer and all
those code you remove, what was the logic behind in the removed code?

Michael

> Thanks,
>
>         tglx
>
>



--
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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Problem when function alarmtimer_suspend returns 0 if time delta is zero
  2023-02-09 11:19               ` Michael Nazzareno Trimarchi
@ 2023-02-09 15:40                 ` Thomas Gleixner
  2023-02-11  1:04                   ` John Stultz
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Gleixner @ 2023-02-09 15:40 UTC (permalink / raw)
  To: Michael Nazzareno Trimarchi
  Cc: Michael, Alexandre Belloni, linux-rtc, Stephen Boyd,
	linux-kernel, John Stultz

Michael!

On Thu, Feb 09 2023 at 12:19, Michael Nazzareno Trimarchi wrote:
> On Wed, Feb 8, 2023 at 7:06 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> I wrote that patch against the back then mainline code. No idea if it's
>> still applying, but the underlying issue is still the same AFAICT.
>>
>> It needs some polishing and a proper changelog.
>>
> Ok, I will try to update it on some mainline kernel in my environment
> and test it back. I need
> a little information if it's possible. Consider that I have no
> experience in this area. I understand how
> code was designed in general but the part around the freezer and all
> those code you remove, what was the logic behind in the removed code?

What I can oracle out of that well commented gem is:

  A userspace task invokes clock_nanosleep(CLOCK_*_ALARM, ...), which
  arms an alarm timer. The expiry of an alarmtimer causes the system to
  come out of suspend.

  As the task invokes schedule() it can also be brought back from
  schedule() via a signal. If that happens then the task cancels the
  alarmtimer and returns to handle the signal. While doing that it can
  be frozen, which means the alarm and therefore the wake from suspend
  is lost.

  To prevent that the code tries to save the earliest expiring alarm if
  the task is marked freezing() and the suspend code takes that into
  account.

John, did I summarize that correctly?

The change I made remove that magic and marks the task freezable when it
goes to schedule, which prevents the signal wakeup. That ensures that
the alarm stays armed during freeze/suspend.

That obviously needs some testing and scrunity by the folks which use
this mechanism. IIRC that's used by android, but I might be wrong.

Thanks,

        tglx



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Problem when function alarmtimer_suspend returns 0 if time delta is zero
  2023-02-09 15:40                 ` Thomas Gleixner
@ 2023-02-11  1:04                   ` John Stultz
  2023-02-11  1:18                     ` John Stultz
  0 siblings, 1 reply; 11+ messages in thread
From: John Stultz @ 2023-02-11  1:04 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Michael Nazzareno Trimarchi, Michael, Alexandre Belloni,
	linux-rtc, Stephen Boyd, linux-kernel

On Thu, Feb 9, 2023 at 7:40 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, Feb 09 2023 at 12:19, Michael Nazzareno Trimarchi wrote:
> > On Wed, Feb 8, 2023 at 7:06 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> I wrote that patch against the back then mainline code. No idea if it's
> >> still applying, but the underlying issue is still the same AFAICT.
> >>
> >> It needs some polishing and a proper changelog.
> >>
> > Ok, I will try to update it on some mainline kernel in my environment
> > and test it back. I need
> > a little information if it's possible. Consider that I have no
> > experience in this area. I understand how
> > code was designed in general but the part around the freezer and all
> > those code you remove, what was the logic behind in the removed code?
>
> What I can oracle out of that well commented gem is:
>
>   A userspace task invokes clock_nanosleep(CLOCK_*_ALARM, ...), which
>   arms an alarm timer. The expiry of an alarmtimer causes the system to
>   come out of suspend.
>
>   As the task invokes schedule() it can also be brought back from
>   schedule() via a signal. If that happens then the task cancels the
>   alarmtimer and returns to handle the signal. While doing that it can
>   be frozen, which means the alarm and therefore the wake from suspend
>   is lost.
>
>   To prevent that the code tries to save the earliest expiring alarm if
>   the task is marked freezing() and the suspend code takes that into
>   account.
>
> John, did I summarize that correctly?
>
> The change I made remove that magic and marks the task freezable when it
> goes to schedule, which prevents the signal wakeup. That ensures that
> the alarm stays armed during freeze/suspend.
>
> That obviously needs some testing and scrunity by the folks which use
> this mechanism. IIRC that's used by android, but I might be wrong.

So, thanks for dredging this old thread up, I'm sorry I didn't see it
the first time it came around.

Not having a clear memory of why we do the (min == 0) early return, I
went digging in, and found it was in the original git commit, so I
went looking to the archives.

It's completely not present in the first version of the patch:
  https://lore.kernel.org/lkml/1288809079-14663-8-git-send-email-john.stultz@linaro.org/

But it did appear in the second version:
  https://lore.kernel.org/lkml/1290136329-18291-6-git-send-email-john.stultz@linaro.org/

And from there it's a clear case of wanting to avoid setting the RTC
if there were just no timers to expire.

But, indeed this is a bug, as initializing min to ktime_set(0, 0) as
the "invalid" case isn't a good plan, as it might be possible that
suspend is run right as an alarmtimer expires, and you get a real zero
delta value (as has been reported).

Instead it seemed I should have used KTIME_MAX as the "invalid" case
(as Thomas' patch uses).

However, before the patch was merged, it changed further to handle the
freezer waking a current sleeper:
  https://lore.kernel.org/lkml/1294280159-2513-13-git-send-email-john.stultz@linaro.org/

Which was then used to initialize the min value (still erroneously
using 0 as an "invalid" value) in the case that the freezer woke a
task sleeping which would cause the alarm to be lost (as Thomas
summarized).

Thomas' patch fixes the erronious 0-as-invalid initialization issue
using KTIME_MAX but also simplifies the logic getting rid of the
freezer handling.

I don't have as much familiarity with the freezer handling change, so
while it looks sane, I can't say I would likely catch an issue doing a
visual review.

Michael: If you are still intending to send the patch out, please do,
otherwise I've already forward ported it so I can do some testing with
it. I'm happy to put together a commit message and send it out if
that's easier for you.

And, just for correct Reported-by: tags: Michael Trimarchi, are you
the same Michael (michael@mipisi.de) that originally reported this
issue?

thanks
-john

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Problem when function alarmtimer_suspend returns 0 if time delta is zero
  2023-02-11  1:04                   ` John Stultz
@ 2023-02-11  1:18                     ` John Stultz
  2023-02-11  6:25                       ` Michael Nazzareno Trimarchi
  0 siblings, 1 reply; 11+ messages in thread
From: John Stultz @ 2023-02-11  1:18 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Michael Nazzareno Trimarchi, Michael, Alexandre Belloni,
	linux-rtc, Stephen Boyd, linux-kernel

On Fri, Feb 10, 2023 at 5:04 PM John Stultz <jstultz@google.com> wrote:
> Thomas' patch fixes the erronious 0-as-invalid initialization issue
> using KTIME_MAX but also simplifies the logic getting rid of the
> freezer handling.
>
> I don't have as much familiarity with the freezer handling change, so
> while it looks sane, I can't say I would likely catch an issue doing a
> visual review.

Actually, because of this, I'm going to split Thomas' change in two.

The first to just use KTIME_MAX as the invalid initialization value,
and the second to cleanup the freezer logic.

That way if the freezer change is problematic we can revert it and not
lose the fix here.

thanks
-john

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Problem when function alarmtimer_suspend returns 0 if time delta is zero
  2023-02-11  1:18                     ` John Stultz
@ 2023-02-11  6:25                       ` Michael Nazzareno Trimarchi
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Nazzareno Trimarchi @ 2023-02-11  6:25 UTC (permalink / raw)
  To: John Stultz
  Cc: Thomas Gleixner, Michael, Alexandre Belloni, linux-rtc,
	Stephen Boyd, linux-kernel

Hi John

On Sat, Feb 11, 2023 at 2:18 AM John Stultz <jstultz@google.com> wrote:
>
> On Fri, Feb 10, 2023 at 5:04 PM John Stultz <jstultz@google.com> wrote:
> > Thomas' patch fixes the erronious 0-as-invalid initialization issue
> > using KTIME_MAX but also simplifies the logic getting rid of the
> > freezer handling.
> >
> > I don't have as much familiarity with the freezer handling change, so
> > while it looks sane, I can't say I would likely catch an issue doing a
> > visual review.
>
> Actually, because of this, I'm going to split Thomas' change in two.
>
> The first to just use KTIME_MAX as the invalid initialization value,
> and the second to cleanup the freezer logic.
>
> That way if the freezer change is problematic we can revert it and not
> lose the fix here.
>
> thanks
> -john

I'm not that Michael but it's ok I think to add both as a reporter. I can try to
test on my side too and then send a tested-by

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2023-02-11  6:25 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <S1728511AbfHaSEm/20190831180442Z+580@vger.kernel.org>
     [not found] ` <08fbdf25-faa1-aa13-4f13-d30acbf27dda@mipisi.de>
2019-09-02  7:49   ` Problem when function alarmtimer_suspend returns 0 if time delta is zero Alexandre Belloni
2019-09-02 10:57     ` Thomas Gleixner
2019-09-03 18:48       ` Michael
2019-09-03 22:49         ` Thomas Gleixner
2023-02-08 15:23           ` Michael Trimarchi
2023-02-08 18:06             ` Thomas Gleixner
2023-02-09 11:19               ` Michael Nazzareno Trimarchi
2023-02-09 15:40                 ` Thomas Gleixner
2023-02-11  1:04                   ` John Stultz
2023-02-11  1:18                     ` John Stultz
2023-02-11  6:25                       ` Michael Nazzareno Trimarchi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).