Messages in this thread | | | Date | Thu, 9 Oct 2003 03:43:34 +0100 | From | viro@parcelfa ... | Subject | Re: [RFC] disable_irq()/enable_irq() semantics and ide-probe.c |
| |
On Wed, Oct 08, 2003 at 07:29:10PM -0700, Linus Torvalds wrote: > > If an interrupt comes during that > > time, we'll get IRQ_INPROGRESS set and not reset until later register_irq() > > (see handle_irq() for details). Note that calling disable_irq() after that > > will kill us on SMP - it will spin waiting for IRQ_INPROGRESS to go away. > > Now _this_ is a bug waiting to happen. I don't think it actually happens > now (since anybody who does disable_irq() _will_ either have registered > the irq already or will do so soon, but I agree that it's just trouble > waiting to happen.
Ummm... probe_hwif() is a good example of the opposite - it can fail past the point where it disables irq and that means no register_irq() after enable_irq() call on cleanup.
> I think the fix to that is to just add a trivial test for "if the handler > list is empty, don't bother synchronizing" in disable_irq(), since clearly > if the list is empty there is nothing to synchronize _with_. After all, > the synchronization is there just to make sure no handler runs > concurrently on another CPU.
How about action = NULL; if (!(status & (IRQ_DISABLED | IRQ_INPROGRESS))) { action = desc->action; status &= ~IRQ_PENDING; /* we commit to handling */ if (likely(action)) status |= IRQ_INPROGRESS; /* we are handling it */ } desc->status = status;
in handle_irq()?
> As far as I can tell, 2.6.x is doing all the right things. Modulo the (not > really supported) concurrent device probing, and the (not implemented) > atomic irq requesting. > > Note that the IRQ_INPROGRESS thing was literally the bit that autodetect > used to test, it got changed it to IRQ_WAITING to clarify the code and > avoid bad interactions with the other uses of IRQ_INPROGRESS. > > And note that we do _not_ clear IRQ_INPROGRESS on "action == NULL" very > much on purpose: that "action == NULL" thing also happens if the IRQ is > disabled, and we need to get the edge replay right. This is why > request_irq() literally _needs_ to clear that bit in 2.6.x.
See above - we shouldn't clear it on action == NULL, but we don't need to set it, AFAICS. > So the fix is to make 2.4.x do what 2.6.x does, methinks.
ObOtherFun: There's another bogosity in quoted ide-probe.c code, according to dwmw2 - he says that there are PCI IDE cards that get IRQ 0, so the test for hwif->irq is b0rken. We probably should stop overloading ->irq == 0 for "none given", but I'm not sure that we *have* a value that would never be used as an IRQ number on all platforms... - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |