| Message ID | 1239036633-10032-1-git-send-email-anemo@mba.ocn.ne.jp |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Tue, 7 Apr 2009 01:50:31 +0900 Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote: > The rtc_update_irq() might be called with irqs enabled, if a interrupt > handler was registered without IRQF_DISABLED. Why? What are the consequences of not merging the patch? Is it a bugfix? If so, what are the user-visible effects of the bug? See, I need to decide if this patch is needed in 2.6.30 (and earlier), but the changelog doesn't include enough info to make that decision. > Use spin_lock_irqsave/spin_unlock_irqrestore instead of > spin_lock/spin_unlock. > > Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp> > --- > drivers/rtc/interface.c | 10 ++++++---- > 1 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c > index 4348c4b..a8641f7 100644 > --- a/drivers/rtc/interface.c > +++ b/drivers/rtc/interface.c > @@ -376,14 +376,16 @@ EXPORT_SYMBOL_GPL(rtc_update_irq_enable); > void rtc_update_irq(struct rtc_device *rtc, > unsigned long num, unsigned long events) > { > - spin_lock(&rtc->irq_lock); > + unsigned long flags; > + > + spin_lock_irqsave(&rtc->irq_lock, flags); > rtc->irq_data = (rtc->irq_data + (num << 8)) | events; > - spin_unlock(&rtc->irq_lock); > + spin_unlock_irqrestore(&rtc->irq_lock, flags); > > - spin_lock(&rtc->irq_task_lock); > + spin_lock_irqsave(&rtc->irq_task_lock, flags); > if (rtc->irq_task) > rtc->irq_task->func(rtc->irq_task->private_data); > - spin_unlock(&rtc->irq_task_lock); > + spin_unlock_irqrestore(&rtc->irq_task_lock, flags); > > wake_up_interruptible(&rtc->irq_queue); > kill_fasync(&rtc->async_queue, SIGIO, POLL_IN); --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
On Thu, 9 Apr 2009 15:39:21 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > > The rtc_update_irq() might be called with irqs enabled, if a interrupt > > handler was registered without IRQF_DISABLED. > > Why? What are the consequences of not merging the patch? Is it a > bugfix? If so, what are the user-visible effects of the bug? rtc_update_irq() is called by a driver, and a driver is supposed to know when it's doing the call. The driver can either use IRQF_DISABLED or disable the interrupts in some other ways. I also suspect this is some legacy we are carrying on, so it's better to leave the decision on the interrupt handling to the driver itself. Unless I'm missing something.
[Add CCs to authers or original committers of each mentioned driver] On Fri, 10 Apr 2009 00:58:20 +0200, Alessandro Zummo <alessandro.zummo@towertech.it> wrote: > On Thu, 9 Apr 2009 15:39:21 -0700 > Andrew Morton <akpm@linux-foundation.org> wrote: > > > > The rtc_update_irq() might be called with irqs enabled, if a interrupt > > > handler was registered without IRQF_DISABLED. > > > > Why? What are the consequences of not merging the patch? Is it a > > bugfix? If so, what are the user-visible effects of the bug? > > rtc_update_irq() is called by a driver, and a driver > is supposed to know when it's doing the call. > > The driver can either use IRQF_DISABLED or disable the > interrupts in some other ways. > > I also suspect this is some legacy we are carrying on, > so it's better to leave the decision on the interrupt > handling to the driver itself. > > Unless I'm missing something. Then here is list of (potentialy) broken rtc drivers: rtc-at32ap700x.c rtc-bfin.c rtc-m48t59.c rtc-pcf50633.c rtc-twl4030.c rtc-wm8350.c I'm not sure there are any real problem on these drivers. It seems IRQF_DISABLED would be suitable for at32ap700x, bfin and m48t59, and local_irq_disable would be suitable for others. The IRQF_DISABLED fixes would be better regardless of the rtc_update_irq() API change. And local_irq_disable fixes are not needed (and should be reverted) if the API change was acked, but no harm for short term fix. Note that just adding IRQF_DISABLED will cause "IRQF_DISABLED is not guaranteed on shared IRQs" warning. So you should consider of removing IRQF_SHARED, or finding other way. --- Atsushi Nemoto --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
On Thu, 23 Apr 2009 23:51:41 +0900 (JST) Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote: > The IRQF_DISABLED fixes would be better regardless of the > rtc_update_irq() API change. And local_irq_disable fixes are not > needed (and should be reverted) if the API change was acked, but no > harm for short term fix. As I said in my last email to Andrew, I think we can call rtc_update_irq with irqs enabled and we don't probably need any IRQF_ to request_irq . Are you willing to make some tests in that direction with your drivers?
On Thu, Apr 23, 2009 at 11:02, Alessandro Zummo wrote: > On Thu, 23 Apr 2009 23:51:41 +0900 (JST) Atsushi Nemoto wrote: >> The IRQF_DISABLED fixes would be better regardless of the >> rtc_update_irq() API change. And local_irq_disable fixes are not >> needed (and should be reverted) if the API change was acked, but no >> harm for short term fix. > > As I said in my last email to Andrew, I think we can call > rtc_update_irq with irqs enabled and we don't probably need > any IRQF_ to request_irq . > > Are you willing to make some tests in that direction with your > drivers? we just removed the shared bit from the Blackfin rtc driver because it didnt really make sense for us. i need to test something else, so is the only change you need is the one posted originally ? that makes more sense to me than forcing everyone to use IRQF_DISABLED. -mike --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
On Thu, 23 Apr 2009 17:02:53 +0200, Alessandro Zummo <alessandro.zummo@towertech.it> wrote: > As I said in my last email to Andrew, I think we can call > rtc_update_irq with irqs enabled and we don't probably need > any IRQF_ to request_irq . > > Are you willing to make some tests in that direction with your > drivers? Yes, if we had consensus of the API change. But since all my drivers have IRQF_DISABLED and I don't want to drop them, I'm not a good tester for this ;) On Thu, 23 Apr 2009 11:07:38 -0400, Mike Frysinger <vapier.adi@gmail.com> wrote: > we just removed the shared bit from the Blackfin rtc driver because it > didnt really make sense for us. i need to test something else, so is > the only change you need is the one posted originally ? that makes > more sense to me than forcing everyone to use IRQF_DISABLED. My original patch should not be merged as is, as David said in other mail: On Thu, 9 Apr 2009 16:27:15 -0700, David Brownell <david-b@pacbell.net> wrote: > Any driver doing that right now is by definition buggy; that > function is clearly defined to need IRQs blocked: > > /** > * rtc_update_irq - report RTC periodic, alarm, and/or update irqs > * @rtc: the rtc device > * @num: how many irqs are being reported (usually one) > * @events: mask of RTC_IRQF with one or more of RTC_PF, RTC_AF, RTC_UF > * Context: in_interrupt(), irqs blocked > */ > > If you're going to change the interface, do it right... > update that kerneldoc and drivers like rtc-ds130[57], > rtc-ds1374, and rtc-test which do extra work to follow > the current interface spec. I agree on this. --- Atsushi Nemoto --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
On Thu, 23 Apr 2009 11:07:38 -0400 Mike Frysinger <vapier.adi@gmail.com> wrote: > we just removed the shared bit from the Blackfin rtc driver because it > didnt really make sense for us. i need to test something else, so is > the only change you need is the one posted originally ? that makes > more sense to me than forcing everyone to use IRQF_DISABLED. for the blackfin you should just check if you have locking issues in your irq routine
On Fri, 24 Apr 2009 00:29:00 +0900 (JST) Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote: > Are you willing to make some tests in that direction with your > > drivers? > > Yes, if we had consensus of the API change. But since all my drivers > have IRQF_DISABLED and I don't want to drop them, I'm not a good > tester for this ;) Is not an API change, we are gradually relaxing it if it proves workable :)
On Thu, Apr 23, 2009 at 11:37, Alessandro Zummo wrote: > On Thu, 23 Apr 2009 11:07:38 -0400 Mike Frysinger wrote: >> we just removed the shared bit from the Blackfin rtc driver because it >> didnt really make sense for us. i need to test something else, so is >> the only change you need is the one posted originally ? that makes >> more sense to me than forcing everyone to use IRQF_DISABLED. > > for the blackfin you should just check if > you have locking issues in your irq routine the Blackfin driver calls rtc_update_irq() from its IRQ handler and the handler is not registered with IRQF_DISABLED. it makes more sense to me to fix rtc_update_irq() than require all RTC drivers to register with IRQF_DISABLED. especially in my case as the Blackfin driver itself doesnt need any locks. -mike --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
On Thu, 23 Apr 2009 12:30:58 -0400 Mike Frysinger <vapier.adi@gmail.com> wrote: > for the blackfin you should just check if > > you have locking issues in your irq routine > > the Blackfin driver calls rtc_update_irq() from its IRQ handler and > the handler is not registered with IRQF_DISABLED. it makes more sense > to me to fix rtc_update_irq() than require all RTC drivers to register > with IRQF_DISABLED. especially in my case as the Blackfin driver > itself doesnt need any locks. I agree. rtc_update_irq is not problematic by itself, it just takes locks that other parts of the driver should take appropriately. For example, if you take irq_lock with irqs enabled you will get in trouble. blackfin seems ok, for the other drivers we need to verify this.
On Thursday 23 April 2009, Alessandro Zummo wrote: > Is not an API change, we are gradually relaxing it if it > proves workable :) Erm, it *is* an API change at least to the extent that the last version of the patch made the interface spec become incorrect. Current interface spec *requires* that function to be called with IRQs disabled. The downside of that spec is that there's no way to test it, since CONFIG_LOCKDEP doesn't understand that almost all IRQ handlers don't disable IRQs. So there are some bugs in RTC drivers that can only be uncovered by code review. - Dave --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
On Thu, 23 Apr 2009 11:15:56 -0700 David Brownell <david-b@pacbell.net> wrote: > On Thursday 23 April 2009, Alessandro Zummo wrote: > > Is not an API change, we are gradually relaxing it if it > > proves workable :) > > Erm, it *is* an API change at least to the extent that > the last version of the patch made the interface spec > become incorrect. I know, I was just playing it down :) > Current interface spec *requires* that function to be > called with IRQs disabled. > > The downside of that spec is that there's no way to test > it, since CONFIG_LOCKDEP doesn't understand that almost > all IRQ handlers don't disable IRQs. So there are some > bugs in RTC drivers that can only be uncovered by code > review. That's was what I was proposing. I'll give a code review but will not be able to test every driver so I'll need help from the authors. But first I need someone to validate the theory that says that we don't really need the IRQs to be disabled, as I stated in the email to Andrew. do you agree? :)
On Thursday 23 April 2009, Alessandro Zummo wrote: > But first I need someone to validate the theory that > says that we don't really need the IRQs to be disabled, > as I stated in the email to Andrew. > > do you agree? :) If (rtc_update_irq() starts disabling them locally, && its interface stops requiring callers to do that) then it's fine; --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
On Thu, 23 Apr 2009 12:02:48 -0700 David Brownell <david-b@pacbell.net> wrote: > > as I stated in the email to Andrew. > > > > do you agree? :) > > If (rtc_update_irq() starts disabling them locally, > && its interface stops requiring callers to do that) > then > it's fine; > but why rtc_update_irq should disable the IRQs? I believe it can work just fine with IRQs enabled.
On Thursday 23 April 2009, Alessandro Zummo wrote: > On Thu, 23 Apr 2009 12:02:48 -0700 > David Brownell <david-b@pacbell.net> wrote: > > > > as I stated in the email to Andrew. > > > > > > do you agree? :) > > > > If (rtc_update_irq() starts disabling them locally, > > && its interface stops requiring callers to do that) > > then > > it's fine; > > > > but why rtc_update_irq should disable the IRQs? I believe > it can work just fine with IRQs enabled. The last patch I saw was just changing from "caller must disable them" to "rtc_update_irq() disables them". If you're talking about a different patch, please forward... --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
On Thu, 23 Apr 2009 12:25:01 -0700 David Brownell <david-b@pacbell.net> wrote: > > but why rtc_update_irq should disable the IRQs? I believe > > it can work just fine with IRQs enabled. > > The last patch I saw was just changing from "caller must > disable them" to "rtc_update_irq() disables them". > > If you're talking about a different patch, please forward... no patch, just theory. the question is, do we need IRQs disabled when calling rtc_update_irq? and if yes, why? to prevent what?
On Thursday 23 April 2009, Alessandro Zummo wrote: > > > If you're talking about a different patch, please forward... > > no patch, just theory. > > the question is, do we need IRQs disabled when > calling rtc_update_irq? If the spinlock is *ever* acquired with IRQs disabled, it must *always* be acquired that way. The typical use is ... from IRQ context, which will in some cases mean IRQs disabled. QED. > and if yes, why? to prevent what? Consider: one context grabs spinlock with IRQs enabled. IRQ arrives. That context tries to grab that same lock, from the same CPU. ==> Self-deadlock. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
On Thu, 23 Apr 2009 12:45:32 -0700 David Brownell <david-b@pacbell.net> wrote: > If the spinlock is *ever* acquired with IRQs disabled, > it must *always* be acquired that way. > > The typical use is ... from IRQ context, which will in > some cases mean IRQs disabled. QED. the spinlock could be acquired with IRQs disabled, with spin_lock_irq, in the alarm setup functions and acquired with the standard spinlock calls in the irq handler. would it work?
On Thursday 23 April 2009, Alessandro Zummo wrote: > > If the spinlock is *ever* acquired with IRQs disabled, > > it must *always* be acquired that way. > > > > The typical use is ... from IRQ context, which will in > > some cases mean IRQs disabled. QED. > > the spinlock could be acquired with IRQs disabled, > with spin_lock_irq, in the alarm setup functions I don't want to make time for a re-audit of this spinlock's usage just now. > and > acquired with the standard spinlock calls in the irq > handler. Which "standard" call? "spin_lock", "spin_lock_irqsave", and "spin_lock_irq" are all standard calls. Recall that it's not the IRQ handler that's directly grabbing the lock; it's code called by that handler. > would it work? The patch I saw -- switching rtc_update_irq() to use spin_lock_irqsave() -- would work, but it's incomplete. It left the doc broken, and didn't clean up the drivers which did the real work to obey the current call spec. It'd be much nicer if lockdep would just do the right thing and report when IRQ handlers do stupid things, instead of covering up that class of bugs. But I'm told it will not get fixed; sigh. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
On Fri, 24 Apr 2009 02:31:12 -0700 David Brownell <david-b@pacbell.net> wrote: > > and > > acquired with the standard spinlock calls in the irq > > handler. > > Which "standard" call? "spin_lock", "spin_lock_irqsave", > and "spin_lock_irq" are all standard calls. sorry, I mean the no irq version of the call. > Recall that it's not the IRQ handler that's directly > grabbing the lock; it's code called by that handler. > > would it work? > > The patch I saw -- switching rtc_update_irq() to use > spin_lock_irqsave() -- would work, but it's incomplete. > It left the doc broken, and didn't clean up the drivers > which did the real work to obey the current call spec. that patch is a no go. period. I'm not talking about it. > It'd be much nicer if lockdep would just do the right > thing and report when IRQ handlers do stupid things, > instead of covering up that class of bugs. But I'm told > it will not get fixed; sigh. I'm not tying to fix call issues or lockdep politics, just to understand if it's possible to avoid disabling the IRQs. i.e., use spin_lock() in the IRQ handler and spin_lock_irq/irq_save in the setup functions.
On Friday 24 April 2009, Alessandro Zummo wrote: > I'm not tying to fix call issues or lockdep politics, > just to understand if it's possible to avoid disabling the IRQs. > > i.e., > > use spin_lock() in the IRQ handler and spin_lock_irq/irq_save > in the setup functions. I think you're describing how the *current* scheme is supposed to work ... except that some IRQ handlers aren't calling the rtc_update_irq() routine with IRQs blocked. Yes, that current scheme works ... modulo those buggy handlers. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
On Fri, 24 Apr 2009 04:10:51 -0700 David Brownell <david-b@pacbell.net> wrote: > > use spin_lock() in the IRQ handler and spin_lock_irq/irq_save > > in the setup functions. > > I think you're describing how the *current* scheme is supposed > to work ... except that some IRQ handlers aren't calling the > rtc_update_irq() routine with IRQs blocked. > > Yes, that current scheme works ... modulo those buggy handlers. ok, but why it's necessary to disable the interrupts? Only because the specs says so or because there's a locking issue I'm missing?
On Fri, 24 Apr 2009 13:13:34 +0200, Alessandro Zummo <alessandro.zummo@towertech.it> wrote: > > > use spin_lock() in the IRQ handler and spin_lock_irq/irq_save > > > in the setup functions. > > > > I think you're describing how the *current* scheme is supposed > > to work ... except that some IRQ handlers aren't calling the > > rtc_update_irq() routine with IRQs blocked. > > > > Yes, that current scheme works ... modulo those buggy handlers. > > ok, but why it's necessary to disable the interrupts? Only because > the specs says so or because there's a locking issue I'm missing? Here is a possible example: 1. RTC alarm interrupt handler takes rtc->irq_lock by spin_lock() 2. A timer interrupt handler calls rtc_uie_timer() for UIE emulation 3. rtc_uie_timer() waits on rtc->irq_lock .... deadlock! This sort of deadlock can happen if a spin lock was used by multple interrupt handlers. --- Atsushi Nemoto --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
On Sat, 25 Apr 2009 01:48:50 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote: > Here is a possible example: > > 1. RTC alarm interrupt handler takes rtc->irq_lock by spin_lock() > 2. A timer interrupt handler calls rtc_uie_timer() for UIE emulation > 3. rtc_uie_timer() waits on rtc->irq_lock .... deadlock! Oops, this is wrong. This deadlock cannot happen since rtc_uie_timer() will be called in bh (softirq) context, not interrupt context. Anyway, I just posted updated patch. Please take a look. Thanks. --- Atsushi Nemoto --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
On Sat, 25 Apr 2009 02:06:12 +0900 (JST) Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote: > RTC alarm interrupt handler takes rtc->irq_lock by spin_lock() > > 2. A timer interrupt handler calls rtc_uie_timer() for UIE emulation > > 3. rtc_uie_timer() waits on rtc->irq_lock .... deadlock! > > Oops, this is wrong. This deadlock cannot happen since > rtc_uie_timer() will be called in bh (softirq) context, not interrupt > context. Correct. And we have only one irq handler per driver. Anything else?
On Fri, 24 Apr 2009 21:41:17 +0200, Alessandro Zummo <alessandro.zummo@towertech.it> wrote: > > RTC alarm interrupt handler takes rtc->irq_lock by spin_lock() > > > 2. A timer interrupt handler calls rtc_uie_timer() for UIE emulation > > > 3. rtc_uie_timer() waits on rtc->irq_lock .... deadlock! > > > > Oops, this is wrong. This deadlock cannot happen since > > rtc_uie_timer() will be called in bh (softirq) context, not interrupt > > context. > > Correct. > > And we have only one irq handler per driver. Anything else? Some drivers (omap, pxa, etc.) have multiple irq handler. Currently all such drivers use IRQF_DISABLED so I do not see any real problem now. So I think my patch is rather "cleanup and bulletproof" than "bugfix". --- Atsushi Nemoto --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
On Sat, 25 Apr 2009 02:06:12 +0900 (JST) Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote: > On Sat, 25 Apr 2009 01:48:50 +0900 (JST), Atsushi Nemoto > <anemo@mba.ocn.ne.jp> wrote: > > Here is a possible example: > > > > 1. RTC alarm interrupt handler takes rtc->irq_lock by spin_lock() > > 2. A timer interrupt handler calls rtc_uie_timer() for UIE emulation > > 3. rtc_uie_timer() waits on rtc->irq_lock .... deadlock! > > Oops, this is wrong. This deadlock cannot happen since > rtc_uie_timer() will be called in bh (softirq) context, not interrupt > context. > > Anyway, I just posted updated patch. Please take a look. Thanks. > AVR32 has interrupts disabled during the interrupt handler, so AFAICT the rtc-at32ap700x.c should be fine.
On Tue, 28 Apr 2009 15:13:53 +0200, Hans-Christian Egtvedt <hans-christian.egtvedt@atmel.com> wrote: > AVR32 has interrupts disabled during the interrupt handler, so AFAICT > the rtc-at32ap700x.c should be fine. If IRQF_DISABLED was not set, handle_IRQ_event() enables interrupts on AVR32, as well as other archs, no? Anyway rtc-at32ap700x uses only one interrupt so there should be no real problem. --- Atsushi Nemoto --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c index 4348c4b..a8641f7 100644 --- a/drivers/rtc/interface.c +++ b/drivers/rtc/interface.c @@ -376,14 +376,16 @@ EXPORT_SYMBOL_GPL(rtc_update_irq_enable); void rtc_update_irq(struct rtc_device *rtc, unsigned long num, unsigned long events) { - spin_lock(&rtc->irq_lock); + unsigned long flags; + + spin_lock_irqsave(&rtc->irq_lock, flags); rtc->irq_data = (rtc->irq_data + (num << 8)) | events; - spin_unlock(&rtc->irq_lock); + spin_unlock_irqrestore(&rtc->irq_lock, flags); - spin_lock(&rtc->irq_task_lock); + spin_lock_irqsave(&rtc->irq_task_lock, flags); if (rtc->irq_task) rtc->irq_task->func(rtc->irq_task->private_data); - spin_unlock(&rtc->irq_task_lock); + spin_unlock_irqrestore(&rtc->irq_task_lock, flags); wake_up_interruptible(&rtc->irq_queue); kill_fasync(&rtc->async_queue, SIGIO, POLL_IN);
The rtc_update_irq() might be called with irqs enabled, if a interrupt handler was registered without IRQF_DISABLED. Use spin_lock_irqsave/spin_unlock_irqrestore instead of spin_lock/spin_unlock. Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp> --- drivers/rtc/interface.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)