Re: [RFC] disable_irq()/enable_irq() semantics and ide-probe.c

[viro@parcelfarce.linux.theplanet.co.uk]

On Thu, Oct 09, 2003 at 09:01:36AM -0700, Linus Torvalds wrote:
> 
> On Thu, 9 Oct 2003 viro@parcelfarce.linux.theplanet.co.uk wrote:
> >
> > IOW, the question is: do we want enable_irq() to undo all effects of
> > disable_irq()?  Whether the current behaviour is intentional or not,
> > it's worth documenting, IMO...
> 
> I think yes, it would be clean to make enable_irq() clean up properly. 
> That includes clearing the IRQ_INPROGRESS bit along with the IRQ_DISABLED 
> but. It won't even make the codepath longer, it just changes a constant.
> 
> And yes, I think we should just remove the clearing of depth in 
> setup_irq(), _and_ make the enabling of the irq be dependent on depth 
> being non-zero.
> 
> In 2.6.x terms, we'd like to go towards something like the appended, but 
> this actually has a _different_ problem: we have separate "->startup()" vs 
> "->enable()" functions for the irq controller, and this means that if the 
> interrupt was disabled when the first irq handler was requested, 
> "->startup()" wouldn't be called at all.
> 
> So my suggestion would be:
>  - do the IRQ_INPROGRESS clearing (safe, since disable_irq() will have 
>    waited for it if it was valid)
>  - leave the depth reset as-is for now, and think about how we'd like to 
>    solve it.
>  - make the synchronize_irq() conditional in irq_disable() (2.6.x now
>    already does that part, so it's not in the patch)
> 
> Hmm?

I've looked through the uses of IRQ_DISABLED in 2.6.  Fun things found:

a) on alpha:
void
i8259a_end_irq(unsigned int irq)
{
        if (!(irq_desc[irq].status & (IRQ_DISABLED|IRQ_INPROGRESS)))
                i8259a_enable_irq(irq);
}
and similar in other ->end() 

on x86:
static void end_8259A_irq (unsigned int irq)
{
        if (!(irq_desc[irq].status & (IRQ_DISABLED|IRQ_INPROGRESS)) &&
                                                        irq_desc[irq].action)
                enable_8259A_irq(irq);
}

b) if we get *two* interrupts while irq is enabled and not registered, we'll
be stuck with IRQ_PENDING in addition to IRQ_INPROGRESS.  Which can, AFAICS,
confuse other code.

c) mind-boggling amount of code duplication - are there any plans to take
that stuff to kernel/*?