Bugfixes: PCI devices get assigned redundant IRQs

Bugfixes: PCI devices get assigned redundant IRQs

[Andreas Block]

Hi,

I'm currently working on a port to a CPCI board with a MPC5200.
When testing the PCI interrupt routing, I discovered the following:
Even devices which don't use interrupts (-> PCI Spec.: Interrupt Pin  
Register is zero),
get an interrupt assigned (this is at least true for most of the  
PPC-targets I looked at).
The cause is pretty obvious in drivers/pci/setup-irq.c.
I guess at least in an ideal world with correctly designed hardware, the  
code should
rather look as in the patch below.
Of course it doesn't hurt anybody to have an unuseable IRQ assigned to a
PCI-to-PCI-bridge (or something alike), but to me it seems a bit strange.
Please correct me, if I'm mislead.

The patch below is tested on the above mentioned CPCI-MPC5200 board and is  
compiler
tested with the latest git-repository kernel on x86.

Best regards,
Andreas

---

Don't assign IRQs to devices, which don't need any

---
commit 2133d61f0bc119d4a06d453b8e79980912a151f0
tree 59ff1b5f7c1ec41780907edd90a35c10a64e3996
parent 99abfeafb5f2eea1bb481330ff37343e1133c924
author Andreas Block <andreas.block@esd-electronics.com> Thu, 25 Jan 2007  
17:12:48 +0100
committer Andreas Block <andreas.block@esd-electronics.com> Thu, 25 Jan  
2007 17:12:48 +0100

  drivers/pci/setup-irq.c |   18 ++++++++++--------
  1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
index a251289..568f187 100644
--- a/drivers/pci/setup-irq.c
+++ b/drivers/pci/setup-irq.c
@@ -24,7 +24,7 @@ pdev_fixup_irq(struct pci_dev *dev,
  	       int (*map_irq)(struct pci_dev *, u8, u8))
  {
  	u8 pin, slot;
-	int irq;
+	int irq = 0;

  	/* If this device is not on the primary bus, we need to figure out
  	   which interrupt pin it will come in on.   We know which slot it
@@ -33,16 +33,18 @@ pdev_fixup_irq(struct pci_dev *dev,
  	   apply the swizzle function.  */

  	pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
-	/* Cope with 0 and illegal. */
-	if (pin == 0 || pin > 4)
+	/* Cope with illegal. */
+	if (pin > 4)
  		pin = 1;

-	/* Follow the chain of bridges, swizzling as we go.  */
-	slot = (*swizzle)(dev, &pin);
+	if (pin != 0) {
+		/* Follow the chain of bridges, swizzling as we go.  */
+		slot = (*swizzle)(dev, &pin);

-	irq = (*map_irq)(dev, slot, pin);
-	if (irq == -1)
-		irq = 0;
+		irq = (*map_irq)(dev, slot, pin);
+		if (irq == -1)
+			irq = 0;
+	}
  	dev->irq = irq;

  	pr_debug("PCI: fixup irq: (%s) got %d\n",
 

Re: Bugfixes: PCI devices get assigned redundant IRQs

[Xavier Bestel]

On Fri, 2007-01-26 at 11:50 +0100, Andreas Block wrote:

>   	u8 pin, slot;
> -	int irq;
> +	int irq = 0;

Aren't there platforms for which irq = 0 is a valid irq ?

	Xav

Re: Bugfixes: PCI devices get assigned redundant IRQs

[Andreas Block]

On Fri, 26 Jan 2007 12:00:55 +0100, Xavier Bestel <xavier.bestel@free.fr>  
wrote:
> On Fri, 2007-01-26 at 11:50 +0100, Andreas Block wrote:
>
>>   	u8 pin, slot;
>> -	int irq;
>> +	int irq = 0;
>
> Aren't there platforms for which irq = 0 is a valid irq ?

As far as I understand the PCI spec, the answer to your question seems to  
be: no
(or I'm missing something).

Don't get me wrong. I'm not talking about system IRQs, but about the value  
of the Interrupt Pin Register in PCI configuration space.

The PCI Local Bus Specification in Revision 3.0 from 3rd February 2004  
says on page 223 about the content of Interrupt Pin register:

Value 0x00: _No_ interrupt
Values 0x01 to 0x04: Interrupt lines A to D
And values 5 to 0xFF are reserved.

So I'd say, the "correction" of greater values than four to a value of one  
seems discussable, too. Because it will break any future changes of the  
PCI spec.

Andreas

-- 
-------------------------------------------------------------------------

                             _/_/_/_/   Andreas Block
                            _/_/_/_/   Dipl.-Ing.
                           _/_/_/_/   andreas.block@esd-electronics.com

       _/_/_/   _/_/_/_/_/_/_/      esd electronic system design gmbh
     _/   _/  _/             _/    Vahrenwalder Str. 207
    _/   _/    _/_/_/   _/   _/   D-30165 Hannover
    _/             _/  _/   _/   Phone: +49-511-37298-0
     _/_/_/_/_/_/_/   _/_/_/    Fax:   +49-511-37298-68
                               http://www.esd-electronics.com

-------------------------------------------------------------------------
 

Re: Bugfixes: PCI devices get assigned redundant IRQs

[H. Peter Anvin]

Andreas Block wrote:
>>
>> Aren't there platforms for which irq = 0 is a valid irq ?
> 
> As far as I understand the PCI spec, the answer to your question seems 
> to be: no
> (or I'm missing something).
> 
> Don't get me wrong. I'm not talking about system IRQs, but about the 
> value of the Interrupt Pin Register in PCI configuration space.
> 
> The PCI Local Bus Specification in Revision 3.0 from 3rd February 2004 
> says on page 223 about the content of Interrupt Pin register:
> 
> Value 0x00: _No_ interrupt
> Values 0x01 to 0x04: Interrupt lines A to D
> And values 5 to 0xFF are reserved.
> 
> So I'd say, the "correction" of greater values than four to a value of 
> one seems discussable, too. Because it will break any future changes of 
> the PCI spec.
> 

I think you're confusing the Interrupt Line register and the Interrupt 
Pin register.  The Interrupt Line register is platform-dependent, but on 
x86 platforms it generally contains the IRQ number (and IRQ 0 is valid, 
although in practice it is never used since IRQ 0 is the system timer 
and is never connected to the PCI bus), or 255 meaning "none" -- see the 
footnote on page 223 of the PCI 3.0 spec.

	-hpa

Re: Bugfixes: PCI devices get assigned redundant IRQs

[Andreas Block]

Am Sun, 28 Jan 2007 09:04:41 +0100 schrieb H. Peter Anvin <hpa@zytor.com>:

> I think you're confusing the Interrupt Line register and the Interrupt  
> Pin register.  The Interrupt Line register is platform-dependent, but on  
> x86 platforms it generally contains the IRQ number (and IRQ 0 is valid,  
> although in practice it is never used since IRQ 0 is the system timer  
> and is never connected to the PCI bus), or 255 meaning "none" -- see the  
> footnote on page 223 of the PCI 3.0 spec.

No, I don't think so. I meant the PCI Interrupt Pin register and not the  
Interrupt line register. I do know, that the latter contains a platform  
dependent interrupt assignment. In the former a device states which  
interrupt "trace" the device is connected to (Int A-D).
Perhaps you take at look at the code, I think, it's dealing with the  
register I described (Interrupt pin).

Linus stated his opinion about the patch and thinks, it should be well  
tested in -mm kernel, because it might break devices, which do not comply  
to the PCI spec.
I second that. If there're indeed devices out there, which violate the  
spec in this point, the patch would hurt more, than it's doing any good.
As I wrote in my first message, the consequences of this rather small bug  
(if it is one and not a workaround for bad devices) are harmless.

Regards,
Andreas