[patch 00/50] genirq: -V3

[patch 00/50] genirq: -V3

[Ingo Molnar]

This is version 3 of the genirq patch-queue, against 2.6.17-rc4. This 
patch-queue improves the generic IRQ layer to be truly generic, by 
adding various abstractions and features to it, without impacting 
existing functionality. It was written by Thomas Gleixner (who has done 
most of the heavy-lifting) and me. We reused many bits of code and many 
concepts from Russell King's ARM IRQ layer.

The ARM architecture has been updated to make use of this improved 
generic IRQ layer. The new code also enables a cleaner and simpler 
implementation of lowlevel irq-chip details, chained handlers and other 
highlevel irq-flow handlers.

The patch-queue consists of 50 individual patches. The queue begins with 
a handful of cleanups, to make sure we are adding features to a cleaned 
up codebase. Then come the features that dont need the irq-chip 
abstractions but are necessary extensions, then comes the core irq-chip 
abstraction (genirq-core), features that rely on it, and finally, the 
conversion of the ARM architecture to the new generic IRQ layer.

The full patch-queue can also be downloaded from:

  http://redhat.com/~mingo/generic-irq-subsystem/

It has been build-tested with allyesconfig, and booted on x86, x86_64 
and various ARM platforms. It has been build-tested on all the 50 ARM 
platforms. Current ARM testing results are at:

  http://www.linutronix.de/index.php?page=46

Many thanks to the ARM developers who ran the initial patches on their 
ARM boards and helped tracking down initial migration bugs.

Review suggestions for past iterations of this code from Andrew Morton, 
Benjamin Herrenschmidt and Christoph Hellwig were incorporated in this 
version.

Comments, suggestions welcome,

	Ingo, Thomas

Re: [patch 00/50] genirq: -V3

[Benjamin Herrenschmidt]

Hi Ingo, Thomas !

Ok, I think it's better :) But I also think it's not there yet....

Separate flow handlers as "the standard recommended way to go" isn't
the right thing to do imho.

While I agree to leave room for such flow handlers per irq_desc for
really broken interrupt controllers, I'm still not convinced that the
"generic" one (ie. __do_IRQ) can't be used for pretty much everything
(maybe with a few changes), and having those 4 separate "default" flow
handlers presented as beeing "the way to go" by the documentation
isn't quite right.

In fact, I also think it would be less robust (I'll give an example later).

I also have reservations on the way the arch code is supposed to
decide how/when to call the various handle_irq_* handlers with
variable locking requirements and is responsible for getting to the
irq_desc (at least a helper here would be of some use).

To summarize before I explain (heh), while I agree that it _might_ be
useful to give the option of having separate flow handlers, I don't
think it should be the default/recommended practice and we shouldn't have
to provide specialized flow handlers in the generic code. In fact, one
standard robust flow handler that deals with the most common cases of
edge,level and percpu interrupts. I'll explain in more details some of
my reasons below.

Let's first go through the changes to irq_chip/hw_interrupt_type
before I dig into the rationale of having (or not having) split
handlers:

>From your previous implementation, you removed the distinction between
irq_type and irq_chip, they are no longer separate structures.
But you still basically merged all the "new" fields together. Thus we
end up with things like both enable/disable/ack/end "high level" and
mask+ack/unmask "low level" callbacks in the irq chip. That makes
things confusing. 

If we go back to the initial hw_interrupt_type (which was a misnamed
hw_interrupt_controller, or irq_chip, I'm not opposing the name
change), we have the enable/disable/ack/end "API" to the main old flow
handler (__do_IRQ) and other API functions. I am not convinced that it
makes sense to add "lower level" functions to it at this level.
Essentially, I think those new callbacks are either redundant or not
necessary. If your intent is to expose a "high level" vs. a "low
level" interface to the controller, then I disagree with the design
since that "low level" interface is essentially tied to the usage of
split flow level handlers and to the way "very dumb" interrupt
controllers work (and even with those, I think it's not necessary).
But let's first look at the callbacks themselves:

First disable/enable at the controller level is essentially identical
to mask/unmask. There is some clear redundancy there. The depth
counting or flag checking shall be done by the caller in any case,
thus the controller enable/disable should just be what they are,
low level dumb mask/unmask.

The remaining one is mask_and_ack. I don't personally think it is
needed, ack is enough. Wether ack should mask or not is, I think,
local to the irq_chip implementation.
If a given chip wants to mask some type of interrupts when ack'ing
them, it's free to do so and unmask them in end() based on the
IRQ_LEVEL flags for example, I don't think it mandates en entire level
of abstraction (separate flow handlers that is) to handle that simple
case. Since a flow handler should imho be specific to a given (broken)
interrupt chip that can't use for some (unknown) reason use the
default one, I see no problem having that irq_chip implementation
of ack do something specifically matching the needs of whatever flow
handler it's using. One could argue that it will add an "if ()" or two
in the ack implementation, and my answer is that's better than an
indirect function call (see later why I think the default handler
shouldn't be a function pointer, same reason)

Now, back to the root of my problem which is why I don't think we need
to generalise having separate flow handlers and keep that a special
case for broken controllers.

First, as we discussed on IRC, I yet have to find a convincing example
of an irq controller that cannot fit the current __do_IRQ() flow
handler.
I've turned the example you gave me of a cascaded demuxer that does edge
interrupts all the ways around, I still can't see why it can't be done
properly without special flow handlers. I suspect such a controller
also has HW/design bugs that I haven't guessed, an explanation from
Russell King would be welcome here.

Despite that, I agreed that it might be ok to leave the _option_ of
overriding the main flow handler for a given irq_desc. But that should
be clearly presented as an option for use by special case/broken
controllers.

For an example (among others) of why I find the split handlers
approach less robust is the logic of handling an IRQ that is already
in progress. This logic is useful for edge interrupts in the normal
case and thus you implemented it in your edge handler. But why remove
it from the level handler ? For "normal" level interrupts, it's not
supposed to happens, but IRQ controllers have bugs, especially smarter
ones, and that logic can't harm. For example, some SMP distributed irq
controllers might occasionally incorrectly deliver a given IRQ to more
than one CPU at once. Depending on the timing and the architecture
(how the vectors are send to the processor), this can result in just a
spurrious interrupt (no vector) or a "short" irq. In that case, with
your "simplified" level handler, you'll end up potentially re-entering
the "user" (ie driver) action handler. With the "security", the worse
that can happen is that the "user" action handler will be called for
an interrupt that is no longer pending, it will do nothing, return
IRQ_NONE and we'll take note of a spurrious interrupt. Probably only
one, since even if it's a level interrupt, it shouldn't re-occur right
away as it's a "short" interrupt. In any way, it's handled in a robust
way, while potentially re-entering the driver handler isn't. I like
that kind of by-design safety.

Also, the "split" handlers enforce the semantic that, for example, a
level interrupt needs to be mask'ed and ack'ed, to be unmasked later
while an edge interrupt should be left free to flow after ack. That
sounds good on paper and matches probably the requirements of dumb
controllers but doesn't quite agrees with smarter things like
OpenPIC/MPIC, XICS, or even hypervisors. As I wrote above, I think the
generic flow handler calling ack() and end() is plenty enough, it's up to
the irq_chip implementation of those two to decide wether they should
mask a certain type of interrupt (and later unmask it).

Of course that means 2 or 3 more lines of code in the implementation
of dumb interrupt chips with one classical source of bugs which is the
unmasking in end() which should only be done if the interrupt didn't
get disabled while being handled. I understand that you are trying to
make life easier for those. Maybe one option here would be to provide
"helpers" for use by these things. The simplest would be in the form
of an irq_end_shall_unmask() function that can be called in end() to
know wether to unmask or not. A more complex option would be to have a
irq_dumb_chip which contains those additional "low level" functions
and have pair of "helper" versions of ack and end that can be used by
a dumb_chip... That's more like we do in linux:

 Core ----> irq_chip (std set of callbacks)
                 |
		 |
		 |--> irq_dumb_chip (extended set of callbacks)

That is the "standard" interface to the driver is the high level one,
and we can provide pluggable helper functions to implement it on top
of a low level driver

But again, that is provided you really think it's important to save
those few lines of code that needs to be implemented in the ack() and
end() handlers of dmub chips... I don't :)


Now, previously, I also said why I didn't like indirect function calls
and that if's are imho better... If you look at the current __do_IRQ()
you might think it would make sense for example to have percpu
handling be a separate flow handler. But in practice, especially with
heavy pipelined CPUs, it tends to actually be a lot slower to branch
through a function pointer than to handle an if. Maybe it's worth
moving that percpu flow handler to a spearate static function, but the
construct:

  if (special_case) {
  	do_special_case();
	return;
  }
  do_normal_case();

Is faster in many situations than calling a function pointer that can
be either do_special_case() or do_normal_case(). The function pointer
abstraction is still useful in many circumstances and has it's own
justification, but I think in our case, we don't want it.

That's also why we should provide a toplevel:

  extern irqreturn_t handle_irq(unsigned int irq, struct pt_regs *regs);

That essentially boils down to:

  if (desc->flow_handler)
  	return desc->flow_handler();
  normal_flow_handler();

(with the later being either a function call, or just the expanded
thing that currently is called __do_IRQ())

I don't like in your current approach what seems to be (unless I
missed something) that the arch code (toplevel) is supposed to find
the irq desc and do the desc locking.

Such a handle_irq() should also will also be called from within a
cascade handler obviously. (Unless you want to use my SA_CASCADE
approach, but I'm not sure it's that useful here).

I think the arch should have a single function (as above) to call when
it gets a toplevel interrupt. That function handles the picking up of
the irq_desc, the locking,... Ideally, the arch shouldn't need to know
irq_desc outside of the actual irq_chip implementation code.

Also note that I'm calling the flow handler without the lock. Your
code seems to be slightly inconsistent in your locking rules for the
flow handlers. I think it should be local to the flow handler (some
flow handlers might operate lockless like percpu interrupts do).

Ok, that's pretty much all, unless I missed a bit or two. There is a
lot of good stuff in your patches, don't get me wrong, I just think the
flow handler thing is a bit "too much" in it's current incarnation, at
least until somebody proves me that it's really useful :)

All of the cleanups look good, the new flags too (NOPROBE,NOREQUEST,
etc...)

Cheers,
Ben.

Re: [patch 00/50] genirq: -V3

[Russell King]

First, I'll say I haven't read Ingo's patches yet, sorry.  I'm only
responding to _this_ message, not commenting on Ingo's work.  So
this is an initial response only.  I'm not going to be able to properly
review this until June.

On Wed, May 17, 2006 at 04:11:56PM +1000, Benjamin Herrenschmidt wrote:
> >From your previous implementation, you removed the distinction between
> irq_type and irq_chip, they are no longer separate structures.
> But you still basically merged all the "new" fields together. Thus we
> end up with things like both enable/disable/ack/end "high level" and
> mask+ack/unmask "low level" callbacks in the irq chip. That makes
> things confusing. 

First question I have for you is whether you've read through the
existing ARM IRQ handling code.  If not, please do so because it
represents real requirements we have.  Almost everything you see
there is not "by accident" but "by design because it was necessary"
to solve real world problems.

For instance, we do not actively mask interrupts when disable_irq()
is called because we have to record events on edge triggered
interrupts to replay them when a subsequent enable_irq() occurs.
(Some people disagree with this, which is fine from an academic
view point, but unfortunately we have to deal with real life
systems and implementations, where things have to work.)

We also have to deal with stupid combinations such as edge triggered
inputs connected to a secondary interrupt controller, which provides
a pulse trigger output.  In turn, this is logically orred with some
other random non-maskable interrupt sources and fed into an edge
triggered input on the primary interrupt controller.

Unfortunately, saying "we don't support that" is not an option.  We
do support that and we support it cleanly on ARM with the code we
have.

> If we go back to the initial hw_interrupt_type (which was a misnamed
> hw_interrupt_controller, or irq_chip, I'm not opposing the name
> change), we have the enable/disable/ack/end "API" to the main old flow
> handler (__do_IRQ) and other API functions. I am not convinced that it
> makes sense to add "lower level" functions to it at this level.
> Essentially, I think those new callbacks are either redundant or not
> necessary.

You are probably correct, but how do we get to that point without
rewriting from scratch something and probably end up breaking a lot
of machines in the process which used to work?

> First, as we discussed on IRC, I yet have to find a convincing example
> of an irq controller that cannot fit the current __do_IRQ() flow
> handler.

Well, I've not been too forthcoming about this whole "generic IRQ"
thing because (a) I remember all the pain that we had in 2.4 kernels
when we modelled our interrupt system on the x86 way, and (b) I re-
designed our model to something which works for all our requirements
and it does work well with the absolute minimum of overhead... on ARM.

So, I'm rather scared of letting go of something that I know fits our
requirements in favour of going back to something which might be able
to be bent to fit our requirements but might involve compromising on
some corner case.

That said, if someone can show that they can implement a generic IRQ
subsystem which works for x86, PPC, Alpha, ARM, etc, and get it tested
on enough ARM platforms that we're reasonably sure that it's going to
work, I'm not going to stand in the way of that.

> For an example (among others) of why I find the split handlers
> approach less robust is the logic of handling an IRQ that is already
> in progress. This logic is useful for edge interrupts in the normal
> case and thus you implemented it in your edge handler. But why remove
> it from the level handler ? For "normal" level interrupts, it's not
> supposed to happens, but IRQ controllers have bugs, especially smarter
> ones, and that logic can't harm.

Firstly, if you require the more "robust" handling, then you can use
the edge method - nothing stops that.  But why impose the considerable
overhead of the edge method on everyone?

Secondly, there are fundamental differences in the way we handle "edge"
and "level" IRQs on ARM - "edge" interrupts are _always_ unmasked
prior to calling the handlers, whereas "level" interrupts must _never_
be unmasked until all handlers have completed.

The constraint on the "edge" case is that if we leave the interrupt
masked while the handlers are called, and, let's say your ethernet
chip receives a packet just as the drivers handler returns, that
edge transition will be lost, and you'll lose your network interface.

The constraint on the "level" case is that if you leave the interrupt
unmasked, as soon as the CPU unmasks it's interrupt (eg, when calling
a handler with SA_INTERRUPT) you immediately take an interrupt exception
and repeat the process, until your kernel stack has gobbled up all
system memory.

> Also, the "split" handlers enforce the semantic that, for example, a
> level interrupt needs to be mask'ed and ack'ed, to be unmasked later
> while an edge interrupt should be left free to flow after ack. That
> sounds good on paper and matches probably the requirements of dumb
> controllers but doesn't quite agrees with smarter things like
> OpenPIC/MPIC, XICS, or even hypervisors.

As you see above, there's good reasons for that difference in behaviour,
and enforcing one common behaviour breaks real-life hardware on ARM.

I don't have much more of a response now - maybe once I've reviewed
the changes in full (see note at the top of this message) I might have
some more comments.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [patch 00/50] genirq: -V3

[Benjamin Herrenschmidt]

> First question I have for you is whether you've read through the
> existing ARM IRQ handling code.  If not, please do so because it
> represents real requirements we have.  Almost everything you see
> there is not "by accident" but "by design because it was necessary"
> to solve real world problems.
> 
> For instance, we do not actively mask interrupts when disable_irq()
> is called because we have to record events on edge triggered
> interrupts to replay them when a subsequent enable_irq() occurs.

Hrm... that is lost with Ingo/Thomas patch at the moment... mostly
because the irq_type structure is now gone and the only remaining of it
is a per-desc "handler" function which allows custom flow handlers, but
not custom disable_irq/enable_irq.

That might be one argument to keep the split between disable/enable and
mask/unmask in the irq_chip structure but I'm not too keen on that,
since that means adding back flow information to irq_chip which the
patch is trying to get rid of.

An option would be to re-introduce irq_type but I really don't like it

> (Some people disagree with this, which is fine from an academic
> view point, but unfortunately we have to deal with real life
> systems and implementations, where things have to work.)

What is the exact reason where you need to do that ? You controller
stops latching edges when they are masked completely ? Or is just not
emitting upstream irqs (but still have the bits set). The old Apple one
doesn't re-emit when re-enabled but we can still read what happened from
the chip and thus we re-emit them ourselves when re-enabling.

> We also have to deal with stupid combinations such as edge triggered
> inputs connected to a secondary interrupt controller, which provides
> a pulse trigger output.  In turn, this is logically orred with some
> other random non-maskable interrupt sources and fed into an edge
> triggered input on the primary interrupt controller.

So you have a secondary controller that takes an edge input and outputs
an edge too, which edge is also shared with another edge interrupt from
another device ? Damn ! Sharing of edge interrupts is fairly insane in
the first place :) Still, I yet have to see why the above is a problem
with the current flow handler ;)

> Unfortunately, saying "we don't support that" is not an option.  We
> do support that and we support it cleanly on ARM with the code we
> have.

Oh I'm sure of that, but I haven't been proven yet that the code we have
in __do_IRQ() can't support that too :)

At this point it was pretty much agreed to have custom flow handlers
(even if I'm still convinced that a generic one works just fine :)
though we don't have custom enable_irq and disable_irq in the flow
handler. Thus you'll still need an irq_chip per type with the current
approach if you want to do that kind of soft-disabe of egde interrupts

> You are probably correct, but how do we get to that point without
> rewriting from scratch something and probably end up breaking a lot
> of machines in the process which used to work?

Well, at least _document_ the old disable/enable callbacks as being
redundant with the new mask/unmask and on the way to obsolescence to
make the situation clear :) I didn't understand why we kept 4 calls
until I finally figured out that they have indeed the same semantic,
it's just a renaming/compatibility issue

> Well, I've not been too forthcoming about this whole "generic IRQ"
> thing because (a) I remember all the pain that we had in 2.4 kernels
> when we modelled our interrupt system on the x86 way, and (b) I re-
> designed our model to something which works for all our requirements
> and it does work well with the absolute minimum of overhead... on ARM.
>
> So, I'm rather scared of letting go of something that I know fits our
> requirements in favour of going back to something which might be able
> to be bent to fit our requirements but might involve compromising on
> some corner case.
>
> That said, if someone can show that they can implement a generic IRQ
> subsystem which works for x86, PPC, Alpha, ARM, etc, and get it tested
> on enough ARM platforms that we're reasonably sure that it's going to
> work, I'm not going to stand in the way of that.

Ok good :) I was afraid you would stay there saying "if the new generic
code isn't exactly like the ARM stuff I'll stay in my fork" :)

> Firstly, if you require the more "robust" handling, then you can use
> the edge method - nothing stops that.  But why impose the considerable
> overhead of the edge method on everyone?

"considerable overhead" ? heh ! One if and a while loop... I wouldn't
call that considerable :)

> Secondly, there are fundamental differences in the way we handle "edge"
> and "level" IRQs on ARM - "edge" interrupts are _always_ unmasked
> prior to calling the handlers, whereas "level" interrupts must _never_
> be unmasked until all handlers have completed.

Yes, I have seen that. My main concern was that "smart" controllers that
handle the flow in HW are unhappy with that level of abstraction
(mask/ummask's being called from the flow handler instead of just
ack/end). That is solved by having a separate "fastack" flow handler for
these though.

> The constraint on the "edge" case is that if we leave the interrupt
> masked while the handlers are called, and, let's say your ethernet
> chip receives a packet just as the drivers handler returns, that
> edge transition will be lost, and you'll lose your network interface.

Well, it's the same issue you are talking about for
enable_irq/disable_irq and edge interrupts, essentially that you don't
get edge interrupts that were masked. Thus my question above: are they
masked prior to being latched (thus totally lost) or just not re-emitted
when unmasking ? In the later case, it's mostly a matter of reading back
and re-emitting.

However, I do like the whole concept of soft-disabling in the _generic_
case (it's useable for level interrupts as well, they just need to be
masked if they happen while disabled). The current patch from Thomas and
Ingo doesn't do soft-disable afaik. Thus you'll still get your
chip->mask called when disable_irq() is called (which you don't want).

I wonder if we can generalise soft-masking in a way that will allow to
nicely handle your case as well without having to have per-chip
high-level disable/enable...

> The constraint on the "level" case is that if you leave the interrupt
> unmasked, as soon as the CPU unmasks it's interrupt (eg, when calling
> a handler with SA_INTERRUPT) you immediately take an interrupt exception
> and repeat the process, until your kernel stack has gobbled up all
> system memory.

Yes well, thank's for interrupts 101 :)

Ben.

Re: [patch 00/50] genirq: -V3

[Russell King]

On Thu, May 18, 2006 at 10:32:41AM +1000, Benjamin Herrenschmidt wrote:
> > (Some people disagree with this, which is fine from an academic
> > view point, but unfortunately we have to deal with real life
> > systems and implementations, where things have to work.)
> 
> What is the exact reason where you need to do that ? You controller
> stops latching edges when they are masked completely ?

Yes.  The only way to disable interrupt detection on these inputs is
to disable the rising and falling edge detection enables.

Hence, if you disable the edge detection to mask the interrupt, and
an edge transition occurs (the one which you're interested in), when
you come to re-enable the edge detection, that transition will have
been missed.

> > We also have to deal with stupid combinations such as edge triggered
> > inputs connected to a secondary interrupt controller, which provides
> > a pulse trigger output.  In turn, this is logically orred with some
> > other random non-maskable interrupt sources and fed into an edge
> > triggered input on the primary interrupt controller.
> 
> So you have a secondary controller that takes an edge input and outputs
> an edge too, which edge is also shared with another edge interrupt from
> another device ? Damn ! Sharing of edge interrupts is fairly insane in
> the first place :) Still, I yet have to see why the above is a problem
> with the current flow handler ;)

Not quite - the other non-maskable interrupt sources are level based
outputs.  In this particular case, these sources are fed through an
FPGA and you have status bits for each, but no way to enable or disable
each individual source.  The output interrupt is just a logical OR of
the sources.

> > Secondly, there are fundamental differences in the way we handle "edge"
> > and "level" IRQs on ARM - "edge" interrupts are _always_ unmasked
> > prior to calling the handlers, whereas "level" interrupts must _never_
> > be unmasked until all handlers have completed.
> 
> Yes, I have seen that. My main concern was that "smart" controllers that
> handle the flow in HW are unhappy with that level of abstraction
> (mask/ummask's being called from the flow handler instead of just
> ack/end). That is solved by having a separate "fastack" flow handler for
> these though.

It sounds like you've solved this problem already.

> > The constraint on the "edge" case is that if we leave the interrupt
> > masked while the handlers are called, and, let's say your ethernet
> > chip receives a packet just as the drivers handler returns, that
> > edge transition will be lost, and you'll lose your network interface.
> 
> Well, it's the same issue you are talking about for
> enable_irq/disable_irq and edge interrupts, essentially that you don't
> get edge interrupts that were masked. Thus my question above: are they
> masked prior to being latched (thus totally lost) or just not re-emitted
> when unmasking ? In the later case, it's mostly a matter of reading back
> and re-emitting.

Totally lost - there is nothing to read back to tell you that the
active edge transition occurred.

> However, I do like the whole concept of soft-disabling in the _generic_
> case (it's useable for level interrupts as well, they just need to be
> masked if they happen while disabled).

That is incredibly wasteful for level interrupts - what you're suggesting
means that to service a level interrupt, you take an interrupt exception,
start processing it, take another interrupt exception, disable the source,
return from that interrupt and continue to service it.  No thanks.

I thought you were the one concerned about interrupt handling overhead
(about the overhead introduced by function pointer calls.) but that
idea _far_ outweighs function pointer overheads.

> > The constraint on the "level" case is that if you leave the interrupt
> > unmasked, as soon as the CPU unmasks it's interrupt (eg, when calling
> > a handler with SA_INTERRUPT) you immediately take an interrupt exception
> > and repeat the process, until your kernel stack has gobbled up all
> > system memory.
> 
> Yes well, thank's for interrupts 101 :)

Sigh, I'm not teaching you to suck eggs - I was trying to justify
clearly _why_ we have these different "flow" handlers on ARM and why
they are important by contrasting the differences between them.

Obviously, I should've just ignored your email since you know everything
already.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [patch 00/50] genirq: -V3

[Benjamin Herrenschmidt]

Hi Russell !

> That is incredibly wasteful for level interrupts - what you're suggesting
> means that to service a level interrupt, you take an interrupt exception,
> start processing it, take another interrupt exception, disable the source,
> return from that interrupt and continue to service it.  No thanks.

Oh no, that's not what I mean... I've come to agree with having several
flow handlers and thus the level flow handler would mask & ack, then
handle, then unmask is it should be for a level interrupt... What I
meant is that disable_irq could do soft-disable in all cases like it
seems to happen right now in the ARM code but not in Thomas patch.

> I thought you were the one concerned about interrupt handling overhead
> (about the overhead introduced by function pointer calls.) but that
> idea _far_ outweighs function pointer overheads.

I think you misunderstood what I meant by soft-disable :) Basically
bring in more of what ARM does in disable_irq/enable_irq.

> Sigh, I'm not teaching you to suck eggs - I was trying to justify
> clearly _why_ we have these different "flow" handlers on ARM and why
> they are important by contrasting the differences between them.

Yup, and I've finally been convinced, and Thomas patch _does_ have
different flow handlers. However, it doesn't do soft-disable or
lazy-disable as your prefer for disable_irq which means that you'll
still lose edge irqs on ARM. There are 2 ways out:

make disable_irq/enable_irq go through a specific implementation by the
flow handler or just ... generically have disable_irq just mark the
interrupt as disabled in the descriptor, and only actually disable it if
it happens to occur while it was marked disabled (in which case it can
be marked "pending" and possibly re-triggered on enable_irq if the
controller doesn't latch). I even had an idea of how to avoid the
re-trigger on controllers that _do_ latch properly easily: by having
chip->unmask return wether it needs re-emitting or not.

> Obviously, I should've just ignored your email since you know everything
> already.

Bah, don't take it that way please ! I was making a joke ...

Cheers,
Ben.

[patchset] Generic IRQ Subsystem: -V4

[Ingo Molnar]

This is Version 4 of the genirq patch-queue, against 2.6.17-rc4. This 
patch-queue improves the generic IRQ layer to be truly generic, by 
adding various abstractions and features to it, without impacting 
existing functionality.

The number of patches has increased to 56 and the total patchsize has 
increased to 294K, so we wont be sending the patches to lkml. The full 
patch-queue can be downloaded from:

  http://redhat.com/~mingo/generic-irq-subsystem/

The split-out queue is at:

  http://redhat.com/~mingo/generic-irq-subsystem/patches/

In -V4 there are lots of changes all around the map. The most 
significant one is the conversion of the i386 and the x86_64 
architectures to a fully irq-chip based IRQ handling model.

This brought nice simplifications for these architectures:

$ diffstat patches/genirq-i386-irqchip.patch

  arch/i386/kernel/i8259.c   |   45 +++--------
  arch/i386/kernel/io_apic.c |  171 ++++++++++++++-------------------------------
  arch/i386/kernel/irq.c     |   19 ++---
  include/asm-i386/hw_irq.h  |    2
  4 files changed, 80 insertions(+), 157 deletions(-)

$ diffstat patches/genirq-x86_64-irqchip.patch

 arch/x86_64/kernel/i8259.c   |   50 +++----------
 arch/x86_64/kernel/io_apic.c |  165 ++++++++++---------------------------------
 arch/x86_64/kernel/irq.c     |    7 +
 include/asm-x86_64/hw_irq.h  |    2 
 4 files changed, 60 insertions(+), 164 deletions(-)

the IRQ handling hotpath for the most optimal flow got faster and 
leaner. The number of functions called in a typical IRQ decreased too. 
The irqchip implementations have become short and sweet:

 static struct irq_chip ioapic_chip __read_mostly = {
         .name           = "IO-APIC",
         .startup        = startup_ioapic_vector,
         .mask           = mask_ioapic_vector,
         .unmask         = unmask_ioapic_vector,
         .ack            = ack_apic,
 #ifdef CONFIG_SMP
         .set_affinity   = set_ioapic_affinity_vector,
 #endif
         .retrigger      = ioapic_retrigger_vector,
 };

 static struct irq_chip lapic_chip __read_mostly = {
         .name           = "local-APIC-edge",
         .mask           = mask_lapic_irq,
         .unmask         = unmask_lapic_irq,
         .ack            = ack_apic,
 };

 static struct irq_chip i8259A_chip = {
         .name           = "XT-PIC",
         .mask           = disable_8259A_irq,
         .unmask         = enable_8259A_irq,
         .mask_ack       = mask_and_ack_8259A,
 };

see the split-out patches at:

 http://redhat.com/~mingo/generic-irq-subsystem/patches/genirq-i386-irqchip.patch
 http://redhat.com/~mingo/generic-irq-subsystem/patches/genirq-x86_64-irqchip.patch

Another new feature is the handle_level_irq_fastack() highlevel irq-flow 
handler, which allows faster IRQ handling without masking/unmasking. 
This is current used by the new i386 and x86_64 IO-APIC code, but it 
could also be useful to most optimally support transparent ACK sequences 
in OpenPIC/MPIC controllers.

There were also extensive renames done to make the irq_chip and irq_desc 
fields clearer.

All changes since -V3:

- fixed set_wake prototype bug (reported by Daniel Walker)

- documentation fixes (Randy Dunlap)

- call desc->handle_irq() highlevel irq-flow handlers from __do_IRQ() 
  too, allowing gradual transition from a __do_IRQ() based model to an 
  irq-chips model. This was used for the i386 and x86_64 transition with 
  great success.

- ARM updates

- More size reduction:

      text    data     bss     dec     hex filename
   3430374  985464 1325080 5740918  579976 vmlinux-x64.orig
   3434234  982216 1321240 5737690  578cda vmlinux-x64.genirq

  while .text got larger, .data and .bss got smaller, so it's a net
  size reduction of ~4K.

- better debugging info from spurious interrupts.

- truly remove the irq_dir[] and smp_affinity_entry[] arrays. (this 
  change was announced in -V3 but went MIA)

- introduced the handle_level_irq_fastack() highlevel flow handler, for 
  fast IO-APIC interrupt handling. This should enable the most optimal 
  flow for transparent-ACK IRQ controllers too. (such as OpenPIC/MPIC)

- pushed desc->lock handling into the highlevel flow handlers. This 
  unified and simplified the arch-level call sites.

- sem2mutex: kernel/irq/autoprobe.c probe_sem => probe_lock mutex 
  conversion.

- changed the desc->handle_irq() handlers to be explicit fastcalls, to 
  keep the 4K_STACKS assembly code simpler.

- removed desc->affinity_entry - nothing uses it

- added chip->name as the primary field - chip->typename is still 
  kept for compatibility purposes.

- probe_irq() related fix in drivers/mfd/ucb1x00-core.c

- include flow information in /proc/interrupts for irq-chip based 
  architectures too.

- genirq-i386-irqchip.patch: change the i386 architecture to be irqchip 
  based.

- genirq-x86_64-irqchip.patch: change the x86_64 architecture to be 
  irqchip based.

- renamed 'desc->handler' to 'desc->chip' - the code got much cleaner 
  due to this.

- retrigger fix on i386 and x86_64, it is now MSI-vector aware.

- renamed 'desc->handle' to 'desc->handle_irq'

- moved the set_irq_handler() method of setting desc->handle_irq() from 
  ARM into the generic code.

-V4 has been build-tested with allyesconfig, and booted on x86, x86_64 
and various ARM platforms. Nevertheless the new i386 and x86_64 IRQ code 
is experimental and has been tested only on a limited number of systems.

	Ingo, Thomas

Re: [patchset] Generic IRQ Subsystem: -V4

[Ingo Oeser]

Hi Ingo,

just a minor nit.

On Thursday, 18. May 2006 00:15, Ingo Molnar wrote:
> - sem2mutex: kernel/irq/autoprobe.c probe_sem => probe_lock mutex 
>   conversion.

Please call this probe_mutex. probe_lock would suggest, 
that this is a spin_lock() when people talk about it.

Didn't look at the actual patch, so maybe you just have a bug
in this list here :-)


Regards

Ingo Oeser

Re: [patchset] Generic IRQ Subsystem: -V4

[Ingo Molnar]

* Ingo Oeser <ioe-lkml@rameria.de> wrote:

> Hi Ingo,
> 
> just a minor nit.
> 
> On Thursday, 18. May 2006 00:15, Ingo Molnar wrote:
> > - sem2mutex: kernel/irq/autoprobe.c probe_sem => probe_lock mutex 
> >   conversion.
> 
> Please call this probe_mutex. probe_lock would suggest, that this is a 
> spin_lock() when people talk about it.

i renamed it to "probing_active", which describes its purpose even 
better. Ok?

	Ingo

Re: [patchset] Generic IRQ Subsystem: -V4

[Ingo Oeser]

Hi Ingo,

On Friday, 19. May 2006 11:31, Ingo Molnar wrote:
> i renamed it to "probing_active", which describes its purpose even 
> better. Ok?

Ok.


Regards

Ingo Oeser

[patchset] Generic IRQ Subsystem: -V5

[Ingo Molnar]

This is Version 5 of the genirq patch-queue, against 2.6.17-rc4. The 
genirq patch-queue improves the generic IRQ layer to be truly generic, 
by adding various abstractions and features to it, without impacting 
existing functionality. It converts ARM, i386 and x86_64 to this new 
generic IRQ layer - while keeping compatibility with all the other 
genirq architectures too.

The full patch-queue can be downloaded from:

  http://redhat.com/~mingo/generic-irq-subsystem/

genirq -V5 is mostly about fixes and smaller cleanups. Suggestions from 
many people were included - let us know if anything is missing. It has 
been tested on i386, x86_64 and ARM systems.

Changes since -V4:

 - fix: added IRQ_INPROGRESS protection to the fastack, level and simple
   flow handlers, to avoid irq reentry due to hardware bugs. (especially 
   on SMP this is not unheard of.)

 - fix: fixed two old-style IRQ probing bugs on non-irqchip
   architectures.

 - fix: bad irq vectors should increase the IRQ kstat too

 - feature: added IRQ_DELAYED_DISABLE support for edge-irq handling on 
   really dumb controllers.

 - docs: various documentation and comment updates

 - cleanup/speedup: cleaned up desc->depth and IRQ_DISABLED handling - 
   the two were not always in sync. Made the generic flow handlers use
   IRQ_DISABLED instead of desc->depth - results in more optimal code.

 - cleanup: introduced generic_handle_irq(irq) for architectures to call 
   when they want the generic layer to handle an interrupt.

 - rename: renamed probe_lock to probing_active

 - rename: renamed handle_level_irq_fastack to handle_fastack_irq - 
   there is no strict requirement of the trigger type of the irq.

The split-out queue can be found at:

  http://redhat.com/~mingo/generic-irq-subsystem/patches/

	Ingo, Thomas

Re: [patchset] Generic IRQ Subsystem: -V5

[Russell King]

On Fri, May 19, 2006 at 04:52:25PM +0200, Ingo Molnar wrote:
> This is Version 5 of the genirq patch-queue, against 2.6.17-rc4. The 
> genirq patch-queue improves the generic IRQ layer to be truly generic, 
> by adding various abstractions and features to it, without impacting 
> existing functionality. It converts ARM, i386 and x86_64 to this new 
> generic IRQ layer - while keeping compatibility with all the other 
> genirq architectures too.
> 
> The full patch-queue can be downloaded from:
> 
>   http://redhat.com/~mingo/generic-irq-subsystem/

Is there an updated series?  This doesn't apply to -rc6 - it seems
that maybe the ia64 folk merged some of the changes.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [patchset] Generic IRQ Subsystem: -V5

[Thomas Gleixner]

On Wed, 2006-06-07 at 17:54 +0100, Russell King wrote:
> On Fri, May 19, 2006 at 04:52:25PM +0200, Ingo Molnar wrote:
> > This is Version 5 of the genirq patch-queue, against 2.6.17-rc4. The 
> > genirq patch-queue improves the generic IRQ layer to be truly generic, 
> > by adding various abstractions and features to it, without impacting 
> > existing functionality. It converts ARM, i386 and x86_64 to this new 
> > generic IRQ layer - while keeping compatibility with all the other 
> > genirq architectures too.
> > 
> > The full patch-queue can be downloaded from:
> > 
> >   http://redhat.com/~mingo/generic-irq-subsystem/
> 
> Is there an updated series?  This doesn't apply to -rc6 - it seems
> that maybe the ia64 folk merged some of the changes.

We did the latest changes against -mm. I can respin it against
2.6.16-rc6.

	tglx

Re: [patchset] Generic IRQ Subsystem: -V5

[Thomas Gleixner]

On Wed, 2006-06-07 at 19:20 +0200, Thomas Gleixner wrote: 
> > Is there an updated series?  This doesn't apply to -rc6 - it seems
> > that maybe the ia64 folk merged some of the changes.
> 
> We did the latest changes against -mm. I can respin it against
> 2.6.16-rc6.

http://www.tglx.de/projects/armirq/2.6.17-rc6/patch-2.6.17-rc6-armirq1.patches.tar.bz2
http://www.tglx.de/projects/armirq/2.6.17-rc6/patch-2.6.17-rc6-armirq1.patch

	tglx

Re: [patchset] Generic IRQ Subsystem: -V5

[Russell King]

On Wed, Jun 07, 2006 at 08:57:30PM +0200, Thomas Gleixner wrote:
> On Wed, 2006-06-07 at 19:20 +0200, Thomas Gleixner wrote: 
> > > Is there an updated series?  This doesn't apply to -rc6 - it seems
> > > that maybe the ia64 folk merged some of the changes.
> > 
> > We did the latest changes against -mm. I can respin it against
> > 2.6.16-rc6.
> 
> http://www.tglx.de/projects/armirq/2.6.17-rc6/patch-2.6.17-rc6-armirq1.patches.tar.bz2
> http://www.tglx.de/projects/armirq/2.6.17-rc6/patch-2.6.17-rc6-armirq1.patch

Okay, works on Versatile (which is a trivial platform) it doesn't work
on Neponset (a rather more complex setup).  Neponset has a case where
there's an interrupt "concentrator" which consists of logically ORing
three interrupt sources, and providing a status register so you know
which occurred.

Hence, there is no "chip" for this, and while it works with the ARM
IRQ subsystem, it doesn't even boot with the genirq stuff.

This doesn't happen with the ARM IRQ subsystem because the "no chip"
handlers are all pointing at a dummy function instead of being NULL.
Could we do the same with genirq ?

SA1111 Microprocessor Companion Chip: silicon revision 1, metal revision 1
Trying to install chained interrupt type for IRQ51
Unable to handle kernel NULL pointer dereference at virtual address 00000000
pgd = c0204000
[00000000] *pgd=00000000
Internal error: Oops: 0 [#1]
Modules linked in:
CPU: 0
PC is at __init_begin+0x3fdf8000/0x30
LR is at __set_irq_handler+0xf4/0x110
pc : [<00000000>]    lr : [<c0258e6c>]    Not tainted
sp : c04dbdb4  ip : 60000093  fp : c04dbdd8
r10: c02577c4  r9 : 00000000  r8 : 00000001
r7 : 60000013  r6 : c0229234  r5 : 00000033  r4 : c040ecc0
r3 : c0416e38  r2 : 00000000  r1 : 00000629  r0 : 00000033
Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  Segment kernel
Control: C020717F  Table: C020717F  DAC: 00000017
Process swapper (pid: 1, stack limit = 0xc04da198)
[<c0258d78>] (__set_irq_handler+0x0/0x110) from [<c0229778>] (sa1111_setup_irq+0x10c/0x128)
[<c022966c>] (sa1111_setup_irq+0x0/0x128) from [<c0229b6c>] (__sa1111_probe+0x10c/0x1b4)
[<c0229a60>] (__sa1111_probe+0x0/0x1b4) from [<c0229f78>] (sa1111_probe+0x54/0x60)
[<c0229f24>] (sa1111_probe+0x0/0x60) from [<c0330b80>] (platform_drv_probe+0x20/0x24)
[<c0330b60>] (platform_drv_probe+0x0/0x24) from [<c032ebac>] (driver_probe_device+0x8c/0xd8)
[<c032eb20>] (driver_probe_device+0x0/0xd8) from [<c032ec08>] (__device_attach+0x10/0x14)
[<c032ebf8>] (__device_attach+0x0/0x14) from [<c032e1a0>] (bus_for_each_drv+0x54/0x84)
[<c032e14c>] (bus_for_each_drv+0x0/0x84) from [<c032ec70>] (device_attach+0x64/0x98)
[<c032ec0c>] (device_attach+0x0/0x98) from [<c032e30c>] (bus_add_device+0x30/0x88)
[<c032e2dc>] (bus_add_device+0x0/0x88) from [<c032d0f0>] (device_add+0xc8/0x14c)
[<c032d028>] (device_add+0x0/0x14c) from [<c032d190>] (device_register+0x1c/0x20)
[<c032d174>] (device_register+0x0/0x20) from [<c03309dc>] (platform_device_add+0xf8/0x16c)
[<c03308e4>] (platform_device_add+0x0/0x16c) from [<c0330ad0>] (platform_device_register+0x20/0x24)
[<c0330ab0>] (platform_device_register+0x0/0x24) from [<c0330750>] (platform_add_devices+0x2c/0x68)
[<c0330724>] (platform_add_devices+0x0/0x68) from [<c02121bc>] (neponset_init+0x7c/0x98)
[<c0212140>] (neponset_init+0x0/0x98) from [<c0208ae4>] (do_initcalls+0x68/0x128)
[<c0208a7c>] (do_initcalls+0x0/0x128) from [<c0208bc4>] (do_basic_setup+0x20/0x24)
[<c0208ba4>] (do_basic_setup+0x0/0x24) from [<c021f0b0>] (init+0x44/0x144)
[<c021f06c>] (init+0x0/0x144) from [<c023babc>] (do_exit+0x0/0x3c4)


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

[PATCH] genirq: Fix missing initializer for unmask in no_irq_chip

[Thomas Gleixner]

On Thu, 2006-06-08 at 12:35 +0100, Russell King wrote:
> Okay, works on Versatile (which is a trivial platform) it doesn't work
> on Neponset (a rather more complex setup).  Neponset has a case where
> there's an interrupt "concentrator" which consists of logically ORing
> three interrupt sources, and providing a status register so you know
> which occurred.
> 
> Hence, there is no "chip" for this, and while it works with the ARM
> IRQ subsystem, it doesn't even boot with the genirq stuff.
> 
> This doesn't happen with the ARM IRQ subsystem because the "no chip"
> handlers are all pointing at a dummy function instead of being NULL.
> Could we do the same with genirq ?

We missed to initialize unmask, which causes problems on neponset.

Signed-off-by: Thomas Gleixner <tglx@linutronix,de>

Index: linux-2.6.17-rc6/kernel/irq/handle.c
===================================================================
--- linux-2.6.17-rc6.orig/kernel/irq/handle.c	2006-06-08 13:57:57.000000000 +0200
+++ linux-2.6.17-rc6/kernel/irq/handle.c	2006-06-08 13:55:56.000000000 +0200
@@ -92,6 +92,7 @@
 	.enable		= noop,
 	.disable	= noop,
 	.ack		= ack_bad,
+	.unmask		= noop,
 	.end		= noop,
 };
 

Re: [PATCH] genirq: Fix missing initializer for unmask in no_irq_chip

[Russell King]

On Thu, Jun 08, 2006 at 02:04:15PM +0200, Thomas Gleixner wrote:
> On Thu, 2006-06-08 at 12:35 +0100, Russell King wrote:
> > Okay, works on Versatile (which is a trivial platform) it doesn't work
> > on Neponset (a rather more complex setup).  Neponset has a case where
> > there's an interrupt "concentrator" which consists of logically ORing
> > three interrupt sources, and providing a status register so you know
> > which occurred.
> > 
> > Hence, there is no "chip" for this, and while it works with the ARM
> > IRQ subsystem, it doesn't even boot with the genirq stuff.
> > 
> > This doesn't happen with the ARM IRQ subsystem because the "no chip"
> > handlers are all pointing at a dummy function instead of being NULL.
> > Could we do the same with genirq ?
> 
> We missed to initialize unmask, which causes problems on neponset.

Okay, with -rc6 + genirq + the following patch, it appears to work
provided you don't stress it.  As soon as I load the system up with
CF activity, a full-sized flood ping and hit "enter" a few times on
the console, it locks up solid - no oops, no nothing, the machine
just completely freezes.

This does not happen with the existing ARM IRQ code.

I'll try to debug this odd behaviour later today, but first I need to
resurect my NMI oopser code for this platform.

--- linux-2.6.17-rc6/kernel/irq/handle.c	2006-06-07 20:54:01.000000000 +0200
+++ b/kernel/irq/handle.c
@@ -91,7 +91,8 @@
 	.shutdown	= noop,
 	.enable		= noop,
 	.disable	= noop,
-	.ack		= ack_bad,
+	.ack		= noop, // needed to quiten down boot time complaints
+	.unmask		= noop,	// needed to prevent oops on sa1111 init
 	.end		= noop,
 };
 
--- linux-2.6.17-rc6/kernel/irq/manage.c	2006-06-07 20:53:54.000000000 +0200
+++ b/kernel/irq/manage.c
@@ -199,8 +199,8 @@
 	if (irq >= NR_IRQS)
 		return -EINVAL;
 
-	if (desc->chip == &no_irq_chip)
-		return -ENOSYS;
+//	if (desc->chip == &no_irq_chip) // prevents smc91x initialising
+//		return -ENOSYS;
 	/*
 	 * Some drivers like serial.c use request_irq() heavily,
 	 * so we have to be careful not to interfere with a


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] genirq: Fix missing initializer for unmask in no_irq_chip

[Thomas Gleixner]

On Thu, 2006-06-08 at 16:29 +0100, Russell King wrote:
> > > This doesn't happen with the ARM IRQ subsystem because the "no chip"
> > > handlers are all pointing at a dummy function instead of being NULL.
> > > Could we do the same with genirq ?
> > 
> > We missed to initialize unmask, which causes problems on neponset.
> 
> Okay, with -rc6 + genirq + the following patch,

I uploaded proper fixups for those:
http://tglx.de/projects/armirq/2.6.17-rc6/patch-2.6.17-rc6-armirq3.patches.tar.bz2

>  it appears to work
> provided you don't stress it.  As soon as I load the system up with
> CF activity, a full-sized flood ping and hit "enter" a few times on
> the console, it locks up solid - no oops, no nothing, the machine
> just completely freezes.
> 
> This does not happen with the existing ARM IRQ code.
> 
> I'll try to debug this odd behaviour later today, but first I need to
> resurect my NMI oopser code for this platform.

Thanks,

	tglx

Re: [PATCH] genirq: Fix missing initializer for unmask in no_irq_chip

[Thomas Gleixner]

On Thu, 2006-06-08 at 17:58 +0200, Thomas Gleixner wrote:
> >  it appears to work
> > provided you don't stress it.  As soon as I load the system up with
> > CF activity, a full-sized flood ping and hit "enter" a few times on
> > the console, it locks up solid - no oops, no nothing, the machine
> > just completely freezes.
> > 
> > This does not happen with the existing ARM IRQ code.
> > 
> > I'll try to debug this odd behaviour later today, but first I need to
> > resurect my NMI oopser code for this platform.

Russell, 

thanks for tracking this down. It turned out to be a reentrancy problem
which was silently ignored by the original ARM implementation.

I uploaded a new patch series which contains the ARM side fix and I
added error messages for that case at the appropriate places.

http://www.tglx.de/projects/armirq/2.6.17-rc6/patch-2.6.17-rc6-armirq4.patch

http://www.tglx.de/projects/armirq/2.6.17-rc6/patch-2.6.17-rc6-armirq4.patches.tar.bz2

Ingo, Ben,

can you please check whether the following patch does trigger any false
positives for you ?

	tglx

 kernel/irq/chip.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Index: linux-2.6.17-rc6/kernel/irq/chip.c
===================================================================
--- linux-2.6.17-rc6.orig/kernel/irq/chip.c	2006-06-08 19:26:16.000000000 +0200
+++ linux-2.6.17-rc6/kernel/irq/chip.c	2006-06-08 19:26:16.000000000 +0200
@@ -208,8 +208,11 @@
 
 	spin_lock(&desc->lock);
 
-	if (unlikely(desc->status & IRQ_INPROGRESS))
+	if (unlikely(desc->status & IRQ_INPROGRESS)) {
+		printk(KERN_ERR "handle_simple_irq reentered while "
+		       "processing irq %d\n", irq);
 		goto out_unlock;
+	}
 	desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
 	kstat_cpu(cpu).irqs[irq]++;
 
@@ -251,8 +254,11 @@
 	spin_lock(&desc->lock);
 	mask_ack_irq(desc, irq);
 
-	if (unlikely(desc->status & IRQ_INPROGRESS))
+	if (unlikely(desc->status & IRQ_INPROGRESS)) {
+		printk(KERN_ERR "handle_level_irq reentered while "
+		       "processing irq %d\n", irq);
 		goto out;
+	}
 	desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
 	kstat_cpu(cpu).irqs[irq]++;
 
@@ -273,9 +279,9 @@
 
 	spin_lock(&desc->lock);
 	desc->status &= ~IRQ_INPROGRESS;
-out:
 	if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
 		desc->chip->unmask(irq);
+out:
 	spin_unlock(&desc->lock);
 }
 

Re: [PATCH] genirq: Fix missing initializer for unmask in no_irq_chip

[Thomas Gleixner]

On Thu, 2006-06-08 at 20:57 +0200, Thomas Gleixner wrote:
> I uploaded a new patch series which contains the ARM side fix and I
> added error messages for that case at the appropriate places.

Sorry, pushed out the wrong one.

Uploaded the non broken version

http://www.tglx.de/projects/armirq/2.6.17-rc6/patch-2.6.17-rc6-armirq5.patch
http://www.tglx.de/projects/armirq/2.6.17-rc6/patch-2.6.17-rc6-armirq5.patches.tar.bz2

	tglx