Patch: Defer bus_config_intr() until bus_alloc_resource()..
Nate Lawson
nate at root.org
Tue Jun 1 14:23:07 PDT 2004
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
More information about the freebsd-acpi
mailing list