Re: [Bulk] Re: [patch 2.6.18] genirq: remove oops with fasteoiirq_chip descriptors

[Thomas Gleixner]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/