Messages in this thread | | | Subject | Re: [Bulk] Re: [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors | From | Thomas Gleixner <> | Date | Thu, 28 Sep 2006 03:18:19 +0200 |
| |
Dave,
On Wed, 2006-09-27 at 17:39 -0700, David Brownell wrote: > - It wouldn't use chip->mask_ack() when that exists and those > other two routines don't, even though mask_ack_irq() is a > conveniently defined inline.
So why not replace it by mask_ack_irq() ?
> - Umm, how could it ever be correct to leave the IRQ active > without a dispatcher? ISTR the rationale for that delayed > disable was not purely to be a PITA for all driver writers, > but was to address some issue with edge triggering. In that > path, triggering was no longer to be allowed ...
Your patch would result in default_disable() when no shutdown function is provided. default_disable() does the delayed disable thing, while you remove the handler. The next event on that line will cause a spurious IRQ.
> - Plus ack()ing the IRQ there just seemed pretty dubious. It's > not like there would be anything preventing that signal line > from being lowered (or raised, etc) immediately after the ack(), > which in some hardware would latch the IRQ until later unmask(). > > Leaving the question: what's the point of it?? The overall > system has to behave sanely with or without the ack(); just > clearing a latch doesn't mean it couldn't get set later.
Fair enough.
> > > So what's the correct fix then ... use enable() and disable()? > > > Oopsing isn't OK... > > > > True, but we can not unconditionally change the semantics. > > Some current semantics are "it oopses". That's a good definition > of semantics that _must_ be changed. We're not Microsoft. ;)
Agreed, it just depends on how they get fixed.
> > Does it break existing or new code ? > > Could any code relying on those previous semantics have been > correct in the first place, though? Seemed to me it couldn't > have been. > > Plus, unregistering IRQ dispatchers is a strange notion. I've > never seen it done in practice ... normally, they get set up once > during chip/board setup then never changed. Bugs in code paths > like that have been known to last for decades unfixed.
Agreed. Nothing is using this currently.
> > Sorry, I did not think about the defaults in the first place. The > > conditionals in manage,c are probably superflous leftovers from one of > > the evolvement. > > And that's how I was taking that particular mask() then ack() too, > especially given it never used mask_ack() when it should have, and > since that logic oopsed in various cases with fasteoi handlers.
The remaining question is whether mask_ack_irq() or shutdown() is the correct approach. Your patch would make it mandatory to implement shutdown at least for such removable stuff.
I'm not sure about that right now as I'm too tired.
tglx
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |