* [patch 0/3] rtc: Assorted bug fixes @ 2011-07-22 9:12 Thomas Gleixner 2011-07-22 9:12 ` [patch 1/3] rtc: Handle errors correctly in rtc_irq_set_state() Thomas Gleixner ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Thomas Gleixner @ 2011-07-22 9:12 UTC (permalink / raw) To: LKML; +Cc: Andrew Morton, John Stultz, Ingo Molnar, Ben Greear The following series fixes a few interesting bugs in the RTC code. Thanks, tglx ^ permalink raw reply [flat|nested] 14+ messages in thread
* [patch 1/3] rtc: Handle errors correctly in rtc_irq_set_state() 2011-07-22 9:12 [patch 0/3] rtc: Assorted bug fixes Thomas Gleixner @ 2011-07-22 9:12 ` Thomas Gleixner 2011-07-22 22:04 ` Andrew Morton 2011-07-22 9:12 ` [patch 3/3] rtc: Limit frequency Thomas Gleixner 2011-07-22 9:12 ` [patch 2/3] rtc: Fix hrtimer deadlock Thomas Gleixner 2 siblings, 1 reply; 14+ messages in thread From: Thomas Gleixner @ 2011-07-22 9:12 UTC (permalink / raw) To: LKML; +Cc: Andrew Morton, John Stultz, Ingo Molnar, Ben Greear, stable [-- Attachment #1: rtc-deal-with-errors-correctly.patch --] [-- Type: text/plain, Size: 875 bytes --] The code checks the correctness of the parameters, but unconditionally arms/disarms the hrtimer. Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Cc: stable@kernel.org --- drivers/rtc/interface.c | 3 +++ 1 file changed, 3 insertions(+) Index: linux-2.6/drivers/rtc/interface.c =================================================================== --- linux-2.6.orig/drivers/rtc/interface.c +++ linux-2.6/drivers/rtc/interface.c @@ -656,6 +656,8 @@ int rtc_irq_set_state(struct rtc_device 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); @@ -664,6 +666,7 @@ int rtc_irq_set_state(struct rtc_device hrtimer_cancel(&rtc->pie_timer); } rtc->pie_enabled = enabled; +out: spin_unlock_irqrestore(&rtc->irq_task_lock, flags); return err; ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch 1/3] rtc: Handle errors correctly in rtc_irq_set_state() 2011-07-22 9:12 ` [patch 1/3] rtc: Handle errors correctly in rtc_irq_set_state() Thomas Gleixner @ 2011-07-22 22:04 ` Andrew Morton 2011-07-23 7:15 ` Thomas Gleixner 0 siblings, 1 reply; 14+ messages in thread From: Andrew Morton @ 2011-07-22 22:04 UTC (permalink / raw) To: Thomas Gleixner; +Cc: LKML, John Stultz, Ingo Molnar, Ben Greear, stable On Fri, 22 Jul 2011 09:12:50 -0000 Thomas Gleixner <tglx@linutronix.de> wrote: > The code checks the correctness of the parameters, but unconditionally > arms/disarms the hrtimer. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: stable@kernel.org The -stable maintainers will want to know why they should merge this. The maintainers of other trees will wonder whether they should backport it too. To help them make these decision we should always provide a description of the user-visible effects of the bug, please. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch 1/3] rtc: Handle errors correctly in rtc_irq_set_state() 2011-07-22 22:04 ` Andrew Morton @ 2011-07-23 7:15 ` Thomas Gleixner 0 siblings, 0 replies; 14+ messages in thread From: Thomas Gleixner @ 2011-07-23 7:15 UTC (permalink / raw) To: Andrew Morton; +Cc: LKML, John Stultz, Ingo Molnar, Ben Greear, stable On Fri, 22 Jul 2011, Andrew Morton wrote: > On Fri, 22 Jul 2011 09:12:50 -0000 > Thomas Gleixner <tglx@linutronix.de> wrote: > > > The code checks the correctness of the parameters, but unconditionally > > arms/disarms the hrtimer. > > > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > > Cc: stable@kernel.org > > The -stable maintainers will want to know why they should merge this. > > The maintainers of other trees will wonder whether they should backport > it too. > > To help them make these decision we should always provide a description > of the user-visible effects of the bug, please. Fair enough. The result is that a random task might arm/disarm rtc timer and surprise the real owner by either generating events or by stopping them. Both undesired behaviour :) ^ permalink raw reply [flat|nested] 14+ messages in thread
* [patch 3/3] rtc: Limit frequency 2011-07-22 9:12 [patch 0/3] rtc: Assorted bug fixes Thomas Gleixner 2011-07-22 9:12 ` [patch 1/3] rtc: Handle errors correctly in rtc_irq_set_state() Thomas Gleixner @ 2011-07-22 9:12 ` Thomas Gleixner 2011-07-22 22:05 ` Andrew Morton 2011-07-22 9:12 ` [patch 2/3] rtc: Fix hrtimer deadlock Thomas Gleixner 2 siblings, 1 reply; 14+ messages in thread From: Thomas Gleixner @ 2011-07-22 9:12 UTC (permalink / raw) To: LKML; +Cc: Andrew Morton, John Stultz, Ingo Molnar, Ben Greear, stable [-- Attachment #1: rtc-limit-frequency.patch --] [-- Type: text/plain, Size: 675 bytes --] The RTC hrtimer is self rearming. We really need to limit the frequency to something sensible. Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Cc: stable@kernel.org --- drivers/rtc/interface.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: linux-2.6/drivers/rtc/interface.c =================================================================== --- linux-2.6.orig/drivers/rtc/interface.c +++ linux-2.6/drivers/rtc/interface.c @@ -708,7 +708,7 @@ int rtc_irq_set_freq(struct rtc_device * int err = 0; unsigned long flags; - if (freq <= 0) + if (freq <= 0 || freq > 5000) return -EINVAL; retry: spin_lock_irqsave(&rtc->irq_task_lock, flags); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch 3/3] rtc: Limit frequency 2011-07-22 9:12 ` [patch 3/3] rtc: Limit frequency Thomas Gleixner @ 2011-07-22 22:05 ` Andrew Morton 2011-07-22 22:39 ` [stable] " Willy Tarreau 2011-07-23 7:17 ` Thomas Gleixner 0 siblings, 2 replies; 14+ messages in thread From: Andrew Morton @ 2011-07-22 22:05 UTC (permalink / raw) To: Thomas Gleixner; +Cc: LKML, John Stultz, Ingo Molnar, Ben Greear, stable On Fri, 22 Jul 2011 09:12:51 -0000 Thomas Gleixner <tglx@linutronix.de> wrote: > The RTC hrtimer is self rearming. We really need to limit the > frequency to something sensible. > Cc: stable@kernel.org Why? What failures does the current code cause? What effect do these failures have upon users? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [stable] [patch 3/3] rtc: Limit frequency 2011-07-22 22:05 ` Andrew Morton @ 2011-07-22 22:39 ` Willy Tarreau 2011-08-05 3:39 ` Joshua Kinard 2011-07-23 7:17 ` Thomas Gleixner 1 sibling, 1 reply; 14+ messages in thread From: Willy Tarreau @ 2011-07-22 22:39 UTC (permalink / raw) To: Andrew Morton Cc: Thomas Gleixner, Ben Greear, Ingo Molnar, John Stultz, LKML, stable On Fri, Jul 22, 2011 at 03:05:50PM -0700, Andrew Morton wrote: > On Fri, 22 Jul 2011 09:12:51 -0000 > Thomas Gleixner <tglx@linutronix.de> wrote: > > > The RTC hrtimer is self rearming. We really need to limit the > > frequency to something sensible. > > > Cc: stable@kernel.org > > Why? What failures does the current code cause? What effect do these > failures have upon users? I would add that if we go that route, we should at least accept values that were documented as possible till now. Man rtc says 2 Hz to 8192 Hz, but Thomas' proposed patch limits it to 5000 Hz, so some breakage is to be expected. Regards, Willy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [stable] [patch 3/3] rtc: Limit frequency 2011-07-22 22:39 ` [stable] " Willy Tarreau @ 2011-08-05 3:39 ` Joshua Kinard 2011-08-05 9:04 ` John Stultz 0 siblings, 1 reply; 14+ messages in thread From: Joshua Kinard @ 2011-08-05 3:39 UTC (permalink / raw) To: Willy Tarreau Cc: Andrew Morton, Thomas Gleixner, Ben Greear, Ingo Molnar, John Stultz, LKML, stable On 07/22/2011 18:39, Willy Tarreau wrote: > On Fri, Jul 22, 2011 at 03:05:50PM -0700, Andrew Morton wrote: >> On Fri, 22 Jul 2011 09:12:51 -0000 >> Thomas Gleixner <tglx@linutronix.de> wrote: >> >>> The RTC hrtimer is self rearming. We really need to limit the >>> frequency to something sensible. >> >>> Cc: stable@kernel.org >> >> Why? What failures does the current code cause? What effect do these >> failures have upon users? > > I would add that if we go that route, we should at least accept values > that were documented as possible till now. Man rtc says 2 Hz to 8192 Hz, > but Thomas' proposed patch limits it to 5000 Hz, so some breakage is to > be expected. > > Regards, > Willy To me, it would make sense to lock it at 8192Hz. I re-wrote a driver for the DS1685 family of Dallas chips that I need to cleanup and re-submit, but in researching that specific RTC, I really could not find an RTC out there that went above 8192. Arguably, 32768Hz might also work. The DS1685 runs at this mode normally, but it disables PIE when it does. My vote is 8192Hz. Also, can we get a wrapper of some kind in the RTC core to allow an RTC driver to override the hrtimer /PIE emulation if needed? I have in the DS1685 driver full support for its PIE mode and really do not want to gut it as part of the cleanup. Some kind of override API would be nice in case anyone ever runs into this chip (or its family members). -- Joshua Kinard Gentoo/MIPS kumba@gentoo.org "The past tempts us, the present confuses us, the future frightens us. And our lives slip away, moment by moment, lost in that vast, terrible in-between." --Emperor Turhan, Centauri Republic ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [stable] [patch 3/3] rtc: Limit frequency 2011-08-05 3:39 ` Joshua Kinard @ 2011-08-05 9:04 ` John Stultz 2011-08-06 7:28 ` Joshua Kinard 0 siblings, 1 reply; 14+ messages in thread From: John Stultz @ 2011-08-05 9:04 UTC (permalink / raw) To: Joshua Kinard Cc: Willy Tarreau, Andrew Morton, Thomas Gleixner, Ben Greear, Ingo Molnar, LKML, stable On Thu, 2011-08-04 at 23:39 -0400, Joshua Kinard wrote: > On 07/22/2011 18:39, Willy Tarreau wrote: > > On Fri, Jul 22, 2011 at 03:05:50PM -0700, Andrew Morton wrote: > >> On Fri, 22 Jul 2011 09:12:51 -0000 > >> Thomas Gleixner <tglx@linutronix.de> wrote: > >> > >>> The RTC hrtimer is self rearming. We really need to limit the > >>> frequency to something sensible. > >> > >>> Cc: stable@kernel.org > >> > >> Why? What failures does the current code cause? What effect do these > >> failures have upon users? > > > > I would add that if we go that route, we should at least accept values > > that were documented as possible till now. Man rtc says 2 Hz to 8192 Hz, > > but Thomas' proposed patch limits it to 5000 Hz, so some breakage is to > > be expected. > > > > Regards, > > Willy > > To me, it would make sense to lock it at 8192Hz. I re-wrote a driver for the DS1685 family of Dallas chips that I need > to cleanup and re-submit, but in researching that specific RTC, I really could not find an RTC out there that went above > 8192. > > Arguably, 32768Hz might also work. The DS1685 runs at this mode normally, but it disables PIE when it does. > > My vote is 8192Hz. Yea, I submitted a slightly different version of Thomas' patch which set it to 8192Hz via the tip tree, but the original one went upstream first. I'm away from home right now, so I'll be re-submitting just that change probably next week. > Also, can we get a wrapper of some kind in the RTC core to allow an > RTC driver to override the hrtimer /PIE emulation if > needed? I have in the DS1685 driver full support for its PIE mode and > really do not want to gut it as part of the > cleanup. Some kind of override API would be nice in case anyone ever > runs into this chip (or its family members). I'm torn here. It would be good to have the functionality listed in some way so it is documented. However as we abstract the RTC to be more of an internal tool for the kernel, rather then hardware directly addressed from userland, the PIE mode is really not as useful (and quite messier to multiplex). I'd much rather try to extend the alarm interfaces to allow for higher resolution, so it could be used as a alarmed highres oneshot timer if the hardware supports it. thanks -john ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [stable] [patch 3/3] rtc: Limit frequency 2011-08-05 9:04 ` John Stultz @ 2011-08-06 7:28 ` Joshua Kinard 0 siblings, 0 replies; 14+ messages in thread From: Joshua Kinard @ 2011-08-06 7:28 UTC (permalink / raw) To: John Stultz Cc: Willy Tarreau, Andrew Morton, Thomas Gleixner, Ben Greear, Ingo Molnar, LKML, stable On 08/05/2011 05:04, John Stultz wrote: > Yea, I submitted a slightly different version of Thomas' patch which set > it to 8192Hz via the tip tree, but the original one went upstream first. > I'm away from home right now, so I'll be re-submitting just that change > probably next week. Awesome, I'll look out for it if I get free time to look at the RTC driver I have. It's broken now due to a loss of __symbol_get(), as I was trying to find a way to make the driver determine if it was a module or not. > I'm torn here. It would be good to have the functionality listed in some > way so it is documented. However as we abstract the RTC to be more of an > internal tool for the kernel, rather then hardware directly addressed > from userland, the PIE mode is really not as useful (and quite messier > to multiplex). I'd much rather try to extend the alarm interfaces to > allow for higher resolution, so it could be used as a alarmed highres > oneshot timer if the hardware supports it. That's why I think a wrapper would be best. By default, it would use the hrtimer code and so, none of the existing drivers would need to be modified. Any driver that wants to, however, could override the base functions if they wanted to provide that functionality separately. It should probably be a CONFIG_* option, though. Right now, consumers of DS1685 are rare: old SGI O2 systems and some embedded board from 'electronic systems Gmbh', one of whose employees wrote the original driver about 2 years ago. The driver I have now handles older versions (DS1689/1693) and the more common DS17x8y versions, and those I think are the most modern. Not sure what systems use those, however. -- Joshua Kinard Gentoo/MIPS kumba@gentoo.org 4096R/D25D95E3 2011-03-28 "The past tempts us, the present confuses us, the future frightens us. And our lives slip away, moment by moment, lost in that vast, terrible in-between." --Emperor Turhan, Centauri Republic ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch 3/3] rtc: Limit frequency 2011-07-22 22:05 ` Andrew Morton 2011-07-22 22:39 ` [stable] " Willy Tarreau @ 2011-07-23 7:17 ` Thomas Gleixner 1 sibling, 0 replies; 14+ messages in thread From: Thomas Gleixner @ 2011-07-23 7:17 UTC (permalink / raw) To: Andrew Morton; +Cc: LKML, John Stultz, Ingo Molnar, Ben Greear, stable On Fri, 22 Jul 2011, Andrew Morton wrote: > On Fri, 22 Jul 2011 09:12:51 -0000 > Thomas Gleixner <tglx@linutronix.de> wrote: > > > The RTC hrtimer is self rearming. We really need to limit the > > frequency to something sensible. > > > Cc: stable@kernel.org > > Why? What failures does the current code cause? What effect do these > failures have upon users? Due to the hrtimer self rearming mode a user can DoS the machine simply because it's starved by hrtimer events. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [patch 2/3] rtc: Fix hrtimer deadlock 2011-07-22 9:12 [patch 0/3] rtc: Assorted bug fixes Thomas Gleixner 2011-07-22 9:12 ` [patch 1/3] rtc: Handle errors correctly in rtc_irq_set_state() Thomas Gleixner 2011-07-22 9:12 ` [patch 3/3] rtc: Limit frequency Thomas Gleixner @ 2011-07-22 9:12 ` Thomas Gleixner 2011-07-22 22:11 ` Andrew Morton 2 siblings, 1 reply; 14+ messages in thread From: Thomas Gleixner @ 2011-07-22 9:12 UTC (permalink / raw) To: LKML; +Cc: Andrew Morton, John Stultz, Ingo Molnar, Ben Greear, stable [-- Attachment #1: rtc-fix-hrtimer-deadlock.patch --] [-- Type: text/plain, Size: 3473 bytes --] 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(). Reported-by: Ben Greear <greearb@candelatech.com> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Cc: stable@kernel.org --- drivers/rtc/interface.c | 56 +++++++++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 19 deletions(-) Index: linux-2.6/drivers/rtc/interface.c =================================================================== --- linux-2.6.orig/drivers/rtc/interface.c +++ linux-2.6/drivers/rtc/interface.c @@ -636,6 +636,29 @@ void rtc_irq_unregister(struct rtc_devic } EXPORT_SYMBOL_GPL(rtc_irq_unregister); +static int rtc_update_hrtimer(struct rtc_device *rtc, int enabled) +{ + /* + * We unconditionally cancel the timer here, because otherwise + * 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; } - rtc->pie_enabled = enabled; -out: spin_unlock_irqrestore(&rtc->irq_task_lock, flags); - return err; } EXPORT_SYMBOL_GPL(rtc_irq_set_state); @@ -690,20 +710,18 @@ int rtc_irq_set_freq(struct rtc_device * if (freq <= 0) return -EINVAL; - +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 == 0) { + if (!err) { rtc->irq_freq = freq; - if (rtc->pie_enabled) { - ktime_t period; - hrtimer_cancel(&rtc->pie_timer); - period = ktime_set(0, NSEC_PER_SEC/rtc->irq_freq); - hrtimer_start(&rtc->pie_timer, period, - HRTIMER_MODE_REL); + if (rtc->pie_enabled && rtc_update_hrtimer(rtc, 1) < 0) { + spin_unlock_irqrestore(&rtc->irq_task_lock, flags); + cpu_relax(); + goto retry; } } spin_unlock_irqrestore(&rtc->irq_task_lock, flags); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch 2/3] rtc: Fix hrtimer deadlock 2011-07-22 9:12 ` [patch 2/3] rtc: Fix hrtimer deadlock Thomas Gleixner @ 2011-07-22 22:11 ` Andrew Morton 2011-07-23 7:22 ` Thomas Gleixner 0 siblings, 1 reply; 14+ messages in thread From: Andrew Morton @ 2011-07-22 22:11 UTC (permalink / raw) To: Thomas Gleixner; +Cc: LKML, John Stultz, Ingo Molnar, Ben Greear, stable 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"? > + * 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? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch 2/3] rtc: Fix hrtimer deadlock 2011-07-22 22:11 ` Andrew Morton @ 2011-07-23 7:22 ` Thomas Gleixner 0 siblings, 0 replies; 14+ messages in thread From: Thomas Gleixner @ 2011-07-23 7:22 UTC (permalink / raw) To: Andrew Morton; +Cc: LKML, John Stultz, Ingo Molnar, Ben Greear, stable 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2011-08-06 7:35 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-07-22 9:12 [patch 0/3] rtc: Assorted bug fixes Thomas Gleixner 2011-07-22 9:12 ` [patch 1/3] rtc: Handle errors correctly in rtc_irq_set_state() Thomas Gleixner 2011-07-22 22:04 ` Andrew Morton 2011-07-23 7:15 ` Thomas Gleixner 2011-07-22 9:12 ` [patch 3/3] rtc: Limit frequency Thomas Gleixner 2011-07-22 22:05 ` Andrew Morton 2011-07-22 22:39 ` [stable] " Willy Tarreau 2011-08-05 3:39 ` Joshua Kinard 2011-08-05 9:04 ` John Stultz 2011-08-06 7:28 ` Joshua Kinard 2011-07-23 7:17 ` Thomas Gleixner 2011-07-22 9:12 ` [patch 2/3] rtc: Fix hrtimer deadlock Thomas Gleixner 2011-07-22 22:11 ` Andrew Morton 2011-07-23 7:22 ` Thomas Gleixner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.