[PATCH,RFC] mmp clockevent handling race

[Eric Miao]

On Wed, Jun 8, 2011 at 12:16 PM, Haojian Zhuang
<haojian.zhuang at marvell.com> wrote:
> On Tue, 2011-06-07 at 20:23 -0700, Eric Miao wrote:
>> On Wed, Jun 8, 2011 at 11:14 AM, Haojian Zhuang
>> <haojian.zhuang at marvell.com> wrote:
>> > On Tue, 2011-06-07 at 20:05 -0700, Eric Miao wrote:
>> >> On Wed, Jun 8, 2011 at 10:25 AM, Haojian Zhuang
>> >> <haojian.zhuang at marvell.com> wrote:
>> >> > On Tue, 2011-06-07 at 11:29 -0700, Nicolas Pitre wrote:
>> >> >> On Tue, 7 Jun 2011, Lennert Buytenhek wrote:
>> >> >>
>> >> >> > Hi!
>> >> >> >
>> >> >> > The patch below has been sitting around in the OLPC XO-1.75 (MMP2-based
>> >> >> > XO) bringup kernel tree for some time now, and upon recent rebasing of
>> >> >> > this tree to 3.0, it was discovered that something like this patch is
>> >> >> > still needed.
>> >> >> >
>> >> >> > Symptoms: e.g. as described here: http://dev.laptop.org/ticket/10945
>> >> >> > -- applications hang for a couple of minutes at a time, and sometimes
>> >> >> > there are several-minute hangs during the boot process.
>> >> >> >
>> >> >> > >From the ticket:
>> >> >> >
>> >> >> >     The problem in the current upstream mmp timer handling code
>> >> >> >     that appears to be triggered here is that it handles clockevents
>> >> >> >     by setting up a comparator on the free-running clocksource timer
>> >> >> >     to match and trigger an interrupt at 'current_time + delta',
>> >> >> >     which if delta is small enough can lead to 'current_time + delta'
>> >> >> >     already being in the past when comparator setup has finished,
>> >> >> >     and the requested event will not trigger.
>> >> >>
>> >> >> This is a classical issue that was solved on the SA1100 more than 10
>> >> >> years ago.
>> >> >>
>> >> >> > What this patch does is to rewrite the timer handling code to use
>> >> >> > two timers, one for the free-running clocksource, and one to trigger
>> >> >> > clockevents with, which is more or less the standard approach to this.
>> >> >> > It's kind of invasive, though (certainly more invasive than it strictly
>> >> >> > needs to be, as it reorganises time.c somewhat at the same time), and
>> >> >> > something like this is probably too invasive for 3.0 at this point.
>> >> >> >
>> >> >> > A safer "fix" for 3.0 may be to disallow NO_HZ/HIGH_RES_TIMERS on mmp.
>> >> >> >
>> >> >> > Any thoughts?
>> >> >>
>> >> >> What about simply this:
>> >> >>
>> >> >> diff --git a/arch/arm/mach-mmp/time.c b/arch/arm/mach-mmp/time.c
>> >> >> index 99833b9..8b8f99a 100644
>> >> >> --- a/arch/arm/mach-mmp/time.c
>> >> >> +++ b/arch/arm/mach-mmp/time.c
>> >> >> @@ -39,7 +39,7 @@
>> >> >>
>> >> >>  #define TIMERS_VIRT_BASE     TIMERS1_VIRT_BASE
>> >> >>
>> >> >> -#define MAX_DELTA            (0xfffffffe)
>> >> >> +#define MAX_DELTA            (0xfffffffe - 16)
>> >> >>  #define MIN_DELTA            (16)
>> >> >>
>> >> >>  static DEFINE_CLOCK_DATA(cd);
>> >> >> @@ -85,7 +85,7 @@ static irqreturn_t timer_interrupt(int irq, void *dev_id)
>> >> >>  static int timer_set_next_event(unsigned long delta,
>> >> >>                               struct clock_event_device *dev)
>> >> >>  {
>> >> >> -     unsigned long flags, next;
>> >> >> +     unsigned long flags, next, now;
>> >> >>
>> >> >>       local_irq_save(flags);
>> >> >>
>> >> >> @@ -95,9 +95,10 @@ static int timer_set_next_event(unsigned long delta,
>> >> >>
>> >> >>       next = timer_read() + delta;
>> >> >>       __raw_writel(next, TIMERS_VIRT_BASE + TMR_TN_MM(0, 0));
>> >> >> +     now = timer_read();
>> >> >>
>> >> >>       local_irq_restore(flags);
>> >> >> -     return 0;
>> >> >> +     return (signed)(next - now) <= MIN_DELTA ? -ETIME : 0;
>> >> >>  }
>> >> >>
>> >> >>  static void timer_set_mode(enum clock_event_mode mode,
>> >> >>
>> >> >>
>> >> >> Nicolas
>> >> > It seems good. But we still need to use two different timer. Because
>> >> > writing match register needs to stop counter register first. This is a
>> >> > limitation in the silicon.
>> >>
>> >> Well, the first thing is Lennert's RFC patch needs a bit cleanup :-) I tried
>> >> very hard to figure out the actual changes.
>> >>
>> >> Using two timers isn't a bad idea. Yet if it has to disable timer0 when
>> >> modifying timer1's next triggering event, it doesn't seem to solve the
>> >> real problem with two timers.
>> > Since timer registers is defined as group mode. Counter register and
>> > match register exist in each group. We can use timer 0 as clock source,
>> > only read timer counter. We can use timer 1 as clock event. While we're
>> > writing match register, we just need to disable counter register of
>> > group 1, not group 0. Since it's silicon limitation.
>>
>> A bit confused, but is this disabling stops the clock source from incrementing
>> for a while?
>>
> Clock source is always free-running. Clock source (timer0) is used to
> keep time always increasing.

That's completely fine then. Thanks Haojian for the explanation.