Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
* Problem when function alarmtimer_suspend returns 0 if time delta is zero
       [not found] <S1728511AbfHaSEm/20190831180442Z+580@vger.kernel.org>
@ 2019-08-31 18:32 ` Michael
  2019-09-02  7:49   ` Alexandre Belloni
  0 siblings, 1 reply; 5+ messages in thread
From: Michael @ 2019-08-31 18:32 UTC (permalink / raw)
  To: linux-rtc

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


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

* Re: Problem when function alarmtimer_suspend returns 0 if time delta is zero
  2019-08-31 18:32 ` Problem when function alarmtimer_suspend returns 0 if time delta is zero Michael
@ 2019-09-02  7:49   ` Alexandre Belloni
  2019-09-02 10:57     ` Thomas Gleixner
  0 siblings, 1 reply; 5+ 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] 5+ messages in thread

* Re: Problem when function alarmtimer_suspend returns 0 if time delta is zero
  2019-09-02  7:49   ` Alexandre Belloni
@ 2019-09-02 10:57     ` Thomas Gleixner
  2019-09-03 18:48       ` Michael
  0 siblings, 1 reply; 5+ 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] 5+ 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; 5+ 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] 5+ 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
  0 siblings, 0 replies; 5+ 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] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <S1728511AbfHaSEm/20190831180442Z+580@vger.kernel.org>
2019-08-31 18:32 ` Problem when function alarmtimer_suspend returns 0 if time delta is zero Michael
2019-09-02  7:49   ` Alexandre Belloni
2019-09-02 10:57     ` Thomas Gleixner
2019-09-03 18:48       ` Michael
2019-09-03 22:49         ` Thomas Gleixner

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org linux-rtc@archiver.kernel.org
	public-inbox-index linux-rtc


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox