Re: If ACPI doesn't find an irq listed, don't accept 0 as a valid PCI irq.

Re: If ACPI doesn't find an irq listed, don't accept 0 as a valid PCI irq.

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

On Sat, 2005-07-02 at 12:08 -0700, Linux Kernel Mailing List wrote:
> If ACPI doesn't find an irq listed, don't accept 0 as a valid PCI irq.
> 
> That zero just means that nothing else found any irq information
> either.

> -               if (dev->irq >= 0 && (dev->irq <= 0xF)) {
> +               if (dev->irq > 0 && (dev->irq <= 0xF)) {

Zero _is_ a valid IRQ number. You're undoing the fix which was made as
part of cset 1.1803.119.54 (attached).

-- 
dwmw2

[-- Attachment #2: Attached message - [ACPI] acpi_pci_irq_enable() now returns 0 on success. --]
[-- Type: message/rfc822, Size: 7556 bytes --]

From: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
To: bk-commits-head@vger.kernel.org
Subject: [ACPI] acpi_pci_irq_enable() now returns 0 on success.
Date: Tue, 09 Nov 2004 08:08:42 +0000
Message-ID: <200412020203.iB223VGb008939@hera.kernel.org>

ChangeSet 1.1803.119.54, 2004/11/09 03:08:42-05:00, len.brown@intel.com

	[ACPI] acpi_pci_irq_enable() now returns 0 on success.
	This bubbles all the way up to pci_enable_device().
	This allows IRQ0 to be used as a legal PCI device IRQ.
	
	The ES7000 uses an interrupt source override to assign pin20 to IRQ0.
	Then platform_rename_gsi assigns pin0 a high-numbered IRQ -- available
	for PCI devices.  But IRQ0 needs to be a legal PCI IRQ in the lookup code
	to make it as far as the re-name code. 
	
	Signed-off-by: Natalie Protasevich <Natalie.Protasevich@UNISYS.com>
	Signed-off-by: Len Brown <len.brown@intel.com>



 pci_irq.c  |   41 ++++++++++++++++++++++++++++-------------
 pci_link.c |   15 ++++++++++-----
 2 files changed, 38 insertions(+), 18 deletions(-)


diff -Nru a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
--- a/drivers/acpi/pci_irq.c	2004-12-01 18:03:42 -08:00
+++ b/drivers/acpi/pci_irq.c	2004-12-01 18:03:42 -08:00
@@ -227,6 +227,11 @@
                           PCI Interrupt Routing Support
    -------------------------------------------------------------------------- */
 
+/*
+ * acpi_pci_irq_lookup
+ * success: return IRQ >= 0
+ * failure: return -1
+ */
 static int
 acpi_pci_irq_lookup (
 	struct pci_bus		*bus,
@@ -249,14 +254,14 @@
 	entry = acpi_pci_irq_find_prt_entry(segment, bus_nr, device, pin); 
 	if (!entry) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "PRT entry not found\n"));
-		return_VALUE(0);
+		return_VALUE(-1);
 	}
 	
 	if (entry->link.handle) {
 		irq = acpi_pci_link_get_irq(entry->link.handle, entry->link.index, edge_level, active_high_low);
-		if (!irq) {
+		if (irq < 0) {
 			ACPI_DEBUG_PRINT((ACPI_DB_WARN, "Invalid IRQ link routing entry\n"));
-			return_VALUE(0);
+			return_VALUE(-1);
 		}
 	} else {
 		irq = entry->link.index;
@@ -269,6 +274,11 @@
 	return_VALUE(irq);
 }
 
+/*
+ * acpi_pci_irq_derive
+ * success: return IRQ >= 0
+ * failure: return < 0
+ */
 static int
 acpi_pci_irq_derive (
 	struct pci_dev		*dev,
@@ -277,7 +287,7 @@
 	int			*active_high_low)
 {
 	struct pci_dev		*bridge = dev;
-	int			irq = 0;
+	int			irq = -1;
 	u8			bridge_pin = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_pci_irq_derive");
@@ -289,7 +299,7 @@
 	 * Attempt to derive an IRQ for this device from a parent bridge's
 	 * PCI interrupt routing entry (eg. yenta bridge and add-in card bridge).
 	 */
-	while (!irq && bridge->bus->self) {
+	while (irq < 0 && bridge->bus->self) {
 		pin = (pin + PCI_SLOT(bridge->devfn)) % 4;
 		bridge = bridge->bus->self;
 
@@ -299,7 +309,7 @@
 			if (!bridge_pin) {
 				ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
 					"No interrupt pin configured for device %s\n", pci_name(bridge)));
-				return_VALUE(0);
+				return_VALUE(-1);
 			}
 			/* Pin is from 0 to 3 */
 			bridge_pin --;
@@ -310,9 +320,9 @@
 			pin, edge_level, active_high_low);
 	}
 
-	if (!irq) {
+	if (irq < 0) {
 		ACPI_DEBUG_PRINT((ACPI_DB_WARN, "Unable to derive IRQ for device %s\n", pci_name(dev)));
-		return_VALUE(0);
+		return_VALUE(-1);
 	}
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Derive IRQ %d for device %s from %s\n",
@@ -321,6 +331,11 @@
 	return_VALUE(irq);
 }
 
+/*
+ * acpi_pci_irq_enable
+ * success: return 0
+ * failure: return < 0
+ */
 
 int
 acpi_pci_irq_enable (
@@ -358,20 +373,20 @@
 	 * If no PRT entry was found, we'll try to derive an IRQ from the
 	 * device's parent bridge.
 	 */
-	if (!irq)
+	if (irq < 0)
  		irq = acpi_pci_irq_derive(dev, pin, &edge_level, &active_high_low);
  
 	/*
 	 * No IRQ known to the ACPI subsystem - maybe the BIOS / 
 	 * driver reported one, then use it. Exit in any case.
 	 */
-	if (!irq) {
+	if (irq < 0) {
 		printk(KERN_WARNING PREFIX "PCI interrupt %s[%c]: no GSI",
 			pci_name(dev), ('A' + pin));
 		/* Interrupt Line values above 0xF are forbidden */
-		if (dev->irq && (dev->irq <= 0xF)) {
+		if (dev->irq >= 0 && (dev->irq <= 0xF)) {
 			printk(" - using IRQ %d\n", dev->irq);
-			return_VALUE(dev->irq);
+			return_VALUE(0);
 		}
 		else {
 			printk("\n");
@@ -388,5 +403,5 @@
 		(active_high_low == ACPI_ACTIVE_LOW) ? "low" : "high",
 		dev->irq);
 
-	return_VALUE(dev->irq);
+	return_VALUE(0);
 }
diff -Nru a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
--- a/drivers/acpi/pci_link.c	2004-12-01 18:03:42 -08:00
+++ b/drivers/acpi/pci_link.c	2004-12-01 18:03:42 -08:00
@@ -577,6 +577,11 @@
 	return_VALUE(0);
 }
 
+/*
+ * acpi_pci_link_get_irq
+ * success: return IRQ >= 0
+ * failure: return -1
+ */
 
 int
 acpi_pci_link_get_irq (
@@ -594,27 +599,27 @@
 	result = acpi_bus_get_device(handle, &device);
 	if (result) {
 		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid link device\n"));
-		return_VALUE(0);
+		return_VALUE(-1);
 	}
 
 	link = (struct acpi_pci_link *) acpi_driver_data(device);
 	if (!link) {
 		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid link context\n"));
-		return_VALUE(0);
+		return_VALUE(-1);
 	}
 
 	/* TBD: Support multiple index (IRQ) entries per Link Device */
 	if (index) {
 		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid index %d\n", index));
-		return_VALUE(0);
+		return_VALUE(-1);
 	}
 
 	if (acpi_pci_link_allocate(link))
-		return_VALUE(0);
+		return_VALUE(-1);
 	   
 	if (!link->irq.active) {
 		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Link active IRQ is 0!\n"));
-		return_VALUE(0);
+		return_VALUE(-1);
 	}
 
 	if (edge_level) *edge_level = link->irq.edge_level;
-
To unsubscribe from this list: send the line "unsubscribe bk-commits-head" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: If ACPI doesn't find an irq listed, don't accept 0 as a valid PCI irq.

[Linus Torvalds]

On Sat, 2 Jul 2005, David Woodhouse wrote:
> 
> Zero _is_ a valid IRQ number.

Not for a "legacy irq" it ain't.

If the ACPI layer itself decides to use IRQ0, then that's fine, and that 
path hasn't changed at all. 

However, if ACPI doesn't find any irq, and ends up just using whatever irq 
the device had for some legacy reason, then irq 0 just means "nobody set 
an irq".

Besides, we'll actually be returning 0 _anyway_. The only thing my patch
fixes (and it _is_ a fix) is that we won't be stupidly thinking that we
actually have a valid irq, and that we should turn it level-sensitive.

		Linus

Re: If ACPI doesn't find an irq listed, don't accept 0 as a valid PCI irq.

[Alexander Nyberg]

> tree e6a38b3d6bf434f08054562113bb660c4227769f
> parent 4a89a04f1ee21a7c1f4413f1ad7dcfac50ff9b63
> author Linus Torvalds <torvalds@g5.osdl.org> Sun, 03 Jul 2005 00:35:33 -0700
> committer Linus Torvalds <torvalds@g5.osdl.org> Sun, 03 Jul 2005 00:35:33 -0700
> 
> If ACPI doesn't find an irq listed, don't accept 0 as a valid PCI irq.
> 
> That zero just means that nothing else found any irq information either.
> 
>  drivers/acpi/pci_irq.c |    2 +-
>  1 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
> --- a/drivers/acpi/pci_irq.c
> +++ b/drivers/acpi/pci_irq.c
> @@ -433,7 +433,7 @@ acpi_pci_irq_enable (
>  		printk(KERN_WARNING PREFIX "PCI Interrupt %s[%c]: no GSI",
>  			pci_name(dev), ('A' + pin));
>  		/* Interrupt Line values above 0xF are forbidden */
> -		if (dev->irq >= 0 && (dev->irq <= 0xF)) {
> +		if (dev->irq > 0 && (dev->irq <= 0xF)) {
>  			printk(" - using IRQ %d\n", dev->irq);
>  			acpi_register_gsi(dev->irq, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW);
>  			return_VALUE(0);

Could this go into stable please? I've got it confirmed it fixes:
http://bugme.osdl.org/show_bug.cgi?id=4824

Which was introduced in -stable 2.6.12.2.

Re: [stable] Re: If ACPI doesn't find an irq listed, don't accept 0 as a valid PCI irq.

[Chris Wright]

* Alexander Nyberg (alexn@telia.com) wrote:
> > tree e6a38b3d6bf434f08054562113bb660c4227769f
> > parent 4a89a04f1ee21a7c1f4413f1ad7dcfac50ff9b63
> > author Linus Torvalds <torvalds@g5.osdl.org> Sun, 03 Jul 2005 00:35:33 -0700
> > committer Linus Torvalds <torvalds@g5.osdl.org> Sun, 03 Jul 2005 00:35:33 -0700
> > 
> > If ACPI doesn't find an irq listed, don't accept 0 as a valid PCI irq.
> > 
> > That zero just means that nothing else found any irq information either.
> > 
> >  drivers/acpi/pci_irq.c |    2 +-
> >  1 files changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
> > --- a/drivers/acpi/pci_irq.c
> > +++ b/drivers/acpi/pci_irq.c
> > @@ -433,7 +433,7 @@ acpi_pci_irq_enable (
> >  		printk(KERN_WARNING PREFIX "PCI Interrupt %s[%c]: no GSI",
> >  			pci_name(dev), ('A' + pin));
> >  		/* Interrupt Line values above 0xF are forbidden */
> > -		if (dev->irq >= 0 && (dev->irq <= 0xF)) {
> > +		if (dev->irq > 0 && (dev->irq <= 0xF)) {
> >  			printk(" - using IRQ %d\n", dev->irq);
> >  			acpi_register_gsi(dev->irq, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW);
> >  			return_VALUE(0);
> 
> Could this go into stable please? I've got it confirmed it fixes:
> http://bugme.osdl.org/show_bug.cgi?id=4824
> 
> Which was introduced in -stable 2.6.12.2.

Thanks Alex, I've had bug reports from this one too (which that patch
does fix).  PCI irq routing is so damn touchy and easy to break, I rather
hate mucking with it in -stable.  But, unless Linus sees a reason not
to include this one, it does appear to fix real problems.

thanks,
-chris