IRQ enable/disable BUG in IDE w/shared IRQs

IRQ enable/disable BUG in IDE w/shared IRQs

[David Miller]

As far as I know this issue has been around forever.

The bug report is at:

https://bugzilla.kernel.org/show_bug.cgi?id=16481

The important part of the backtrace is:

[    4.003171] WARNING: at kernel/irq/manage.c:290 __enable_irq+0x2f/0x55()
 ...
[    4.003176] Unbalanced enable for IRQ 19
 ...
[    4.003255]  [<c107314d>] ? __enable_irq+0x2f/0x55
[    4.003259]  [<c1073415>] ? enable_irq+0x31/0x4c
[    4.003270]  [<fb915145>] ? ide_probe_port+0x4b7/0x4de [ide_core]
[    4.003280]  [<fb915641>] ? ide_host_register+0x204/0x4e7 [ide_core]

IRQ 19 on this system is shared between the it8213 IDE controller
and one of the USB host controllers.

The condition doesn't trigger every time, only some boots, which sort
of implies a race.

The function ide_probe_port() will unconditionally do a disable_irq()/
enable_irq() around the drive probing sequence.  It does this even if
the IRQ has not been registered yet by IDE.

Normally, this is fine, since if the IRQ is not registered then
irq_desc->depth is 1 and the disable/enable sequence will behave as
essentially a NOP.  If the IRQ is registered by IDE itself, then this
sequence would really disable and then re-enable the IRQ.  That's fine
too.

But this all falls apart if the IDE interrupt line is shared and one
of those other entities does the request_irq() in the middle the
enable/disable sequence.

As far as I can tell the bad sequence is:

	ide_probe_irq			USB host controller driver

			->depth starts at "1"

	disable_irq()
	    ->depth now "2"

					request_irq()
					  ->depth reset to "0"
	enable_irq()
	    ->depth is "0", warning triggers

It would seem that there is an implicit disallowing of playing
with the enable/disable state of an IRQ until request_irq() has
been by someone first, otherwise the above sequence can trigger.

The ATA layer doesn't operate this way, it always requests the IRQ
before doing anything else wrt. interrupts on it.

I suppose I can change IDE to behave this way too, but the less
changes I make to IDE the better and there are also some subtle issues
wrt. dynamic IDE probing I need to look into to get this right.

I have a hard time believing we've gotten away with this for so long.
Maybe it really is that rare to share the IDE interrupts with other
stuff?

Any comments?

Re: IRQ enable/disable BUG in IDE w/shared IRQs

[Thomas Gleixner]

On Thu, 13 Jan 2011, David Miller wrote:
> As far as I know this issue has been around forever.
> 
> The bug report is at:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=16481
> 
> The important part of the backtrace is:
> 
> [    4.003171] WARNING: at kernel/irq/manage.c:290 __enable_irq+0x2f/0x55()
>  ...
> [    4.003176] Unbalanced enable for IRQ 19
>  ...
> [    4.003255]  [<c107314d>] ? __enable_irq+0x2f/0x55
> [    4.003259]  [<c1073415>] ? enable_irq+0x31/0x4c
> [    4.003270]  [<fb915145>] ? ide_probe_port+0x4b7/0x4de [ide_core]
> [    4.003280]  [<fb915641>] ? ide_host_register+0x204/0x4e7 [ide_core]
> 
> IRQ 19 on this system is shared between the it8213 IDE controller
> and one of the USB host controllers.
> 
> The condition doesn't trigger every time, only some boots, which sort
> of implies a race.
> 
> The function ide_probe_port() will unconditionally do a disable_irq()/
> enable_irq() around the drive probing sequence.  It does this even if
> the IRQ has not been registered yet by IDE.
> 
> Normally, this is fine, since if the IRQ is not registered then
> irq_desc->depth is 1 and the disable/enable sequence will behave as
> essentially a NOP.  If the IRQ is registered by IDE itself, then this
> sequence would really disable and then re-enable the IRQ.  That's fine
> too.
> 
> But this all falls apart if the IDE interrupt line is shared and one
> of those other entities does the request_irq() in the middle the
> enable/disable sequence.
> 
> As far as I can tell the bad sequence is:
> 
> 	ide_probe_irq			USB host controller driver
> 
> 			->depth starts at "1"
> 
> 	disable_irq()
> 	    ->depth now "2"
> 
> 					request_irq()
> 					  ->depth reset to "0"
> 	enable_irq()
> 	    ->depth is "0", warning triggers
> 
> It would seem that there is an implicit disallowing of playing
> with the enable/disable state of an IRQ until request_irq() has
> been by someone first, otherwise the above sequence can trigger.

Probably nobody every thought about this :)
 
> The ATA layer doesn't operate this way, it always requests the IRQ
> before doing anything else wrt. interrupts on it.
> 
> I suppose I can change IDE to behave this way too, but the less
> changes I make to IDE the better and there are also some subtle issues
> wrt. dynamic IDE probing I need to look into to get this right.

We could check for depth > 1 and just decrement depth by one in
request_irq, but I'm quite reluctant to do so w/o extensive testing.

OTOH, nested disables should not be the common case either, but who
knows what legacy stuff will break.

> I have a hard time believing we've gotten away with this for so long.
> Maybe it really is that rare to share the IDE interrupts with other
> stuff?

IIRC, the legacy IDE interrupts were 14/15 and those were never shared.

Thanks,

	tglx

Re: IRQ enable/disable BUG in IDE w/shared IRQs

[Jeff Garzik]

On 01/14/2011 05:30 AM, Thomas Gleixner wrote:
> On Thu, 13 Jan 2011, David Miller wrote:
>> I have a hard time believing we've gotten away with this for so long.
>> Maybe it really is that rare to share the IDE interrupts with other
>> stuff?
>
> IIRC, the legacy IDE interrupts were 14/15 and those were never shared.

Yes, non-shared 14/15 were the 95% common case for the longest time. 
Sharing IDE interrupts within old-IDE worked FSVO "working", but the 
interrupt probes were written for a non-shared interrupt, then hacking 
into working for shared interrupts when PCI first starting showing up on 
the scene.

	Jeff