Re: [Bulk] Re: [patch 2.6.18] genirq: remove oops with fasteoiirq_chip descriptors
From: Thomas Gleixner
Date: Wed Sep 27 2006 - 21:23:48 EST
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
> - 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.
> > > 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.
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/