Re: Regression: ONE CPU fails bootup at Re: [3.2.0-RC7] BUG: unable to handle kernel NULL pointer dereference at 0000000000000598 [ 1.478005] IP: [<ffffffff8107a6c4>] queue_work_on+0x4/0x30

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3215 bytes --]

On Tue, 03 Jan 2012 16:53:00 -0800 John Stultz <john.stultz@linaro.org> wrote:

> On Wed, 2012-01-04 at 11:31 +1100, NeilBrown wrote:
> > On Tue, 03 Jan 2012 15:09:48 -0800 John Stultz <john.stultz@linaro.org> wrote:
> > > >From the stack trace, we've kicked off a rtc_timer_do_work, probably
> > > from the rtc_initialize_alarm() schedule_work call added in Neil's
> > > patch.  From there, we call __rtc_set_alarm -> cmos_set_alarm ->
> > > cmos_rq_disable -> cmos_checkintr -> rtc_update_irq -> schedule_work.
> > > 
> > > So, what it looks to me is that in cmos_checkintr, we grab the cmos->rtc
> > > and pass that along. Unfortunately, since the cmos->rtc value isn't set
> > > until after rtc_device_register() returns its null at that point. So
> > > your patch isn't really fixing the issue, but just reducing the race
> > > window for the second cpu to schedule the work.
> > > 
> > > Sigh. I'd guess dropping the schedule_work call from
> > > rtc_initialize_alarm() is the right approach (see below). When reviewing
> > > Neil's patch it seemed like a good idea there, but it seems off to me
> > > now.
> > > 
> > > Neil, any thoughts on the following? Can you expand on the condition you
> > > were worried about in around that call?
> > 
> > If you set an alarm in the future, then shutdown and boot again after that
> > time, then you will end up with a timer_queue node which is in the past.
> 
> Thanks for explaining this again.
> 
> Hrm. It seems the easy answer is to simply not add alarms that are in
> the past.  Further, I'm a bit perplexed, as if they are in the past, the
> enabled flag shouldn't be set.   __rtc_read_alarm() does check the
> current time, so maybe we can make sure we don't return old values? I
> guess I assumed __rtc_read_alarm() avoided returning stale values, but
> apparently not.

That would probably be a more robust approach.  Also it might make sense to
clean out old alarms whenever we are about to add a new one.

> 
> > When this happens the queue gets stuck.  That entry-in-the-past won't get
> > removed until and interrupt happens and an interrupt won't happen because the
> > RTC only triggers an interrupt when the alarm is "now".
> > 
> > So you'll find that e.g. "hwclock" will always tell you that 'select' timed
> > out.
> > 
> > So we force the interrupt work to happen at the start just in case.
> 
> Unfortunately its too early. 
> 
> > Did you see my proposed patch which converted those calls to do the work
> > in-process rather than passing it to a worker-thread?  I think that is a
> > clean fix.
> 
> I don't think I saw it today. Was it from before the holidays?

About 4 hours ago:
  Subject: Re: Patch Upstream: rtc: Expire alarms after the time is set.

> 
> Even so, at this point, I don't know if we have enough time for testing,
> so I'm thinking we either just drop the problematic sched_work call or
> revert the whole thing and try again for 3.3

I wouldn't object to that.  The bug only triggers in unusual circumstances
and is quite easy to work around so it is safer  to wait until we have a
really good fix.

Thanks,
NeilBrown


> 
> thanks
> -john
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]