Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 1 Jun 2004 17:42:57 -0400
From:      John Baldwin <jhb@FreeBSD.org>
To:        Nate Lawson <nate@root.org>
Cc:        marcel@FreeBSD.org
Subject:   Re: Patch: Defer bus_config_intr() until bus_alloc_resource()..
Message-ID:  <200406011742.57991.jhb@FreeBSD.org>
In-Reply-To: <20040601141424.I29932@root.org>
References:  <200406011531.09077.jhb@FreeBSD.org> <200406011638.04400.jhb@FreeBSD.org> <20040601141424.I29932@root.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Tuesday 01 June 2004 05:23 pm, Nate Lawson wrote:
> On Tue, 1 Jun 2004, John Baldwin wrote:
> > On Tuesday 01 June 2004 04:25 pm, Nate Lawson wrote:
> > > On Tue, 1 Jun 2004, John Baldwin wrote:
> > > > I need the patch below in order to turn on bus_config_intr() when
> > > > using the I/O APICs.  The original problem is that the _CRS of link
> > > > devices is configured which is the PIC IRQ and thus screws up intpins
> > > > when using the APIC.  It basically takes bus_config_intr() out of the
> > > > resource parsing code and does the config when an IRQ is allocated
> > > > via
> > > > bus_alloc_resource() for normal devices, and when a PCI IRQ is routed
> > > > for PCI link devices.
> > >
> > > I appreciate what you're trying to do but I don't like this approach.
> > > Deferring half the parsing to alloc time and moving it from
> > > acpi_resource.c results in a lot of unnecessary duplication and
> > > layering violation.  The real issue you're trying to work around is
> > > that you want to defer the actual config_intr until you're sure which
> > > intr you're going to use.
> >
> > Well, arguably it exposes an improper layering violation when
> > bus_config_intr() was added.  bus_config_intr() was probably added in the
> > place that it is now simply because it was easier to do so.  That doesn't
> > mean it's in the correct place.  I don't really consider having an
> > ACPI-specific routine understand ACPI-specific resources.  One
> > possibility though might be to add a wrapper function to acpi_resource.c
> > that does the bus_config_intr() on a passed-in pointer to an ACPI
> > resource.  That might reduce at least some of the duplication.
>
> I still don't like this.  Right now we have a single parse routine whose
> goal is to parse a resource object and then store the info it finds in a
> FreeBSD-compatible format, via bus_set_resource().  Just because
> bus_config_intr() doesn't stick to bus_set_resource() semantics doesn't
> mean we should hack acpi to bits to fix it.

FreeBSD has no format for this and I don't really have time to add multiple 
resource types (which is what ACPI does, and probably what we will need to do 
eventually) to struct resource.  What I have done in the current form is 
added some wrapper functions to acpi_resource.c (so all the magic about what 
rid's match up, etc. is in one file) in the patch below.

> > > Some suggestions...  Make polarity and trigger real resource types
> > > (sys/i386/include/resource.h) and do a bus_set_resource of them in the
> > > resource parsing code.  Then in the alloc code do a bus_get_resource
> > > for them and then call BUS_CONFIG_INTR.  Additionally, instead of doing
> > > the deferred BUS_CONFIG_INTR in the alloc code, it should actually be
> > > done in the MD code for bus_setup_intr().  This seems cleaner since
> > > allocating an irq resource shouldn't poke the hw until
> > > bus_setup_intr().
> >
> > *sigh*  Trigger and polarity are not resources, they are a property of
> > the IRQ resource perhaps.  Unfortunately, our rman(9) interface doesn't
> > support type-specific resource attributes and I'm really not up for
> > chainsawing rman(9) enough to get that into place.  Getting this bug
> > fixed is holding up a lot of other work.
>
> Yes, I agree they are properties of the irq resource.  I'm not asking you
> to go the full route of implementing sub-types in rman.
>
> The simplest and backwards-compatible way to fix this is to add a MD
> SYS_RES_IRQ_CFG resource and set that in the parse routine.  Then, in the
> implementation of bus_setup_intr(), look for that resource and use it for
> the intr configuration.  The only deficiency here is that it creates a
> "top-level" resource type instead of a sub-type, but since there are only
> 6 resource types in i386/include/resource.h right now, this is hardly a
> huge deviation.  With this change, bus_config_intr() can go away (or be a
> no-op for backwards API compat).
>
> This seems the simplest and cleanest approach.

No, I don't think this is clean. Note that ia64 needs bus_config_intr() too.  
In fact, that's why it was added in the first place!

--- //depot/vendor/freebsd/src/sys/dev/acpica/acpi.c	2004/05/06 01:05:39
+++ //depot/user/jhb/acpipci/dev/acpica/acpi.c	2004/06/01 14:05:57
@@ -855,9 +855,21 @@
 {
     struct acpi_device *ad = device_get_ivars(child);
     struct resource_list *rl = &ad->ad_rl;
+    struct resource *res;
+    ACPI_RESOURCE ares;
 
-    return (resource_list_alloc(rl, bus, child, type, rid, start, end, count,
-	    flags));
+    res = resource_list_alloc(rl, bus, child, type, rid, start, end, count,
+	flags);
+    if (res == NULL || device_get_parent(child) != bus)
+	    return (res);
+    switch (type) {
+    case SYS_RES_IRQ:
+	    if (ACPI_FAILURE(acpi_lookup_irq_resource(dev, *rid, res, &ares)))
+		    break;
+	    acpi_config_intr(dev, &ares);
+	    break;
+    }
+    return (res);
 }
 
 static int
--- //depot/vendor/freebsd/src/sys/dev/acpica/acpi_pcib.c	2004/05/05 19:22:26
+++ //depot/user/jhb/acpipci/dev/acpica/acpi_pcib.c	2004/06/01 14:05:57
@@ -121,10 +121,11 @@
 
     ACPI_FUNCTION_TRACE((char *)(uintptr_t)__func__);
     
+    crsres = NULL;
     buf.Pointer = NULL;
     crsbuf.Pointer = NULL;
     prsbuf.Pointer = NULL;
-    interrupt = 255;
+    interrupt = PCI_INVALID_IRQ;
 
     /* ACPI numbers pins 0-3, not 1-4 like the BIOS. */
     pin--;
@@ -348,6 +349,7 @@
 
     /* XXX Data.Irq and Data.ExtendedIrq are implicitly structure-copied. */
     crsbuf.Pointer = NULL;
+    crsres = NULL;
     if (prsres->Id == ACPI_RSTYPE_IRQ) {
 	resbuf.Id = ACPI_RSTYPE_IRQ;
 	resbuf.Length = ACPI_SIZEOF_RESOURCE(ACPI_RESOURCE_IRQ);
@@ -378,6 +380,7 @@
 		      AcpiFormatException(status));
 	goto out;
     }
+    crsres = &resbuf;
     
     /* Return the interrupt we just routed. */
     device_printf(pcib, "slot %d INT%c routed to irq %d via %s\n", 
@@ -386,6 +389,8 @@
     interrupt = Interrupts[0];
 
 out:
+    if (PCI_INTERRUPT_VALID(interrupt) && crsres != NULL)
+	acpi_config_intr(dev, crsres);
     if (crsbuf.Pointer != NULL)
 	AcpiOsFree(crsbuf.Pointer);
     if (prsbuf.Pointer != NULL)
@@ -393,6 +398,5 @@
     if (buf.Pointer != NULL)
 	AcpiOsFree(buf.Pointer);
 
-    /* XXX APIC_IO interrupt mapping? */
     return_VALUE (interrupt);
 }
--- //depot/vendor/freebsd/src/sys/dev/acpica/acpi_resource.c	2004/04/09 
11:15:38
+++ //depot/user/jhb/acpipci/dev/acpica/acpi_resource.c	2004/06/01 14:05:57
@@ -44,6 +44,88 @@
 #define _COMPONENT	ACPI_BUS
 ACPI_MODULE_NAME("RESOURCE")
 
+ACPI_STATUS
+acpi_lookup_irq_resource(device_t dev, int rid, struct resource *res,
+    ACPI_RESOURCE *acpi_res)
+{
+    ACPI_BUFFER buf;
+    ACPI_STATUS status;
+    ACPI_RESOURCE *ares;
+    char *curr, *last;
+    int i, found;
+
+    buf.Length = ACPI_ALLOCATE_BUFFER;
+    status = AcpiGetCurrentResources(acpi_get_handle(dev), &buf);    
+    if (ACPI_FAILURE(status))
+	    break;
+    i = 0;
+    found = 0;
+    curr = buf.Pointer;
+    last = (char *)buf.Pointer + buf.Length;
+    while (curr < last) {
+	    ares = (ACPI_RESOURCE *)curr;
+	    curr += ares->Length;
+	    switch (ares->Id) {
+	    case ACPI_RSTYPE_END_TAG:
+		    curr = last;
+		    break;
+	    case ACPI_RSTYPE_IRQ:
+		    if (ares->Data.Irq.NumberOfInterrupts != 1)
+			    break;
+		    if (i != rid) {
+			    i++;
+			    break;
+		    }
+		    KASSERT(ares->Data.Irq.Interrupts[0] ==
+			rman_get_start(res), ("IRQ resources do not match"));
+		    *acpi_res = *ares;
+		    found++;
+		    break;
+	    case ACPI_RSTYPE_EXT_IRQ:
+		    if (ares->Data.ExtendedIrq.NumberOfInterrupts != 1)
+			    break;
+		    if (i != rid) {
+			    i++;
+			    break;
+		    }
+		    KASSERT(ares->Data.ExtendedIrq.Interrupts[0] ==
+			rman_get_start(res), ("IRQ resources do not match"));
+		    *acpi_res = *ares;
+		    found++;
+		    break;
+	    }
+    }
+    AcpiOsFree(buf.Pointer);
+    if (found == 0)
+	    return (AE_NOT_FOUND);
+    return (AE_OK);
+}
+
+void
+acpi_config_intr(device_t dev, ACPI_RESOURCE *res)
+{
+    u_int irq;
+    int pol, trig;
+
+    switch (res->Id) {
+    case ACPI_RSTYPE_IRQ:
+	irq = res->Data.Irq.Interrupts[0];
+	trig = res->Data.Irq.EdgeLevel;
+	pol = res->Data.Irq.ActiveHighLow;
+	break;
+    case ACPI_RSTYPE_EXT_IRQ:
+	irq = res->Data.ExtendedIrq.Interrupts[0];
+	trig = res->Data.ExtendedIrq.EdgeLevel;
+	pol = res->Data.ExtendedIrq.ActiveHighLow;
+	break;
+    default:
+	panic("%s: bad resource type %u", __func__, res->Id);
+    }
+    BUS_CONFIG_INTR(dev, irq, (trig == ACPI_EDGE_SENSITIVE) ?
+	INTR_TRIGGER_EDGE : INTR_TRIGGER_LEVEL, (pol == ACPI_ACTIVE_HIGH) ?
+	INTR_POLARITY_HIGH : INTR_POLARITY_LOW);	
+}
+
 /*
  * Fetch a device's resources and associate them with the device.
  *
@@ -495,9 +577,6 @@
 	return;
 
     bus_set_resource(dev, SYS_RES_IRQ, cp->ar_nirq++, *irq, 1);
-    BUS_CONFIG_INTR(dev, *irq, (trig == ACPI_EDGE_SENSITIVE) ?
-	INTR_TRIGGER_EDGE : INTR_TRIGGER_LEVEL, (pol == ACPI_ACTIVE_HIGH) ?
-	INTR_POLARITY_HIGH : INTR_POLARITY_LOW);
 }
 
 static void
--- //depot/vendor/freebsd/src/sys/dev/acpica/acpivar.h	2004/05/06 01:05:39
+++ //depot/user/jhb/acpipci/dev/acpica/acpivar.h	2004/06/01 14:05:57
@@ -283,6 +283,11 @@
 extern ACPI_STATUS	acpi_parse_resources(device_t dev, ACPI_HANDLE handle,
 			    struct acpi_parse_resource_set *set, void *arg);
 
+ACPI_STATUS	acpi_lookup_irq_resource(device_t dev, int rid, struct resource 
*res,
+    ACPI_RESOURCE *acpi_res);
+void		acpi_config_intr(device_t dev, ACPI_RESOURCE *res);
+
+
 /* ACPI event handling */
 extern UINT32	acpi_event_power_button_sleep(void *context);
 extern UINT32	acpi_event_power_button_wake(void *context);

-- 
John Baldwin <jhb@FreeBSD.org>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200406011742.57991.jhb>