Commit dae373be authored by John Stultz's avatar John Stultz

alarmtimer: Use hrtimer per-alarm instead of per-base

Arve Hjønnevåg reported numerous crashes from the
"BUG_ON(timer->state != HRTIMER_STATE_CALLBACK)" check
in __run_hrtimer after it called alarmtimer_fired.

It ends up the alarmtimer code was not properly handling
possible failures of hrtimer_try_to_cancel, and because
these faulres occur when the underlying base hrtimer is
being run, this limits the ability to properly handle
modifications to any alarmtimers on that base.

Because much of the logic duplicates the hrtimer logic,
it seems that we might as well have a per-alarmtimer
hrtimer, and avoid the extra complextity of trying to
multiplex many alarmtimers off of one hrtimer.

Thus this patch moves the hrtimer to the alarm structure
and simplifies the management logic.

Changelog:
v2:
* Includes a fix for double alarm_start calls found by
  Arve

Cc: Arve Hjønnevåg <arve@android.com>
Cc: Colin Cross <ccross@android.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Reported-by: default avatarArve Hjønnevåg <arve@android.com>
Tested-by: default avatarArve Hjønnevåg <arve@android.com>
Signed-off-by: default avatarJohn Stultz <john.stultz@linaro.org>
parent 59a93c27
...@@ -35,6 +35,7 @@ enum alarmtimer_restart { ...@@ -35,6 +35,7 @@ enum alarmtimer_restart {
*/ */
struct alarm { struct alarm {
struct timerqueue_node node; struct timerqueue_node node;
struct hrtimer timer;
enum alarmtimer_restart (*function)(struct alarm *, ktime_t now); enum alarmtimer_restart (*function)(struct alarm *, ktime_t now);
enum alarmtimer_type type; enum alarmtimer_type type;
int state; int state;
...@@ -43,7 +44,7 @@ struct alarm { ...@@ -43,7 +44,7 @@ struct alarm {
void alarm_init(struct alarm *alarm, enum alarmtimer_type type, void alarm_init(struct alarm *alarm, enum alarmtimer_type type,
enum alarmtimer_restart (*function)(struct alarm *, ktime_t)); enum alarmtimer_restart (*function)(struct alarm *, ktime_t));
void alarm_start(struct alarm *alarm, ktime_t start); int alarm_start(struct alarm *alarm, ktime_t start);
int alarm_try_to_cancel(struct alarm *alarm); int alarm_try_to_cancel(struct alarm *alarm);
int alarm_cancel(struct alarm *alarm); int alarm_cancel(struct alarm *alarm);
......
...@@ -37,7 +37,6 @@ ...@@ -37,7 +37,6 @@
static struct alarm_base { static struct alarm_base {
spinlock_t lock; spinlock_t lock;
struct timerqueue_head timerqueue; struct timerqueue_head timerqueue;
struct hrtimer timer;
ktime_t (*gettime)(void); ktime_t (*gettime)(void);
clockid_t base_clockid; clockid_t base_clockid;
} alarm_bases[ALARM_NUMTYPE]; } alarm_bases[ALARM_NUMTYPE];
...@@ -132,21 +131,17 @@ static inline void alarmtimer_rtc_timer_init(void) { } ...@@ -132,21 +131,17 @@ static inline void alarmtimer_rtc_timer_init(void) { }
* @base: pointer to the base where the timer is being run * @base: pointer to the base where the timer is being run
* @alarm: pointer to alarm being enqueued. * @alarm: pointer to alarm being enqueued.
* *
* Adds alarm to a alarm_base timerqueue and if necessary sets * Adds alarm to a alarm_base timerqueue
* an hrtimer to run.
* *
* Must hold base->lock when calling. * Must hold base->lock when calling.
*/ */
static void alarmtimer_enqueue(struct alarm_base *base, struct alarm *alarm) static void alarmtimer_enqueue(struct alarm_base *base, struct alarm *alarm)
{ {
if (alarm->state & ALARMTIMER_STATE_ENQUEUED)
timerqueue_del(&base->timerqueue, &alarm->node);
timerqueue_add(&base->timerqueue, &alarm->node); timerqueue_add(&base->timerqueue, &alarm->node);
alarm->state |= ALARMTIMER_STATE_ENQUEUED; alarm->state |= ALARMTIMER_STATE_ENQUEUED;
if (&alarm->node == timerqueue_getnext(&base->timerqueue)) {
hrtimer_try_to_cancel(&base->timer);
hrtimer_start(&base->timer, alarm->node.expires,
HRTIMER_MODE_ABS);
}
} }
/** /**
...@@ -154,28 +149,17 @@ static void alarmtimer_enqueue(struct alarm_base *base, struct alarm *alarm) ...@@ -154,28 +149,17 @@ static void alarmtimer_enqueue(struct alarm_base *base, struct alarm *alarm)
* @base: pointer to the base where the timer is running * @base: pointer to the base where the timer is running
* @alarm: pointer to alarm being removed * @alarm: pointer to alarm being removed
* *
* Removes alarm to a alarm_base timerqueue and if necessary sets * Removes alarm to a alarm_base timerqueue
* a new timer to run.
* *
* Must hold base->lock when calling. * Must hold base->lock when calling.
*/ */
static void alarmtimer_remove(struct alarm_base *base, struct alarm *alarm) static void alarmtimer_remove(struct alarm_base *base, struct alarm *alarm)
{ {
struct timerqueue_node *next = timerqueue_getnext(&base->timerqueue);
if (!(alarm->state & ALARMTIMER_STATE_ENQUEUED)) if (!(alarm->state & ALARMTIMER_STATE_ENQUEUED))
return; return;
timerqueue_del(&base->timerqueue, &alarm->node); timerqueue_del(&base->timerqueue, &alarm->node);
alarm->state &= ~ALARMTIMER_STATE_ENQUEUED; alarm->state &= ~ALARMTIMER_STATE_ENQUEUED;
if (next == &alarm->node) {
hrtimer_try_to_cancel(&base->timer);
next = timerqueue_getnext(&base->timerqueue);
if (!next)
return;
hrtimer_start(&base->timer, next->expires, HRTIMER_MODE_ABS);
}
} }
...@@ -190,42 +174,23 @@ static void alarmtimer_remove(struct alarm_base *base, struct alarm *alarm) ...@@ -190,42 +174,23 @@ static void alarmtimer_remove(struct alarm_base *base, struct alarm *alarm)
*/ */
static enum hrtimer_restart alarmtimer_fired(struct hrtimer *timer) static enum hrtimer_restart alarmtimer_fired(struct hrtimer *timer)
{ {
struct alarm_base *base = container_of(timer, struct alarm_base, timer); struct alarm *alarm = container_of(timer, struct alarm, timer);
struct timerqueue_node *next; struct alarm_base *base = &alarm_bases[alarm->type];
unsigned long flags; unsigned long flags;
ktime_t now;
int ret = HRTIMER_NORESTART; int ret = HRTIMER_NORESTART;
int restart = ALARMTIMER_NORESTART; int restart = ALARMTIMER_NORESTART;
spin_lock_irqsave(&base->lock, flags); spin_lock_irqsave(&base->lock, flags);
now = base->gettime(); alarmtimer_remove(base, alarm);
while ((next = timerqueue_getnext(&base->timerqueue))) { spin_unlock_irqrestore(&base->lock, flags);
struct alarm *alarm;
ktime_t expired = next->expires;
if (expired.tv64 > now.tv64)
break;
alarm = container_of(next, struct alarm, node);
timerqueue_del(&base->timerqueue, &alarm->node);
alarm->state &= ~ALARMTIMER_STATE_ENQUEUED;
alarm->state |= ALARMTIMER_STATE_CALLBACK;
spin_unlock_irqrestore(&base->lock, flags);
if (alarm->function)
restart = alarm->function(alarm, now);
spin_lock_irqsave(&base->lock, flags);
alarm->state &= ~ALARMTIMER_STATE_CALLBACK;
if (restart != ALARMTIMER_NORESTART) { if (alarm->function)
timerqueue_add(&base->timerqueue, &alarm->node); restart = alarm->function(alarm, base->gettime());
alarm->state |= ALARMTIMER_STATE_ENQUEUED;
}
}
if (next) { spin_lock_irqsave(&base->lock, flags);
hrtimer_set_expires(&base->timer, next->expires); if (restart != ALARMTIMER_NORESTART) {
hrtimer_set_expires(&alarm->timer, alarm->node.expires);
alarmtimer_enqueue(base, alarm);
ret = HRTIMER_RESTART; ret = HRTIMER_RESTART;
} }
spin_unlock_irqrestore(&base->lock, flags); spin_unlock_irqrestore(&base->lock, flags);
...@@ -331,6 +296,9 @@ void alarm_init(struct alarm *alarm, enum alarmtimer_type type, ...@@ -331,6 +296,9 @@ void alarm_init(struct alarm *alarm, enum alarmtimer_type type,
enum alarmtimer_restart (*function)(struct alarm *, ktime_t)) enum alarmtimer_restart (*function)(struct alarm *, ktime_t))
{ {
timerqueue_init(&alarm->node); timerqueue_init(&alarm->node);
hrtimer_init(&alarm->timer, alarm_bases[type].base_clockid,
HRTIMER_MODE_ABS);
alarm->timer.function = alarmtimer_fired;
alarm->function = function; alarm->function = function;
alarm->type = type; alarm->type = type;
alarm->state = ALARMTIMER_STATE_INACTIVE; alarm->state = ALARMTIMER_STATE_INACTIVE;
...@@ -341,17 +309,19 @@ void alarm_init(struct alarm *alarm, enum alarmtimer_type type, ...@@ -341,17 +309,19 @@ void alarm_init(struct alarm *alarm, enum alarmtimer_type type,
* @alarm: ptr to alarm to set * @alarm: ptr to alarm to set
* @start: time to run the alarm * @start: time to run the alarm
*/ */
void alarm_start(struct alarm *alarm, ktime_t start) int alarm_start(struct alarm *alarm, ktime_t start)
{ {
struct alarm_base *base = &alarm_bases[alarm->type]; struct alarm_base *base = &alarm_bases[alarm->type];
unsigned long flags; unsigned long flags;
int ret;
spin_lock_irqsave(&base->lock, flags); spin_lock_irqsave(&base->lock, flags);
if (alarmtimer_active(alarm))
alarmtimer_remove(base, alarm);
alarm->node.expires = start; alarm->node.expires = start;
alarmtimer_enqueue(base, alarm); alarmtimer_enqueue(base, alarm);
ret = hrtimer_start(&alarm->timer, alarm->node.expires,
HRTIMER_MODE_ABS);
spin_unlock_irqrestore(&base->lock, flags); spin_unlock_irqrestore(&base->lock, flags);
return ret;
} }
/** /**
...@@ -365,18 +335,12 @@ int alarm_try_to_cancel(struct alarm *alarm) ...@@ -365,18 +335,12 @@ int alarm_try_to_cancel(struct alarm *alarm)
{ {
struct alarm_base *base = &alarm_bases[alarm->type]; struct alarm_base *base = &alarm_bases[alarm->type];
unsigned long flags; unsigned long flags;
int ret = -1; int ret;
spin_lock_irqsave(&base->lock, flags);
if (alarmtimer_callback_running(alarm))
goto out;
if (alarmtimer_is_queued(alarm)) { spin_lock_irqsave(&base->lock, flags);
ret = hrtimer_try_to_cancel(&alarm->timer);
if (ret >= 0)
alarmtimer_remove(base, alarm); alarmtimer_remove(base, alarm);
ret = 1;
} else
ret = 0;
out:
spin_unlock_irqrestore(&base->lock, flags); spin_unlock_irqrestore(&base->lock, flags);
return ret; return ret;
} }
...@@ -809,10 +773,6 @@ static int __init alarmtimer_init(void) ...@@ -809,10 +773,6 @@ static int __init alarmtimer_init(void)
for (i = 0; i < ALARM_NUMTYPE; i++) { for (i = 0; i < ALARM_NUMTYPE; i++) {
timerqueue_init_head(&alarm_bases[i].timerqueue); timerqueue_init_head(&alarm_bases[i].timerqueue);
spin_lock_init(&alarm_bases[i].lock); spin_lock_init(&alarm_bases[i].lock);
hrtimer_init(&alarm_bases[i].timer,
alarm_bases[i].base_clockid,
HRTIMER_MODE_ABS);
alarm_bases[i].timer.function = alarmtimer_fired;
} }
error = alarmtimer_rtc_interface_setup(); error = alarmtimer_rtc_interface_setup();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment