Message-Id: <200609271739.10215.david-b@pacbell.net>
Date:	Wed, 27 Sep 2006 17:39:09 -0700
From:	David Brownell <david-b@...bell.net>
To:	tglx@...utronix.de
Cc:	Linux Kernel list <linux-kernel@...r.kernel.org>, mingo@...hat.com
Subject: Re: [Bulk] Re: [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors

On Wednesday 27 September 2006 4:54 pm, Thomas Gleixner wrote:
> Dave,
> 
> On Wed, 2006-09-27 at 16:21 -0700, David Brownell wrote:
> > As you should know, irq_chip_set_defaults() ensures that every
> > irqchip has startup() and shutdown() methods.  Their default
> > implementations use enable() and disable() ... which in turn
> > have default implementations using mask()/unmask(), for use
> > with non-EIO handlers. 
> 
> True. Still you change the semantics.
> 
> 	chip->mask();
> 	chip->ack();
> 
> is not equal to :
> 
> 	if (!(desc->status & IRQ_DELAYED_DISABLE))
> 		irq_desc[irq].chip->mask(irq);

And I'd said as much.  But you'll notice that code looked like
it had lots of correctness issues in the first place:

 - Of course, there's the fact that it would oops.

 - 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.

 - 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 ...

 - 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.  ;)

 
> 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.

Now if IRQs were dynamically allocated rather than having hard
platform limits of NR_IRQS, I could understand that changing.


> > It was certainly _tested_ on a 2.6.18 ARM board, so you're clearly wrong
> > about at least that part of your feedback as well as the bits about the
> > shartup and shutdown calls being "optional" (in any practical sense, since
> > they are in fact _always_ present).
> 
> 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.

- Dave

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists