From: Michael <michael@mipisi.de>
To: Thomas Gleixner <tglx@linutronix.de>,
Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: linux-rtc@vger.kernel.org, John Stultz <john.stultz@linaro.org>,
Stephen Boyd <sboyd@kernel.org>,
linux-kernel@vger.kernel.org
Subject: Re: Problem when function alarmtimer_suspend returns 0 if time delta is zero
Date: Tue, 3 Sep 2019 20:48:59 +0200 [thread overview]
Message-ID: <4fc3a016-ec2f-a15e-5fd1-6794a001e2d9@mipisi.de> (raw)
In-Reply-To: <alpine.DEB.2.21.1909021247250.3955@nanos.tec.linutronix.de>
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;
next prev parent reply other threads:[~2019-09-03 18:55 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
[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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4fc3a016-ec2f-a15e-5fd1-6794a001e2d9@mipisi.de \
--to=michael@mipisi.de \
--cc=alexandre.belloni@bootlin.com \
--cc=john.stultz@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rtc@vger.kernel.org \
--cc=sboyd@kernel.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).