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

[Linus Torvalds]

On Thu, 9 Oct 2003 viro@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx 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?

		Linus

----
--- 1.43/arch/i386/kernel/irq.c	Wed Oct  8 20:47:36 2003
+++ edited/arch/i386/kernel/irq.c	Thu Oct  9 08:55:54 2003
@@ -380,7 +380,7 @@
 	spin_lock_irqsave(&desc->lock, flags);
 	switch (desc->depth) {
 	case 1: {
-		unsigned int status = desc->status & ~IRQ_DISABLED;
+		unsigned int status = desc->status & ~(IRQ_DISABLED | IRQ_INPROGRESS);
 		desc->status = status;
 		if ((status & (IRQ_PENDING | IRQ_REPLAY)) == IRQ_PENDING) {
 			desc->status = status | IRQ_REPLAY;
@@ -884,8 +884,7 @@
 
 	*p = new;
 
-	if (!shared) {
-		desc->depth = 0;
+	if (!shared && !desc->depth) {
 		desc->status &= ~(IRQ_DISABLED | IRQ_AUTODETECT | IRQ_WAITING | IRQ_INPROGRESS);
 		desc->handler->startup(irq);
 	}

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