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 <[email protected]>
---
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);
--
1.5.6.3
On Tue, 7 Apr 2009 01:50:31 +0900
Atsushi Nemoto <[email protected]> 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 <[email protected]>
> ---
> 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);
On Thu, 9 Apr 2009 15:39:21 -0700
Andrew Morton <[email protected]> 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.
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
[Add CCs to authers or original committers of each mentioned driver]
On Fri, 10 Apr 2009 00:58:20 +0200, Alessandro Zummo <[email protected]> wrote:
> On Thu, 9 Apr 2009 15:39:21 -0700
> Andrew Morton <[email protected]> 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
On Thu, 23 Apr 2009 23:51:41 +0900 (JST)
Atsushi Nemoto <[email protected]> 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?
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
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
On Thu, 23 Apr 2009 17:02:53 +0200, Alessandro Zummo <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
On Thu, 23 Apr 2009 11:07:38 -0400
Mike Frysinger <[email protected]> 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
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
On Fri, 24 Apr 2009 00:29:00 +0900 (JST)
Atsushi Nemoto <[email protected]> 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 :)
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
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
On Thu, 23 Apr 2009 12:30:58 -0400
Mike Frysinger <[email protected]> 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.
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
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
On Thu, 23 Apr 2009 11:15:56 -0700
David Brownell <[email protected]> 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? :)
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
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;
On Thu, 23 Apr 2009 12:02:48 -0700
David Brownell <[email protected]> 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.
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
On Thursday 23 April 2009, Alessandro Zummo wrote:
> On Thu, 23 Apr 2009 12:02:48 -0700
> David Brownell <[email protected]> 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...
On Thu, 23 Apr 2009 12:25:01 -0700
David Brownell <[email protected]> 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?
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
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.
On Thu, 23 Apr 2009 12:45:32 -0700
David Brownell <[email protected]> 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?
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
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.
On Fri, 24 Apr 2009 02:31:12 -0700
David Brownell <[email protected]> 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.
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
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.
On Fri, 24 Apr 2009 04:10:51 -0700
David Brownell <[email protected]> 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?
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
On Fri, 24 Apr 2009 13:13:34 +0200, Alessandro Zummo <[email protected]> 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
On Sat, 25 Apr 2009 01:48:50 +0900 (JST), Atsushi Nemoto <[email protected]> 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
On Sat, 25 Apr 2009 02:06:12 +0900 (JST)
Atsushi Nemoto <[email protected]> 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?
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
On Fri, 24 Apr 2009 21:41:17 +0200, Alessandro Zummo <[email protected]> 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
On Sat, 25 Apr 2009 02:06:12 +0900 (JST)
Atsushi Nemoto <[email protected]> wrote:
> On Sat, 25 Apr 2009 01:48:50 +0900 (JST), Atsushi Nemoto
> <[email protected]> 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.
--
Best regards,
Hans-Christian Egtvedt
On Tue, 28 Apr 2009 15:13:53 +0200, Hans-Christian Egtvedt <[email protected]> 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