On Fri, 2006-09-08 at 11:16 +0100, David Howells wrote:
> Benjamin Herrenschmidt <[email protected]> wrote:
>
> > > + .end = frv_fpga_end,
> > > +};
> >
> > Why do you have a end() handler ? (and an empty one....) Your FPGA
> > interrupts ase using the level generic flow handler right ? That doesn't
> > call end(). Though you might want to have a mask_ack() "combined"
> > callback to avoid two indirect calls (it's an optional optimisation).
>
> This is called unconditionally from __do_IRQ(). It's also not listed in
> genericirq.tmpl, though ack(), mask(), etc. are. The header file (irq.h)
> doesn't say that it is optional, though it does say that startup(), shutdown(),
> etc. are, therefore the assumption would be that it isn't optional.
Well, you shoud not use __do_IRQ if you use the genirq stuff... the
callbacks that are needed depend on which flow handler you use.
> > Ahem... __do_IRQ()... That's probably part of your bloat... With a
> > conversion to genirq, you shouldn't use __do_IRQ() anymore (and thus it
> > shouldn't be compiled in). In fact, that defeats genirq completely as
> > you aren't calling the new handlers at all there. You should call
> > generic_handle_irq() instead.
>
> It's not mentioned in the docs:
>
> warthog>grep generic_handle_irq Documentation/DocBook/genericirq.tmpl
> warthog1>
>
> Looking at the code itself, generic_handle_irq() is a wrapper around
> __do_IRQ():
>
No, it's a wrapper around desc->handler_irq, which is the flow handler
you install. The fallback to __do_IRQ is only if you don't implement a
flow handler (still have one of your PICs not using it). However, that
means that if you use generic_handle_irq(), then __do_IRQ will have to
be linked in, that's a good point, we might want to provide a different
wrapper for archs that have done a total conversion...
> static inline void generic_handle_irq(unsigned int irq,
> struct pt_regs *regs)
> {
> struct irq_desc *desc = irq_desc + irq;
>
> if (likely(desc->handle_irq))
> desc->handle_irq(irq, desc, regs);
> else
> __do_IRQ(irq, regs);
> }
>
> So I don't see how it can help but make the code larger. The if-statement in
> it can't be optimised down because its condition depends on a value held in
> memory and is not subject to compile-time evaluation.
That helper is my fault :) It's mostly because at one point, I had a
partial conversion and some of my PICs were still using __do_IRQ(). Ingo
initial design wants you to call desc->handle_irq() directly.
> It will make things quicker, probably, since it bypasses __do_IRQ() when
> possible, but if it can't, it will make things slower as you'll have this
> check, the condition will fail against expectations, and then you'll still have
> to do __do_IRQ() anyway.
Yes, we can avoid it though as I just wrote.
> > > +static struct irqaction fpga_irq[4] = {
> > ...
> > .../...
>
> Can you rephrase your comment?
>
> > > + setup_irq(IRQ_CPU_EXTERNAL0, &fpga_irq[0]);
> > > + setup_irq(IRQ_CPU_EXTERNAL1, &fpga_irq[1]);
> > > + setup_irq(IRQ_CPU_EXTERNAL2, &fpga_irq[2]);
> > > + setup_irq(IRQ_CPU_EXTERNAL3, &fpga_irq[3]);
> > > }
> >
> > Your approach to cascades might be wrong here. Instead of setting up an
> > irq handler, you could just attach a chained flow handler. Much less
> > overhead.
>
> That's an undocumented feature:
I haven't checked the docs. I suggest you look how I did it on powerpc.
I'll see if I can improve the doc next week.
> warthog>grep -i chain Documentation/DocBook/genericirq.tmpl
> warthog1>
>
> > > + .enable = frv_fpga_enable,
> > > + .disable = frv_fpga_disable,
> > > + .ack = frv_fpga_ack,
> > > + .mask = frv_fpga_disable,
> > > + .unmask = frv_fpga_enable,
> > > + .end = frv_fpga_end,
> > > +};
> >
> > SImilar comments. Also, you are using enable/disable here. Just leave
> > them NULL and the generic code will call your mask/unmask.
>
> Hmmm... I see that irq_chip_set_defaults() installs intermediary functions in
> the chip ops table for anything it doesn't have. Surely it'd be better to
> require people to fill in the appropriate default functions directly as you
> could then make the table const. That's what you've got documentation for,
> right?
Well... enable/disable are higher level than mask/umask... it's not
completely clear to me if we should keep them under control of the
irq_chip at all ... It's mostly useful for thigns that haven't yet been
ported...
> > You do that in another one at least... And I have to go now so I can't
> > finish reviewing your patch :) But it looks like you aren't properly
> > "converting" to the genirq code and thus not getting all of the benefit,
> > like faster code path to cascade handlers, etc... and you aren't getting
> > rid of __do_IRQ() so at the end of the day, you aren't using the new
> > stuff and still link in the old one :)
>
> Documentation/DocBook/genericirq.tmpl doesn't do a very good job of explaining
> it. The comments in include/linux/irq.h also need a check as some of them are
> out of date.
>
> Take the comment at the head of struct irq_desc for example, it mentions fields
> that are absent in the structure, the HTML doc generator spits out warnings in
> relation to linux/irq.h.
>
> I would also recommend that a line be added to the banner comments at the top
> of linux/irq.h to point to the documentation, just like is done in the .c files
> implementing genirq.
We should indeed improve the documentation.
Cheers,
Ben.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
