lkml.org 
[lkml]   [2011]   [Jul]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 2/3] rtc: Fix hrtimer deadlock
    On Fri, 22 Jul 2011, Andrew Morton wrote:

    > On Fri, 22 Jul 2011 09:12:51 -0000
    > Thomas Gleixner <tglx@linutronix.de> wrote:
    >
    > > Ben reported a lockup related to rtc. The lockup happens due to:
    > >
    > > CPU0 CPU1
    > >
    > > rtc_irq_set_state() __run_hrtimer()
    > > spin_lock_irqsave(&rtc->irq_task_lock) rtc_handle_legacy_irq();
    > > spin_lock(&rtc->irq_task_lock);
    > > hrtimer_cancel()
    > > while (callback_running);
    > >
    > > So the running callback never finishes as it's blocked on
    > > rtc->irq_task_lock.
    > >
    > > Use hrtimer_try_to_cancel() instead and drop rtc->irq_task_lock while
    > > waiting for the callback. Fix this for both rtc_irq_set_state() and
    > > rtc_irq_set_freq().
    > >
    > > ...
    > >
    > > +static int rtc_update_hrtimer(struct rtc_device *rtc, int enabled)
    > > +{
    > > + /*
    > > + * We unconditionally cancel the timer here, because otherwise
    >
    > The comment seems wrong. If hrtimer_try_to_cancel() fails, we simply
    > bale out so we did not "unconditionally cancel the timer"?

    Well, what I meant is that we cancel it before we start it. That's
    required for self rearming timers. Will reword.

    > > + * we could run into BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
    > > + * when we manage to start the timer before the callback
    > > + * returns HRTIMER_RESTART.
    > > + *
    > > + * We cannot use hrtimer_cancel() here as a running callback
    > > + * could be blocked on rtc->irq_task_lock and hrtimer_cancel()
    > > + * would spin forever.
    > > + */
    > > + if (hrtimer_try_to_cancel(&rtc->pie_timer) < 0)
    > > + return -1;
    > > +
    > > + if (enabled) {
    > > + ktime_t period = ktime_set(0, NSEC_PER_SEC / rtc->irq_freq);
    > > +
    > > + hrtimer_start(&rtc->pie_timer, period, HRTIMER_MODE_REL);
    > > + }
    > > + return 0;
    > > +}
    > > +
    > > /**
    > > * rtc_irq_set_state - enable/disable 2^N Hz periodic IRQs
    > > * @rtc: the rtc device
    > > @@ -651,24 +674,21 @@ int rtc_irq_set_state(struct rtc_device
    > > int err = 0;
    > > unsigned long flags;
    > >
    > > +retry:
    > > spin_lock_irqsave(&rtc->irq_task_lock, flags);
    > > if (rtc->irq_task != NULL && task == NULL)
    > > err = -EBUSY;
    > > if (rtc->irq_task != task)
    > > err = -EACCES;
    > > - if (err)
    > > - goto out;
    > > -
    > > - if (enabled) {
    > > - ktime_t period = ktime_set(0, NSEC_PER_SEC/rtc->irq_freq);
    > > - hrtimer_start(&rtc->pie_timer, period, HRTIMER_MODE_REL);
    > > - } else {
    > > - hrtimer_cancel(&rtc->pie_timer);
    > > + if (!err) {
    > > + if (rtc_update_hrtimer(rtc, enabled) < 0) {
    > > + spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
    > > + cpu_relax();
    > > + goto retry;
    > > + }
    > > + rtc->pie_enabled = enabled;
    >
    > Well this is rather nasty. Sort of an open-coded expensive spinlock.
    > All rather pointless on SMP=n builds, too.
    >
    > Is there no better way, such as fixing up the locking properly?

    Probably there is, but that requires a rather large patch and a
    complete locking rewrite, nothing you want to push back into
    stable. And we want this as the deadlock has been observed and
    reported already.



    \
     
     \ /
      Last update: 2011-07-23 09:25    [W:0.026 / U:0.804 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site