On Fri, 2009-02-27 at 15:18 -0800, David Brownell wrote:
>
> This stuff just pokes at some annoying current gaps in the
> IRQ framework. I'll be glad when eventually there's no
> need to work around those weaknesses ... that is, when
> real threaded IRQ support is available.
Its unfortunate that you prefer these dinky little hacks over helping
out providing whatever infrastructure you need.
There's plenty good reasons for mandating that irq handlers run with
irqs disabled, if you need threaded handlers -- that's fine, but then
teach the generic code about them.
What you do _NOT_ do is hack your way around things, that's not how
Linux works, and by doing that you make the world a slightly worse place
for everyone.
On Monday 02 March 2009, Peter Zijlstra wrote:
> On Fri, 2009-02-27 at 15:18 -0800, David Brownell wrote:
> >
> > This stuff just pokes at some annoying current gaps in the
> > IRQ framework. I'll be glad when eventually there's no
> > need to work around those weaknesses ... that is, when
> > real threaded IRQ support is available.
>
> Its unfortunate that you prefer these dinky little hacks over
> helping out providing whatever infrastructure you need.
That's an unfair and unreasonable criticism. You don't
know what I "prefer", or how much time I have to "help"
with. Plus, you discount the work involved in actually
tracking down root causes ... and your evaluation of at
least this issue is biased.
What's unfortunate is that you prefer not to fix that
IRQF_DISABLED bug in lockdep, which you co-"maintain".
When running with lockdep, that bug (a) introduces bugs
in some drivers and (b) hides bugs in others. You've
rejected even a minimal warning fix, to help minimize
the amount of time developers waste on (a) and (b).
Attacking folk for having to cope with such bugs escalates
things beyond "unfortunate". If lockdep is "maintained",
your response should be fixing that lockdep bug. Once
that's done, all workarounds for that bug can be removed.
> There's plenty good reasons for mandating that irq handlers run with
> irqs disabled, if you need threaded handlers -- that's fine, but then
> teach the generic code about them.
There are many ways to "teach" such things, of course,
and the learning goes both ways ... the initial set of
perceived requirements changes when
And since there have been folk at work on such issues for
some time, I'm not sure why you think I shouldn't give
them a fair chance to deliver, and help improve those
(long anticipated) solutions. There's a saying that
"too many cooks spoil the broth"; and in software, it's
well known that extra developers aren't necessarily any
kind of help.
> What you do _NOT_ do is hack your way around things, that's not how
> Linux works, and by doing that you make the world a slightly worse place
> for everyone.
Linux has lots of bug workarounds and odd constraints, like
any other large system does. The "world" is full of them;
DNA-coded machinery is felt to consist *mostly* of them.
(And many folk argue that monoculture is dangerous...)
The sign of a bad workaround is that it sits in core code
and impacts everything using it. Like, oh, that lockdep
bug, preventing various systems from ever using it because
they rely on mechanisms broken by that bug.
On the other hand, the main valid complaint about one-line
workarounds is needing them at all. Boiling down a system
misbehavior to a one-line workaround *IS* a way to help
make the (kernel) world a better place, by fingerpointing
a real issue ... setting trail markers for a better way.
In this case there's a real bug in the lockdep code.
The recent threaded IRQ proposal moves some support down
a layer (into genirq) and generalizes it a bit, but some
drivers (e.g. I2C ones) have threaded IRQs for a long time
now -- out of necessity, not just performance tweaking.
- Dave
On Mon, 2009-03-02 at 14:10 -0800, David Brownell wrote:
> What's unfortunate is that you prefer not to fix that
> IRQF_DISABLED bug in lockdep, which you co-"maintain".
> When running with lockdep, that bug (a) introduces bugs
> in some drivers and (b) hides bugs in others. You've
> rejected even a minimal warning fix, to help minimize
> the amount of time developers waste on (a) and (b).
I've come to the conclusion that the only technically sound solution is
to do as I proposed today, utterly eliminate !IRQF_DISABLED handlers.
Apparently you had enough time to come up with the creative genirq abuse
of twl4030, I think that with a similar effort you could have
implemented generic threaded irq stuff like proposed by Thomas.
> Attacking folk for having to cope with such bugs escalates
> things beyond "unfortunate". If lockdep is "maintained",
> your response should be fixing that lockdep bug. Once
> that's done, all workarounds for that bug can be removed.
I state there is no lockdep bug in this respect. The bug is trying to
enable interrupts from hardirq context and running code that assumes
hardirq context from task context.
On Monday 02 March 2009, Peter Zijlstra wrote:
> On Mon, 2009-03-02 at 14:10 -0800, David Brownell wrote:
>
> > What's unfortunate is that you prefer not to fix that
> > IRQF_DISABLED bug in lockdep, which you co-"maintain".
> > When running with lockdep, that bug (a) introduces bugs
> > in some drivers and (b) hides bugs in others. You've
> > rejected even a minimal warning fix, to help minimize
> > the amount of time developers waste on (a) and (b).
>
> I've come to the conclusion that the only technically sound solution is
> to do as I proposed today, utterly eliminate !IRQF_DISABLED handlers.
As you announced today. If you truly believe that, then
you should at least submit a warning patch for 2.6.29-rc
("driver X isn't setting IRQF_DISABLED, reimplement!")
with a Documentation/feature-removal-schedule.txt plan
for removing that mechanism. And maybe updating the
handling of IRQF_SHARED at the same time... most shared
IRQs don't want IRQF_DISABLED. And ... surely more,
even just to start phasing out such a longstanding
mechanism.
(Or more likely, start a real LKML discussion on that
specific topic. I suspect it won't complete in time
to merge such a patch with any sort of consensus.)
> Apparently you had enough time to come up with the creative genirq abuse
> of twl4030, I think that with a similar effort you could have
> implemented generic threaded irq stuff like proposed by Thomas.
Actually, I made time to clean that code up a lot;
I didn't come up with that original stuff at all.
Why would you think otherwise, after reading the
copyrights at the top of twl4030-irq.c ??
And when I was doing that cleanup, the Official Plan
was that Thomas' code would be ready for merge into
at least the -next tree soon. I don't know about
you, but stepping all over his work didn't seem like
it would be at all constructive. Having a significant
driver be ready to try using it ... seemed like a
more productive approach.
> > Attacking folk for having to cope with such bugs escalates
> > things beyond "unfortunate". If lockdep is "maintained",
> > your response should be fixing that lockdep bug. Once
> > that's done, all workarounds for that bug can be removed.
>
> I state there is no lockdep bug in this respect. The bug is trying to
> enable interrupts from hardirq context and running code that assumes
> hardirq context from task context.
Well, I state that you're wrong about those points...
Regardless of what either of us states, the truth of
the matter is that there are drivers that don't work
with CONFIG_LOCKDEP and the *only* reason is the code
removed by the (untested) patchlet below: semantics
of irqs change when lockdep is enabled. (That is, your
characterization above is incorrect.) I call that a
lockdep bug; you don't want to accept that label.
Are those incorrect lockdep assumptions really so hard
to fix? Where do they surface? I scanned lockdep.c
briefly, and it talks about "hardirq" in several places.
Is it conflating that with "irqs disabled"? The two
concepts should be distinct.
- Dave
---
kernel/irq/manage.c | 6 ------
1 file changed, 6 deletions(-)
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -816,12 +816,6 @@ int request_irq_quickcheck(unsigned int
"guaranteed on shared IRQs\n",
irq, devname);
-#ifdef CONFIG_LOCKDEP
- /*
- * Lockdep wants atomic interrupt handlers:
- */
- irqflags |= IRQF_DISABLED;
-#endif
/*
* Sanity-check: shared interrupts must pass in a real dev-ID,
* otherwise we'll have trouble later trying to figure out
* David Brownell <[email protected]> wrote:
> On Monday 02 March 2009, Peter Zijlstra wrote:
> > On Mon, 2009-03-02 at 14:10 -0800, David Brownell wrote:
> >
> > > What's unfortunate is that you prefer not to fix that
> > > IRQF_DISABLED bug in lockdep, which you co-"maintain".
> > > When running with lockdep, that bug (a) introduces bugs
> > > in some drivers and (b) hides bugs in others. You've
> > > rejected even a minimal warning fix, to help minimize
> > > the amount of time developers waste on (a) and (b).
> >
> > I've come to the conclusion that the only technically sound solution is
> > to do as I proposed today, utterly eliminate !IRQF_DISABLED handlers.
>
> As you announced today. If you truly believe that, then
> you should at least submit a warning patch for 2.6.29-rc
> ("driver X isn't setting IRQF_DISABLED, reimplement!")
i have changed the BUG_ON() to a WARN_ONCE() message so the
warning is in place now.
> with a Documentation/feature-removal-schedule.txt plan for
> removing that mechanism. [...]
you are misunderstanding the workings and purpose of
feature-removal-schedule.txt. It is mainly used for
functionality that is user-visible. It is sometimes used for
functionality that a subsystem has exported to a lot of drivers
consciously and which is being removed.
It is never used for a single driver finding a core kernel
symbol and abusing it in a way that was never intended. Abuse of
kernel internals by kernel code was never a 'feature'. Just
because you find a symbol (which is not even exported to
drivers) does not mean you can use it like that.
If you want to work on genirq threaded IRQ handlers them please
check out and test the threaded IRQ handlers patches that are
being worked on at lkml. See:
[patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2
Ingo
On Monday 02 March 2009, Ingo Molnar wrote:
> If you want to work on genirq threaded IRQ handlers them please
> check out and test the threaded IRQ handlers patches that are
> being worked on at lkml. See:
>
> [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2
I did check them out, as noted earlier in this thread.
The significant omission is lack of support for chaining
such threads. Example, an I2C device that exposes
several dozen IRQs with mask/ack/... operations that
require I2C access.
I'm not sure what Thomas intends to do with that issue,
if anything. It does touch on messy bits of genirq.
Those V2 patches do look to handle simple cases well,
of the flavor that's often handled today by creating
a singlethreaded workqueue in the driver. I think
it's good to have such support, but that's not enough
to handle the hardware I've come across.
- Dave
On Monday 02 March 2009, Ingo Molnar wrote:
>
> > > > What's unfortunate is that you prefer not to fix that
> > > > IRQF_DISABLED bug in lockdep, which you co-"maintain".
> > > > When running with lockdep, that bug (a) introduces bugs
> > > > in some drivers and (b) hides bugs in others. ?You've
> > > > rejected even a minimal warning fix, to help minimize
> > > > the amount of time developers waste on (a) and (b).
> > >
> > > I've come to the conclusion that the only technically sound solution is
> > > to do as I proposed today, utterly eliminate !IRQF_DISABLED handlers.
> >
> > As you announced today. ?If you truly believe that, then
> > you should at least submit a warning patch for 2.6.29-rc
> > ("driver X isn't setting IRQF_DISABLED, reimplement!")
>
> i have changed the BUG_ON() to a WARN_ONCE() message so the
> warning is in place now.
The patch Peter sent doesn't relate in the least to removing
the IRQF_DISABLED flag though. Patches addressing that
would be in setup_irq() code paths not IRQ dispatch.
> > with a Documentation/feature-removal-schedule.txt plan for
> > removing that mechanism. [...]
>
> you are misunderstanding the workings and purpose of
> feature-removal-schedule.txt. It is mainly used for
> functionality that is user-visible.
If by "user" you include "kernel developers", yes;
otherwise, I'd dispute "mainly". The first several
entries right now relate to kernel interfaces, as
do quite a lot of the others.
> It is sometimes used for
> functionality that a subsystem has exported to a lot of drivers
> consciously and which is being removed.
The IRQ framework has very consciously exported IRQF_DISABLED.
That functionality has been around for a very long time; I'm
thinking at least ten years now.
So removing IRQF_DISABLED -- if it's even agreed to be
a good idea, which seems to be a minority opinion so far
on this thread -- is very much the sort of thing one would
expect to appear in that schedule.
* David Brownell <[email protected]> wrote:
> On Monday 02 March 2009, Ingo Molnar wrote:
> > If you want to work on genirq threaded IRQ handlers them please
> > check out and test the threaded IRQ handlers patches that are
> > being worked on at lkml. See:
> >
> > [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2
>
> I did check them out, as noted earlier in this thread.
>
> The significant omission is lack of support for chaining
> such threads. Example, an I2C device that exposes
> several dozen IRQs with mask/ack/... operations that
> require I2C access.
Well, those are rarely used, embedded-only constructs - the main
focus of IRQ threading patches are the more common patterns.
Since you care about them - could you please send patches on top
of the IRQ threading patches to add support for them?
Thanks,
Ingo
* David Brownell <[email protected]> wrote:
> On Monday 02 March 2009, Ingo Molnar wrote:
> >
> > > > > What's unfortunate is that you prefer not to fix that
> > > > > IRQF_DISABLED bug in lockdep, which you co-"maintain".
> > > > > When running with lockdep, that bug (a) introduces bugs
> > > > > in some drivers and (b) hides bugs in others. ?You've
> > > > > rejected even a minimal warning fix, to help minimize
> > > > > the amount of time developers waste on (a) and (b).
> > > >
> > > > I've come to the conclusion that the only technically sound solution is
> > > > to do as I proposed today, utterly eliminate !IRQF_DISABLED handlers.
> > >
> > > As you announced today. ?If you truly believe that, then
> > > you should at least submit a warning patch for 2.6.29-rc
> > > ("driver X isn't setting IRQF_DISABLED, reimplement!")
> >
> > i have changed the BUG_ON() to a WARN_ONCE() message so the
> > warning is in place now.
>
> The patch Peter sent doesn't relate in the least to removing
> the IRQF_DISABLED flag though. Patches addressing that would
> be in setup_irq() code paths not IRQ dispatch.
yes, i referred to the BUG_ON(!irq_irq()) patch.
Ingo
On Monday 02 March 2009, Ingo Molnar wrote:
> >
> > The significant omission is lack of support for chaining
> > such threads. ?Example, an I2C device that exposes
> > several dozen IRQs with mask/ack/... operations that
> > require I2C access.
>
> Well, those are rarely used, embedded-only constructs - the main
> focus of IRQ threading patches are the more common patterns.
Yes, mostly for embedded, where "system bus" more likely
means I2C than PCI.
> Since you care about them - could you please send patches on top
> of the IRQ threading patches to add support for them?
I'll look at that, and try to prepare something on top
of the version of the threading patches that gets into
the -next tree. I got the impression there was going
to be a v3 of those patches soonish...
I expect there will be two basic parts of that work:
- One to cope with the upcoming change to handle_irq(),
insisting that it live in hardirq context instead of
just an irqs-off context (and thereby preventing use
of standard chaining calls in irq threads, sigh).
- Another to set up a chaining thread, since chain
setup bypasses setup_irq() and friends.
That latter might touch what the v2 patches added,
since I'd want it to share code.
- Dave
p.s. Note that those changes would still leave the
lockdep bug around ... it will still be breaking
various drivers that use normal IRQs, by forcibly
enabling IRQF_DISABLED.
* David Brownell <[email protected]> wrote:
> On Monday 02 March 2009, Ingo Molnar wrote:
> > >
> > > The significant omission is lack of support for chaining
> > > such threads. ?Example, an I2C device that exposes
> > > several dozen IRQs with mask/ack/... operations that
> > > require I2C access.
> >
> > Well, those are rarely used, embedded-only constructs - the main
> > focus of IRQ threading patches are the more common patterns.
>
> Yes, mostly for embedded, where "system bus" more likely
> means I2C than PCI.
>
>
> > Since you care about them - could you please send patches on top
> > of the IRQ threading patches to add support for them?
>
> I'll look at that, and try to prepare something on top
> of the version of the threading patches that gets into
> the -next tree. I got the impression there was going
> to be a v3 of those patches soonish...
Great! We'll sort out any conflicts so dont worry about that -
you can pick up v2 just fine and post patches.
> I expect there will be two basic parts of that work:
>
> - One to cope with the upcoming change to handle_irq(),
> insisting that it live in hardirq context instead of
> just an irqs-off context (and thereby preventing use
> of standard chaining calls in irq threads, sigh).
>
> - Another to set up a chaining thread, since chain
> setup bypasses setup_irq() and friends.
If you mean to push the chaining bits into the IRQ thread too, i
think the chaining bits actually should never be threaded. Is
there a good reason to do that? It's not like they will really
be preemptible (preempting a chaining thread would mean the
whole demuxing chain is held up => bad).
> That latter might touch what the v2 patches added,
> since I'd want it to share code.
Sure.
>
> - Dave
>
> p.s. Note that those changes would still leave the
> lockdep bug around ... it will still be breaking
> various drivers that use normal IRQs, by forcibly
> enabling IRQF_DISABLED.
it's not a bug - and i think Peter explained that already. It's
not really breaking things either - we've had this for more than
2 years.
Ingo
On Monday 02 March 2009, Ingo Molnar wrote:
> >
> > p.s. Note that those changes would still leave the
> > ? ? ?lockdep bug around ... it will still be breaking
> > ? ? ?various drivers that use normal IRQs, by forcibly
> > ? ? ?enabling IRQF_DISABLED.
>
> it's not a bug - and i think Peter explained that already.
No. But I did get a non-response that didn't include any
explanation, and relied totally on unfounded assertions
combined with the presumption that someday IRQF_DISABLED
will be forced on in all drivers.
> It's
> not really breaking things either - we've had this for more than
> 2 years.
I happened across two MMC host adapter drivers which it
broke. They preceded the patch whereby lockdep changes
request_irq() semantics. The drivers break only with
lockdep ... so platforms relying on those can't use lockdep.
No reason to think there aren't more such bugs lurking.
We also wasted quite a few months tracking down USB bugs
that were masked by that patch ... the indeterminacy of
IRQF_DISABLED behavior given IRQF_SHARED was hidden by
enabling lockdep. Led to a lot of totally crapulous oops
traces, I assure you.
So ... it has most certainly broken things. There has
seemed to be a bit of stick-fingers-in-ears going on, that
prevents hearing about such problems though.
- Dave
On Mon, 2009-03-02 at 18:37 -0800, David Brownell wrote:
> No. But I did get a non-response that didn't include any
> explanation, and relied totally on unfounded assertions
> combined with the presumption that someday IRQF_DISABLED
> will be forced on in all drivers.
Enabling IRQs in hardirq context is BAD because:
- IRQ handler nesting leads to stack overflow
- It gives the false impression its OK for IRQ handlers to be slow,
it is _NOT_, as you still generate horrible preemption latency.
Therefore IRQF_DISABLED _will_ be forced on everybody some day soon, and
I'll provide an IRQF_ENABLED for use by broken hardware only (and make a
TAINT flag for that too).
* Peter Zijlstra <[email protected]> wrote:
> On Mon, 2009-03-02 at 18:37 -0800, David Brownell wrote:
> > No. But I did get a non-response that didn't include any
> > explanation, and relied totally on unfounded assertions
> > combined with the presumption that someday IRQF_DISABLED
> > will be forced on in all drivers.
>
> Enabling IRQs in hardirq context is BAD because:
>
> - IRQ handler nesting leads to stack overflow
> - It gives the false impression its OK for IRQ handlers to be slow,
> it is _NOT_, as you still generate horrible preemption latency.
>
> Therefore IRQF_DISABLED _will_ be forced on everybody some day
> soon, and I'll provide an IRQF_ENABLED for use by broken
> hardware only (and make a TAINT flag for that too).
Basically the problem why !IRQF_DISABLED is bad that if there
are enough interrupt handlers we can get nesting like this:
<irq 20>
<handler runs with irqs enabled>
<irq 21>
<handler runs with irqs enabled>
<irq 22>
<handler runs with irqs enabled>
<irq 23>
<handler runs with irqs enabled>
<irq 24>
<handler runs with irqs enabled>
Suppose each handler gets interruped while it already used up
1000 bytes of the stack (conservative estimation - often it's
more) - the above sequence is already 5000 bytes into the stack.
There is no protection against stack overflow there and such
bugs can be _very_ hard to trigger and find. If there's a
sufficient number of devices and a high enough load it can
trigger spuriously.
Yes, in a few limited embedded environments where you dont have
more than 3-4 IRQ sources you might decide that it's safe to do
(or you might decide that you dont care). Also, there's a few
legacy pieces of hardware with either very long hw access
latencies or too short buffers. Plus there might be any number
of other hw factors - or architecture details (such as the use
of separate per IRQ stacks) that limit IRQ handler parallelism
in practice.
So we'll have the quirk flag for the weird cases - but these are
the exceptions that strengthen the general rule. The concept of
enabling interrupts in a hardirq handler is a no-no on a general
purpose kernel and no modern driver should make use of it.
I hope this explains why lockdep never supported this case.
Ingo
> Therefore IRQF_DISABLED _will_ be forced on everybody some day soon, and
> I'll provide an IRQF_ENABLED for use by broken hardware only (and make a
> TAINT flag for that too).
I don't think you understand how the kernel project works. If everyone
thinks your change is inappropriate it won't get in. You can talk about
forcing things all you like but "force" used that way generally means "new
maintainer required"
* Alan Cox <[email protected]> wrote:
> > Therefore IRQF_DISABLED _will_ be forced on everybody some
> > day soon, and I'll provide an IRQF_ENABLED for use by broken
> > hardware only (and make a TAINT flag for that too).
>
> I don't think you understand how the kernel project works. If
> everyone thinks your change is inappropriate it won't get in.
The change that people had a problem with was the immediate
removal of IRQF_ENABLED, and that's not on the plate anymore.
I dont think anyone offered any example where IRQF_ENABLED is
used in a healthy way - they are all legacy or special hw quirks
where we limp along with enabling IRQs in a hacky way.
Furthermore, even these quirky cases can be supported cleanly
_without_ IRQF_ENABLED: where an IRQ handler can take a long
time to execute, the handler can be converted to a threaded IRQ
handler - where it's fine to enable IRQs as there are no stack
nesting issues.
So there's no real technical problem here.
Ingo
> _without_ IRQF_ENABLED: where an IRQ handler can take a long
> time to execute, the handler can be converted to a threaded IRQ
> handler - where it's fine to enable IRQs as there are no stack
> nesting issues.
Only if you can mask the interrupt on the APIC without losing it or
having the APIC throw a fit.
> So there's no real technical problem here.
In the long term no - but forcing people to make sudden changes to
critical I/O drivers isn't the right way to do it.
On Tue, 2009-03-03 at 10:30 +0000, Alan Cox wrote:
>
> > So there's no real technical problem here.
>
> In the long term no - but forcing people to make sudden changes to
> critical I/O drivers isn't the right way to do it.
My plan is:
1) find all drivers that do not use IRQF_DISABLED
2) add IRQF_LEGACY_ENABLED to those
3) make IRQF_DISABLED 0 and remove its functionality
while adding IRQF_LEGACY_ENABLED
4) make request_irq() print a warning for IRQF_LEGACY_ENABLED
5) make an actual IRQF_LEGACY_ENABLED irq firing taint the kernel
After that its cleanup time, and I'll try to help out converting some of
these to threaded IRQs where appropriate.
This will not break any driver, nor force sudden change. Stuff will
continue working as expected, just a little more boot noise for those
with crappy drivers/hardware.
* Alan Cox <[email protected]> wrote:
> > _without_ IRQF_ENABLED: where an IRQ handler can take a long
> > time to execute, the handler can be converted to a threaded
> > IRQ handler - where it's fine to enable IRQs as there are no
> > stack nesting issues.
>
> Only if you can mask the interrupt on the APIC without losing
> it or having the APIC throw a fit.
Hm, that reads like the boot IRQ erratas of certain chipsets -
the APIC could throw a fit essentially locking up the system.
FYI, we have fixes for that upstream already.
Do you have any description about that problem, which hardware
it affects, whether it's manufactured today and any (ballpark
figure) estimation about the Linux installed base on it? Can
they live with the quirk flag?
> > So there's no real technical problem here.
>
> In the long term no - but forcing people to make sudden
> changes to critical I/O drivers isn't the right way to do it.
i think you severely over-estimate the importance and ratio of
drivers that enable irqs within irq handlers. (Nor does anyone
want to break them really - we want to have a sane default and
we want to flag the broken cases as broken.)
The thing is, while you seem to spend precious resources on
weird legacy cases, we have a _lot_ of everyday systems in
bugzilla that do not boot or do not work for one reason or
another. Most of that is not in the weird-hardware category at
all.
You might also have noticed that over the past 2-3 years the
term "hard lockup" in regression reports has gone down by about
an order of magnitude - and much of that can be attributed to
the lockdep coverage we have in place. So in terms of real
everyday quality impact on Linux Peter is very, very, very
efficient.
And frankly, while Peter's patch here needs modifications, as a
maintainer i prefer Peter as a contributor so much not only
because he is fantastically productive in terms of fixing
locking crap all over the kernel, but also because he
concentrates on the big picture and on the common case and on
the net effect on Linux instead of just stubbornly concentrating
on an extreme-0.01% of the hardware space.
So your attack on him is quite misguided and unfair:
>> [..] You can talk about forcing things all you like but
>> "force" used that way generally means "new maintainer
>> required" [...]
Btw., Peter submitted a genirq patch and FYI he does not
maintain the genirq subsystem and never maintained it.
Ingo
> Hm, that reads like the boot IRQ erratas of certain chipsets -
> the APIC could throw a fit essentially locking up the system.
> FYI, we have fixes for that upstream already.
Good - certainly it used to be the case that masking APIC IRQs and
leaving them masked from the IRQ handled used to do funny things
sometimes.
> i think you severely over-estimate the importance and ratio of
> drivers that enable irqs within irq handlers. (Nor does anyone
> want to break them really - we want to have a sane default and
> we want to flag the broken cases as broken.)
IDE. A lot less people use the IDE stack nowdays but its a big item and
getting it wrong tends to eat your files.
I do object to the attitude shown about "forcing" people. It's a
community project built by a large number of people on a mix of
pragmatic and elegant design balances. Maybe it's just unfortunate choice
of wording but it is the wrong sentiment.
* Ingo Molnar <[email protected]> wrote:
>
> * Alan Cox <[email protected]> wrote:
>
> > > _without_ IRQF_ENABLED: where an IRQ handler can take a long
> > > time to execute, the handler can be converted to a threaded
> > > IRQ handler - where it's fine to enable IRQs as there are no
> > > stack nesting issues.
> >
> > Only if you can mask the interrupt on the APIC without
> > losing it or having the APIC throw a fit.
>
> Hm, that reads like the boot IRQ erratas of certain chipsets -
> the APIC could throw a fit essentially locking up the system.
> FYI, we have fixes for that upstream already.
>
> Do you have any description about that problem, which hardware
> it affects, whether it's manufactured today and any (ballpark
> figure) estimation about the Linux installed base on it? Can
> they live with the quirk flag?
btw., i definitely do not say that threaded IRQ handlers will
work in each an every case (it changes the hardware programming
pattern and as such it can bring out new erratas) so i
definitely agree with the argument that the conversion has to be
careful and case by case.
The plan Peter outlined looks sane. In case you worry about a
forced removal of irq-enable - you should not.
Ingo
* Alan Cox <[email protected]> wrote:
> > Hm, that reads like the boot IRQ erratas of certain chipsets
> > - the APIC could throw a fit essentially locking up the
> > system. FYI, we have fixes for that upstream already.
>
> Good - certainly it used to be the case that masking APIC IRQs
> and leaving them masked from the IRQ handled used to do funny
> things sometimes.
I think being careful is definitely warranted in the case of
IDE. I'd not be surprised if not all chipsets are mapped via the
boot-IRQ quirks: those quirks are opt-in based on PCI IDs -
those tend to be the quirk mechanisms with the least coverage.
(The IDs were also derived from enterprise testing of -rt, which
tends to under-emphasise cheap broken chipsets.)
Plus the erratum you described about doing an IRQ masking
mid-PIO-transfer confusing the chipset can also be a separate
standalone bug not permitting an irq-masking based IRQ flow at
all on such hardware.
So your worries are spot on IMO and are not being dismissed
forcibly.
> > i think you severely over-estimate the importance and ratio
> > of drivers that enable irqs within irq handlers. (Nor does
> > anyone want to break them really - we want to have a sane
> > default and we want to flag the broken cases as broken.)
>
> IDE. A lot less people use the IDE stack nowdays but its a big
> item and getting it wrong tends to eat your files.
>
> I do object to the attitude shown about "forcing" people. It's
> a community project built by a large number of people on a mix
> of pragmatic and elegant design balances. Maybe it's just
> unfortunate choice of wording but it is the wrong sentiment.
It was in the heat of the argument i think ...
(I do think we need to be somewhat less permissive in terms of
weird driver practices, but that's just my opinion and not
'enforced' via any artificial way.)
Ingo
On Mon, 2 Mar 2009, David Brownell wrote:
> On Monday 02 March 2009, Ingo Molnar wrote:
> > If you want to work on genirq threaded IRQ handlers them please
> > check out and test the threaded IRQ handlers patches that are
> > being worked on at lkml. See:
> >
> > [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2
>
> I did check them out, as noted earlier in this thread.
>
> The significant omission is lack of support for chaining
> such threads. Example, an I2C device that exposes
> several dozen IRQs with mask/ack/... operations that
> require I2C access.
Well, the significant omission is on your side. Instead of talking to
us about the problems and possible shortcomings of the genirq code you
went there and created your private form of abuse and now you are
complaining about that. The lockdep issue is not caused by lockdep,
it's caused by your using code which is designed to run in hardirq
context from a thread context. It does not become more correct just
because it works fine w/o lockdep.
> I'm not sure what Thomas intends to do with that issue,
I can do something about that, when I know about it, but I have just
learned about the details in the last few days.
> if anything. It does touch on messy bits of genirq.
Which mess are you referring to ?
The problem you described is straight forward and as I said before
it's not rocket science to provide support for that in the genirq
code. Your use case does not need to use the chained handler setup at
all, it just needs to request the main IRQ as a simple type and handle
the ack/mask/unmask from the two handler parts.
irqreturn_t hardirq_handler()
{
ack_mask(irq);
return IRQ_WAKE_THREAD;
}
irqreturn_t thread_handler()
{
pending = read_i2c_pending_irqs();
while (pending) {
....
d->handle_irq(module_irq, d);
}
unmask(irq);
}
The only change in the generic code which is needed is a new handler
function for the chained irqs "handle_irq_simple_threaded()" which is
designed to handle the calls from thread context.
Thanks,
tglx
On Tuesday 03 March 2009, Thomas Gleixner wrote:
> >
> > > [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2
> >
> > I did check them out, as noted earlier in this thread.
> >
> > The significant omission is lack of support for chaining
> > such threads. ?Example, an I2C device that exposes
> > several dozen IRQs with mask/ack/... operations that
> > require I2C access.
>
> Well, the significant omission is on your side.
The facts don't quite match up with that story though ... for
starters, as I've already pointed out in this thread, I didn't
write that code (or even "create a private form of abuse" as
you put it). My name isn't even on the copyright.
I did however clean it up a lot, in hope that such cleanup
would make later updates easier. Anyone could tell such
updates would be needed. In fact ...
> Instead of talking to
> us about the problems and possible shortcomings of the genirq code you
> went there and created your private form of abuse and now you are
> complaining about that.
... I told you about that *SPECIFIC* driver at the kernel summit,
as something to address with the threaded IRQ infrastructure you
presented at that time. (The prototype Overo hardware you received
at that time uses this driver, so you could even have tested such
things. If you went a bit out of your way to do so.) ISTR cc'ing
you on the IRQ details of that driver a few times around, for that
matter, in case you had some feedback.
Your IRQ threading patches appeared well after this driver went
to mainline. So I did talk to "us" about those problems, earlier,
but it doesn't seem to have gotten your attention until now.
> The lockdep issue is not caused by lockdep,
> it's caused by your using code which is designed to run in hardirq
> context from a thread context. It does not become more correct just
> because it works fine w/o lockdep.
No; there are two lockdep symptoms caused by forcing
IRQF_DISABLED on in all cases. I'm repeating myself
again here, again ...
- one symptom shows up in standard hardirq code, I
gave the example of two MMC host adapters which
don't work because of those semantic changes.
- this driver shows a related symptom, since it needs
to chain IRQ threads.
You're referring to the second issue. The code in
question doesn't actually have any dependency on
hardirq context though.
> > I'm not sure what Thomas intends to do with that issue,
>
> I can do something about that, when I know about it, but I have just
> learned about the details in the last few days.
Well, I did tell you about this earlier.
> > if anything. ?It does touch on messy bits of genirq.
>
> Which mess are you referring to ?
Assuming all IRQ configuring and dispatching runs with IRQs
disabled. Your threaded IRQ patches kick in only *after*
dispatching has been done. So it affects just one of the
three main unusual bits of behavior involved here.
Which mess were you thinking of? :)
> The problem you described is straight forward and as I said before
> it's not rocket science to provide support for that in the genirq
> code. Your use case does not need to use the chained handler setup at
> all, it just needs to request the main IRQ as a simple type and handle
> the ack/mask/unmask from the two handler parts.
When there is a "main IRQ" that calls the handlers, that's
exactly what chaining involves ...
> The only change in the generic code which is needed is a new handler
> function for the chained irqs "handle_irq_simple_threaded()" which is
> designed to handle the calls from thread context.
I'm not 100% sure that's right; the dispatching is a bit quirky.
That is however where I'll start.
The top level handler (for the PIH module) could easily use a
"handle_irq_simple_threaded()", yes ... but the secondary (SIH)
handlers have a few oddball behaviors including mixes of edge
and level trigger modes.
- Dave
David,
On Wed, 4 Mar 2009, David Brownell wrote:
> On Tuesday 03 March 2009, Thomas Gleixner wrote:
> > >
> > > > [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2
> > >
> > > I did check them out, as noted earlier in this thread.
> > >
> > > The significant omission is lack of support for chaining
> > > such threads. ?Example, an I2C device that exposes
> > > several dozen IRQs with mask/ack/... operations that
> > > require I2C access.
> >
> > Well, the significant omission is on your side.
>
> The facts don't quite match up with that story though ... for
> starters, as I've already pointed out in this thread, I didn't
> write that code (or even "create a private form of abuse" as
> you put it). My name isn't even on the copyright.
>
> I did however clean it up a lot, in hope that such cleanup
> would make later updates easier. Anyone could tell such
> updates would be needed. In fact ...
Sorry, did not realize that it was not your design in the first place.
> Your IRQ threading patches appeared well after this driver went
> to mainline. So I did talk to "us" about those problems, earlier,
> but it doesn't seem to have gotten your attention until now.
Fair enough. I did not realize the horror of this chip until now. From
what you told me at KS I figured it would be a halfways straight
forward thing.
> You're referring to the second issue. The code in
> question doesn't actually have any dependency on
> hardirq context though.
Err. handle_IRQ_event was never meant to run in thread context,
neither the handle_TYPE functions.
> Assuming all IRQ configuring and dispatching runs with IRQs
> disabled. Your threaded IRQ patches kick in only *after*
> dispatching has been done. So it affects just one of the
> three main unusual bits of behavior involved here.
>
> Which mess were you thinking of? :)
None, there is no mess in the irq code.
> > The problem you described is straight forward and as I said before
> > it's not rocket science to provide support for that in the genirq
> > code. Your use case does not need to use the chained handler setup at
> > all, it just needs to request the main IRQ as a simple type and handle
> > the ack/mask/unmask from the two handler parts.
>
> When there is a "main IRQ" that calls the handlers, that's
> exactly what chaining involves ...
And how does this rabulistic nit picking help us here ? :)
Again: the chained_handler functionality was never designed to run in
a thread.
> > The only change in the generic code which is needed is a new handler
> > function for the chained irqs "handle_irq_simple_threaded()" which is
> > designed to handle the calls from thread context.
>
> I'm not 100% sure that's right; the dispatching is a bit quirky.
> That is however where I'll start.
>
> The top level handler (for the PIH module) could easily use a
> "handle_irq_simple_threaded()", yes ... but the secondary (SIH)
> handlers have a few oddball behaviors including mixes of edge
> and level trigger modes.
I took a closer look at this code and the more I look the more it
confuses me.
You told me that the demux handler runs the secondary handlers in its
thread context, but looking at the code there is a work queue as well.
The mask/unmask functions which are called for the secondary handlers
are just queueing work. I really do not understand the logic here:
primary interrupt happens
->primary thread is woken up
primary thread runs
-> primary thread raises secondary irq via
generic_handle_irq(irq), which results in:
desc->handle_irq(irq, desc);
The secondary handler has is set to: handle_edge_irq and
handle_edge_irq() does: desc->chip->ack(irq);
But the irqchip, which is set for those secondary irqs has a NULL ack
function. How can this work at all ?
I'm probably missing something and I would appreciate if you could
shed some light on this. An abstract description of the requirements
of the hardware w/o any reference to the current implementation would
be definitely helpful.
Thanks,
tglx
On Tuesday 03 March 2009, Ingo Molnar wrote:
> i think you severely over-estimate the importance and ratio of
> drivers that enable irqs within irq handlers. (Nor does anyone
> want to break them really - we want to have a sane default and
> we want to flag the broken cases as broken.)
For the record, I've been running for some time now
with a patch that issues a warning for each IRQ that
lockdep forces to use IRQF_DISABLED.
On my x86 systems, pretty much every driver triggers
that warning. Which makes me think maybe that shoe
is being placed on the wrong foot: use of IRQF_DISABLED
is the *EXCEPTION* not the rule. At least on one major
Linux platform...
On Monday 02 March 2009, Ingo Molnar wrote:
>
> > > Since you care about them - could you please send patches on top
> > > of the IRQ threading patches to add support for them?
> >
> > I'll look at that, and try to prepare something on top
> > of the version of the threading patches that gets into
> > the -next tree. ?I got the impression there was going
> > to be a v3 of those patches soonish...
>
> Great! We'll sort out any conflicts so dont worry about that -
> you can pick up v2 just fine and post patches.
One such patch is about to come, with different $SUBJECT
and trimmed CC list.
Note however that it's completely independent of Thomas'
patches. It only affects the IRQ dispatch sub-problem;
other bits seem to be needed too.
> If you mean to push the chaining bits into the IRQ thread too, i
> think the chaining bits actually should never be threaded. Is
> there a good reason to do that?
The chaining bits *MUST* be threaded. The lack of that
support was a key issue with Thomas' patch. The issue
being that access to the IRQ status registers, as needed
to dispatch the IRQ, is only possible in contexts that
can sleep. That seems like a good reason. :)
> It's not like they will really
> be preemptible (preempting a chaining thread would mean the
> whole demuxing chain is held up => bad).
See above. The reason to thread this IRQ handling is
that it can't be done outside of threads ... there's
no other way to access registers via I2C (or SPI, etc).
Accordingly there's no way to avoid preemption ... but
since these IRQs aren't on performance-critical paths,
that's really no bother.
From: David Brownell <[email protected]>
Define a new flow handler, handle_threaded_irq(), for IRQ threads
to use when chaining IRQs.
Unlike existing flow handlers, handle_simple_irq() and siblings,
this one is used only from sleep-capable contexts. It always calls
irqaction handlers from that same (shared) sleep-capable context.
This is independent of Thomas' irq threading patchset, and can be
viewed as a complement to it. This adds support for IRQs whose
handlers must *ONLY* ever run in thread contexts ... instead of
offloading code from hardirq context into a thread. Another way
this differs is that it doesn't create more kernel threads; it
only leverages an existing thread.
Signed-off-by: David Brownell <[email protected]>
---
include/linux/irq.h | 7 ++++-
kernel/irq/chip.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+), 2 deletions(-)
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -283,8 +283,8 @@ static inline int irq_balancing_disabled
extern int handle_IRQ_event(unsigned int irq, struct irqaction *action);
/*
- * Built-in IRQ handlers for various IRQ types,
- * callable via desc->chip->handle_irq()
+ * IRQ flow handlers for various IRQ types, callable via
+ * generic_handle_irq*() or desc->handle_irq()
*/
extern void handle_level_irq(unsigned int irq, struct irq_desc *desc);
extern void handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc);
@@ -293,6 +293,9 @@ extern void handle_simple_irq(unsigned i
extern void handle_percpu_irq(unsigned int irq, struct irq_desc *desc);
extern void handle_bad_irq(unsigned int irq, struct irq_desc *desc);
+/* Flow handler that must only be called from sleeping context */
+extern void handle_threaded_irq(unsigned int irq, struct irq_desc *desc);
+
/*
* Monolithic do_IRQ implementation.
*/
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -295,6 +295,67 @@ static inline void mask_ack_irq(struct i
}
/**
+ * handle_threaded_irq - flow handler reusing current irq thread
+ * @irq: the interrupt number
+ * @desc: the interrupt description structure for this irq
+ * Context: irq thread, with IRQs enabled
+ *
+ * IRQ threads which demultiplex IRQs may use this flow handler
+ * to chain those demultiplexed IRQs to subsidiary handlers, when
+ * all that IRQ dispatch logic must run in sleeping contexts.
+ *
+ * Examples include some multifunction I2C and SPI based devices
+ * (where access to registers, including ones involved in IRQ
+ * dispatching, requires sleeping) that have multiple independent
+ * maskable interupts.
+ *
+ * The irq thread using this flow handler must handle any ack,
+ * clear, mask or unmask issues needed.
+ */
+void
+handle_threaded_irq(unsigned int irq, struct irq_desc *desc)
+{
+ struct irqaction *action;
+ irqreturn_t action_ret;
+
+ spin_lock_irq(&desc->lock);
+
+ if (unlikely(desc->status & IRQ_INPROGRESS))
+ goto out_unlock;
+ desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
+ kstat_incr_irqs_this_cpu(irq, desc);
+
+ action = desc->action;
+ if (unlikely(!action || (desc->status & IRQ_DISABLED)))
+ goto out_unlock;
+
+ desc->status |= IRQ_INPROGRESS;
+ spin_unlock_irq(&desc->lock);
+
+ /* simplified handle_IRQ_event(): no random sampling;
+ * IRQs are always enabled so action->handler may sleep;
+ * no hooks for handing off to yet another irq thread.
+ */
+ action_ret = IRQ_NONE;
+ do {
+ /* REVISIT can we get some explicit knowledge that this
+ * handler expects to run in thread context? Maybe an
+ * IRQF_THREADED check, or a new handler type ...
+ */
+ action_ret |= action->handler(irq, action->dev_id);
+ action = action->next;
+ } while (action);
+
+ if (!noirqdebug)
+ note_interrupt(irq, desc, action_ret);
+
+ spin_lock_irq(&desc->lock);
+ desc->status &= ~IRQ_INPROGRESS;
+out_unlock:
+ spin_unlock_irq(&desc->lock);
+}
+
+/**
* handle_simple_irq - Simple and software-decoded IRQs.
* @irq: the interrupt number
* @desc: the interrupt description structure for this irq
This is a followup to help address one of the omissions in the
threaded IRQ patches (v2) posted recently. To recap, those
provided a quickcheck() routine that could wake an irq thread,
and a way to register a handler using that mechanism.
This addresses an orthogonal problem: given some thread
demultiplexing the subsidiary IRQs reported to some top-level
interrupt, how can that kick in flow handlers chaining to
the subsidiary IRQs' handlers?
The answer here is more or less as suggested by Thomas:
using handle_threaded_irq(), a flow handler which doesn't
use handle_IRQ_event() since that doesn't much like being
called from thread context, at least when lockdep is active.
These two patches:
- add handle_threaded_irq() flow handler;
- kick it in for some twl4030 code
Tested on 2.6.29-rc8 code, both with and without the v2
irqthread patches.
From: David Brownell <[email protected]>
Make the toplevel twl4030 irq dispatch code use the new
handle_threaded_irq() flow handler. Also, minor cleanup,
use the newish generic_handle_irq_desc().
Since that flow handler guarantees the IRQ handlers are
called only in a normal (sleeping) thread context, remove
some of the workarounds for the lockdep goofage whereby
it breaks various drivers by forcing IRQF_DISABLED on.
Signed-off-by: David Brownell <[email protected]>
---
drivers/mfd/twl4030-irq.c | 15 +++++----------
drivers/rtc/rtc-twl4030.c | 8 --------
drivers/usb/otg/twl4030-usb.c | 8 --------
3 files changed, 5 insertions(+), 26 deletions(-)
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -215,7 +215,6 @@ static int twl4030_irq_thread(void *data
}
/* these handlers deal with the relevant SIH irq status */
- local_irq_disable();
for (module_irq = twl4030_irq_base;
pih_isr;
pih_isr >>= 1, module_irq++) {
@@ -235,10 +234,9 @@ static int twl4030_irq_thread(void *data
note_interrupt(module_irq, d,
IRQ_NONE);
else
- d->handle_irq(module_irq, d);
+ generic_handle_irq_desc(module_irq, d);
}
}
- local_irq_enable();
desc->chip->unmask(irq);
}
@@ -578,7 +576,7 @@ static inline int sih_read_isr(const str
}
/*
- * Generic handler for SIH interrupts ... we "know" this is called
+ * Generic handler for SIH interrupts ... we know this is called
* in task context, with IRQs enabled.
*/
static void handle_twl4030_sih(unsigned irq, struct irq_desc *desc)
@@ -588,10 +586,7 @@ static void handle_twl4030_sih(unsigned
int isr;
/* reading ISR acks the IRQs, using clear-on-read mode */
- local_irq_enable();
isr = sih_read_isr(sih);
- local_irq_disable();
-
if (isr < 0) {
pr_err("twl4030: %s SIH, read ISR error %d\n",
sih->name, isr);
@@ -658,7 +653,7 @@ int twl4030_sih_setup(int module)
irq = irq_base + i;
set_irq_chip_and_handler(irq, &twl4030_sih_irq_chip,
- handle_edge_irq);
+ handle_threaded_irq);
set_irq_chip_data(irq, agent);
activate_irq(irq);
}
@@ -666,7 +661,7 @@ int twl4030_sih_setup(int module)
status = irq_base;
twl4030_irq_next += i;
- /* replace generic PIH handler (handle_simple_irq) */
+ /* replace generic PIH handler (handle_threaded_irq) */
irq = sih_mod + twl4030_irq_base;
set_irq_data(irq, agent);
set_irq_chained_handler(irq, handle_twl4030_sih);
@@ -719,7 +714,7 @@ int twl_init_irq(int irq_num, unsigned i
for (i = irq_base; i < irq_end; i++) {
set_irq_chip_and_handler(i, &twl4030_irq_chip,
- handle_simple_irq);
+ handle_threaded_irq);
activate_irq(i);
}
twl4030_irq_next = i;
--- a/drivers/rtc/rtc-twl4030.c
+++ b/drivers/rtc/rtc-twl4030.c
@@ -325,14 +325,6 @@ static irqreturn_t twl4030_rtc_interrupt
int res;
u8 rd_reg;
-#ifdef CONFIG_LOCKDEP
- /* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
- * we don't want and can't tolerate. Although it might be
- * friendlier not to borrow this thread context...
- */
- local_irq_enable();
-#endif
-
res = twl4030_rtc_read_u8(&rd_reg, REG_RTC_STATUS_REG);
if (res)
goto out;
--- a/drivers/usb/otg/twl4030-usb.c
+++ b/drivers/usb/otg/twl4030-usb.c
@@ -517,14 +517,6 @@ static irqreturn_t twl4030_usb_irq(int i
struct twl4030_usb *twl = _twl;
int status;
-#ifdef CONFIG_LOCKDEP
- /* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
- * we don't want and can't tolerate. Although it might be
- * friendlier not to borrow this thread context...
- */
- local_irq_enable();
-#endif
-
status = twl4030_usb_linkstat(twl);
if (status != USB_LINK_UNKNOWN) {
On Friday 06 March 2009, Thomas Gleixner wrote:
> > > The only change in the generic code which is needed is a new handler
> > > function for the chained irqs "handle_irq_simple_threaded()" which is
> > > designed to handle the calls from thread context.
> >
> > I'm not 100% sure that's right; the dispatching is a bit quirky.
> > That is however where I'll start.
I just sent a pair of patches for that.
They address only part of the chaining issue ...
dispatching from the top level IRQ task, not how
to set it up so that toplevel IRQ doesn't wrongly
appear in interrupt statistics.
> > The top level handler (for the PIH module) could easily use a
> > "handle_irq_simple_threaded()", yes ... but the secondary (SIH)
> > handlers have a few oddball behaviors including mixes of edge
> > and level trigger modes.
>
> I took a closer look at this code and the more I look the more it
> confuses me.
Good thing you didn't see the earlier versions!! ;)
> You told me that the demux handler runs the secondary handlers in its
> thread context, but looking at the code there is a work queue as well.
The workqueue is for irq_chip methods.
THAT is the fundamental squishy thing here: when registers
associated with an irq_chip (not just the interrupting device)
are inaccessible except in task/sleeping contexts.
It means that three different things must always run in task
context: (a) irq_chip methods, (b) IRQ flow handlers, and
(c) IRQ action handlers.
Overstating things somewhat ... your irq threading patches
are best suited for offloading code from hardirq context
into task context -- related to case (c), except here the
state needed by the action handler *can't* be accessed in
hardirq context. But they don't much help with cases (a)
or (b), where such hardirq context was never relevant in
the first place because the irq management registers aren't
accessible there.
> The mask/unmask functions which are called for the secondary handlers
> are just queueing work. I really do not understand the logic here:
>
> primary interrupt happens
> ->primary thread is woken up
>
> primary thread runs
> -> primary thread raises secondary irq via
> generic_handle_irq(irq), which results in:
> desc->handle_irq(irq, desc);
That's not quite exact; there can be more than one level
of dispatch. In more detail (it affects answers to your
questions):
1) Primary thread queries the PIH module (up to 8 IRQ
status bits) to determine which SIH module(s) raised
an interrupt.
2) Then it does a normal dispatch -- desc->handle_irq(),
maybe wrapped in a convenience function -- to the
next level, specific to that SIH (not limited to
just 8 status bits).
3) Now, depending on how that SIH was set up, one
of two things will happen:
A) Dispatch through generic SIH module code; as
with GPIO and "power" IRQs. Interrupts for
MMC card detect, RTC alarm, USB transciver,
"power button", and a few other things go
this way. Dispatched by another indirection
through a desc->handle_irq() invocation.
B) Dispatch through non-generic SIH module code.
That's how the keypad and ADC drivers work
right now; also the battery charger, but that
driver isn't used much yet due to HW issues.
> The secondary handler has is set to: handle_edge_irq and
> handle_edge_irq() does: desc->chip->ack(irq);
That's the (3A) case above. I was never certain that
needed to be dispatched with the "edge" flow handler
instead of the "simple" one. And it turns out that
something like "simple" can work fine, as it currently
does in case (3B).
(The trigger type for those IRQs is typically "edge".
But the "edge" flow handler doesn't seem to be the
best match.)
> But the irqchip, which is set for those secondary irqs has a NULL ack
> function. How can this work at all ?
For the PIH level, this hardware is quite odd: there
are no ack or mask primitives at all. At that level,
the only way to clear IRQ status is through the the
child (SIH) level status.
For the SIH level, these chips have a mode you may well
have seen on other hardware. Reading the SIH IRQ status
register implicitly acks the pending IRQs ... "clear on
read" (COR) mode mentioned in the driver. So no separate
ack() is needed in that case either.
> I'm probably missing something and I would appreciate if you could
> shed some light on this. An abstract description of the requirements
> of the hardware w/o any reference to the current implementation would
> be definitely helpful.
You're probably trying to avoid reading over 900 pages
of the tps65930 (public flavor of twl4030) technical
reference manual ... ;)
I'm not sure how abstract you want. I'll hope my words
above -- possibly in conjunction with comments in the
current twl4030-irq.c code -- clarify.
- Dave
Hi,
On Wed, Mar 18, 2009 at 03:19:47AM +0100, David Brownell wrote:
[..]
> @@ -295,6 +295,67 @@ static inline void mask_ack_irq(struct i
> }
>
> /**
> + * handle_threaded_irq - flow handler reusing current irq thread
> + * @irq: the interrupt number
> + * @desc: the interrupt description structure for this irq
> + * Context: irq thread, with IRQs enabled
> + *
> + * IRQ threads which demultiplex IRQs may use this flow handler
> + * to chain those demultiplexed IRQs to subsidiary handlers, when
> + * all that IRQ dispatch logic must run in sleeping contexts.
> + *
> + * Examples include some multifunction I2C and SPI based devices
> + * (where access to registers, including ones involved in IRQ
> + * dispatching, requires sleeping) that have multiple independent
> + * maskable interupts.
> + *
> + * The irq thread using this flow handler must handle any ack,
> + * clear, mask or unmask issues needed.
> + */
> +void
> +handle_threaded_irq(unsigned int irq, struct irq_desc *desc)
> +{
> + struct irqaction *action;
> + irqreturn_t action_ret;
> +
> + spin_lock_irq(&desc->lock);
> +
> + if (unlikely(desc->status & IRQ_INPROGRESS))
> + goto out_unlock;
> + desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
> + kstat_incr_irqs_this_cpu(irq, desc);
> +
> + action = desc->action;
> + if (unlikely(!action || (desc->status & IRQ_DISABLED)))
> + goto out_unlock;
you say below irqs are always enabled so this branch is something we
never want to happen. How about adding a WARN() then ?
--
balbi
On Wednesday 18 March 2009, Felipe Balbi wrote:
> > +?????action = desc->action;
> > +?????if (unlikely(!action || (desc->status & IRQ_DISABLED)))
> > +?????????????goto out_unlock;
>
> you say below irqs are always enabled
Right here they're always disabled by spin_lock_irq().
The "below" follows spin_unlock_irq(), which re-enables
them to traverse that (locked) action list.
> so this branch is something we
> never want to happen. How about adding a WARN() then ?
When some one says "irqs are enabled" they mean that,
local_irq_disable() or friends have not been called,
so for example a timer or other IRQ could arrive.
The IRQ_DISABLED flag in an IRQ descriptor means
something different: "don't try *handling* this".
That particular check is used in *ALL* flow handlers.
It guards against things like races in disable_irq()
paths, which could allow an IRQ that was in flight
to arrive "after" the IRQ was disabled.
In the case of an IRQ enable/disable mask sitting
across an I2C bus boundary, it's particularly easy
to see how such a race might happen ... since both
the thread masking the IRQ, and the one handling it,
are subject to preemption and scheduling.
On Wed, Mar 18, 2009 at 07:31:22PM +0100, David Brownell wrote:
> On Wednesday 18 March 2009, Felipe Balbi wrote:
> > > +?????action = desc->action;
> > > +?????if (unlikely(!action || (desc->status & IRQ_DISABLED)))
> > > +?????????????goto out_unlock;
> >
> > you say below irqs are always enabled
>
> Right here they're always disabled by spin_lock_irq().
> The "below" follows spin_unlock_irq(), which re-enables
> them to traverse that (locked) action list.
>
>
> > so this branch is something we
> > never want to happen. How about adding a WARN() then ?
>
> When some one says "irqs are enabled" they mean that,
> local_irq_disable() or friends have not been called,
> so for example a timer or other IRQ could arrive.
>
> The IRQ_DISABLED flag in an IRQ descriptor means
> something different: "don't try *handling* this".
>
> That particular check is used in *ALL* flow handlers.
> It guards against things like races in disable_irq()
> paths, which could allow an IRQ that was in flight
> to arrive "after" the IRQ was disabled.
>
> In the case of an IRQ enable/disable mask sitting
> across an I2C bus boundary, it's particularly easy
> to see how such a race might happen ... since both
> the thread masking the IRQ, and the one handling it,
> are subject to preemption and scheduling.
aha, I see. Thanks for the explanation ;-)
--
balbi