Date: Tue, 1 Jun 2004 15:05:32 -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: <20040601145357.P30070@root.org> In-Reply-To: <200406011742.57991.jhb@FreeBSD.org> References: <200406011531.09077.jhb@FreeBSD.org> <200406011638.04400.jhb@FreeBSD.org> <20040601141424.I29932@root.org> <200406011742.57991.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 05:23 pm, Nate Lawson wrote:
> > 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.
This is slightly better and might be acceptable as a temporary workaround
if you clean it up a bit (remove duplicate rid == i checks, make style
match surrounding file, etc.) But I want to make sure you actually have
plans to fix bus_config_intr() and not leave this hack behind before being
ok with this workaround.
> > > > 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!
I understand this but it doesn't have anything to do with my suggestion.
sys/amd64/include/resource.h:#define SYS_RES_IRQ 1 /* interrupt lines */
sys/amd64/include/resource.h:#define SYS_RES_DRQ 2 /* isa dma lines */
sys/amd64/include/resource.h:#define SYS_RES_MEMORY 3 /* i/o memory */
sys/amd64/include/resource.h:#define SYS_RES_IOPORT 4 /* i/o ports */
sys/i386/include/resource.h:#define SYS_RES_IRQ 1 /* interrupt lines */
sys/i386/include/resource.h:#define SYS_RES_DRQ 2 /* isa dma lines */
sys/i386/include/resource.h:#define SYS_RES_MEMORY 3 /* i/o memory */
sys/i386/include/resource.h:#define SYS_RES_IOPORT 4 /* i/o ports */
sys/ia64/include/resource.h:#define SYS_RES_IRQ 1 /* interrupt lines */
sys/ia64/include/resource.h:#define SYS_RES_DRQ 2 /* isa dma lines */
sys/ia64/include/resource.h:#define SYS_RES_MEMORY 3 /* i/o memory */
sys/ia64/include/resource.h:#define SYS_RES_IOPORT 4 /* i/o ports */
Add a #define SYS_RES_IRQ_CFG to each of these files. Have acpi set it on
child devices. Only implement code to read these "resource types" in
{i386,amd64,ia64} bus_setup_intr() implementations. Other archs will get
ENOENT if they try to read them, which would be a bug anyway. These are
all MD files.
The impact of this approach is only 1 define in the $ARCH .h files, 2
lines of code in $ARCH/$ARCH/nexus.c to call bus_get_resource() and
intr_config_intr(), and 2 lines of code in acpi_parse_resources to call
bus_set_resource(). This is for only 3 archs. Are you thinking it's
something bigger than this?
-Nate
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040601145357.P30070>