Messages in this thread | | | Date | Sat, 23 Jul 2011 09:22:06 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [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.
| |