Viresh Kumar viresh.kumar@linaro.org writes:
A clockevent device should be stopped, or its events should be masked, if next event is expected at KTIME_MAX, i.e. no events are required for very long time.
This would normally happen with NO_HZ (both NO_HZ_IDLE and NO_HZ_FULL) when tick-sched timer is removed and we don't have any more timers scheduled for long.
If we don't STOP clockevent device, we are guaranteed to receive at least one spurious interrupt, as hrtimer_force_reprogram() isn't reprogramming the event device (on expires == KTIME_MAX). Depending on particular implementation of clockevent device, this can be a spurious interrupt at tick-rate.. (When driver is emulating ONESHOT over PERIODIC mode. This was observed on at least one implementation: arm_arch_timer.c).
A simple (yet hacky) solution to get this fixed could be: update hrtimer_force_reprogram() to always reprogram clockevent device and update clockevent drivers to STOP generating events (or delay it to max time), when 'expires' is set to KTIME_MAX. But the drawback here is that every clockevent driver has to be hacked for this particular case and its very easy for new ones to miss this.
However, Thomas suggested to add an optional mode: ONESHOT_STOPPED (lkml.org/lkml/2014/5/9/508) to solve this problem.
clockevents core would need to reconfigure mode later, when event is required again, for platforms that implement ONESHOT_STOPPED mode and so it must know which platforms implement it and which don't. This requires ->set_mode() callback to have capability to return 'success' or 'failure'. Currently its return type is 'void'.
This paragraph is a copy/paste from cover letter, so has the same readability problems I mentioned there.
To change return type of set_mode(), Thomas suggested to implement another callback: ->set_dev_mode(), with return type 'int'. We can then convert clockevent drivers to use this interface instead of existing ->set_mode() and then finally remove ->set_mode()'s support.
This patch adds support for ->set_dev_mode() in clockevents core.
For now, just add a WARN_ON_ONCE() to callers of ->set_dev_mode() as none of currently implemented modes are expected/allowed to fail.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
[...]
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index ad362c2..f9bed16 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -105,7 +105,14 @@ void clockevents_set_mode(struct clock_event_device *dev, enum clock_event_mode mode) { if (dev->mode != mode) {
dev->set_mode(mode, dev);
if (dev->set_dev_mode) {
/* WARN: Currently available modes shouldn't fail */
if (WARN_ON_ONCE(dev->set_dev_mode(mode, dev)))
return;
} else {
dev->set_mode(mode, dev);
}
- dev->mode = mode;
/* @@ -446,8 +453,13 @@ int __clockevents_update_freq(struct clock_event_device *dev, u32 freq) if (dev->mode == CLOCK_EVT_MODE_ONESHOT) return clockevents_program_event(dev, dev->next_event, false);
- if (dev->mode == CLOCK_EVT_MODE_PERIODIC)
dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
- if (dev->mode == CLOCK_EVT_MODE_PERIODIC) {
if (dev->set_dev_mode)
As discussed already, PERIODIC should never fail, so maybe a comment here stating that we don't ever expect periodic mode to fail.
Kevin