[PATCH] Tulip interrupt uses non IRQ safe spinlock

[PATCH] Tulip interrupt uses non IRQ safe spinlock

[Mark Broadbent]

The interrupt handling code in the tulip network driver appears to use a non 
IRQ safe spinlock in an interrupt context.  The following patch should correct 
this.

Signed-off-by: Mark Broadbent <markb@wetlettuce.com>

Index: linux-2.6.11/drivers/net/tulip/interrupt.c
===================================================================
--- linux-2.6.11.orig/drivers/net/tulip/interrupt.c	2005-03-07 18:11:23.000000000 +0000
+++ linux-2.6.11/drivers/net/tulip/interrupt.c	2005-04-28 16:16:23.000000000 +0100
@@ -567,8 +567,9 @@
 
 		if (csr5 & (TxNoBuf | TxDied | TxIntr | TimerInt)) {
 			unsigned int dirty_tx;
+			unsigned long flags;
 
-			spin_lock(&tp->lock);
+			spin_lock_irqsave(&tp->lock, flags);
 
 			for (dirty_tx = tp->dirty_tx; tp->cur_tx - dirty_tx > 0;
 				 dirty_tx++) {
@@ -640,7 +641,7 @@
 						   dev->name, csr5, ioread32(ioaddr + CSR6), tp->csr6);
 				tulip_restart_rxtx(tp);
 			}
-			spin_unlock(&tp->lock);
+			spin_unlock_irqrestore(&tp->lock, flags);
 		}
 
 		/* Log errors. */

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Herbert Xu]

Mark Broadbent <markb@wetlettuce.com> wrote:
> The interrupt handling code in the tulip network driver appears to use a non 
> IRQ safe spinlock in an interrupt context.  The following patch should correct 
> this.

This is bogus.  Interrupts are already disabled when you're in the
interrupt handler.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[David S. Miller]

On Fri, 29 Apr 2005 07:26:41 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Mark Broadbent <markb@wetlettuce.com> wrote:
> > The interrupt handling code in the tulip network driver appears to use a non 
> > IRQ safe spinlock in an interrupt context.  The following patch should correct 
> > this.
> 
> This is bogus.  Interrupts are already disabled when you're in the
> interrupt handler.

Warning, guru area :-)

Look at most interrupt handlers in the kernel, they use
spin_lock_irqsave() rather consistently.  If an interrupt
is registered with SA_SHIRQ, this is a requirement.
Here is why.

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();

	for each irq registered {
		x->handler();
	}
	local_irq_disable();

(see kernel/irq/handle.c)

At the top level from that handle_IRQ_event() function, the
IRQ source is ACK'd after those calls.

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.

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).

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]

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

> Look at most interrupt handlers in the kernel, they use
> spin_lock_irqsave() rather consistently.  If an interrupt
> is registered with SA_SHIRQ, this is a requirement.
> Here is why.
> 
> 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();
> 
> 	for each irq registered {
> 		x->handler();
> 	}
> 	local_irq_disable();
> 
> (see kernel/irq/handle.c)
> 
> At the top level from that handle_IRQ_event() function, the
> IRQ source is ACK'd after those calls.
> 
> 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.

If what you wrote above is really correct, this means that
Documentation/DocBook/kernel-locking.sgml contains wrong information:

>>> The irq handler does not to use spin_lock_irq(), because the
>>> softirq cannot run while the irq handler is running: it can use
>>> spin_lock(), which is slightly faster. The only exception would
>>> be if a different hardware irq handler uses the same lock:
>>> spin_lock_irq() will stop that from interrupting us.

AFAIK, even if interrupts are enabled, the IRQ line which is currently
handled is disabled in the interrupt controller, therefore the
interrupt handler cannot be reentered (for the same device instance).
Did this really change?

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Francois Romieu]

kernel/irq/handle.c::__do_IRQ
[...]
        spin_lock(&desc->lock);
        desc->handler->ack(irq);
        /*
         * REPLAY is when Linux resends an IRQ that was dropped earlier
         * WAITING is used by probe to mark irqs that are being tested
         */
        action = NULL;
        if (likely(!(status & (IRQ_DISABLED | IRQ_INPROGRESS)))) {
                action = desc->action;
                status &= ~IRQ_PENDING; /* we commit to handling */
                status |= IRQ_INPROGRESS; /* we are handling it */
        }
        desc->status = status;

handle_IRQ_event(irq, regs, action) is issued a few lines below
if action != NULL

I thought this (strangely locked) code was supposed to disable reentrancy.

--
Ueimor

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Herbert Xu]

On Fri, Apr 29, 2005 at 09:35:21AM -0700, David S. Miller wrote:
> 
> 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.
 
> 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?

> 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Alan Cox]

On Iau, 2005-04-28 at 21:42, Mark Broadbent wrote:
> The interrupt handling code in the tulip network driver appears to use a non 
> IRQ safe spinlock in an interrupt context.  The following patch should correct 
> this.

In what situations is this code path executed outside the IRQ handler ?

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Herbert Xu]

Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Iau, 2005-04-28 at 21:42, Mark Broadbent wrote:
>> The interrupt handling code in the tulip network driver appears to use a non 
>> IRQ safe spinlock in an interrupt context.  The following patch should correct 
>> this.
> 
> In what situations is this code path executed outside the IRQ handler ?

Only through poll_tulip via netpoll.  However, that is safe as well
since poll_tulip does a disable_irq(dev->irq).

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Mark Broadbent]

Sergey Vlasov wrote:
> On Fri, 29 Apr 2005 09:35:21 -0700 David S. Miller wrote:
> 
> 
>>Look at most interrupt handlers in the kernel, they use
>>spin_lock_irqsave() rather consistently.  If an interrupt
>>is registered with SA_SHIRQ, this is a requirement.
>>Here is why.
>>
>>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();
>>
>>	for each irq registered {
>>		x->handler();
>>	}
>>	local_irq_disable();
>>
>>(see kernel/irq/handle.c)
>>
>>At the top level from that handle_IRQ_event() function, the
>>IRQ source is ACK'd after those calls.
>>
>>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.
> 
> 
> If what you wrote above is really correct, this means that
> Documentation/DocBook/kernel-locking.sgml contains wrong information:

See Documentation/spin-locking.txt line 137, this states that
spin_[un]lock() should not be used in IRQ handlers.

>>>>The irq handler does not to use spin_lock_irq(), because the
>>>>softirq cannot run while the irq handler is running: it can use
>>>>spin_lock(), which is slightly faster. The only exception would
>>>>be if a different hardware irq handler uses the same lock:
>>>>spin_lock_irq() will stop that from interrupting us.
> 
> 
> AFAIK, even if interrupts are enabled, the IRQ line which is currently
> handled is disabled in the interrupt controller, therefore the
> interrupt handler cannot be reentered (for the same device instance).
> Did this really change?

As far as I can tell this is the case (disclaimer applies) [see my other
reply to Herbert Xu].

Thanks
Mark

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Mark Broadbent]

Herbert Xu wrote:
> On Fri, Apr 29, 2005 at 09:35:21AM -0700, David S. Miller wrote:
> 
>>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.
>  
> 
>>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


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

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Paulo Marques]

Mark Broadbent wrote:
> The interrupt handling code in the tulip network driver appears to use a non 
> IRQ safe spinlock in an interrupt context.  The following patch should correct 
> this.

Huh? Can a network interrupt handler be interrupted by the same interrupt?

AFAIK, the spin_lock_irqsave is to disable interruptions so that an
interrupt can not happen in the critical section, so that the interrupt
handler can not make modifications to shared data. Am I wrong?

-- 
Paulo Marques - www.grupopie.com

All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Herbert Xu]

On Mon, May 02, 2005 at 01:56:46PM +0100, Mark Broadbent wrote:
>
> > If what you wrote above is really correct, this means that
> > Documentation/DocBook/kernel-locking.sgml contains wrong information:
> 
> See Documentation/spin-locking.txt line 137, this states that
> spin_[un]lock() should not be used in IRQ handlers.

Line 137 in my spin-locking.txt is a blank line :) Please quote the
exact text.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Herbert Xu]

On Mon, May 02, 2005 at 01:57:28PM +0100, Mark Broadbent wrote:
>
> > 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,

desc->status is set to IRQ_INPROGRESS (kernel/irq/handle.c:144) which
prevents the same IRQ handlers from being invoked again
(kernel/irq/handle.c:141).

> interrupts are enabled when the handler is called (kernel/softirq.c:89).

Soft IRQs do not run when we're in an IRQ handler (kernel/softirq.c:121).

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Herbert Xu]

On Mon, May 02, 2005 at 12:43:58PM -0700, David S. Miller wrote:
> 
> Now, a seperate issue.  If we wish to disable IRQs at all for another
> reason (say to make the critical section not get interrupted by timers
> or some other device's handler) than we must use _irqsave/_irqrestore
> instead of _irq because there is no guarentee whether IRQs are disabled
> upon entry to an IRQ handler or not.  The SA_INTERRUPT behavior is not

Agreed.

However, AFAIK the tulip driver doesn't care about other interrupts.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[David S. Miller]

On Tue, 3 May 2005 07:32:20 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> However, AFAIK the tulip driver doesn't care about other interrupts.

Even the netdev watchdog?  :-)

tulip_tx_timeout takes the tp->lock.  Therefore it seems that Mark's
patch is correct _and_ needed, after all.

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Herbert Xu]

On Mon, May 02, 2005 at 02:32:32PM -0700, David S. Miller wrote:
> On Tue, 3 May 2005 07:32:20 +1000
> Herbert Xu <herbert@gondor.apana.org.au> wrote:
> 
> > However, AFAIK the tulip driver doesn't care about other interrupts.
> 
> Even the netdev watchdog?  :-)

The watchdog runs in softirq context which is disabled while the IRQ
handler is running.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Herbert Xu]

On Tue, May 03, 2005 at 09:25:26AM +0100, Mark Broadbent wrote:
>
> >> See Documentation/spin-locking.txt line 137, this states that
> >> spin_[un]lock() should not be used in IRQ handlers.
> 
> "If you have a case where you have to protect a data structure across
> several CPU's and you want to use spinlocks you can potentially use
> cheaper versions of the spinlocks. IFF you know that the spinlocks are
> never used in interrupt handlers, you can use the non-irq versions:
> 
> 	spin_lock(&lock);
> 	...
> 	spin_unlock(&lock);
> "

Yes this isn't very clear.

What it's trying to say that if you're in a softirq/user context
and the spin lock may be taken in an IRQ context elsewhere then
you must use the IRQ-disabling version.

However, if you're in the IRQ context yourself, and yours is the
only IRQ context that takes that spin lock, then you may use the
straight spin_lock version.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Tulip interrupt uses non IRQ safe spinlock

[Jeff Garzik]

Mark Broadbent wrote:
> The interrupt handling code in the tulip network driver appears to use a non 
> IRQ safe spinlock in an interrupt context.  The following patch should correct 
> this.
> 
> Signed-off-by: Mark Broadbent <markb@wetlettuce.com>
> 
> Index: linux-2.6.11/drivers/net/tulip/interrupt.c
> ===================================================================
> --- linux-2.6.11.orig/drivers/net/tulip/interrupt.c	2005-03-07 18:11:23.000000000 +0000
> +++ linux-2.6.11/drivers/net/tulip/interrupt.c	2005-04-28 16:16:23.000000000 +0100
> @@ -567,8 +567,9 @@
>  
>  		if (csr5 & (TxNoBuf | TxDied | TxIntr | TimerInt)) {
>  			unsigned int dirty_tx;
> +			unsigned long flags;
>  
> -			spin_lock(&tp->lock);
> +			spin_lock_irqsave(&tp->lock, flags);
>  
>  			for (dirty_tx = tp->dirty_tx; tp->cur_tx - dirty_tx > 0;
>  				 dirty_tx++) {
> @@ -640,7 +641,7 @@
>  						   dev->name, csr5, ioread32(ioaddr + CSR6), tp->csr6);
>  				tulip_restart_rxtx(tp);
>  			}
> -			spin_unlock(&tp->lock);
> +			spin_unlock_irqrestore(&tp->lock, flags);

It's already inside the interrupt handler, so this patch is not needed.

	Jeff