On Fri, 22 Jul 2011, Andrew Morton wrote:
> On Fri, 22 Jul 2011 09:12:51 -0000
> Thomas Gleixner <t...@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.
_______________________________________________
stable mailing list
stable@linux.kernel.org
http://linux.kernel.org/mailman/listinfo/stable
