* 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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 = ¤t->restart_block; if (restart->nanosleep.type != TT_NONE) { struct timespec64 rmt; ^ permalink raw reply [flat|nested] 12+ 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; 12+ 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 = ¤t->restart_block; > if (restart->nanosleep.type != TT_NONE) { > struct timespec64 rmt; ^ permalink raw reply [flat|nested] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ messages in thread
end of thread, other threads:[~2023-02-11 6:25 UTC | newest] Thread overview: 12+ 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 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).