untested on 8390 hardware, but ought to solve the lockdep false
positive.
-----------------
Subject: locking validator: special rule: 8390.c disable_irq()
From: Ingo Molnar <[email protected]>
8390.c knows that ei_local->page_lock can only be used by an irq
context that it disabled - and can hence take the ->page_lock
without disabling hardirqs. Teach lockdep about this.
Signed-off-by: Ingo Molnar <[email protected]>
---
drivers/net/8390.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Index: linux/drivers/net/8390.c
===================================================================
--- linux.orig/drivers/net/8390.c
+++ linux/drivers/net/8390.c
@@ -249,7 +249,7 @@ void ei_tx_timeout(struct net_device *de
/* Ugly but a reset can be slow, yet must be protected */
- disable_irq_nosync(dev->irq);
+ disable_irq_nosync_lockdep(dev->irq);
spin_lock(&ei_local->page_lock);
/* Try to restart the card. Perhaps the user has fixed something. */
@@ -257,7 +257,7 @@ void ei_tx_timeout(struct net_device *de
NS8390_init(dev, 1);
spin_unlock(&ei_local->page_lock);
- enable_irq(dev->irq);
+ enable_irq_lockdep(dev->irq);
netif_wake_queue(dev);
}
On Wed, 2006-05-31 at 22:02 +0200, Ingo Molnar wrote:
> untested on 8390 hardware, but ought to solve the lockdep false
> positive.
>
> -----------------
> Subject: locking validator: special rule: 8390.c disable_irq()
> From: Ingo Molnar <[email protected]>
>
> 8390.c knows that ei_local->page_lock can only be used by an irq
> context that it disabled -
btw I think this is no longer correct with the irq polling stuff Alan
added to the kernel recently...
* Arjan van de Ven <[email protected]> wrote:
> On Wed, 2006-05-31 at 22:02 +0200, Ingo Molnar wrote:
> > untested on 8390 hardware, but ought to solve the lockdep false
> > positive.
> >
> > -----------------
> > Subject: locking validator: special rule: 8390.c disable_irq()
> > From: Ingo Molnar <[email protected]>
> >
> > 8390.c knows that ei_local->page_lock can only be used by an irq
> > context that it disabled -
>
> btw I think this is no longer correct with the irq polling stuff Alan
> added to the kernel recently...
hm, indeed. misrouted_irq() goes through all irq descriptors and ignores
IRQ_DISABLED flag - rendering disable_irq() useless in essence, and
introducing the kind of deadlocks that lockdep warned about.
Andrew, as far as i can see with irqfixup this isnt a lockdep false
positive but a real deadlock scenario - a spurious IRQ might arrive
during vortex_timer() execution and might cause the execution of
misrouted_irq(), which could execute vortex_interrupt() => deadlock.
Alan, is this a necessary property of irqpoll/irqfixup? Shouldnt
irqfixup leave irq descriptors alone that are disabled?
Ingo
On Wed, May 31, 2006 at 10:31:40PM +0200, Arjan van de Ven wrote:
> > 8390.c knows that ei_local->page_lock can only be used by an irq
> > context that it disabled -
>
> btw I think this is no longer correct with the irq polling stuff Alan
> added to the kernel recently...
We could make the misrouted IRQ logic skip all handlers on a disabled IRQ
but that might actually be worse than the disease we are trying to cure by
doing so.
Alan
On Wed, 2006-05-31 at 17:41 -0400, Alan Cox wrote:
> On Wed, May 31, 2006 at 10:31:40PM +0200, Arjan van de Ven wrote:
> > > 8390.c knows that ei_local->page_lock can only be used by an irq
> > > context that it disabled -
> >
> > btw I think this is no longer correct with the irq polling stuff Alan
> > added to the kernel recently...
>
> We could make the misrouted IRQ logic skip all handlers on a disabled IRQ
> but that might actually be worse than the disease we are trying to cure by
> doing so.
yeah since misrouted irqs will cause the kernel do disable irqs 'at
random' more or less .. for which the handlers now would become
unreachable which isn't good.
* Arjan van de Ven <[email protected]> wrote:
> On Wed, 2006-05-31 at 17:41 -0400, Alan Cox wrote:
> > On Wed, May 31, 2006 at 10:31:40PM +0200, Arjan van de Ven wrote:
> > > > 8390.c knows that ei_local->page_lock can only be used by an irq
> > > > context that it disabled -
> > >
> > > btw I think this is no longer correct with the irq polling stuff Alan
> > > added to the kernel recently...
> >
> > We could make the misrouted IRQ logic skip all handlers on a disabled IRQ
> > but that might actually be worse than the disease we are trying to cure by
> > doing so.
>
> yeah since misrouted irqs will cause the kernel do disable irqs 'at
> random' more or less .. for which the handlers now would become
> unreachable which isn't good.
couldnt most of these problems be avoided by tracking whether a handler
_ever_ returned a success status? That means that irqpoll could safely
poll handlers for which we know that they somehow arent yet matched up
to any IRQ line?
Ingo
On Wed, 2006-05-31 at 23:47 +0200, Ingo Molnar wrote:
> * Arjan van de Ven <[email protected]> wrote:
>
> > On Wed, 2006-05-31 at 17:41 -0400, Alan Cox wrote:
> > > On Wed, May 31, 2006 at 10:31:40PM +0200, Arjan van de Ven wrote:
> > > > > 8390.c knows that ei_local->page_lock can only be used by an irq
> > > > > context that it disabled -
> > > >
> > > > btw I think this is no longer correct with the irq polling stuff Alan
> > > > added to the kernel recently...
> > >
> > > We could make the misrouted IRQ logic skip all handlers on a disabled IRQ
> > > but that might actually be worse than the disease we are trying to cure by
> > > doing so.
> >
> > yeah since misrouted irqs will cause the kernel do disable irqs 'at
> > random' more or less .. for which the handlers now would become
> > unreachable which isn't good.
>
> couldnt most of these problems be avoided by tracking whether a handler
> _ever_ returned a success status? That means that irqpoll could safely
> poll handlers for which we know that they somehow arent yet matched up
> to any IRQ line?
I suspect the real solution is to have a
disable_irq_handler(irq, handler)
function which does 2 things
1) disable the irq at the hardware level
2) mark the handler as "don't call me"
it matches the semantics here; what these drivers want is 1) not get an
irq handler called and 2) not get an irq flood
* Arjan van de Ven <[email protected]> wrote:
> > couldnt most of these problems be avoided by tracking whether a handler
> > _ever_ returned a success status? That means that irqpoll could safely
> > poll handlers for which we know that they somehow arent yet matched up
> > to any IRQ line?
>
> I suspect the real solution is to have a
>
> disable_irq_handler(irq, handler)
>
> function which does 2 things
> 1) disable the irq at the hardware level
> 2) mark the handler as "don't call me"
>
> it matches the semantics here; what these drivers want is 1) not get
> an irq handler called and 2) not get an irq flood
ok, this would work. But there is a practical problem: only in drivers/*
there's 310 disable_irq() calls - each would have to be changed to
disable_irq_handler() [and i dont see any good way to automate that
conversion] ...
Ingo
On Thu, 2006-06-01 at 00:00 +0200, Ingo Molnar wrote:
> * Arjan van de Ven <[email protected]> wrote:
>
> > > couldnt most of these problems be avoided by tracking whether a handler
> > > _ever_ returned a success status? That means that irqpoll could safely
> > > poll handlers for which we know that they somehow arent yet matched up
> > > to any IRQ line?
> >
> > I suspect the real solution is to have a
> >
> > disable_irq_handler(irq, handler)
> >
> > function which does 2 things
> > 1) disable the irq at the hardware level
> > 2) mark the handler as "don't call me"
> >
> > it matches the semantics here; what these drivers want is 1) not get
> > an irq handler called and 2) not get an irq flood
>
> ok, this would work. But there is a practical problem: only in drivers/*
> there's 310 disable_irq() calls - each would have to be changed to
> disable_irq_handler() [and i dont see any good way to automate that
> conversion] ...
want to take a bet on the number of those 310 that are just totally
bogus ?