Date: Tue, 1 Jun 2004 14:23:07 -0700 (PDT) From: Nate Lawson <nate@root.org> To: John Baldwin <jhb@FreeBSD.org> Cc: marcel@FreeBSD.org Subject: Re: Patch: Defer bus_config_intr() until bus_alloc_resource().. Message-ID: <20040601141424.I29932@root.org> In-Reply-To: <200406011638.04400.jhb@FreeBSD.org> References: <200406011531.09077.jhb@FreeBSD.org> <20040601131817.N29571@root.org> <200406011638.04400.jhb@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. > > 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. -Nate
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040601141424.I29932>