Re: [RFC][PATCH] IRQ: Fix oneshot irq race between irq_finalize_oneshotand handle_level_irq

[Thomas Gleixner]

B1;2005;0cOn Tue, 9 Mar 2010, Lars-Peter Clausen wrote:
>  
> -	desc->status |= IRQ_INPROGRESS;
> +	desc->status |= IRQ_INPROGRESS | IRQ_ONESHOT_INPROGRESS;
>  	raw_spin_unlock(&desc->lock);

That keeps the IRQ_ONESHOT_INPROGRESS dangling for non ONESHOT
interrupts. Not a big deal, but not pretty either.
  
The race between the thread and the irq handler exists indeed on SMP,
but I think there are more fundamental issues about the state which
need to be addressed.

The first thing is that we do not mark the status MASKED when we
actually mask the interrupt in mask_ack_irq(). 

That conditional MASKED after running the primary handler is really
horrible - I already ranted in private at the moron who committed that
crime :)

So the following patch fixes that and the SMP race scenario:

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index d70394f..3e53334 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -359,6 +359,7 @@ static inline void mask_ack_irq(struct irq_desc *desc, int irq)
 		if (desc->chip->ack)
 			desc->chip->ack(irq);
 	}
+	desc->status |= IRQ_MASKED;
 }
 
 /*
@@ -484,10 +485,11 @@ handle_level_irq(unsigned int irq, struct irq_desc *desc)
 	raw_spin_lock(&desc->lock);
 	desc->status &= ~IRQ_INPROGRESS;
 
-	if (unlikely(desc->status & IRQ_ONESHOT))
-		desc->status |= IRQ_MASKED;
-	else if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
+	if (!(desc->status & (IRQ_DISABLED | IRQ_ONESHOT)) &&
+	    desc->chip->unmask) {
 		desc->chip->unmask(irq);
+		desc->status &= ~IRQ_MASKED;
+	}
 out_unlock:
 	raw_spin_unlock(&desc->lock);
 }

But that change opens up another race which is similar to the one you
pointed out:

 CPU0                            CPU1
 hande_level_irq(irq X)
   mask_ack_irq(irq X)
   handle_IRQ_event(irq X)
     wake_up(thread_handler)
                                 thread handler(irq X) runs
     interrupt irqY
       handle_*_irq(irq Y)
       .....
                                 finalize_oneshot(irq X)
                                   unmask(irq X)
     interrupt irq X
       handle_level_irq(irq X)
       mask_ack_irq(irq X)
     return from irq X due to IRQ_INPROGRESS

   return from irq Y

 return from irq X w/o unmask due to IRQ_ONESHOT

That's pretty unlikely, but possible. I know that your patch would
mitigate that problem, but I'm not happy about making it go away with
non obvious state magic. I think I know how to fix it, but I need some
sleep now.

Thanks,

	tglx


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