Herbert Xu wrote:

On Fri, Apr 29, 2005 at 09:35:21AM -0700, David S. Miller wrote:

quoted

On i386 (or any other platform using the generic IRQ layer),
for example, unless you specify SA_INTERRUPT at
request_irq() time, the handler dispatch is:

local_irq_enable();

Yes you're absolutely correct.
 

quoted

However, if you have multiple devices on that IRQ line, you
run into a problem.  Let's say TUlip interrupts first and
we go into the Tulip driver and grab the lock, next the other
device interrupts and we jump into the Tulip interrupt handler
again, we will deadlock but what we should have done is use
IRQ disabling spin locking like Mark's fix does.

However, I don't see how this can happen.  __do_IRQ ensures
that the handlers on a single IRQ aren't reentered by desc->lock
and desc->status.  Softirqs are also kept out by irq_enter.  Am
I missing something?

As far as I can see desc->lock is dropped before handle_IRQ_event() is
called in __do_IRQ() (kernel/irq/handle.c:170) and desc->status does not
prevent the execution of the IRQ handler.  Same with softirqs,
interrupts are enabled when the handler is called (kernel/softirq.c:89).

Thanks
Mark

quoted

Therefore I think his patch is perfectly fine and this is an
excellent area for an audit of the entire tree.  I even just
noticed recently that some of the Sparc drivers/serial/
drivers were not taking the interrupt handler lock correctly like
this (ie. using irqsave).

Unless these drivers are registering two different IRQ lines that
can nest within each other, I still think that a plain spin_lock is
safe and desirable.

Cheers,