[patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors

[patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors

[David Brownell]

The irq handler code can oops when used with an irq_chip with just
enable/disable/eoi methods, appropriate for handle_fasteoi_irq(),
either by (a) uninstalling, or (b) using it with a chained handler.

The problem was that the original code expected there would always
be mask/unmask/ack methods, and the fix is to instead use methods
which are always present and which more closely correspond to the
flag manipulation being done.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

--- d26.rc4test.orig/kernel/irq/chip.c	2006-09-21 16:41:11.000000000 -0700
+++ d26.rc4test/kernel/irq/chip.c	2006-09-21 18:04:07.000000000 -0700
@@ -482,10 +482,8 @@ __set_irq_handler(unsigned int irq,
 
 	/* Uninstall? */
 	if (handle == handle_bad_irq) {
-		if (desc->chip != &no_irq_chip) {
-			desc->chip->mask(irq);
-			desc->chip->ack(irq);
-		}
+		if (desc->chip != &no_irq_chip)
+			desc->chip->shutdown(irq);
 		desc->status |= IRQ_DISABLED;
 		desc->depth = 1;
 	}
@@ -495,7 +493,7 @@ __set_irq_handler(unsigned int irq,
 		desc->status &= ~IRQ_DISABLED;
 		desc->status |= IRQ_NOREQUEST | IRQ_NOPROBE;
 		desc->depth = 0;
-		desc->chip->unmask(irq);
+		desc->chip->startup(irq);
 	}
 	spin_unlock_irqrestore(&desc->lock, flags);
 }

Re: [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors

[Thomas Gleixner]

On Fri, 2006-09-22 at 06:43 -0700, David Brownell wrote:
> The irq handler code can oops when used with an irq_chip with just
> enable/disable/eoi methods, appropriate for handle_fasteoi_irq(),
> either by (a) uninstalling, or (b) using it with a chained handler.
> 
> The problem was that the original code expected there would always
> be mask/unmask/ack methods, and the fix is to instead use methods
> which are always present and which more closely correspond to the
> flag manipulation being done.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

NACK. 

# grep startup kernel/irq/manage.c
307:                    if (desc->chip->startup)
308:                            desc->chip->startup(irq);

# grep shutdown kernel/irq/manage.c
386:                            if (desc->chip->shutdown)
387:                                    desc->chip->shutdown(irq);

startup() and shutdown() are optional and mostly there to keep the not
converted do_IRQ() users working.

This patch will simply break _all_ ARM and PowerPC in one go and as well
the i386 and x86_64 conversion which is in -mm.

	tglx

> --- d26.rc4test.orig/kernel/irq/chip.c	2006-09-21 16:41:11.000000000 -0700
> +++ d26.rc4test/kernel/irq/chip.c	2006-09-21 18:04:07.000000000 -0700
> @@ -482,10 +482,8 @@ __set_irq_handler(unsigned int irq,
>  
>  	/* Uninstall? */
>  	if (handle == handle_bad_irq) {
> -		if (desc->chip != &no_irq_chip) {
> -			desc->chip->mask(irq);
> -			desc->chip->ack(irq);
> -		}
> +		if (desc->chip != &no_irq_chip)
> +			desc->chip->shutdown(irq);
>  		desc->status |= IRQ_DISABLED;
>  		desc->depth = 1;
>  	}
> @@ -495,7 +493,7 @@ __set_irq_handler(unsigned int irq,
>  		desc->status &= ~IRQ_DISABLED;
>  		desc->status |= IRQ_NOREQUEST | IRQ_NOPROBE;
>  		desc->depth = 0;
> -		desc->chip->unmask(irq);
> +		desc->chip->startup(irq);
>  	}
>  	spin_unlock_irqrestore(&desc->lock, flags);
>  }

Re: [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors

[David Brownell]

On Wednesday 27 September 2006 2:38 pm, Thomas Gleixner wrote:
> On Fri, 2006-09-22 at 06:43 -0700, David Brownell wrote:
> > The irq handler code can oops when used with an irq_chip with just
> > enable/disable/eoi methods, appropriate for handle_fasteoi_irq(),
> > either by (a) uninstalling, or (b) using it with a chained handler.
> > 
> > The problem was that the original code expected there would always
> > be mask/unmask/ack methods, and the fix is to instead use methods
> > which are always present and which more closely correspond to the
> > flag manipulation being done.
> > 
> > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> 
> NACK. 

Explain yourself then ...


> # grep startup kernel/irq/manage.c
> 307:                    if (desc->chip->startup)
> 308:                            desc->chip->startup(irq);
> 
> # grep shutdown kernel/irq/manage.c
> 386:                            if (desc->chip->shutdown)
> 387:                                    desc->chip->shutdown(irq);
> 
> startup() and shutdown() are optional and mostly there to keep the not
> converted do_IRQ() users working.

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. 

So what's the correct fix then ... use enable() and disable()?
Oopsing isn't OK...


> This patch will simply break _all_ ARM and PowerPC in one go and as well
> the i386 and x86_64 conversion which is in -mm.

I can understand breaking patches in MM that I've not seen, but not about
breaking anything else.

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

- Dave


> 	tglx
> 
> > --- d26.rc4test.orig/kernel/irq/chip.c	2006-09-21 16:41:11.000000000 -0700
> > +++ d26.rc4test/kernel/irq/chip.c	2006-09-21 18:04:07.000000000 -0700
> > @@ -482,10 +482,8 @@ __set_irq_handler(unsigned int irq,
> >  
> >  	/* Uninstall? */
> >  	if (handle == handle_bad_irq) {
> > -		if (desc->chip != &no_irq_chip) {
> > -			desc->chip->mask(irq);
> > -			desc->chip->ack(irq);
> > -		}
> > +		if (desc->chip != &no_irq_chip)
> > +			desc->chip->shutdown(irq);
> >  		desc->status |= IRQ_DISABLED;
> >  		desc->depth = 1;
> >  	}
> > @@ -495,7 +493,7 @@ __set_irq_handler(unsigned int irq,
> >  		desc->status &= ~IRQ_DISABLED;
> >  		desc->status |= IRQ_NOREQUEST | IRQ_NOPROBE;
> >  		desc->depth = 0;
> > -		desc->chip->unmask(irq);
> > +		desc->chip->startup(irq);
> >  	}
> >  	spin_unlock_irqrestore(&desc->lock, flags);
> >  }
> 

Re: [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors

[Thomas Gleixner]

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


> So what's the correct fix then ... use enable() and disable()?
> Oopsing isn't OK... 

True, but we can not unconditionally change the semantics. 

Does it break existing or new code ?

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

	tglx

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

[David Brownell]

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

Re: [Bulk] Re: [patch 2.6.18] genirq: remove oops with fasteoi irq_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

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

[David Brownell]

Hi Thomas,

On Wednesday 27 September 2006 6:18 pm, Thomas Gleixner wrote:
> 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() ?

It'd still oops on chips with just enable(), disable(), and eoi().

Which, on my brief scan of the codebase, appears to be one of the
accepted ways to craft a fasteoi irq_chip.


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

That may be an argument that the default shutdown() should not be the
same as the default disable().  Unless shutdown() is going away??

I still dislike that delayed disable() mechanism.  Every time I've
seen ("tripped over") it in action it's been the cause of bugs.

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

I thought maybe submitting a reasonably sane patch would be the
best way to start that discussion.  :)

The only issue appears to be how that rarely-used "get rid of
the handler" code path should work.

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

Aha!  So if it's "nothing" then that rarely/not-used path can change
without negative impact...


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

Well, an implementation of shutdown() _is_ always provided.  At least
now; I don't have time to track your MM patches.


> I'm not sure about that right now as I'm too tired.

I expect that after you sleep on this, something will come to mind.  ;)

- Dave