From mboxrd@z Thu Jan 1 00:00:00 1970 From: eric.y.miao@gmail.com (Eric Miao) Date: Wed, 8 Jun 2011 13:25:25 +0800 Subject: [PATCH,RFC] mmp clockevent handling race In-Reply-To: <1307506618.8890.16.camel@Lily> References: <20110607140456.GE11275@wantstofly.org> <1307499937.8890.2.camel@Lily> <1307502890.8890.9.camel@Lily> <1307506618.8890.16.camel@Lily> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Jun 8, 2011 at 12:16 PM, Haojian Zhuang wrote: > On Tue, 2011-06-07 at 20:23 -0700, Eric Miao wrote: >> On Wed, Jun 8, 2011 at 11:14 AM, Haojian Zhuang >> wrote: >> > On Tue, 2011-06-07 at 20:05 -0700, Eric Miao wrote: >> >> On Wed, Jun 8, 2011 at 10:25 AM, Haojian Zhuang >> >> 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.