From owner-freebsd-acpi@FreeBSD.ORG Tue Jun 1 15:05:32 2004 Return-Path: Delivered-To: freebsd-acpi@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 0B87816A4D0 for ; Tue, 1 Jun 2004 15:05:32 -0700 (PDT) Received: from root.org (root.org [67.118.192.226]) by mx1.FreeBSD.org (Postfix) with SMTP id C45F443D48 for ; Tue, 1 Jun 2004 15:05:31 -0700 (PDT) (envelope-from nate@root.org) Received: (qmail 30171 invoked by uid 1000); 1 Jun 2004 22:05:32 -0000 Date: Tue, 1 Jun 2004 15:05:32 -0700 (PDT) From: Nate Lawson To: John Baldwin In-Reply-To: <200406011742.57991.jhb@FreeBSD.org> Message-ID: <20040601145357.P30070@root.org> References: <200406011531.09077.jhb@FreeBSD.org> <200406011638.04400.jhb@FreeBSD.org> <20040601141424.I29932@root.org> <200406011742.57991.jhb@FreeBSD.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: freebsd-acpi@FreeBSD.org cc: marcel@FreeBSD.org Subject: Re: Patch: Defer bus_config_intr() until bus_alloc_resource().. X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 01 Jun 2004 22:05:32 -0000 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