[PATCH,RFC] mmp clockevent handling race

[haojian.zhuang@marvell.com (Haojian Zhuang)]

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.

Thanks
Haojian