RE: 2.6.9-rc2-mm1

RE: 2.6.9-rc2-mm1

[Protasevich, Natalie]

> On Thursday 16 September 2004 11:14 am, Jesse Barnes wrote:
> > On Thursday, September 16, 2004 2:40 am, Andrew Morton wrote:
> >  bk-acpi.patch
> >
> > Looks like some changes in this patch break sn2.  In particular,
this 
> > hunk in
> > acpi_pci_irq_enable():
> >
> > -               if (dev->irq && (dev->irq <= 0xF)) {
> > +               if (dev->irq >= 0 && (dev->irq <= 0xF)) {
> >                        printk(" - using IRQ %d\n", dev->irq);
> >                        return_VALUE(dev->irq);
> >                }
> >                else {
> >                        printk("\n");
> > -                       return_VALUE(0);
> > +                       return_VALUE(-EINVAL);
> >                }
> > 
> > Now instead of returning 0, we'll get -EINVAL when a driver calls 
> > pci_enable_device.  This is arguably correct since there's no _PRT 
> > entry (and in fact no ACPI namespace on sn2), but shouldn't the code

> > above be looking at the 'pin' value instead of dev->irq?  The sn2 
> > specific PCI code sets up each
> > dev->irq long before this with the correct values...
>
> I think the change above is actually from
>    incorrect-pci-interrupt-assignment-on-es7000-for-pin-zero.patch

> of which I am officially ignorant :-)

I realize now that this is very involved piece of code and a lot was
built around the assumption that IRQ0 is a timer interrupt (pin 0 is for
PCI on ES7000), and assumption that everyone honors this assumption :)
However, it seems wrong that we are not able to read literally what ACPI
says, such as irq 0 for INTA. Maybe, it would be better if the code
above was returing an error code, not an irq, which is returned in dev
anyway. It should be some creative way to resolve this issue... I think
the idea in the comment above by Jesse Barnes has good potential.  

Thanks,
--Natalie

RE: 2.6.9-rc2-mm1

[Len Brown]

On Fri, 2004-09-17 at 01:18, Protasevich, Natalie wrote:
> > On Thursday 16 September 2004 11:14 am, Jesse Barnes wrote:
> > > On Thursday, September 16, 2004 2:40 am, Andrew Morton wrote:
> > >  bk-acpi.patch
> > >
> > > Looks like some changes in this patch break sn2.  In particular,
> this
> > > hunk in
> > > acpi_pci_irq_enable():
> > >
> > > -               if (dev->irq && (dev->irq <= 0xF)) {
> > > +               if (dev->irq >= 0 && (dev->irq <= 0xF)) {
> > >                        printk(" - using IRQ %d\n", dev->irq);
> > >                        return_VALUE(dev->irq);
> > >                }
> > >                else {
> > >                        printk("\n");
> > > -                       return_VALUE(0);
> > > +                       return_VALUE(-EINVAL);
> > >                }
> > >
> > > Now instead of returning 0, we'll get -EINVAL when a driver calls
> > > pci_enable_device.  This is arguably correct since there's no _PRT
> > > entry (and in fact no ACPI namespace on sn2), but shouldn't the
> code
> 
> > > above be looking at the 'pin' value instead of dev->irq?  The sn2
> > > specific PCI code sets up each
> > > dev->irq long before this with the correct values...

No, in this context, the variable "pin" is to select PCI INTA/B/C/D, not
a interrupt controller pin.

If SN2 is using its pre-determied interrupt configuration, then is is
probably a bug that it calls down into this code at all, since SN2 wants
this code to be a NOP, yes?

> > I think the change above is actually from
> >    incorrect-pci-interrupt-assignment-on-es7000-for-pin-zero.patch
> 
> > of which I am officially ignorant :-)

yeah, probably these IRQ patches should come through me, but haven't b/c
my patch throughput has been very low the last couple of weeks.  At
least if I ship no new patches I don't get blamed for breaking stuff;-)

> I realize now that this is very involved piece of code and a lot was
> built around the assumption that IRQ0 is a timer interrupt (pin 0 is
> for
> PCI on ES7000), and assumption that everyone honors this assumption :)
> However, it seems wrong that we are not able to read literally what
> ACPI
> says, such as irq 0 for INTA. Maybe, it would be better if the code
> above was returing an error code, not an irq, which is returned in dev
> anyway. It should be some creative way to resolve this issue... I
> think
> the idea in the comment above by Jesse Barnes has good potential. 

Yes, there are lots of places where IRQ0 is an error condition.  I
expect that this was lazyness based on the fact that the timer code is
hard-wired to use IRQ0, so it is always taken on IA32.  My suggestion
to un-hard-wire the timer code was not greeted with enthusiasm, so this
is how things sit.

But IRQ0 should not be a problem on the ES7000, as there is an override
to supply IRQ0 from pin 0:20.  pin 0:0, on the other hand is a PCI
interrupt on the ES7000, completely valid to be assigned from the _PRT
and to be assigned by the es7000-specific code to any arbitrary IRQ#.

I'm not sure exactly that the patch above was trying to fix.  Looks like
it is time to examine the latest ew7000 changes in detail.  But I think
the patch has pointed out that this routine really should be returning 0
for success and non zero for failure; and returning dev->irq was
probably a latent bug all along.

-Len

Re: 2.6.9-rc2-mm1

[Jesse Barnes]

On Thursday, September 16, 2004 11:50 pm, Len Brown wrote:
> I'm not sure exactly that the patch above was trying to fix.  Looks like
> it is time to examine the latest ew7000 changes in detail.  But I think
> the patch has pointed out that this routine really should be returning 0
> for success and non zero for failure; and returning dev->irq was
> probably a latent bug all along.

Right, and I'd like success to be defined a little more broadly.  If dev->irq 
is already valid, we should just return 0 right away.  That would take care 
of platforms that already have the dev in question setup properly.

Thanks,
Jesse

acpi_pci_irq_enable (RE: 2.6.9-rc2-mm1)

[Len Brown]

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

On Fri, 2004-09-17 at 02:50, Len Brown wrote:
> On Fri, 2004-09-17 at 01:18, Protasevich, Natalie wrote:
> > > On Thursday 16 September 2004 11:14 am, Jesse Barnes wrote:
> > > > On Thursday, September 16, 2004 2:40 am, Andrew Morton wrote:
> > > >  bk-acpi.patch
> > > >
> > > > Looks like some changes in this patch break sn2.  In particular,
> > this
> > > > hunk in acpi_pci_irq_enable():
> > > >
> > > > -                       return_VALUE(0);
> > > > +                       return_VALUE(-EINVAL);
> > > > 

> I think the patch has pointed out that this routine really should be returning 0
> for success and non zero for failure; and returning dev->irq was
> probably a latent bug all along.

Jesse,
I think this one is correct, please let me know if you have any trouble
with it.

thanks,
-Len



[-- Attachment #2: irq0.patch --]
[-- Type: text/plain, Size: 5473 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   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>
# 
# drivers/acpi/pci_link.c
#   2004/11/09 03:08:36-05:00 len.brown@intel.com +10 -5
#   Import patch irq0_checks3.patch
# 
# drivers/acpi/pci_irq.c
#   2004/11/09 03:08:36-05:00 len.brown@intel.com +28 -13
#   Import patch irq0_checks3.patch
# 
diff -Nru a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
--- a/drivers/acpi/pci_irq.c	2004-11-09 03:45:33 -05:00
+++ b/drivers/acpi/pci_irq.c	2004-11-09 03:45:33 -05: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-11-09 03:45:33 -05:00
+++ b/drivers/acpi/pci_link.c	2004-11-09 03:45:33 -05: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;