Skip to contentSkip to conversation

pirq_enable_irq cleanup

PATCH In review agrover 3 participants 4 messages Aug 4, 2004 – Aug 7, 2004

lore
Share & tools
lore

agrover

author
#0 lore raw

PATCH 1/1 pirq_enable_irq cleanup

Hi, this should apply cleanly against mm2 or rc3.

This is a cleanup of pirq_enable_irq. I couldn't understand this function easily so I cleaned it up.

- Hoisted Via quirk to top -- shouldn't break anything but who knows - can someone with this chipset test?

- Hoisted legacy IDE check too.

- Reduced indenting, added comments.

Regards -- Andy

===== arch/i386/pci/irq.c 1.47 vs edited =====

arch/i386/pci/irq.c +56 -52
@@ -1003,64 +1003,68 @@
 	u8 pin;
 	extern int interrupt_line_quirk;
 	struct pci_dev *temp_dev;
+	char *msg;
+	msg = "";
+
+	/* VIA bridges use interrupt line for apic/pci steering across
+	   the V-Link */
+	if (interrupt_line_quirk)
+		pci_write_config_byte(dev, PCI_INTERRUPT_LINE, dev->irq & 15);
 
 	pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
-	if (pin && !pcibios_lookup_irq(dev, 1) && !dev->irq) {
-		char *msg;
-		msg = "";
-		if (io_apic_assign_pci_irqs) {
-			int irq;
 
-			if (pin) {
-				pin--;		/* interrupt pins are numbered starting from 1 */
-				irq = IO_APIC_get_PCI_irq_vector(dev->bus->number, PCI_SLOT(dev->devfn), pin);
-				/*
-				 * Busses behind bridges are typically not listed in the MP-table.
-				 * In this case we have to look up the IRQ based on the parent bus,
-				 * parent slot, and pin number. The SMP code detects such bridged
-				 * busses itself so we should get into this branch reliably.
-				 */
-				temp_dev = dev;
-				while (irq < 0 && dev->bus->parent) { /* go back to the bridge */
-					struct pci_dev * bridge = dev->bus->self;
-
-					pin = (pin + PCI_SLOT(dev->devfn)) % 4;
-					irq = IO_APIC_get_PCI_irq_vector(bridge->bus->number, 
-							PCI_SLOT(bridge->devfn), pin);
-					if (irq >= 0)
-						printk(KERN_WARNING "PCI: using PPB(B%d,I%d,P%d) to get irq %d\n", 
-							bridge->bus->number, PCI_SLOT(bridge->devfn), pin, irq);
-					dev = bridge;
-				}
-				dev = temp_dev;
-				if (irq >= 0) {
+	/* No irq for devices that don't need them, like legacy IDE. */
+	if (!pin || (dev->class >> 8 == PCI_CLASS_STORAGE_IDE && !(dev->class & 0x5)))
+		return 0;
+
+	/* Try $PIR table first */
+	if (pcibios_lookup_irq(dev, 1) || dev->irq)
+		return 0;
+
+	/* Next, try MPS */
+	if (io_apic_assign_pci_irqs) {
+		int irq;
+
+		pin--;	/* interrupt pins are numbered starting from 1 */
+		irq = IO_APIC_get_PCI_irq_vector(dev->bus->number, PCI_SLOT(dev->devfn), pin);
+		/*
+		 * Busses behind bridges are typically not listed in the MP-table.
+		 * In this case we have to look up the IRQ based on the parent bus,
+		 * parent slot, and pin number. The SMP code detects such bridged
+		 * busses itself so we should get into this branch reliably.
+		 */
+		temp_dev = dev;
+		while (irq < 0 && dev->bus->parent) { /* go back to the bridge */
+			struct pci_dev * bridge = dev->bus->self;
+
+			pin = (pin + PCI_SLOT(dev->devfn)) % 4;
+			irq = IO_APIC_get_PCI_irq_vector(bridge->bus->number, 
+					PCI_SLOT(bridge->devfn), pin);
+			if (irq >= 0)
+				printk(KERN_WARNING "PCI: using PPB(B%d,I%d,P%d) to get irq %d\n", 
+					bridge->bus->number, PCI_SLOT(bridge->devfn), pin, irq);
+			dev = bridge;
+		}
+		dev = temp_dev;
+		if (irq >= 0) {
 #ifdef CONFIG_PCI_MSI
-					if (!platform_legacy_irq(irq))
-						irq = IO_APIC_VECTOR(irq);
+			if (!platform_legacy_irq(irq))
+				irq = IO_APIC_VECTOR(irq);
 #endif
-					printk(KERN_INFO "PCI->APIC IRQ transform: (B%d,I%d,P%d) -> %d\n",
-						dev->bus->number, PCI_SLOT(dev->devfn), pin, irq);
-					dev->irq = irq;
-					return 0;
-				} else
-					msg = " Probably buggy MP table.";
-			}
-		} else if (pci_probe & PCI_BIOS_IRQ_SCAN)
-			msg = "";
-		else
-			msg = " Please try using pci=biosirq.";
-			
-		/* With IDE legacy devices the IRQ lookup failure is not a problem.. */
-		if (dev->class >> 8 == PCI_CLASS_STORAGE_IDE && !(dev->class & 0x5))
+			printk(KERN_INFO "PCI->APIC IRQ transform: (B%d,I%d,P%d) -> %d\n",
+				dev->bus->number, PCI_SLOT(dev->devfn), pin, irq);
+			dev->irq = irq;
 			return 0;
-			
-		printk(KERN_WARNING "PCI: No IRQ known for interrupt pin %c of device %s.%s\n",
-		       'A' + pin - 1, pci_name(dev), msg);
-	}
-	/* VIA bridges use interrupt line for apic/pci steering across
-	   the V-Link */
-	else if (interrupt_line_quirk)
-		pci_write_config_byte(dev, PCI_INTERRUPT_LINE, dev->irq & 15);
+		} else
+			msg = " Probably buggy MP table.";
+	} else if (pci_probe & PCI_BIOS_IRQ_SCAN)
+		msg = "";
+	else
+		msg = " Please try using pci=biosirq.";
+
+	printk(KERN_WARNING "PCI: No IRQ known for interrupt pin %c of device %s.%s\n",
+	       'A' + pin - 1, pci_name(dev), msg);
+
 	return 0;
 }
 
#1 lore raw
Quoted 9 lines · agrover

Hi, this should apply cleanly against mm2 or rc3.

This is a cleanup of pirq_enable_irq. I couldn't understand this function easily so I cleaned it up.

- Hoisted Via quirk to top -- shouldn't break anything but who knows - can someone with this chipset test?

- Hoisted legacy IDE check too.

This looks odd (its hard to read it in diff format in this case). The IDE check is only meant to be done if we look for an IRQ and find none. This tells us the device is only connected for legacy mode.

The VIA one is fairly simple. After the IRQ has been identified or selected the VIA needs the "true" PCI IRQ number in the IRQ_LINE register because internal V-Bus devices are routed via IRQ line not via IRQ pin as the PCI spec says.

#2 lore raw
Quoted 6 lines · Alan Cox
Quoted 5 lines · agrover

This is a cleanup of pirq_enable_irq. I couldn't understand this function easily so I cleaned it up.

- Hoisted Via quirk to top -- shouldn't break anything but who knows - can someone with this chipset test?
- Hoisted legacy IDE check too.

Quoted 8 lines

This looks odd (its hard to read it in diff format in this case). The IDE check is only meant to be done if we look for an IRQ and find none. This tells us the device is only connected for legacy mode.

The VIA one is fairly simple. After the IRQ has been identified or selected the VIA needs the "true" PCI IRQ number in the IRQ_LINE register because internal V-Bus devices are routed via IRQ line not via IRQ pin as the PCI spec says.

Thanks for the explanation.

So perhaps do you think there's any alternative ways we can make this function understandable? I gotta believe there's a way for it to do what it needs to without 5 layers of nested if()s.

Regards -- Andy

#3 lore raw
Quoted 3 lines · Andy Grover

So perhaps do you think there's any alternative ways we can make this function understandable? I gotta believe there's a way for it to do what it needs to without 5 layers of nested if()s.

I'm not actually sure all the if nesting is right looking at the code. I don't see how the VIA stuff became an else if for example or how the VIA quirk works under vector mode.