* [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors [not found] <200609220641.58938.david-b@pacbell.net> @ 2006-09-22 13:43 ` David Brownell 2006-09-27 21:38 ` Thomas Gleixner 0 siblings, 1 reply; 7+ messages in thread From: David Brownell @ 2006-09-22 13:43 UTC (permalink / raw) To: Linux Kernel list; +Cc: tglx, mingo 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); } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors 2006-09-22 13:43 ` [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors David Brownell @ 2006-09-27 21:38 ` Thomas Gleixner 2006-09-27 23:21 ` David Brownell 0 siblings, 1 reply; 7+ messages in thread From: Thomas Gleixner @ 2006-09-27 21:38 UTC (permalink / raw) To: David Brownell; +Cc: Linux Kernel list, mingo 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); > } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors 2006-09-27 21:38 ` Thomas Gleixner @ 2006-09-27 23:21 ` David Brownell 2006-09-27 23:54 ` Thomas Gleixner 0 siblings, 1 reply; 7+ messages in thread From: David Brownell @ 2006-09-27 23:21 UTC (permalink / raw) To: tglx; +Cc: Linux Kernel list, mingo 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); > > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors 2006-09-27 23:21 ` David Brownell @ 2006-09-27 23:54 ` Thomas Gleixner 2006-09-28 0:39 ` [Bulk] " David Brownell 0 siblings, 1 reply; 7+ messages in thread From: Thomas Gleixner @ 2006-09-27 23:54 UTC (permalink / raw) To: David Brownell; +Cc: Linux Kernel list, mingo 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bulk] Re: [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors 2006-09-27 23:54 ` Thomas Gleixner @ 2006-09-28 0:39 ` David Brownell 2006-09-28 1:18 ` Thomas Gleixner 0 siblings, 1 reply; 7+ messages in thread From: David Brownell @ 2006-09-28 0:39 UTC (permalink / raw) To: tglx; +Cc: Linux Kernel list, mingo 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bulk] Re: [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors 2006-09-28 0:39 ` [Bulk] " David Brownell @ 2006-09-28 1:18 ` Thomas Gleixner 2006-09-28 1:40 ` [Bulk] " David Brownell 0 siblings, 1 reply; 7+ messages in thread From: Thomas Gleixner @ 2006-09-28 1:18 UTC (permalink / raw) To: David Brownell; +Cc: Linux Kernel list, mingo 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bulk] Re: [Bulk] Re: [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors 2006-09-28 1:18 ` Thomas Gleixner @ 2006-09-28 1:40 ` David Brownell 0 siblings, 0 replies; 7+ messages in thread From: David Brownell @ 2006-09-28 1:40 UTC (permalink / raw) To: tglx; +Cc: Linux Kernel list, mingo 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2006-09-28 1:40 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200609220641.58938.david-b@pacbell.net>
2006-09-22 13:43 ` [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors David Brownell
2006-09-27 21:38 ` Thomas Gleixner
2006-09-27 23:21 ` David Brownell
2006-09-27 23:54 ` Thomas Gleixner
2006-09-28 0:39 ` [Bulk] " David Brownell
2006-09-28 1:18 ` Thomas Gleixner
2006-09-28 1:40 ` [Bulk] " David Brownell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.