Re: [PATCH] tick: add check for the existence of broadcast clockevent device

[Feng Tang]

On Sat, 6 Jun 2009 17:24:50 +0800
Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

> On Fri, 5 Jun 2009, Feng Tang wrote:
> 
> > >From 2f076e1867c8bbb145b74d289358174644d9fed8 Mon Sep 17 00:00:00
> > >2001
> > From: Feng Tang <feng.tang@xxxxxxxxx>
> > Date: Fri, 5 Jun 2009 10:36:15 +0800
> > Subject: [PATCH] tick: add check for the existence of broadcast
> > clock event device
> > 
> > Some platform may have no broadcast clock event device, as it use
> > always-on external timers for per-cpu timer and has no extra one
> > for broadcast device. This check will secure the access to bc
> > device when system get some boradcast on/off and enter/exit message
> >
> > Signed-off-by: Feng Tang <feng.tang@xxxxxxxxx>
> > ---
> >  kernel/time/tick-broadcast.c |    8 +++++++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> > 
> > diff --git a/kernel/time/tick-broadcast.c
> > b/kernel/time/tick-broadcast.c index 118a3b3..110e0bc 100644
> > --- a/kernel/time/tick-broadcast.c
> > +++ b/kernel/time/tick-broadcast.c
> > @@ -214,10 +214,13 @@ static void tick_do_broadcast_on_off(void
> > *why) 
> >  	spin_lock_irqsave(&tick_broadcast_lock, flags);
> >  
> > +	bc = tick_broadcast_device.evtdev;
> > +	if (!bc)
> > +		goto out;
> > +
> 
> This check is not necessary because we check whether the percpu device
> is affected by the stops in C3 madness _before_ we touch the broadcast
> device.
> 
> 	 if (!dev || !(dev->features & CLOCK_EVT_FEAT_C3STOP))
> 	    	  got out;
> 
> If your percpu devices are always on (not affected by C3 stop) then
> you never dereference bc. So why do we need an extra check for !bc ?

Hi tglx,

Thanks for the explanation. But we really ran into the NULL pointer case, in our platform, there are 2 X86 CPUs which have lapic, also it has 2 external timers which are pretty similar with HPET timers, those 2 external timers will be used as per-cpu timers (higher rating than lapic timer). In system's power cycle of suspend and resume, disable_nontboot_cpus will be called before goto suspend state,and enable_nonboot_cpus will be called for the resume process, so lapic timer of cpu1 will be first registered as per-cpu timer, and our external timer will be registered later after get a CPU_ONLINE notifier (similar with HPET), right in this time slot that lapic is the per-cpu timer, when system get the CLOCK_EVT_BROADCAST_ENTER/EXIT msg, tick_do_broadcast_on_off() is called and hit the NULL pointer case.

Our external timer driver is very similar with HPET dirver, why HPET doesn't see such an issue? becuase HPET has enough number of timers, and it use "hpet0" as the bc device, while our platform doesn't have a extra one to act as bc.

- Feng 




> 
> Thanks,
> 
> 	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/