Date:	Fri, 18 May 2012 11:22:34 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [WARNING] at kernel/irq/manage.c:421 __enable_irq

On Thu, 2012-05-17 at 00:35 +0200, Thomas Gleixner wrote:
> On Wed, 16 May 2012, Steven Rostedt wrote:
> > On Wed, 2012-05-16 at 13:27 +0200, Thomas Gleixner wrote:
> > > I have an idea how to fix that proper. Will send out patches later.
> > 
> > I'll test them, but it's a bit of a pain, as it would require me
> > shutting down evolution, xchat, firefox, chrome and all my emacs
> > windows :-)  This is why I don't reboot this box often.
> 
> Ok. Here you go. Compile tested only.

I booted it and I didn't see the warning. The system seems to be stable.
I'll keep it up and running over the weekend before I boot back into the
stable kernel. And then I'll give it a Tested-by tag.

-- Steve

> 
> --------------->
> Subject: ide: Request irq before calling irq_probe_port
> From: Thomas Gleixner <tglx@...utronix.de>
> Date: Wed, 16 May 2012 15:53:41 +0200
> 
> Steven reported that the warning in __enable_irq(), which yells about
> an unbalanced enable_irq() call, is triggered in context of
> ide_probe_port().
> 
> What happens is:
> 
> CPU0               CPU1
> disable_irq(X);
> ide_do_probe();    request_irq(X, ....);
> enable_irq(X)
> 
> request_irq() which happens on a non used interrupt undoes nested
> disables and enables the interrupt immediately. So the magic probe
> code in ide_probe_port() will trigger the unbalanced warning when
> enabling the irq at the end of the probe routine. A similar problem
> exists when a concurrent device init uses the irq auto probe
> functions.
> 
> The only solution for this is to request the interrupt _BEFORE_
> calling the probe code. That prevents a parallel request_irq for the
> same irq line to unconditionally undo the disabled state.
> 
> The code contains a few magics which are - as far as I understand the
> current code - simply leftovers of the historic ide implementation.
> 
> The following comment is completely bogus:
>  /*
>   * Use cached IRQ number. It might be (and is...) changed by probe
>   * code above
>   */
> 
> hwif->irq is set up _BEFORE_ the probe code is called. There is no
> function which modifies it in the context of the probe code.
> 
> Sigh, why can't people just clean up the mess when they change the
> code flow?
> 
> Also the check for hwif->irq != 0 there in the probe code is backwards
> as well. Why should we do probing first and then discard the hw
> interface because the function which installs the irq handler disables
> the interface when hwif->irq == 0 ???
> 
> There is nothing wrong with installing the interrupt handler before
> calling the probe code. All it takes is a check in the interrupt
> handler, so it can nicely coexist with another shared interrupt
> handler on the same irq line. 
> 
> Famous last words: This shouldn't break anything as the code needs to
> deal with shared interrupts anyway.
> 
> While at it remove the host.irq_handler member as it is unused and
> just adds extra confusion. TBH, I can't be bothered to split that into
> a separate patch. I wasted enough time already staring into that
> trainwreck.
> 
> Reported-by: Steven Rostedt <rostedt@...dmis.org>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Link: http://lkml.kernel.org/r/1337131823.6724.11.camel@gandalf.stny.rr.com


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

Powered by blists - more mailing lists